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

feat: add message when JWT secret is less than 32 characters long #3628

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

laurenceisla
Copy link
Member

Closes #3607 and addresses part of #1840.

@laurenceisla
Copy link
Member Author

laurenceisla commented Jun 29, 2024

IO tests are failing in "no-default config" tests:

jwt-secret = "c2VjdXJpdHl0aHJvdWdob2JzY3VyaXR5"
jwt-secret-is-base64 = true

This is because, while the base64 encoded jwt-secret is at least 32 chars, once it's decoded it has less than 32 characters. If I'm not mistaken, this is how it worked before, but now we're failing before starting PostgREST.

Also, could this be considered a breaking change or just a feature/fix? PostgREST will fail and won't run or reload the config (if it's already running) when the secret has less than 32 characters. I think this is better because it's mostly an admin issue with the config, not for the end user.

@laurenceisla
Copy link
Member Author

This is related to #3629. When setting, for example, set_config('pgrst.jwt_secret', 'small-secret', true);, then it will happen what's mentioned in that issue.

@wolfgangwalther
Copy link
Member

This is because, while the base64 encoded jwt-secret is at least 32 chars, once it's decoded it has less than 32 characters. If I'm not mistaken, this is how it worked before, but now we're failing before starting PostgREST.

I don't think so, the limit was enforced after decoding.

Also, could this be considered a breaking change or just a feature/fix? PostgREST will fail and won't run or reload the config (if it's already running) when the secret has less than 32 characters. I think this is better because it's mostly an admin issue with the config, not for the end user.

I'd say it's a feature/change, not a fix. PostgREST was able to serve anonymous requests with a short secret, so that technically takes some use-cases away, I'd say. It might even be a breaking change, but that's not really important at this stage, because we have merged a few things already which will require a major version bump anyway.

@laurenceisla
Copy link
Member Author

the limit was enforced after decoding.

Ah I worded it wrongly, that's what I meant to say.

It might even be a breaking change, but that's not really important at this stage, because we have merged a few things already which will require a major version bump anyway.

👍

@laurenceisla laurenceisla force-pushed the fix-jwt-max branch 2 times, most recently from 5dc3346 to 81518f6 Compare July 1, 2024 17:04
@laurenceisla laurenceisla marked this pull request as ready for review July 1, 2024 17:04
breaking change: PostgREST now fails to start or reload the config when the JWT secret is less than 32 characters long.
Comment on lines +231 to +232
decodeLoadFiles :: AppConfig -> IO (Either IOException AppConfig)
decodeLoadFiles parsedConfig = try $
Copy link
Member Author

Choose a reason for hiding this comment

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

This handles what's mentioned here #3628 (comment). According to this conversation: #3634 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Re-implement minimal length for jwt-secret
2 participants