Clarifies Behavior of SANITIZE_FIELD_NAMES by astorm · Pull Request #378 · elastic/apm

astorm · 2020-12-05T00:52:37Z

While we (Node Agent) were working on the SANITIZE_FIELD_NAMES feature we discovered some ambiguities in the spec -- likely due to there not being a Node Agent team when this spec was agreed on :)

This PR attempts to resolve those ambiguities and is intended as the start of a conversation, not the end of one. Our intention is to better understand how all Agents are doing things.

After landing, update remote config description

ghost · 2020-12-05T00:55:43Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-10-11T04:29:00.401+0000
Duration: 3 min 2 sec
Commit: bd32292

eyalkoren

Thanks for clarifying the spec. I don't know how we managed to do anything in your absence!
I hope it is OK if I approve (on behalf of the Java agent) as if it is the end of the discussion 🙂
Only a few minor suggestions/points.

specs/agents/sanitization.md

eyalkoren · 2020-12-06T13:25:27Z

specs/agents/sanitization.md

+Agents MUST provide a minimum default configuration of
+
+    [ 'password', 'passwd', 'pwd', 'secret', '*key', '*token*', '*session*',
+      '*credit*','*card*', 'authorization', 'set-cookie']
+
+for the `sanitize_field_names` configuration value.  Agent's MAY include the
+following extra fields in their default configuration to avoid breaking changes
+
+    ['pw','pass','connect.sid']


The fact that some agents have different defaults should be reflected in the remote config description.

Today I learned there's a remote config description :) These extra fields were/are for the Node.js agent specifically -- they're fields that we previously redacted that would be tricky/cumbersome (but not impossible) to change. However, since it sounds like there's downstream dependencies I'm learning towards just removing this and going with the defaults.

@eyalkoren (or anyone) A few follow up questions for you -- is the the remote config description used by the central configuration system?

That is -- with those defaults set in general_settings.ts does that mean the central configuration system will send these values by default? The follow on question that is -- do agents typically need to set these default values themselves, or can/should we rely on them being set by central configuration?

is the the remote config description used by the central configuration system?

No, it is only a string for description of the default.

does that mean the central configuration system will send these values by default? The follow on question that is -- do agents typically need to set these default values themselves, or can/should we rely on them being set by central configuration?

The central config does not send these values, agents need to set them themselves. After all, the central config is just one more configuration system for agents, which need to have proper defaults even if it is disabled, and you would typically use the same default regardless of the configuration source.

@astorm So you leaning towards having the apm-agent-nodejs drop ['pw','pass','connect.sid'] by default? I don't disagree. We'll need to defer doing so until a major ver bump, right?

Thank you @eyalkoren -- the description of this issue has been updated with a TO DO that reflects the need to update the remote config description.

@trentm I suspect we'll talk off of GitHub about this, but my intent was to spec ['pw','pass','connect.sid'] this way in order to give us some flexibility in keeping these extra keys redacted and not force a major version bump on us immediately.

specs/agents/sanitization.md

felixbarny

Thanks for clarifying the spec!

specs/agents/sanitization.md

felixbarny · 2020-12-07T07:37:19Z

specs/agents/sanitization.md

+
+Fields that MUST be sanitized are the HTTP Request headers, HTTP Response
+headers, and form fields in an `application/x-www-form-urlencoded` request
+body.  No fields (including `set-cookie` headers) are exempt from this.


There's also transaction.context.request.cookies which is a map of cookie names to values. This map must be redacted, too.

Only transaction.context.request.cookies map should be filtered or those Set-Cookie HTTP header(s) that reference excluded cookie names as well (assuming that the configured value is different from the default and Set-Cookie is not included)?

Set-Cookie is a response header. The corresponding request header is Cookie. As the content of the Cookie header is the same as transaction.context.request.cookies, in the Java agent, we're removing Cookie from transaction.context.request.headers. This both minimizes the amount of duplicate data and eliminates the complication of having to redact parts of a header value.

For response headers, there's no equivalent to transaction.context.request.cookies, so we just sanitize the transaction.context.response.headers map.

See also https://github.com/elastic/apm-agent-java/blob/master/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/SanitizingWebProcessor.java

in the Java agent, we're removing Cookie from transaction.context.request.headers.

Should we include it in the spec?

@felixelastic @SergeyKleyman @basepi Thank you all. If I'm reading the above correctly, it sounds like there's three data structures/values to consider here

The Set-Cookie response header

The Cookie request header

A representation of the key/values pairs of a Set-Cookie header that's stored in transaction.context.request.cookies

If I spec-ed this as something like

MUST redact transaction.context.request.cookies

If agents capture values for Set-Cookie or Cookie request/response headers, it MUST redact the field values here as well.

would that capture what we're after? Or is there more nuance here?

@SergeyKleyman Re:

can this behavior be achieved by adding cookie to SANITIZE_FIELD_NAMES default value?

Good question! Thinking that through -- I'm not sure this would achieve what we want. Here's my reasoning: If we added cookie to SANITIZE_FIELD_NAMES, we'd be instructing the agent to REDACT the a header named cookie, and not to remove it completely.

Also, I've been operating under the assumption that the default fields were decided on a while ago and would be a pain to change now after some agent teams have already landed this functionality or are midstream on it.

@felixbarny Thank you for sharing those data structures -- I learned something. It looks like the Node.js Agent doesn't capture transaction.context.request.cookies at all. Wondering if we need a spec for what part of the request gets captured or if that's just going spec wild.

More pertinent to our discussion though -- What do you think about this as final language for future proofing things?

Fields that MUST be sanitized are the HTTP Request headers, HTTP Response
headers, and form fields in an application/x-www-form-urlencoded request
body. Additionally, if cookie headers are parsed into name/value pairs and reported
to APM Server via the agent (for example, transaction.context.request.cookies), the
cookie names must be sanitized as well.

Ah, I didn't know some agents don't send request.cookies.
That property is spec'd in the intake API but it's not a required field.

I like that wording!

Here's my reasoning: If we added cookie to SANITIZE_FIELD_NAMES, we'd be instructing the agent to REDACT the a header named cookie, and not to remove it completely.

The main reason for my suggestion is that it might look strange to the users that Set-Cookie header is redacted while Cookie header is removed completely.

Additionally, if cookie headers are parsed into name/value pairs and reported
to APM Server via the agent (for example, transaction.context.request.cookies), the
cookie names must be sanitized as well.

If we decide to go with hardcoding cookie header removal it might be worth adding something like:

In addition parsed cookie headers MUST NOT be included in the set of headers (in the example above in transaction.context.request.headers)

specs/agents/sanitization.md

Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

* Align sanitize_field_names with cross-agent spec * Add CHANGELOG * Remove value sanitization The value of the credit card check was dubious, and this change will improve performance. * Move to MASK recommended by elastic/apm#378 * Update changelog * Remove querystring sanitization * Update pre-commit to match upstream black behavior * Dynamically generate BASE_SANITIZE_FIELD_NAMES * Fix the merge commit

) * Align sanitize_field_names with cross-agent spec * Add CHANGELOG * Remove value sanitization The value of the credit card check was dubious, and this change will improve performance. * Move to MASK recommended by elastic/apm#378 * Update changelog * Remove querystring sanitization * Update pre-commit to match upstream black behavior * Dynamically generate BASE_SANITIZE_FIELD_NAMES * Fix the merge commit

Alan Storm added 5 commits December 4, 2020 16:43

first pass to disambiguate some points in the spec

5966ca4

fix: removing a should

2f33854

fix: grammar

f8b2b9f

fix: clarifying value sanitization

18e3193

fix: grammar

4981138

astorm requested review from a team as code owners December 5, 2020 00:52

astorm requested review from AlexanderWert, basepi, felixbarny, graphaelli and trentm December 5, 2020 00:59

eyalkoren approved these changes Dec 6, 2020

View reviewed changes

felixbarny reviewed Dec 7, 2020

View reviewed changes

trentm reviewed Dec 7, 2020

View reviewed changes

specs/agents/sanitization.md Outdated Show resolved Hide resolved

basepi approved these changes Dec 8, 2020

View reviewed changes

basepi added a commit to basepi/apm-agent-python that referenced this pull request Dec 8, 2020

Move to MASK recommended by elastic/apm#378

92927a1

basepi reviewed Dec 8, 2020

View reviewed changes

specs/agents/sanitization.md Outdated Show resolved Hide resolved

basepi mentioned this pull request Dec 8, 2020

[META 319] Align sanitize_field_names with cross-agent spec elastic/apm-agent-python#982

Merged

astorm and others added 3 commits December 15, 2020 16:54

Update specs/agents/sanitization.md

4ed30f4

Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

Update specs/agents/sanitization.md

218ccb6

Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

chore: change query string parameters from a MUST NOT to a SHOULD NOT

a14bd8a

Alan Storm added 4 commits December 15, 2020 17:09

chore: remove remove/merge clarification

7629a1e

fix: recommend a specific REDACTED string but still allow for others.

ff04c63

fix: h2 to h4

997594f

fix: clarify that we're moving away from value sanitization

cf9732f

mikker approved these changes Dec 17, 2020

View reviewed changes

AlexanderWert approved these changes May 31, 2021

View reviewed changes

astorm added 2 commits May 31, 2021 22:27

Merge branch 'master' into astorm/sanitize-spec

6a21885

Merge branch 'master' into astorm/sanitize-spec

bd32292

gregkalapos approved these changes Oct 12, 2021

View reviewed changes

astorm merged commit 7b4962a into elastic:master Oct 13, 2021

Conversation

astorm commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 5, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astorm Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKleyman Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

astorm commented Dec 5, 2020 •

edited

Loading

ghost commented Dec 5, 2020 •

edited by ghost

Loading

astorm Dec 16, 2020 •

edited

Loading

SergeyKleyman Dec 17, 2020 •

edited

Loading