[#375] Add optional strict check to `aud` member of introspection endpoint response #378

MartinFlores751 · 2024-12-02T20:43:02Z

This has the changes made from #376, with the suggestions of that PR applied in this PR.

Reject access tokens without aud claim

korydraughn · 2024-12-02T20:57:08Z

core/src/openid.cpp

@@ -155,6 +155,15 @@ namespace irods::http::openid
 				return std::nullopt;
 			}
 		}
+		// Some IAM servers (e.g. keycloak) could be set up
+		// to exclude `aud' from a bearer token payload


Please use the same convention for highlighting specific properties in comments.

`aud' -> 'aud' or "aud"

korydraughn · 2024-12-02T20:59:54Z

core/src/openid.cpp

@@ -155,6 +155,15 @@ namespace irods::http::openid
 				return std::nullopt;
 			}
 		}
+		// Some IAM servers (e.g. keycloak) could be set up
+		// to exclude `aud' from a bearer token payload


Let's add a period to the end of this sentence.

korydraughn · 2024-12-02T21:01:36Z

README.md

@@ -257,6 +257,10 @@ Notice how some of the configuration values are wrapped in angle brackets (e.g.
                // If provided, it MUST be base64url encoded.
                "nonstandard_id_token_secret": "xxxxxxxxxxxxxxx",

+                // Determines if the aud member is required in the


Wrap aud in double quotes.

korydraughn · 2024-12-02T21:02:55Z

README.md

+                // Determines if the aud member is required in the
+                // response from the introspection endpoint.
+                "strict_introspection_endpoint_aud": false,


Please explain to me what this does?

core/src/openid.cpp

trel · 2024-12-02T22:09:08Z

hmmm....

strict_introspection_endpoint_aud

let's also consider/discuss...

strict_validation_of_aud
enforce_validity_of_aud

and it occurs to me... is it just the presence of the aud claim thai is being required? or is it also that we need to make sure the value of the claim matches something in particular as well?

require_aud_claim_in_access_token
require_aud_claim_from_introspection_endpoint

alanking · 2024-12-02T22:39:15Z

and it occurs to me... is it just the presence of the aud claim thai is being required? or is it also that we need to make sure the value of the claim matches something in particular as well?

And to dog-pile onto that... are there other members which we might want to make required? In addition to aud, the members iss, exp, sub, and iat are marked as required in RFC 9068, but all of these are marked as optional in RFC 7519. In our code, we are handling iss as an optional member of the JWT as well (didn't see exp, sub, or iat in the explicitly optional section). Should we also add an option for iss? Or could it be a whole list?

Maybe this is a naive suggestion, so please shoot down as appropriate.

MartinFlores751 · 2024-12-02T22:43:03Z

hmmm....

strict_introspection_endpoint_aud

let's also consider/discuss...
* `strict_validation_of_aud`

* `enforce_validity_of_aud`

Yes, more naming suggestions please!

* `require_aud_claim_from_introspection_endpoint`

While this was lower down, going to group with the rest up here.

Quick wording suggestion (JSON vs. JWT):

- require_aud_claim_from_introspection_endpoint
+ require_aud_member_from_introspection_endpoint

and it occurs to me... is it just the presence of the aud claim thai is being required? or is it also that we need to make sure the value of the claim matches something in particular as well?

In this PR for the introspection endpoint, by requiring the aud response to be present, we are also requiring that our client_id is in the aud member. This behavior is expected, as access tokens should only be used if we are part of the intended audience.

The checks we do are as follows:

irods_client_http_api/core/src/openid.cpp

Lines 133 to 157 in 3824a09

    
           // We should be part of the intended audience for the access token. 
        
           if (const auto aud_iter{json_res.find("aud")}; aud_iter != std::end(json_res)) { 
        
           	logging::trace("{}: Attempting [aud] validation.", __func__); 
        
           	const auto& client_id{irods::http::globals::oidc_configuration().at("client_id")}; 
        
           	const auto& aud{*aud_iter}; 
        
           	if (aud.is_array()) { 
        
           		auto aud_list_iter{aud.items()}; 
        
           		auto client_id_in_list{std::any_of( 
        
           			std::begin(aud_list_iter), std::end(aud_list_iter), [&client_id](const auto& _kvp) -> bool { 
        
           				return _kvp.value() == client_id; 
        
           			})}; 
        
           		// If we are not in the list, reject the token 
        
           		if (!client_id_in_list) { 
        
           			logging::warn("{}: Could not find our [client_id] in [aud]. Validation failed.", __func__); 
        
           			return std::nullopt; 
        
           		} 
        
           	} 
        
           	else if (aud.get_ref<const std::string&>() != client_id) { 
        
           		logging::warn("{}: Could not find our [client_id] in [aud]. Validation failed.", __func__); 
        
           		return std::nullopt; 
        
           	} 
        
           }

* `require_aud_claim_in_access_token`

Don't think this name is a good fit.

While the introspection endpoint does use an access token, we should narrow the name to the endpoint. It may be confusing, as it implies we don't require aud in JWT access tokens, which we do.

The standard for the introspection endpoint and the standard for JWT Access Tokens (and ID Tokens) are different. Both the JWT Access Tokens and the ID Tokens are required to have an aud with the appropriate value (our client_id). They will be rejected otherwise.

MartinFlores751 · 2024-12-02T22:55:18Z

and it occurs to me... is it just the presence of the aud claim thai is being required? or is it also that we need to make sure the value of the claim matches something in particular as well?

And to dog-pile onto that... are there other members which we might want to make required? In addition to aud, the members iss, exp, sub, and iat are marked as required in RFC 9068, but all of these are marked as optional in RFC 7519. In our code, we are handling iss as an optional member of the JWT as well (didn't see exp, sub, or iat in the explicitly optional section). Should we also add an option for iss? Or could it be a whole list?

Maybe this is a naive suggestion, so please shoot down as appropriate.

See the "Additional context for the Introspection Endpoint" in my response: #375 (comment)

This might be relevant too, not sure: #358 (comment)

I'm not too sure we should make the other fields "opt-in require". The authorization server should be doing these checks. Requiring these would mean you need to make sure these members are returned. This means you would need to change the server configuration appropriately. Partial quote of myself from the issue:

if this requires appropriate configuration of the authorization server for this, would the authorization server not handle the check itself?

ll4strw · 2024-12-03T11:30:08Z

Thanks everybody for looking into this.
Just to make sure we are on the same page:

Handle missing aud claim in access token #375 exposed that the absence of an aud claim in an access token could let a user bypass the aud check of the json response returned by the token introspection endpoint. This happened after a local JWT validation failure

validate_using_local_validation: JWT verification failed [decoded JWT is missing required claim(s)].

and a successive call to the token introspection endpoint. Clearly, if an access token does not present an aud claim, then the token introspection json response will follow suit.

[#375] Handle missing aud claim in access token #376 intended to deny access to bearer tokens missing an aud claim by simply restoring an aud check in the token introspection response. With the changes proposed, an aud-deficient token would produce the following log and be rejected

[2024-12-03 10:34:37.696] [P:8173] [error] [T:8173] validate_using_local_validation: JWT verification failed [decoded JWT is missing required claim(s)].
[2024-12-03 10:34:37.719] [P:8173] [debug] [T:8173] hit_introspection_endpoint: Received the following response: [{"exp":1733218777,"iat":1733218477,"jti":"99d6ac87-628f-481f-a1a5-7257595f138f","iss":"http://localhost:8080/realms/iRODS","typ":"Bearer","azp":"iRODSMobile","sid":"dde4c23e-f9b3-4e64-99db-c363d99dc96c","acr":"1","allowed-origins":["/*","http://localhost:8100"],"realm_access":{"roles":["offline_access"]},"scope":"openid offline_access email profile","email_verified":false,"name":"llll Wow","preferred_username":"leonardo","given_name":"llll","family_name":"Wow","email":"[email protected]","client_id":"iRODSMobile","username":"leonardo","token_type":"Bearer","active":true}]
[2024-12-03 10:34:37.719] [P:8173] [warning] [T:8173] validate_using_introspection_endpoint: Bearer token payload is missing [aud]. Validation failed.

[#375] Add optional strict check to aud member of introspection endpoint response #378 proposes a new configuration parameter so that the presence of an aud claim - and the resulting aud check - is optional.

If this is the case, I am not sure that adding a new configuration parameter (require_aud_member_from_introspection_endpoint or whatever name) would make a huge difference to the end user eventually. This is because

the local JWT validation will fail regardless unless we extend the optional check there too.
the vast majority of IAM servers always include a default aud claim in their access tokens and in their introspection responses. Modifying this behavior must be done intentionally, but it would constitute bad practice I assume.

In my opinion, if the decision taken is the addition of this extra configuration parameter, I would rather choose for a generic

require_aud_presence

and apply it to both the local JWT validation and the introspection json response. This would make the story more consistent, but it is just an idea.

PS: keycloak offers huge flexibility with aud claims. It is possible to exclude an aud claim altogether (not setting it up), add it only to access tokens, to both access tokens and json introspection responses, or to add it only to json introspection responses.

MartinFlores751

From discussion in standup.

core/src/main.cpp

core/src/openid.cpp

README.md

MartinFlores751 · 2024-12-03T20:04:06Z

Thanks everybody for looking into this. Just to make sure we are on the same page:

Handle missing aud claim in access token #375 exposed that the absence of an aud claim in an access token could let a user bypass the aud check of the json response returned by the token introspection endpoint. This happened after a local JWT validation failure
validate_using_local_validation: JWT verification failed [decoded JWT is missing required claim(s)].
and a successive call to the token introspection endpoint. Clearly, if an access token does not present an aud claim, then the token introspection json response will follow suit.

Yes, the HTTP API does fall through to using the introspection endpoint if local validation fails. Perhaps this should be changed too? See #380.

[#375] Handle missing aud claim in access token #376 intended to deny access to bearer tokens missing an aud claim by simply restoring an aud check in the token introspection response. With the changes proposed, an aud-deficient token would produce the following log and be rejected

[2024-12-03 10:34:37.696] [P:8173] [error] [T:8173] validate_using_local_validation: JWT verification failed [decoded JWT is missing required claim(s)].
[2024-12-03 10:34:37.719] [P:8173] [debug] [T:8173] hit_introspection_endpoint: Received the following response: [{"exp":1733218777,"iat":1733218477,"jti":"99d6ac87-628f-481f-a1a5-7257595f138f","iss":"http://localhost:8080/realms/iRODS","typ":"Bearer","azp":"iRODSMobile","sid":"dde4c23e-f9b3-4e64-99db-c363d99dc96c","acr":"1","allowed-origins":["/*","http://localhost:8100"],"realm_access":{"roles":["offline_access"]},"scope":"openid offline_access email profile","email_verified":false,"name":"llll Wow","preferred_username":"leonardo","given_name":"llll","family_name":"Wow","email":"[email protected]","client_id":"iRODSMobile","username":"leonardo","token_type":"Bearer","active":true}]
[2024-12-03 10:34:37.719] [P:8173] [warning] [T:8173] validate_using_introspection_endpoint: Bearer token payload is missing [aud]. Validation failed.

[#375] Add optional strict check to aud member of introspection endpoint response #378 proposes a new configuration parameter so that the presence of an aud claim - and the resulting aud check - is optional.

If this is the case, I am not sure that adding a new configuration parameter (require_aud_member_from_introspection_endpoint or whatever name) would make a huge difference to the end user eventually. This is because

the local JWT validation will fail regardless unless we extend the optional check there too.

the vast majority of IAM servers always include a default aud claim in their access tokens and in their introspection responses. Modifying this behavior must be done intentionally, but it would constitute bad practice I assume.

In my opinion, if the decision taken is the addition of this extra configuration parameter, I would rather choose for a generic

require_aud_presence

and apply it to both the local JWT validation and the introspection json response. This would make the story more consistent, but it is just an idea.

While it may make the story more consistent to have a generic require_aud_presence, we do want aud to be validated. What we don't know yet is just how OpenID Providers aside from Keycloak interpret the introspection endpoint RFC. Until we more thoroughly understand how OpenID Providers treat the introspection endpoint, we think it's best to provide an option for this specific case.

If there is interest in having strict enforcement of this member, then it may make sense to have strict enforcement of the other members (see #381). Later on, these options may be enforced by default.

PS: keycloak offers huge flexibility with aud claims. It is possible to exclude an aud claim altogether (not setting it up), add it only to access tokens, to both access tokens and json introspection responses, or to add it only to json introspection responses.

This seems like a test case we should include, thank you for letting us know.

trel

looks pretty good. a few kory comments still in here too.

awaiting another response from @ll4strw

trel · 2024-12-04T01:24:50Z

core/src/main.cpp

@@ -235,6 +235,9 @@ constexpr auto default_jsonschema() -> std::string_view
                                "nonstandard_id_token_secret": {{
                                    "type": "string"
                                }},
+                                "require_aud_member_from_introspection_endpoint": {{
+                                    "type": "boolean"
+                                }},
                                "redirect_uri": {{


redirect before require in alpha order

trel · 2024-12-04T01:24:55Z

core/src/main.cpp

@@ -264,6 +267,7 @@ constexpr auto default_jsonschema() -> std::string_view
                                "provider_url",
                                "mode",
                                "client_id",
+                                "require_aud_member_from_introspection_endpoint",
                                "redirect_uri",


redirect before require in alpha order

trel · 2024-12-04T01:25:00Z

core/src/main.cpp

@@ -508,6 +512,7 @@ auto print_configuration_template() -> void
                "client_id": "<string>",
                "client_secret": "<string>",
                "mode": "client",
+                "require_aud_member_from_introspection_endpoint": false,
                "redirect_uri": "<string>",


redirect before require in alpha order

ll4strw and others added 6 commits November 29, 2024 12:13

[irods#375] Handle missing aud claim in access token

75c257f

Reject access tokens without aud claim

Add new configuration option to schema

3fe7ea3

Add configuration option to README

0d8f927

Add optional strict check to aud

0f0fc51

Run git-clang-format

1e0960b

Move comment out of previous if block

63d0523

MartinFlores751 mentioned this pull request Dec 2, 2024

[#375] Handle missing aud claim in access token #376

Open

korydraughn reviewed Dec 2, 2024

View reviewed changes

MartinFlores751 commented Dec 3, 2024

View reviewed changes

core/src/main.cpp Show resolved Hide resolved

core/src/main.cpp Outdated Show resolved Hide resolved

core/src/openid.cpp Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

MartinFlores751 added 3 commits December 3, 2024 22:13

Update configuration name

7997715

Make config option required

d72d9fe

Add default value to template

c16adb0

trel approved these changes Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#375] Add optional strict check to `aud` member of introspection endpoint response #378

[#375] Add optional strict check to `aud` member of introspection endpoint response #378

MartinFlores751 commented Dec 2, 2024

korydraughn Dec 2, 2024

korydraughn Dec 2, 2024

korydraughn Dec 2, 2024

korydraughn Dec 2, 2024

trel commented Dec 2, 2024

alanking commented Dec 2, 2024

MartinFlores751 commented Dec 2, 2024

MartinFlores751 commented Dec 2, 2024

ll4strw commented Dec 3, 2024

MartinFlores751 left a comment

MartinFlores751 commented Dec 3, 2024

trel left a comment

trel Dec 4, 2024

trel Dec 4, 2024

trel Dec 4, 2024

[#375] Add optional strict check to aud member of introspection endpoint response #378

Are you sure you want to change the base?

[#375] Add optional strict check to aud member of introspection endpoint response #378

Conversation

MartinFlores751 commented Dec 2, 2024

korydraughn Dec 2, 2024

Choose a reason for hiding this comment

korydraughn Dec 2, 2024

Choose a reason for hiding this comment

korydraughn Dec 2, 2024

Choose a reason for hiding this comment

korydraughn Dec 2, 2024

Choose a reason for hiding this comment

trel commented Dec 2, 2024

alanking commented Dec 2, 2024

MartinFlores751 commented Dec 2, 2024

MartinFlores751 commented Dec 2, 2024

ll4strw commented Dec 3, 2024

MartinFlores751 left a comment

Choose a reason for hiding this comment

MartinFlores751 commented Dec 3, 2024

trel left a comment

Choose a reason for hiding this comment

trel Dec 4, 2024

Choose a reason for hiding this comment

trel Dec 4, 2024

Choose a reason for hiding this comment

trel Dec 4, 2024

Choose a reason for hiding this comment

[#375] Add optional strict check to `aud` member of introspection endpoint response #378

[#375] Add optional strict check to `aud` member of introspection endpoint response #378