-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore: New environment variable obfuscation functionality #355
base: main
Are you sure you want to change the base?
Conversation
c913faa
to
39ceb12
Compare
Signed-off-by: Matthias Glastra <[email protected]>
39ceb12
to
790470e
Compare
@jkjell would you be able to do a review? |
attestation/environment/obfuscate.go
Outdated
return map[string]struct{}{ | ||
"*_TOKEN": {}, | ||
"SECRET_*": {}, | ||
"*_API_KEY": {}, | ||
"*_PASSWORD": {}, | ||
"*_JWT": {}, | ||
} |
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.
Maybe I'm missing some context here, but for code maintainability, my suggestion would be:
Detach the
go-witness/attestation/environment/blocklist.go
Lines 19 to 25 in 790470e
return map[string]struct{}{ | |
"AWS_ACCESS_KEY_ID": {}, | |
"AWS_SECRET_ACCESS_KEY": {}, | |
"AMAZON_AWS_ACCESS_KEY_ID": {}, | |
"AMAZON_AWS_SECRET_ACCESS_KEY": {}, | |
"ALGOLIA_API_KEY": {}, | |
"AZURE_CLIENT_ID": {}, |
(...)
from the DefaultBlockList
and add it to a specific separated file, i.e., sensitive_env_vars.go
, and use it for blocklist and obfuscate.
I would also extend this list to have the wildcards you added here.
About the wildcards, I think we should not use *_<WORD>
but *<WORD>
as it may not start with a prefix i.e. NAME_TOKEN
but TOKEN_NAME
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.
Context: Currently the block list completely removes those variables. The obfuscate will keep them but hide the content.
I am not sure if combining those lists is a good option in that case.
About the wildcard. I can understand. I do think that this obfuscate defaults is just a 'primary' list. And users might need to extend it for their own cases. Actually many of the items in the block list 'could' be removed and be covered by the obfuscate.
As I am thinking about this now it might still be very valuable to extend the blocklist. Because you do not want to expose to consumers where you are using a specific token. 🤔 The obfuscate is basically a stop gap solution. So maybe you are right and we should obfuscate any *<WORD>*
like *TOKEN*
.
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 am not sure if combining those lists is a good option in that case.
I don't understand why the source list of blocking and obfuscating should be different, as both just have different behaviors when removing or obfuscating the data.
In other words. As a user, I expect them to differ on the "remove"/"obfuscate" my environment variables.
As a user, I shouldn't need to understand that actions uses different criteria
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.
True. Obfuscate now is complementary to the blocklist without changing the current behavior. But in the end it would be better to use a flag that says e.g. "block and obfuscate by default".
Co-authored-by: Kairo Araujo <[email protected]> Signed-off-by: Matthias Glastra <[email protected]>
With this latest change I go from default blocking behavior to default obfuscate behavior. And the default list Introducing several arguments:
I believe that as we are still in the v0.x.y versions it should not be a problem to change the default @mikhailswift I need a bit of help on the |
Adding flags to behave blocking or obfuscating and adding new keys. Signed-off-by: Matthias Glastra <[email protected]>
9ec0ae2
to
1e45a7b
Compare
Forget the part about the |
95f22f3
to
da61a58
Compare
Signed-off-by: Matthias Glastra <[email protected]>
da61a58
to
b33029c
Compare
My last changes fixed the tests. I also renamed block to filter. Which seems to me that it is a much more logical. |
ef6a908
to
7265fbb
Compare
Signed-off-by: Matthias Glastra <[email protected]>
7265fbb
to
aedf429
Compare
What this PR does / why we need it
Implementing obfuscation of environment variables. Capturing secret values like tokens and api keys is a security risk and attestation should not hold that information. Although it is important to know which values are used to adjust the behavior for scripts.
Which issue(s) this PR fixes (optional)
Partially fixes #275
Acceptance Criteria Met
Special notes for your reviewer:
The default list can be extended with more variable that should be hidden. This can be extended in the cli with flags that add more items to the list. I think that the default obfuscation list does not need to be 'overridden' but only needs extension.
It also obfuscates based on glob. So if an key would be added like
REASON_*
it will pick up allREASON_TEST
etc. variables. There are a few glob keys that are added by default.