Skip to content
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

Api Keys hashed in DB #687

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Gepardgame
Copy link
Contributor

@Gepardgame Gepardgame commented Oct 14, 2024

Description

This PR addresses #532 by changing from API Keys being stored in plain to be stored as a hash.

Will be hashed with SHA3_256. Only at creation time are once returned in plain, so it can be used. Adds a suffix to the API Key, on which the key will be retrieved from the db. Masked Key is now at the ending the suffix. Hashed Key will be also not returned in the API.

@Gepardgame Gepardgame changed the title Api Keys are now stored hashed in DB Api Keys hashed in DB Oct 14, 2024
@Gepardgame Gepardgame force-pushed the feat/hashed-api-keys branch from 14c1e97 to 98c4461 Compare October 16, 2024 05:55
Copy link
Collaborator

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. A few comments.

Also, do you have a migration strategy in mind for this? How do we deal with existing API keys that are not hashed yet?

@Gepardgame
Copy link
Contributor Author

Also, do you have a migration strategy in mind for this? How do we deal with existing API keys that are not hashed yet?

Simpliest way would be to provide a script, that simply gets every key, hashs it and update it in the DB. Would that suffice, or should it something be, there the user doesn't need to do anything? Then maybe temporarily support both and update it in the code, and after a while only support hashed ones. Or maybe add a function to the code that once gets all keys and updates them. That way you need to once upgrade to this version to get it to work later.

@Gepardgame
Copy link
Contributor Author

2 things remaining:

  1. when response contains an API Key, key value should not be showen, only if you create/regenerate it.
  2. Team Creation should no longer support creating Api key, as the Clear API Key is never returned, and therefore cannot be used.

@nscuro nscuro added this to the 3.2.0 milestone Oct 21, 2024
@Gepardgame Gepardgame marked this pull request as draft October 24, 2024 11:23
@Gepardgame Gepardgame marked this pull request as ready for review November 27, 2024 06:56
@Gepardgame
Copy link
Contributor Author

@nscuro In Team Creation its no longer possible to create an api Key, which was never even used in DT.
About returning the key or not, that qould need to be done in DT, and the upgrading as well, cause we don't have upgrade scripts in ALpine directly, but only in DT then, as far as I know

@Gepardgame Gepardgame requested a review from nscuro November 27, 2024 07:14
@Gepardgame Gepardgame force-pushed the feat/hashed-api-keys branch 2 times, most recently from 2bf9b9d to 0c46cd1 Compare December 3, 2024 10:47
Only at creation are once returned in plain

Signed-off-by: Thomas Schauer-Köckeis <[email protected]>
Now changing API Key after creation should also work

Signed-off-by: Thomas Schauer-Köckeis <[email protected]>
create apiKey

Signed-off-by: Thomas Schauer-Koeckeis <[email protected]>
Signed-off-by: Thomas Schauer-Koeckeis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants