search

PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
23.27k stars 1.02k forks source link

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

Closed laurenceisla closed 2 months ago

laurenceisla commented 3 months ago

Closes #3607 and addresses part of #1840.

laurenceisla commented 3 months ago

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

https://github.com/PostgREST/postgrest/blob/40ed349bba160247ee2d10b7a48e836077a5ac7e/test/io/configs/no-defaults.config#L24-L25

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

:+1: