Fixing the API key Visibility issue in debug mode #3330
Conversation
Signed-off-by: PredictiveManish <manisht0914@gmail.com>
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
Thanks for the contribution @PredictiveManish! Posted some feedback inline - overall this is a great improvement to the codebase. Most of my substantial feedback is honestly questions for @sgoggins etc about how best to slightly reorganize things.
augur/tasks/util/random_key_auth.py
Outdated
| def key_fingerprint(key: str, length: int = 12) -> str: | ||
| """Return a short non-reversible fingerprint of the key for correlation.""" | ||
| h = hashlib.sha256(key.encode("utf-8")).hexdigest() | ||
| return h[:length] |
There was a problem hiding this comment.
i think in the original issue I was intending to propose either blanking out the middle or hashing. I think blanking out the middle is better overall since current users are probably more used to seeing their keys in this form (ie.. when they input them or use the CLI to check key expiry). I'll wait to see what the other maintainers think, but I suspect the value of the hashing isnt yet worth the additional dependency
There was a problem hiding this comment.
That’s a fair point. I thought that having a short hash helps correlate which key was used without exposing any part of it (useful in environments where masking might still reveal too much).
But I agree the masking approach is simpler and more familiar.
Happy to revert to that if maintainers prefer consistency with existing CLI behavior.
There was a problem hiding this comment.
@MoralCode makes a really good point here. I like hashing from a security perspective ... if we leave the first 4 and last 4 in place, and hash the middle part it is WAY more secure when logging, and also allows us to debug key issues as they related to particular keys.
Specifically, keys go bad for one reason or another. If we preserve no part of the key it is more secure, but then quite difficult to identify the bad keys.
One possible compromise (and replacing the middle seems like a good one already) would be to include the id of the key from the augur_operations.worker_oauth table. Such as:
oauth_id name consumer_key consumer_secret access_token access_token_secret repo_directory platform
20004 x@x.com 0 0 ghp_NOTREAL 0 github
21000 doge@x.com 0 0 ghp_NOTREAL 0 github
20005 lessla@x.com 0 0 ghp_NOTREAL 0 githubso ... 20004, 21000, and 20005 would be included in the log ...
But that "feels" like more work than removing the middle. I'm ok with either.
There was a problem hiding this comment.
removed key_fingerprint() as of now, if it requires will add it later as per advancement of repo!
MoralCode
added
the
ready
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
I made a few suggestions that may require some small changes. The pyproject.toml question is probably the most significant question.
augur/tasks/util/random_key_auth.py
Outdated
| def key_fingerprint(key: str, length: int = 12) -> str: | ||
| """Return a short non-reversible fingerprint of the key for correlation.""" | ||
| h = hashlib.sha256(key.encode("utf-8")).hexdigest() | ||
| return h[:length] |
There was a problem hiding this comment.
@MoralCode makes a really good point here. I like hashing from a security perspective ... if we leave the first 4 and last 4 in place, and hash the middle part it is WAY more secure when logging, and also allows us to debug key issues as they related to particular keys.
Specifically, keys go bad for one reason or another. If we preserve no part of the key it is more secure, but then quite difficult to identify the bad keys.
One possible compromise (and replacing the middle seems like a good one already) would be to include the id of the key from the augur_operations.worker_oauth table. Such as:
oauth_id name consumer_key consumer_secret access_token access_token_secret repo_directory platform
20004 x@x.com 0 0 ghp_NOTREAL 0 github
21000 doge@x.com 0 0 ghp_NOTREAL 0 github
20005 lessla@x.com 0 0 ghp_NOTREAL 0 githubso ... 20004, 21000, and 20005 would be included in the log ...
But that "feels" like more work than removing the middle. I'm ok with either.
MoralCode
changed the title
Signed-off-by: PredictiveManish <manisht0914@gmail.com>
Signed-off-by: PredictiveManish <manisht0914@gmail.com>
Signed-off-by: PredictiveManish <manisht0914@gmail.com>
3 participants
Description
This PR Secures the API Key logging in
RandomKeyAuth, improves the security by ensuring the keys never logged in plaintext. Specifically:mask_key()which obfuscate API keys in logs, by only showing first 6 and last 3 char and in between there're asterisks.
key_fingerprint()to log a hash which is short and non-reversibleThis PR fixes #3328
Signed commits