change: Make jwt-aud config value a regular expression

[mkleczek]

This change adds flexibility to aud claim validation. jwt-aud configuration property can now be specified as a regular expression. For example, it is now possible to

• configure multiple acceptable aud values with '|' regex operator, eg: 'audience1|audience2|audience3'
• accept any audience from a particular domain, eg: 'https://[a-z0-9]*\.example\.com'

Resolves #2099

• Update documentation
• Update changelog

Subsequent #4419 changes default value of jwt-aud to address #4134

[steve-chavez]

@mkleczek Could you add a summary of the PR? From reading the issues I'm not sure what's the final design here. This would make it easier to review.

Also, is the breaking change avoidable by changing the default configuration?

[mkleczek]

@mkleczek Could you add a summary of the PR? From reading the issues I'm not sure what's the final design here. This would make it easier to review.

Done.

Also, is the breaking change avoidable by changing the default configuration?

Yes.

I've decided to set default jwt-aud to .* (accept anything) for now. That means the change no longer addresses #4134 but is not breaking.

[laurenceisla]

Suggested change

	**Default** `n/a`
	**Default** .*

Perhaps also mention in the description something like: "For example, the default value .* matches any aud claim."?

[steve-chavez]

I've decided to set default jwt-aud to .* (accept anything) for now. That means the change no longer addresses #4134 but is not breaking.

@mkleczek Just to clarify, then this PR would not be a change: (as per the title), just a new feature right?

[steve-chavez]

Just to wrap my head around this.

By the docs, right now we support:

If the aud value is a JSON array of strings, it will search every element for a match.

But that's just the JWT and not the jwt-aud config.

So with this change now jwt-aud can have a list of audiences (using or expression), and the JWT can too specify a list of audiences (using JSON array).

[mkleczek]

Just to wrap my head around this.

By the docs, right now we support:

If the aud value is a JSON array of strings, it will search every element for a match.

But that's just the JWT and not the jwt-aud config.

Yes.
(I think this sentence was not changed)

So with this change now jwt-aud can have a list of audiences (using or expression), and the JWT can too specify a list of audiences (using JSON array).

Yes.
That's what #2099 is about.

[steve-chavez]

Is ReDoS a possibility?

What would be the performance impact of this new feature? Does the JWT cache help here?

[mkleczek]

Is ReDoS a possibility?

We validate aud only after JWT authentication (ie. we verify JWT signature first). So it is only possible if an attacker can issue tokens.
Short answer: IMHO no

What would be the performance impact of this new feature? Does the JWT cache help here?

Performance impact needs to be verified.

Right now we do not cache claims validation results so JWT won't help.
We can change it but that will require reloading JWT cache not only whenjwt-secret changes but also when jwt-aud is modified. I would postpone this until we are sure regex matching really affects performance.

There is also another potential performance related issue:

60c8a98 introduces syntactic validation of aud claim. Before, as implemented by @taimoorzaeem in #4140, we didn't really check if aud claim is a valid StringOrURI - we only verified that jwt-aud config is syntactically valid. So:

• we did not validate aud claims syntactically when jwt-aud was not set
• we returned wrong error message when jwt-aud was set and aud claim was invalid URI: instead of "aud syntax error" we returned "JWT not in audience" (that's disputable as both are valid rejection reasons)
• in case of jwt-aud config being a regular expression we cannot really check if it is a valid StringOrURI anymore (the main reason to implement it in this PR)

Checking aud claim syntactically is additional work to perform upon every request so will affect performance (we don't know if noticeably).

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 aud claim is that important anyway. OTOH caching aud claims validation in JWT cache would help with this case as well.

[steve-chavez]

There is also another potential performance related issue:
60c8a98 introduces syntactic validation of aud claim.
(Note: the above was released on v13.0.4)

@mkleczek Q: The performance hit would only happen if jwt-aud is set right? And with this new PR, jwt-aud will always be set hence the perf impact will happen for every installation (the jwt-aud='.*' regex check will always be done).

Performance impact needs to be verified.
Checking aud claim syntactically is additional work to perform upon every request so will affect performance (we don't know if noticeably).

So to check the above we would need new loadtests with the jwt-aud enabled right?

[mkleczek]

There is also another potential performance related issue:
60c8a98 introduces syntactic validation of aud claim.
(Note: the above was released on v13.0.4)

@mkleczek Q: The performance hit would only happen if jwt-aud is set right? And with this new PR, jwt-aud will always be set hence the perf impact will happen for every installation (the jwt-aud='.*' regex check will always be done).

No. Currently (ie. in main) we don't check aud claim syntactically at all. We only syntactically check jwt-aud config. But we use equality check to validate aud claim hence we don't accept syntactically invalid auds if jwt-aud is set but accept invalid auds if jwt-aud is not set.

This change was supposed to change that and validate auds syntactically always, before even checking them against jwt-aud config.

Nevertheless, I removed commit introducing this check for now.

Performance impact needs to be verified.
Checking aud claim syntactically is additional work to perform upon every request so will affect performance (we don't know if noticeably).

So to check the above we would need new loadtests with the jwt-aud enabled right?

Nope, see above.