-
Notifications
You must be signed in to change notification settings - Fork 75
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
Team API Keys & Access Token management with hashing #295
Team API Keys & Access Token management with hashing #295
Conversation
4d386af
to
3352161
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.
LGTM, although—not to be that guy—but needs tests.
packages/api/internal/auth/key.go
Outdated
suffixLength := keySuffixLength | ||
|
||
lastFour := value[len(value)-suffixLength:] |
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.
- is it always "the last four" ? in which case maybe being more explicit about that expected count in the naming of hardcoded values.
- boring but good idea to apply defensive programming for critical areas like this, e.g. validate the length of
value
(even if expected to be sha sum) beforehand to avoid a negative value after subtraction.
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.
is it always "the last four" ?
Right now that's how we've defined the masked API key to look like. It doesn't have any real effect on the keys though, this is just for the user. What would be your proposal for the renaming?
boring but good idea to apply defensive programming
Okay, I will add 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.
ohh, i see where my confusion stemmed from. The term "mask" for me has another meaning in the byte manipulation/crypto world. To be clearer i would probably renamed this as "hiding"
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.
do you mean HideKey? I was just following the naming convention we've already had there
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.
yeah HideKey
is clearer IMO
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 would keep the MaskKey as I think the mask is actually used to do exactly this action (based on what I've found)
49dbc33
to
608700d
Compare
688c036
to
13a4fd0
Compare
Added unit and integration tests |
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 missing logs, at least for error and info in the end of an successful interaction as deleting an api key
all http endpoints are logged with their results, if it is an internal error, we log also the reason (sendAPIStoreError) |
Changes
Hashing
Hashing of Team API Keys and Access Tokens is done using sha256. It should be okay, as we have already 160bits secure API Keys/Token generation + API Keys are "confidential data at rest". Hash is only generated from the key part, key/access_token prefix is excluded. It uses bytes array directly.
Hashes are stored in the format:
"$sha256$%hash"
DB Changes
Access Tokens
Team API Keys
Endpoints
Why to move API Key managent to the Infra API