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
Add SensitiveDataMasker to exceptions messages #42940
Conversation
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.
You also would have to invoke wipeSensitiveData
on DB::Exception::addMessage
, remote(remote_) | ||
{ | ||
handle_error_code(msg, code, remote, getStackFramePointers()); | ||
handle_error_code(msg_masked.msg, code, remote, getStackFramePointers()); |
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.
I've implemented a different solution recently, which is based on a AST tree, not on regular expressions, so my solution is going to work better for more various cases like s3(url, key, secret_key)
or CREATE USER IDENTIFIER WITH password
. But it seems I forgot to cover this case in my solution, so I'll still have to improve it. I'd prefer to take your test but use that my solution with replacings in AST tree. Ok?
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.
I'm not sure if it can really be done 100% safe on AST level...
Imagine you can't parse the query correctly, but it have sensitive data, or that sensitive data does not come from the query at all. So i think that those 'sensitivedatamaster' is still needed (and also it's ok to have some predefined stuff on AST level)
And test - sure you can reuse 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.
I didn't suggest to remove the regular expression approach. In my opinion it's better to use both: first hardcoded wiping based on AST and then customizable wiping based on regexp.
988d42e
to
19d39d7
Compare
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow to remove sensitive information from the exception messages also. Resolves #41418