Patterns support for allowed subjects by the JWT realm #102426

albertzaharovits · 2023-11-21T17:05:44Z

This adds support for allowing JWT token sub claims with Lucene patterns and wildcards,
by introducing a new JWT realm setting allowed_subject_patterns that can be used
alongside the exist allowed_subjects realm setting.

elasticsearchmachine · 2023-11-27T12:22:16Z

Pinging @elastic/es-security (Team:Security)

elasticsearchmachine · 2023-11-27T12:22:16Z

Hi @albertzaharovits, I've created a changelog YAML for you.

slobodanadamovic

Overall functional changes are looking good. I left a few non-blocking comments for the code change. 👍

I still need to review tests and documentation changes. Just wanted to leave the initial feedback first.

...curity/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtStringClaimValidator.java

slobodanadamovic · 2023-11-28T09:17:58Z

...curity/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtStringClaimValidator.java

    ) {
        this.claimName = claimName;
-        this.fallbackClaimNames = fallbackClaimNames;
-        this.allowedClaimValues = allowedClaimValues;


nit: The allowedClaimValues is not nullable anymore and will cause NPE when used below. We do validate it before constructing the validator, but maybe we should assert it as well. Same is for newly added allowedClaimValuePatterns.

I pushed the asserts, but in principal I prefer we annotate nullable parameters and assume the others cannot be null.

slobodanadamovic · 2023-11-28T09:34:07Z

...curity/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtStringClaimValidator.java

@@ -63,16 +85,14 @@ public void validate(JWSHeader jwsHeader, JWTClaimsSet jwtClaimsSet) {
        if (claimValues == null) {
            throw new IllegalArgumentException("missing required string claim [" + fallbackableClaim + "]");
        }
-
-        if (allowedClaimValues != null && false == claimValues.stream().anyMatch(allowedClaimValues::contains)) {
+        if (false == claimValues.stream().anyMatch(allowedClaimValuesPredicate)) {


I like that we don't accept null anymore, but the null here was nicely short-circuiting the check to mean allow all and avoid any checks. Maybe we could do something similar and introduce a new private constructor which instead of allowedClaimValues and allowedClaimValuePatterns collections simply accepts allowedClaimValuesPredicate . This way we can create ALLOW_ALL_SUBJECTS which always returns true and thus avoid string matching against *.

On a second thought, this would not solve it completely. The proper way to short-circuit it would be if allowedClaimValuesPredicate accepted a list of strings to check.

The * used in the ALLOW_ALL_SUBJECTS benefits of some short-circuting of sorts:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Automatons.java

Lines 214 to 216 in 5c26cf0

} else if (pattern.equals("*")) {

return MATCH_ALL;

} else {

In the JwtStringClaimValidator constructor I could short-circuit the predicate building logic if either one of the "allowed" lists is empty, but I think that would complicate the logic for very little benefit (e.g. I expect the JVM will figure out that set stays empty and expedite the contains check).

Wasn't aware of a short-circuit in Automatons. Agreed, let's keep it simple and optimize if we need to.

slobodanadamovic · 2023-11-28T11:49:52Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtAuthenticator.java

+        );
+    }
+
+    private static void validateAllowedSubjectsSettings(RealmConfig realmConfig) {


optional: I don't feel strongly, but we could potentially move this settings validation to JwtRealmSetting class and implement a custom Setting.Validator for both allowed_subjects and allowed_subject_patterns.

Good point. I've pushed f095252 , but the "richness" of the error description decreased because it's not possible to tell if the dependent setting (of some other setting) is specified or not (validation has access to the default value in case it's not specified).
So now the error message only tells you that one of allowed_subjects or allowed_subject_patterns must be specified and not be empty.

docs/reference/security/authentication/jwt-realm.asciidoc

slobodanadamovic · 2023-11-28T15:09:25Z

docs/reference/security/authentication/jwt-realm.asciidoc

+`allowed_subject_patterns`::
+Analogous to `allowed_subjects` but it accepts a list of <<regexp-syntax,Lucene regexp>>
+and wildcards for the allowed JWT subjects. Wildcards use the `*` and `?` special
+characters (which are escaped by `\`) to mean "any string" and "any single character"


optional: Would be nice to include a small example (similarly like you did for Lucene regex) that shows usage of both wildcards (including escaping).

Pushed cc62b3b .

slobodanadamovic · 2023-11-28T16:39:19Z

...y/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtStringClaimValidatorTests.java

@@ -167,6 +180,253 @@ public void testDoesNotSupportWildcardOrRegex() throws ParseException {
        }
    }



Nice job on testing.
small nit: We could potentially add tests which demonstrate that:

sub validation is case sensitive

? can be used as a single special character that must be present

an empty ("") sub is rejected

Additional test suggestion, we could adjust existing JwtRestIT integration test to randomly include allowed_subjects or allowed_subject_patterns (e.g. service_*@app?.example.com).

Pushed 59c5bd9 30cee67 2200f51 and 910a334

slobodanadamovic

LGTM 👍
Left a couple of non-blocking nits and suggestions. Feel free to address at your discretion.

Co-authored-by: Slobodan Adamović <slobodanadamovic@users.noreply.github.com>

jakelandis

LGTM

jakelandis · 2023-12-04T19:56:01Z

...curity/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtStringClaimValidator.java

+        try {
+            return Automatons.predicate(patterns);
+        } catch (Exception e) {
+            throw new SettingsException("Invalid patterns for allowed claim values for [" + claimName + "].", e);


nit: should this also include the fallback sub if it is defined ?

Maybe, if we go with also using patterns for more claims in the future. I'm not convinced for subjects only, today, it's warranted, and in any case, it's slightly messy to implement.
This error shows up when the allowed_subject_patterns is invalid, and it will show up in the logs as: "Invalid patterns for allowed claim values for [sub]." Even if the settings also contain something such as fallback_claims.sub: client_id I think the error is telling enough.

...curity/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtStringClaimValidator.java

Patterns for all

647c538

albertzaharovits self-assigned this Nov 21, 2023

elasticsearchmachine added the v8.12.0 label Nov 21, 2023

albertzaharovits added 4 commits November 22, 2023 11:57

Merge branch 'main' into JWT-regex-support-for-allowed-subject

49b7267

Merge branch 'main' into JWT-regex-support-for-allowed-subject

4df43f8

Nit

52095cb

Almost

782675e

albertzaharovits changed the title ~~Permit regex patterns for JWT realm allowed_subjects setting~~ Permit patterns for JWT realm allowed_subjects setting Nov 23, 2023

albertzaharovits changed the title ~~Permit patterns for JWT realm allowed_subjects setting~~ Patterns for JWT realm allowed_subjects setting Nov 23, 2023

albertzaharovits added the WIP label Nov 23, 2023

albertzaharovits added 15 commits November 23, 2023 14:43

Invalid claim patterns test

f865b65

Merge branch 'main' into JWT-regex-support-for-allowed-subject

6464bc7

Spotless

4f5f35d

2 settings

6837e4d

Merge branch 'main' into JWT-regex-support-for-allowed-subject

3487246

Nit

b4de833

2 settings, no tests

7029b61

Some tests

318ad11

More tests for JwtStringClaimValidator

12e13ab

Merge branch 'main' into JWT-regex-support-for-allowed-subject

233f926

Done tests JwtStringClaimValidatorTests

7f6bef2

Javadoc

7dd981b

Some JwtAuthenticatorTests

cfba75e

Merge branch 'main' into JWT-regex-support-for-allowed-subject

b6a5a10

testNoAllowedSubjectInvalidSettings

cf6cfb1

albertzaharovits changed the title ~~Patterns for JWT realm allowed_subjects setting~~ Patterns for JWT realm allowed_subject_patterns setting Nov 27, 2023

albertzaharovits changed the title ~~Patterns for JWT realm allowed_subject_patterns setting~~ Patterns support for allowed subjects by the JWT realm Nov 27, 2023

albertzaharovits added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >enhancement labels Nov 27, 2023

slobodanadamovic self-requested a review November 27, 2023 09:00

albertzaharovits added >feature and removed WIP labels Nov 27, 2023

elasticsearchmachine added the Team:Security Meta label for security team label Nov 27, 2023

Update docs/changelog/102426.yaml

421bc55

slobodanadamovic reviewed Nov 28, 2023

View reviewed changes

Merge branch 'main' into JWT-regex-support-for-allowed-subject

94f1682

slobodanadamovic reviewed Nov 28, 2023

View reviewed changes

slobodanadamovic approved these changes Nov 28, 2023

View reviewed changes

albertzaharovits and others added 14 commits November 29, 2023 09:48

Changed settings validation

f095252

assert non null

9d468c0

Nit

0c31b45

Merge branch 'main' into JWT-regex-support-for-allowed-subject

0ddd91b

Update docs/reference/security/authentication/jwt-realm.asciidoc

e81eaf4

Co-authored-by: Slobodan Adamović <slobodanadamovic@users.noreply.github.com>

Update docs/reference/security/authentication/jwt-realm.asciidoc

40962d9

Co-authored-by: Slobodan Adamović <slobodanadamovic@users.noreply.github.com>

wildcard example

cc62b3b

Spotless

8e36fef

testEmptyAllowedSubjectNotAllowed

2200f51

Validation failure for invalid match

59c5bd9

Merge branch 'main' into JWT-regex-support-for-allowed-subject

4239b2e

testWildcardAndRegexMatchingClaimValues

30cee67

JwtRestIT with allowed_subject_patterns

910a334

Merge branch 'main' into JWT-regex-support-for-allowed-subject

f85a23f

jakelandis approved these changes Dec 4, 2023

View reviewed changes

albertzaharovits added 2 commits December 5, 2023 11:06

Merge branch 'main' into JWT-regex-support-for-allowed-subject

8dbd9a3

Replace claimValues.stream()

552a6cb

albertzaharovits merged commit f64bb49 into elastic:main Dec 5, 2023
19 checks passed

albertzaharovits deleted the JWT-regex-support-for-allowed-subject branch December 5, 2023 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patterns support for allowed subjects by the JWT realm #102426

Patterns support for allowed subjects by the JWT realm #102426

albertzaharovits commented Nov 21, 2023 •

edited

elasticsearchmachine commented Nov 27, 2023

elasticsearchmachine commented Nov 27, 2023

slobodanadamovic left a comment •

edited

slobodanadamovic Nov 28, 2023

albertzaharovits Nov 29, 2023

slobodanadamovic Nov 28, 2023

slobodanadamovic Nov 28, 2023

albertzaharovits Nov 29, 2023

slobodanadamovic Nov 29, 2023

slobodanadamovic Nov 28, 2023 •

edited

albertzaharovits Nov 29, 2023

slobodanadamovic Nov 28, 2023 •

edited

albertzaharovits Nov 29, 2023

slobodanadamovic Nov 28, 2023

slobodanadamovic Nov 28, 2023

albertzaharovits Nov 29, 2023

slobodanadamovic left a comment

jakelandis left a comment

jakelandis Dec 4, 2023

albertzaharovits Dec 5, 2023 •

edited

		@@ -167,6 +180,253 @@ public void testDoesNotSupportWildcardOrRegex() throws ParseException {
		}
		}

Patterns support for allowed subjects by the JWT realm #102426

Patterns support for allowed subjects by the JWT realm #102426

Conversation

albertzaharovits commented Nov 21, 2023 • edited

elasticsearchmachine commented Nov 27, 2023

elasticsearchmachine commented Nov 27, 2023

slobodanadamovic left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slobodanadamovic Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slobodanadamovic Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slobodanadamovic left a comment

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Dec 5, 2023 • edited

Choose a reason for hiding this comment

albertzaharovits commented Nov 21, 2023 •

edited

slobodanadamovic left a comment •

edited

slobodanadamovic Nov 28, 2023 •

edited

slobodanadamovic Nov 28, 2023 •

edited

albertzaharovits Dec 5, 2023 •

edited