Skip to content
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

THREESCALE-9009 fix OIDC jwt key verification #1389

Merged
merged 6 commits into from
Feb 21, 2023

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Feb 16, 2023

what

Fixes THREESCALE-9009 Token generated for a realm associated with a different 3scale Product is able to reach the upstream with no verification performed

When a oidc JWT belonging to realm A is used in a request targeting a 3scale service configured with OIDC authentication with clients configured in realm B, the first conflict APIcast finds is that the referenced public key (in the JWT) does not exist. This PR address this issue adding a check on the JWKey object.

In the absence of this check, the lua code was working with a nil object and raised an error (runtime error: nil pointer dereference) captured only by the policy chain manager.

local status, return_val = pcall(self[i][phase_name], self[i], context)

When this error is raised, the current policy processing stage is halted and skipped, moving on to the next policy in the chain. The token is not even verified with the JWT library as it should be in the next step

local pubkey = jwk_obj.pem          -> Raises runtime error                                                
    
if jwk_obj.alg and jwk_obj.alg ~= jwt.header.alg then          -> Not executed                      
  return false, '[jwt] alg mismatch'                                                
end                                                                                 
                                                                                    
jwt = JWT:verify_jwt_obj(pubkey, jwt, self.jwt_claims)    ->Not executed                                      

That causes the APIcast context to enter in a inconsistent state. Specifically, nginx ctx.credentials is not populated. Maybe more info but I have not gone through all of them because it is inconsistent, hence unpredictable behavior:

  • When the routing policy is added, the request makes its way to upstream and APIcast sends a request to backend with a missing app_id. Backend responds with 404 Not Found (as expected) but APICast ignores this and returns 200 OK to the downstream client.
  • When the routing policy is not in place, APIcast (under this incosisten state) is unable to find an upstream for the request and returns 404 Not Found to the upstream client. Additionally, APIcast sends a request to backend with a missing app_id. Backend responds with 404 Not Found (as expected).

With the check on the JWKey object, APIcast deals with a managed error (the token is invalid) and the behavior is expected, returning 403 Forbidden to the downstream client as expected. In that case, backend is not called.

Verification steps

make development
make dependencies
  • Run apicast locally
THREESCALE_DEPLOYMENT_ENV=staging APICAST_LOG_LEVEL=debug APICAST_WORKER=1 APICAST_CONFIGURATION_LOADER=lazy APICAST_CONFIGURATION_CACHE=0 THREESCALE_PORTAL_ENDPOINT=https://token@3scale-admin.example.com ./bin/apicast
  • Generate token from a realm B (not the same as configured for the Product A)
export ACCESS_TOKEN=$(curl -k -H "Content-Type: application/x-www-form-urlencoded" \
        -d 'grant_type=password' \
        -d 'client_id=my-client' \
        -d 'username=bob' \
        -d 'password=p' "https://myoidcprovider.example.com/auth/realms/B/protocol/openid-connect/token" | jq -r '.access_token')
  • Run query with the invalid jwt
# capture apicast IP
APICAST_IP=$(docker inspect apicast_build_0-development-1 | yq e -P '.[0].NetworkSettings.Networks.apicast_build_0_default.IPAddress' -)

curl -v -k -H "Host: example.com:443" -H "Accept: application/json" -H "Authorization: Bearer ${ACCESS_TOKEN}" http://${APICAST_IP}:8080/
  • APIcast should notify the invalid token error in logs
2023/02/16 17:05:33 [debug] 100542#100542: *33 proxy.lua:287: rewrite(): oauth failed with [jwk] jwk not found, token might not belong to right realm, requestID=94a325d7d76eff70d6dd70d6a490a157
  • The response should be HTTP/1.1 403 Forbidden
< HTTP/1.1 403 Forbidden
< Server: openresty
< Date: Thu, 16 Feb 2023 17:05:33 GMT
< Content-Type: text/plain; charset=us-ascii
< Transfer-Encoding: chunked
< Connection: keep-alive
< 
* Connection #0 to host 172.20.0.3 left intact
Authentication failed

@eguzki eguzki marked this pull request as ready for review February 16, 2023 17:08
@eguzki eguzki requested a review from a team as a code owner February 16, 2023 17:08
@eguzki eguzki changed the title THREESCALE-9009 fix OIDC issuer verification THREESCALE-9009 fix OIDC jwt key verification Feb 17, 2023
@eguzki eguzki force-pushed the THREESCALE-9009-fix-oidc-issuer-verification branch from 6713c46 to 22559eb Compare February 17, 2023 10:00
Copy link
Member

@kevprice83 kevprice83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a change to the error message printed which would be super helpful in future troubleshooting and a minor change to the description of the integration test so we can check the scenarios tested as well in future.

The rest looks good otherwise.

@@ -185,8 +185,12 @@ function _M:verify(jwt, cache_key)
-- Find jwk with matching kid for current JWT in request
local jwk_obj = find_jwk(jwt, self.keys)

if jwk_obj == nil then
return false, '[jwk] jwk not found, token might not belong to right realm'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest the following log message which would be more explicit and accurate:

ngx.log(ngx.ERR, "[jwt] failed verification for kid: ", jwt.header.kid)
return false, '[jwk] not found, token might belong to a different realm'

This would help customers identify the issue quicker and without having to enable debug log level.

t/apicast-oidc.t Outdated
@@ -362,3 +362,43 @@ my $jwt = 'eyJraWQiOiJzb21la2lkIiwiYWxnIjoiSFMyNTYifQ.'.
"Authorization: Bearer $jwt"
--- error_log
[jwt] alg mismatch

=== TEST 8: Key not available
(Happens when the token belongs to a different realm)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the more accurate test description is that the "token was signed by a different key". The same key could be used accross multiple realms but with this fix the iss claim will be correctly validated against the issuer field in the proxy config. Equally if the kids don't match then it won't even find the jwk.

In fact we can see that further down in this test, we have different realms but the logic never gets that far in the validation process because it fails before that with "[jwk] jwk not found"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test for the verification of the expected issuer

t/apicast-oidc.t Outdated
exp => time + 3600 }, key => \$::private_key, alg => 'RS256', extra_headers => { kid => 'otherkid' });
"Authorization: Bearer $jwt"
--- error_log
[jwk] jwk not found
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use the same error message suggested in my review then this should be updated to look for [jwk] not found, to save you from a failing test you would need to fix 😉

@eguzki
Copy link
Member Author

eguzki commented Feb 21, 2023

Thanks for the review @kevprice83. All comments addressed. Ready for a new review

@eguzki eguzki merged commit c3e529d into master Feb 21, 2023
@eguzki eguzki deleted the THREESCALE-9009-fix-oidc-issuer-verification branch February 21, 2023 14:36
Copy link
Member

@kevprice83 kevprice83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that additional last test yes! Also it's using the reason field on the verification object so I think this makes the test super clear. Nice improvement :)

Everything else looks good 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants