Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clarifies Behavior of SANITIZE_FIELD_NAMES #378
Changes from 5 commits
5966ca4
2f33854
f8b2b9f
18e3193
4981138
4ed30f4
218ccb6
a14bd8a
7629a1e
ff04c63
997594f
cf9732f
6a21885
bd32292
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
No, it is only a string for description of the default.
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.
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 andSet-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 isCookie
. As the content of theCookie
header is the same astransaction.context.request.cookies
, in the Java agent, we're removingCookie
fromtransaction.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 thetransaction.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
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.
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
Set-Cookie
response headerCookie
request headerSet-Cookie
header that's stored intransaction.context.request.cookies
If I spec-ed this as something like
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:
Good question! Thinking that through -- I'm not sure this would achieve what we want. 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.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?
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.
The main reason for my suggestion is that it might look strange to the users that
Set-Cookie
header is redacted whileCookie
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.
If we decide to go with hardcoding
cookie
header removal it might be worth adding something like: