Add query string automatic redaction#2283
Conversation
cd4f348 to
cd9711d
Compare
There was a problem hiding this comment.
I think these should be notified in the changelog as well.
Should we also have them shown at install time via the gemspec post install callback?
There was a problem hiding this comment.
The first step is to go through the GettingStarted.md and replacing all places where we want quantize: { base: :show }, { query: { show: :all }, obfuscate: :internal }} (Is this the desired behaviour? Please let me know) to be the new default and recommend people to use that configuration.
New users installing ddtrace will see those instructions and starting using 2.0 defaults today.
Regarding how to inform users, it seems to me that we need an official way to track future breaking changes, both to inform clients in advance and also for ourselves to not forget about them in the major version migration guide.
I think the way you capture it here is a good start, under a Deprecation notice: section.
I'm not 100% on the best way to warn users, but definitely a changelog entry for the next 1.x release should be present.
There was a problem hiding this comment.
The first step is to go through the GettingStarted.md and replacing all places where we want
Very good point!
quantize: { base: :show }, { query: { show: :all }, obfuscate: :internal }}(Is this the desired behaviour? Please let me know)
Definitely, although query: { obfuscate: :internal } defaults query: { show: to :all.
Regarding how to inform users, it seems to me that we need an official way to track future breaking changes, both to inform clients in advance and also for ourselves to not forget about them in the major version migration guide.
Yeah, I was thinking about leveraging gem post install, and having a list of warnings encoded in a (ruby? json? yaml? custom?) file that can be both programmatically referred to (thus outputting only relevant messages) as well as human readable for users to follow and go through in the repo.
There was a problem hiding this comment.
I think I'll do that in another PR, along with this suggestion: #2265 (comment)
There was a problem hiding this comment.
An alternative to quantize.query.obfuscate is to have another value for quantize.query.show, e.g :obfuscate, but then it becomes trickier to control which are shown, as well as providing obfuscator options.
There was a problem hiding this comment.
This method now has crazy logic to implement the expected behaviour, especially when :show (whether Array or :all), :exclude, and :obfuscate interact.
There was a problem hiding this comment.
Short circuit when :all is known not to come from :obfuscate, but it still needs to go through if options[:exclude] is set.
There was a problem hiding this comment.
Needed because of the (options[:obfuscate] && options[:exclude]) case, which may have options[:show] == :all.
There was a problem hiding this comment.
Contrary to what I previously thought, obfuscation does not operate on key value pairs but on the string itself. Therefore it is a separate, second stage, taking place after collect_query.
cd9711d to
9290466
Compare
bccc876 to
e35bcbd
Compare
There was a problem hiding this comment.
The first step is to go through the GettingStarted.md and replacing all places where we want quantize: { base: :show }, { query: { show: :all }, obfuscate: :internal }} (Is this the desired behaviour? Please let me know) to be the new default and recommend people to use that configuration.
New users installing ddtrace will see those instructions and starting using 2.0 defaults today.
Regarding how to inform users, it seems to me that we need an official way to track future breaking changes, both to inform clients in advance and also for ourselves to not forget about them in the major version migration guide.
I think the way you capture it here is a good start, under a Deprecation notice: section.
I'm not 100% on the best way to warn users, but definitely a changelog entry for the next 1.x release should be present.
e35bcbd to
016beca
Compare
marcotc
left a comment
There was a problem hiding this comment.
Looks good! Only a few minor comments left to address, but ✅ overall.
6c9c329 to
d374ea6
Compare
|
I think all items have been addressed, on to fix the linter breakage. |
Co-authored-by: Marco Costa <[email protected]>
d374ea6 to
3c7bacf
Compare
In practice this cop seems to be disabled as soon as we reach the limit.
What does this PR do?
Redacts query string sensitive data using a regex-based heuristic.
Motivation
Smartly prevent information leakage when query quantization is set to all
Additional Notes
Needed by ASM to allow
quantize.query.showto safely change its default to:all.The proposed code attempts to implement the query string obfuscator specification as accurately as possible.
How to test the change?
Specs updated with as much corner cases as I could think of.
Manually, try something like this: