Merge pull request #1391 from 3scale/backport-THREESCALE-9009-fix-oid…

…c-issuer-verification Backport 2.12 THREESCALE-9009 fix OIDC issuer verification
3scale · Feb 21, 2023 · e4f7cd7 · e4f7cd7
2 parents 109811b + 91c0b46
commit e4f7cd7
Show file tree

Hide file tree

Showing 4 changed files with 218 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,10 +5,26 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
-
-## [3.11.0] 2021-09-03
 ## [Unreleased]
 
+## [3.12.2] 2023-02-21
+
+- Fixed: OIDC jwt key verification [PR #1391](https://github.com/3scale/APIcast/pull/1391) [THREESCALE-9009](https://issues.redhat.com/browse/THREESCALE-9009)
+
+## [3.12.0] 2022-07-07
+
+### Fixed
+
+- Fixed warning messages [PR #1318](https://github.com/3scale/APIcast/pull/1318) [THREESCALE-7906](https://issues.redhat.com/browse/THREESCALE-7906)
+- Fixed dirty context [PR #1328](https://github.com/3scale/APIcast/pull/1328) [THREESCALE-8000](https://issues.redhat.com/browse/THREESCALE-8000) [THREESCALE-8007](https://issues.redhat.com/browse/THREESCALE-8007)
+- Fixed jwk alg confusion [PR #1329](https://github.com/3scale/APIcast/pull/1329) [THREESCALE-8249](https://issues.redhat.com/browse/THREESCALE-8249)
+- Fixed issue with resolving target server hostnames to IP when using CONNECT method [PR #1323](https://github.com/3scale/APIcast/pull/1323) [THREESCALE-7967](https://issues.redhat.com/browse/THREESCALE-7967)
+- Fixed issue with resolving target server hostnames to IPs when forwarding requests through http/s proxy [PR #1323](https://github.com/3scale/APIcast/pull/1323) [THREESCALE-7967](https://issues.redhat.com/browse/THREESCALE-7967)
+- Fixed dirty context [PR #1328](https://github.com/3scale/APIcast/pull/1328) [THREESCALE-8000](https://issues.redhat.com/browse/THREESCALE-8000) [THREESCALE-8007](https://issues.redhat.com/browse/THREESCALE-8007) [THREESCALE-8252](https://issues.redhat.com/browse/THREESCALE-8252)
+- Fixed dirty context (part 2 of PR #1328) when tls termination policy is in the policy chain [PR #1333](https://github.com/3scale/APIcast/pull/1333)
+
+## [3.11.0] 2022-02-17
+
 ### Fixed
 
 - Fixed hostname_rewrite incompatibility with Routing Policy [PR #1263](https://github.com/3scale/APIcast/pull/1263) [THREESCALE-6723](https://issues.redhat.com/browse/THREESCALE-6723)
@@ -948,3 +964,5 @@ Apart from the changes mentioned in this section, this version also includes the
 [3.10.0-beta1]: https://github.com/3scale/apicast/compare/v3.10.0-alpha2..v3.10.0-beta1
 [3.10.0]: https://github.com/3scale/apicast/compare/v3.10.0-beta1..v3.10.0
 [3.11.0]: https://github.com/3scale/apicast/compare/v3.10.0..v3.11.0
+[3.12.0]: https://github.com/3scale/apicast/compare/v3.11.0..v3.12.0
+[3.12.2]: https://github.com/3scale/apicast/compare/v3.12.0..v3.12.2
diff --git a/gateway/src/apicast/oauth/oidc.lua b/gateway/src/apicast/oauth/oidc.lua
@@ -105,7 +105,7 @@ end
 
 local function find_jwk(jwt, keys)
   local jwk = keys and keys[jwt.header.kid]
-  if jwk then return jwk end
+  return jwk
 end
 
 -- Parses the token - in this case we assume it's a JWT token
@@ -185,8 +185,15 @@ 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
+    ngx.log(ngx.ERR, "[jwt] failed verification for kid: ", jwt.header.kid)
+    return false, '[jwk] not found, token might belong to a different realm'
+  end
+
   local pubkey = jwk_obj.pem
-  if jwk_obj.alg ~= jwt.header.alg then
+  -- Check the jwk for the alg field and if not present skip the validation as it is
+  -- OPTIONAL according to https://www.rfc-editor.org/rfc/rfc7517#section-4.4
+  if jwk_obj.alg and jwk_obj.alg ~= jwt.header.alg then
     return false, '[jwt] alg mismatch'
   end
 

diff --git a/spec/oauth/oidc_spec.lua b/spec/oauth/oidc_spec.lua
@@ -45,6 +45,11 @@ describe('OIDC', function()
       config = { id_token_signing_alg_values_supported = { 'RS256', 'HS256' } },
       keys = { somekid = { pem = rsa.pub, alg = 'RS256' } },
     }
+    local oidc_config_no_alg = {
+      issuer = 'https://example.com/auth/realms/apicast',
+      config = { id_token_signing_alg_values_supported = { 'RS256', 'HS256' } },
+      keys = { somekid = { pem = rsa.pub } },
+    }
 
     before_each(function() jwt_validators.set_system_clock(function() return 0 end) end)
 
@@ -268,6 +273,61 @@ describe('OIDC', function()
       assert(credentials, err)
     end)
 
+    it('validation passes when jwk.alg does not exist', function()
+      local oidc = _M.new(oidc_config_no_alg)
+      local access_token = jwt:sign(rsa.private, {
+        header = { typ = 'JWT', alg = 'RS256', kid = 'somekid' },
+        payload = {
+          iss = oidc_config.issuer,
+          aud = 'notused',
+          azp = 'ce3b2e5e',
+          sub = 'someone',
+          nbf = 0,
+          exp = ngx.now() + 10,
+          typ = 'Bearer'
+        },
+      })
+
+      local credentials, _, _, err = oidc:transform_credentials({ access_token = access_token })
+      assert(credentials, err)
+    end)
+
+    it('token was signed by a different key', function()
+      local oidc = _M.new(oidc_config)
+      local access_token = jwt:sign(rsa.private, {
+        header = { typ = 'JWT', alg = 'RS256', kid = 'otherkid' },
+        payload = {
+          iss = oidc_config.issuer,
+          aud = 'notused',
+          azp = 'ce3b2e5e',
+          sub = 'someone',
+          exp = ngx.now() + 10,
+        },
+      })
+
+      local credentials, _, _, err = oidc:transform_credentials({ access_token = access_token })
+
+      assert.match('[jwk] not found, token might belong to a different realm', err, nil, true)
+    end)
+
+    it('token was signed by a different issuer', function()
+      local oidc = _M.new(oidc_config)
+      local access_token = jwt:sign(rsa.private, {
+        header = { typ = 'JWT', alg = 'RS256', kid = 'somekid' },
+        payload = {
+          iss = 'other_issuer',
+          aud = 'notused',
+          azp = 'ce3b2e5e',
+          sub = 'someone',
+          exp = ngx.now() + 10,
+        },
+      })
+
+      local credentials, _, _, err = oidc:transform_credentials({ access_token = access_token })
+
+      assert.match('Claim \'iss\' (\'other_issuer\') returned failure', err, nil, true)
+    end)
+
     describe('getting client_id from any JWT claim', function()
 
       before_each(function()

diff --git a/t/apicast-oidc.t b/t/apicast-oidc.t
@@ -264,9 +264,7 @@ my $jwt = encode_jwt(payload => {
 --- no_error_log
 [error]
 
-
-
-=== TEST 2: JWT verification fails when no alg is present in the jwk to match against jwt.header.alg
+=== TEST 6: JWT verification does not fail when no alg is present in the jwk to match against jwt.header.alg
 --- configuration env eval
 use JSON qw(to_json);
 
@@ -303,7 +301,7 @@ to_json({
     }
   }
 --- request: GET /test
---- error_code: 403
+--- error_code: 200
 --- more_headers eval
 use Crypt::JWT qw(encode_jwt);
 my $jwt = encode_jwt(payload => {
@@ -313,5 +311,132 @@ my $jwt = encode_jwt(payload => {
   iss => 'https://example.com/auth/realms/apicast',
   exp => time + 3600 }, key => \$::private_key, alg => 'RS256', extra_headers => { kid => 'somekid' });
 "Authorization: Bearer $jwt"
+--- no_error_log
+
+=== TEST 7: JWT verification fails when jwk.alg exists AND does not match jwt.header.alg
+(see THREESCALE-8249 for steps to generate tampered JWT. rsa.pub from fixtures used to sign)
+--- configuration env eval
+use JSON qw(to_json);
+
+to_json({
+  services => [{
+    id => 42,
+    backend_version => 'oauth',
+    backend_authentication_type => 'provider_key',
+    backend_authentication_value => 'fookey',
+    proxy => {
+        authentication_method => 'oidc',
+        oidc_issuer_endpoint => 'https://example.com/auth/realms/apicast',
+        api_backend => "http://test:$TEST_NGINX_SERVER_PORT/",
+        proxy_rules => [
+          { pattern => '/', http_method => 'GET', metric_system_name => 'hits', delta => 1  }
+        ]
+    }
+  }],
+  oidc => [{
+    issuer => 'https://example.com/auth/realms/apicast',
+    config => { id_token_signing_alg_values_supported => [ 'RS256', 'HS256' ] },
+    keys => { somekid => { pem => $::public_key, alg => 'RS256' } },
+  }]
+});
+--- upstream
+  location /test {
+    echo "yes";
+  }
+--- backend
+  location = /transactions/oauth_authrep.xml {
+    content_by_lua_block {
+      local expected = "provider_key=fookey&service_id=42&usage%5Bhits%5D=1&app_id=appid"
+      require('luassert').same(ngx.decode_args(expected), ngx.req.get_uri_args(0))
+    }
+  }
+--- request: GET /test
+--- error_code: 403
+--- more_headers eval
+use Crypt::JWT qw(encode_jwt);
+my $jwt = 'eyJraWQiOiJzb21la2lkIiwiYWxnIjoiSFMyNTYifQ.'.
+'eyJleHAiOjcxNzA1MzE2NDMwLCJhenAiOiJhcHBpZCIsInN1YiI6In'.
+'NvbWVvbmUiLCJhdWQiOiJzb21ldGhpbmciLCJpc3MiOiJodHRwczov'.
+'L2V4YW1wbGUuY29tL2F1dGgvcmVhbG1zL2FwaWNhc3QifQ.1rFq5QN'.
+'b99W6aqQjsx7GJGLDpdkDLI6-huZLzMAmxGQ';
+"Authorization: Bearer $jwt"
 --- error_log
 [jwt] alg mismatch
+
+=== TEST 8: Token was signed by a different key
+--- configuration env eval
+use JSON qw(to_json);
+
+to_json({
+  services => [{
+    id => 42,
+    backend_version => 'oauth',
+    backend_authentication_type => 'provider_key',
+    backend_authentication_value => 'fookey',
+    proxy => {
+        authentication_method => 'oidc',
+        oidc_issuer_endpoint => 'https://example.com/auth/realms/a',
+        api_backend => "http://test:$TEST_NGINX_SERVER_PORT/",
+        proxy_rules => [
+          { pattern => '/', http_method => 'GET', metric_system_name => 'hits', delta => 1  }
+        ]
+    }
+  }],
+  oidc => [{
+    issuer => 'https://example.com/auth/realms/a',
+    config => { id_token_signing_alg_values_supported => [ 'RS256' ] },
+    keys => { somekid => { pem => $::public_key, alg => 'RS256' } },
+  }]
+});
+--- request: GET /test
+--- error_code: 403
+--- more_headers eval
+use Crypt::JWT qw(encode_jwt);
+my $jwt = encode_jwt(payload => {
+  aud => 'something',
+  azp => 'appid',
+  sub => 'someone',
+  iss => 'https://example.com/auth/realms/b',
+  exp => time + 3600 }, key => \$::private_key, alg => 'RS256', extra_headers => { kid => 'otherkid' });
+"Authorization: Bearer $jwt"
+--- error_log
+[jwk] not found, token might belong to a different realm
+
+=== TEST 9: Token was signed by a different issuer
+--- configuration env eval
+use JSON qw(to_json);
+
+to_json({
+  services => [{
+    id => 42,
+    backend_version => 'oauth',
+    backend_authentication_type => 'provider_key',
+    backend_authentication_value => 'fookey',
+    proxy => {
+        authentication_method => 'oidc',
+        oidc_issuer_endpoint => 'https://example.com/auth/realms/apicast',
+        api_backend => "http://test:$TEST_NGINX_SERVER_PORT/",
+        proxy_rules => [
+          { pattern => '/', http_method => 'GET', metric_system_name => 'hits', delta => 1  }
+        ]
+    }
+  }],
+  oidc => [{
+    issuer => 'https://example.com/auth/realms/apicast',
+    config => { id_token_signing_alg_values_supported => [ 'RS256' ] },
+    keys => { somekid => { pem => $::public_key, alg => 'RS256' } },
+  }]
+});
+--- request: GET /test
+--- error_code: 403
+--- more_headers eval
+use Crypt::JWT qw(encode_jwt);
+my $jwt = encode_jwt(payload => {
+  aud => 'something',
+  azp => 'appid',
+  sub => 'someone',
+  iss => 'unexpected_issuer',
+  exp => time + 3600 }, key => \$::private_key, alg => 'RS256', extra_headers => { kid => 'somekid' });
+"Authorization: Bearer $jwt"
+--- error_log eval
+[ qr/Claim 'iss' \('unexpected_issuer'\) returned failure/ ]