-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarifies Behavior of SANITIZE_FIELD_NAMES #378
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that some agents have different defaults should be reflected in the remote config description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying the spec!
|
||
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also transaction.context.request.cookies
which is a map of cookie names to values. This map must be redacted, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the Java agent, we're removing
Cookie
fromtransaction.context.request.headers
.
Should we include it in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 intransaction.context.request.cookies
If I spec-ed this as something like
MUST redact
transaction.context.request.cookies
If agents capture values for
Set-Cookie
orCookie
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 anapplication/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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my reasoning: If we added
cookie
toSANITIZE_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>
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
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.