-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
change: Make jwt-aud config value a regular expression #4397
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,13 +193,17 @@ PostgREST has built-in validation of the `JWT audience claim <https://datatracke | |
| It works this way: | ||
|
|
||
| - If :ref:`jwt-aud` is not set (the default), PostgREST identifies with all audiences and allows the JWT for any ``aud`` claim. | ||
| - If :ref:`jwt-aud` is set to a specific audience, PostgREST will check if this audience is present in the ``aud`` claim: | ||
| - If :ref:`jwt-aud` is set, PostgREST will treat it as a regular expression and check if it matches the ``aud`` claim: | ||
|
|
||
| + If the ``aud`` value is a JSON string, it will match it to the :ref:`jwt-aud`. | ||
| + If the ``aud`` value is a JSON array of strings, it will search every element for a match. | ||
| + If the match fails or if the ``aud`` value is not a string or array of strings, then the token will be rejected with a :ref:`401 Unauthorized <pgrst303>` error. | ||
| + If the ``aud`` key **is not present** or if its value is ``null`` or ``[]``, PostgREST will interpret this token as allowed for all audiences and will complete the request. | ||
|
|
||
| Examples: | ||
| - To make PostgREST accept ``aud`` claim value from a set ``audience1``, ``audience2``, ``otheraudience``, :ref:`jwt-aud` claim should be set to ``audience1|audience2|otheraudience``. | ||
| - To make PostgREST accept ``aud`` claim value matching any ``https`` URI pointing to a host in ``example.com`` domain, :ref:`jwt-aud` claim should be set to ``https://[a-zA-Z0-9_]*\.example\.com``. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is ReDoS a possibility? What would be the performance impact of this new feature? Does the JWT cache help here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We validate
Performance impact needs to be verified. Right now we do not cache claims validation results so JWT won't help. There is also another potential performance related issue: 60c8a98 introduces syntactic validation of
Checking I've implemented it in a separate commit so that we can easily get rid of it (as nothing depends on it). Not sure if syntactical check of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@mkleczek Q: The performance hit would only happen if
So to check the above we would need new loadtests with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No. Currently (ie. in This change was supposed to change that and validate Nevertheless, I removed commit introducing this check for now.
Nope, see above. |
||
|
|
||
| .. _jwt_caching: | ||
|
|
||
| JWT Cache | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -596,14 +596,14 @@ jwt-aud | |||||
| ------- | ||||||
|
|
||||||
| =============== ================================= | ||||||
| **Type** String | ||||||
| **Type** String (must be a valid regular expression) | ||||||
| **Default** `n/a` | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Perhaps also mention in the description something like: "For example, the default value |
||||||
| **Reloadable** Y | ||||||
| **Environment** PGRST_JWT_AUD | ||||||
| **In-Database** pgrst.jwt_aud | ||||||
| =============== ================================= | ||||||
|
|
||||||
| Specifies an audience for the JWT ``aud`` claim. See :ref:`jwt_aud`. | ||||||
| Specifies a regular expression to match against the JWT ``aud`` claim. See :ref:`jwt_aud`. | ||||||
|
|
||||||
| .. _jwt-role-claim-key: | ||||||
|
|
||||||
|
|
||||||
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.
Just to wrap my head around this.
By the docs, right now we support:
But that's just the JWT and not the
jwt-audconfig.So with this change now
jwt-audcan have a list of audiences (using or expression), and the JWT can too specify a list of audiences (using JSON array).Uh oh!
There was an error while loading. Please reload this page.
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.
Yes.
(I think this sentence was not changed)
Yes.
That's what #2099 is about.