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.

Sign up for GitHub

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

Jump to bottom

feat: do not store clear api keys in internal storage #2555

Merged
merged 16 commits into from
Nov 7, 2024
Merged

feat: do not store clear api keys in internal storage #2555

merged 16 commits into from
Nov 7, 2024

Conversation

Juiced66
Copy link
Contributor

@Juiced66 Juiced66 commented Oct 15, 2024 •
edited
Loading

KUZSUPPORT-68

Goals:

  • remove the seed from internal storage in case we have secret provided by config ✔️
    • removed _persistSecret method to use a _initSecret that will load secret at backend start
    • set some changes in TokenRepository init method to detect seed changes and invalidate all tokens
    • refactor validation errors throw order to avoid sending expired if token is invalid && expired (not fully sure it is possible but hey, costs nothing)
    • Warn users at start when using generated seed image
    • add authToken config support because jwt is deprecated but authToken is not usable
    • Updated tests to reflect new uses cases.
    • remove test _getSecret wich is not necessary anymore.
    • add documentation warning to encourage use a custom seed when deploying to production using ENV
  • do not store clear api keys in internal storage ✔️
    • ensure no breaking changes
    • ensure old api keys still works

review point of interest :

  • I decided to remove apikeys from cache at it was stored in clear as in the database. It could be kept using fingerprint as a part of the key instead of api key itself ? May need some refactor

@Juiced66 Juiced66 self-assigned this Oct 15, 2024
@Juiced66 Juiced66 changed the base branch from master to 2-dev October 15, 2024 07:58
@Juiced66 Juiced66 changed the title Kzlsupport 68 api keys security feat : do not store clear api keys in internal storage Oct 15, 2024
@Juiced66 Juiced66 marked this pull request as draft October 15, 2024 08:04
@Juiced66 Juiced66 force-pushed the KZLSUPPORT-68-api-keys-security branch from 1d0cea9 to c9cb2b8 Compare October 30, 2024 11:00
@Juiced66 Juiced66 force-pushed the KZLSUPPORT-68-api-keys-security branch from c41e4bf to f17c37f Compare October 30, 2024 11:44
@Juiced66 Juiced66 marked this pull request as ready for review October 31, 2024 08:20
fmauNeko
fmauNeko requested changes Oct 31, 2024
View reviewed changes
lib/core/security/tokenRepository.ts Show resolved Hide resolved
lib/kuzzle/internalIndexHandler.js Outdated Show resolved Hide resolved
@Juiced66 Juiced66 changed the title feat : do not store clear api keys in internal storage feat: do not store clear api keys in internal storage Oct 31, 2024
etrousset
etrousset requested changes Nov 5, 2024
View reviewed changes
Copy link
Member

@etrousset etrousset left a comment

Choose a reason for hiding this comment

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

Kuzzle need to log a big WARNING message when it uses a seed that is stored in ES to informa that it's ok on dev environements, but NEVER on production environement

@Juiced66
Copy link
Contributor Author

Juiced66 commented Nov 6, 2024

I'll pass it to draft to let time to @rolljee to do manuals test to try finding side effects

@Juiced66 Juiced66 marked this pull request as draft November 6, 2024 08:17
rolljee
rolljee requested changes Nov 6, 2024
View reviewed changes
doc/2/guides/getting-started/deploy-your-application/index.md Outdated Show resolved Hide resolved
lib/core/security/tokenRepository.ts Show resolved Hide resolved
lib/core/security/tokenRepository.ts Show resolved Hide resolved
lib/core/security/tokenRepository.ts Show resolved Hide resolved
alexandrebouthinon
alexandrebouthinon approved these changes Nov 6, 2024
View reviewed changes
Copy link
Member

@alexandrebouthinon alexandrebouthinon left a comment

Choose a reason for hiding this comment

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

Just tested on a local environment with already created API Keys by deleting the doc containing seed and pass it have env variable. Worked like a charm

@Juiced66 Juiced66 marked this pull request as ready for review November 7, 2024 11:28
fmauNeko
fmauNeko requested changes Nov 7, 2024
View reviewed changes
lib/kuzzle/internalIndexHandler.js Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@Juiced66 Juiced66 merged commit c465450 into 2-dev Nov 7, 2024
53 checks passed
@Juiced66 Juiced66 deleted the KZLSUPPORT-68-api-keys-security branch November 7, 2024 13:44
@kuzzle
Copy link
Contributor

kuzzle commented Nov 7, 2024

🎉 This PR is included in version 2.34.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kuzzle
Copy link
Contributor

kuzzle commented Nov 7, 2024

🎉 This PR is included in version 2.34.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kuzzle kuzzle added the released This issue/pull request has been released. label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etrousset etrousset etrousset approved these changes

@rolljee rolljee rolljee approved these changes

@alexandrebouthinon alexandrebouthinon alexandrebouthinon approved these changes

@sebtiz13 sebtiz13 sebtiz13 approved these changes

@fmauNeko fmauNeko fmauNeko approved these changes

Assignees

@Juiced66 Juiced66

Labels
released on @beta released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Juiced66 @kuzzle @fmauNeko @sebtiz13 @alexandrebouthinon @rolljee @etrousset