-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Handle unmodified sensitived fields when updating connections #53943
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
Conversation
You just found a bug. I was able to reproduce on main, seems to happen when 'extra_json' is a str. I created the issue: #53963, more info there. |
@shubhamraj-git I was able to reproduce on main, I just opened an issue there: |
5542e33
to
1499d29
Compare
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 addressing my concerns and for the follow up issues, lgtm
Backport failed to create: v3-0-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker 0abcfdf v3-0-test This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue |
Manual backport #53973 |
For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer
For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer
For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer
For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer
…#53943) * Fix redacted values editing * Small improvements * Small adjustments * Update UI and fix some errors * Address PR comments
For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer
…53977) * Allow secrets redact function to have different redaction than `***` For logs, using `***` is fine, but as part of the changes introduced in #53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer * Deal with OpenLineage subclassing SecretsMasker class
…#53943) * Fix redacted values editing * Small improvements * Small adjustments * Update UI and fix some errors * Address PR comments
…pache#53977) * Allow secrets redact function to have different redaction than `***` For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer * Deal with OpenLineage subclassing SecretsMasker class
…#53943) * Fix redacted values editing * Small improvements * Small adjustments * Update UI and fix some errors * Address PR comments
…pache#53977) * Allow secrets redact function to have different redaction than `***` For logs, using `***` is fine, but as part of the changes introduced in apache#53943 we decided it might be nice to use an even-less-frequently-appearing thing than `***` so we can detect modified secrets. This gives us the ability to do that at the redaction layer * Deal with OpenLineage subclassing SecretsMasker class
closes: #52301
closes: #53753
How it works:
For connection, 'password' and 'extras' are merged with their original value when doing an update. The function works similarly to the
redact
function, it will recursively handle all sort of data types and detect sensitive values that were not modified in the 'new_value' and then restore the value from the unredacted previous value.See the warning note:


After adding a key 'new_key' to the extra and saving this is what we get in the UI:

And from the CLI we can see that both password, and extra redacted field were preserved:

The only downside is that we cannot 'insert' a real '***' for redacted value because this is how we detect that the value didn't change. I think it's a fair limitation, '***' shouldn't never be a valid value for a sensitive field anyway. @ashb is working on a follow up PR to instead use unicode characters that looks like '***' but are not, to make it even less likely that it will be blocking for users. (They would have to chose a very weird value for their secret).
Another example, it also handle well arrays: