From 10a52564ec0a7fcde20d50ee277bcca267445bc3 Mon Sep 17 00:00:00 2001 From: Eloy Coto Date: Wed, 15 May 2019 15:32:42 +0200 Subject: [PATCH 1/3] [THREESCALE-2236] Add methods option on Keycloak policy. This commits add the `methods` option on the Keycloak policy. That allow users to define a new policy from any jwt claim, resource and method. To be backwards compatible 'ANY' method is in place, and if methods are not defined this global method will be used and all will work as normal. Example policy: ``` "policy_chain": [ { "name": "apicast.policy.keycloak_role_check", "configuration": { "scopes": [ { "realm_roles": [ { "name": "director" } ], "resource": "/confidential", "methods": ["POST"] } ], "type": "blacklist" } }, { "name": "apicast.policy.apicast" } ] ``` Signed-off-by: Eloy Coto --- CHANGELOG.md | 2 +- .../policy/keycloak_role_check/README.md | 14 ++ .../keycloak_role_check/apicast-policy.json | 20 +++ .../keycloak_role_check.lua | 48 ++++-- .../keycloak_role_check_spec.lua | 90 ++++++++++ t/apicast-policy-keycloak-role-check.t | 157 ++++++++++++++++++ 6 files changed, 317 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 681f701aa..338888a39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - "Upstream Connection" policy. It allows to configure several options for the connections to the upstream [PR #1025](https://github.com/3scale/APIcast/pull/1025), [THREESCALE-2166](https://issues.jboss.org/browse/THREESCALE-2166) - Enable APICAST_EXTENDED_METRICS environment variable to provide additional details [PR #1024](https://github.com/3scale/APIcast/pull/1024) [THREESCALE-2150](https://issues.jboss.org/browse/THREESCALE-2150) - Add the option to obtain client_id from any JWT claim [THREESCALE-2264](https://issues.jboss.org/browse/THREESCALE-2264) [PR #1034](https://github.com/3scale/APIcast/pull/1034) - - Added `APICAST_PATH_ROUTING_ONLY` variable that allows to perform path-based routing without falling back to the default host-based routing [PR #1035](https://github.com/3scale/APIcast/pull/1035), [THREESCALE-1150](https://issues.jboss.org/browse/THREESCALE-1150) +- Added the option to manage access based on method on Keycloak Policy. [THREESCALE-2236](https://issues.jboss.org/browse/THREESCALE-2236) [PR #1039](https://github.com/3scale/APIcast/pull/1039) ### Fixed diff --git a/gateway/src/apicast/policy/keycloak_role_check/README.md b/gateway/src/apicast/policy/keycloak_role_check/README.md index 0b14ace57..40cf6ab8a 100644 --- a/gateway/src/apicast/policy/keycloak_role_check/README.md +++ b/gateway/src/apicast/policy/keycloak_role_check/README.md @@ -104,3 +104,17 @@ ] } ``` + +- When you want to allow those who have the realm role `role1` to access `/resource1` and only methods GET and POST. + + ```json + { + "scopes": [ + { + "realm_roles": [ { "name": "role1" } ], + "resource": "/resource1", + "methods": ["GET", "POST"] + } + ] + } + ``` diff --git a/gateway/src/apicast/policy/keycloak_role_check/apicast-policy.json b/gateway/src/apicast/policy/keycloak_role_check/apicast-policy.json index e655ab5c1..780b49d21 100644 --- a/gateway/src/apicast/policy/keycloak_role_check/apicast-policy.json +++ b/gateway/src/apicast/policy/keycloak_role_check/apicast-policy.json @@ -77,6 +77,26 @@ "resource_type": { "description": "How to evaluate 'resource'", "$ref": "#/definitions/value_type" + }, + "methods": { + "description": "Allowed methods", + "type": "array", + "default": ["ANY"], + "items": { + "type": "string", + "enum": [ + "ANY", + "GET", + "HEAD", + "POST", + "PUT", + "DELETE", + "PATCH", + "OPTIONS", + "TRACE", + "CONNECT" + ] + } } } } diff --git a/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua b/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua index 3fb7e71c7..6368d9258 100644 --- a/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua +++ b/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua @@ -60,11 +60,13 @@ local default_type = 'plain' local new = _M.new +local any_method = 'ANY' + local function create_template(value, value_type) return TemplateString.new(value, value_type or default_type) end -local function build_templates(scopes) +local function build_scopes(scopes) for _, scope in ipairs(scopes) do if scope.realm_roles then @@ -86,8 +88,12 @@ local function build_templates(scopes) scope.resource_template_string = create_template( scope.resource, scope.resource_type) + if (not scope.methods) or (scope.methods and #scope.methods == 0 ) then + scope.methods = { any_method } + end end + end function _M.new(config) @@ -95,7 +101,7 @@ function _M.new(config) self.type = config.type or "whitelist" self.scopes = config.scopes or {} - build_templates(self.scopes) + build_scopes(self.scopes) return self end @@ -152,38 +158,54 @@ local function match_client_roles(scope, context) return true end -local function scope_check(scopes, context) - local uri = ngx.var.uri - - if not context.jwt then - return false - end - - for _, scope in ipairs(scopes) do +local function validate_scope_access(scope, context, uri, request_method) + for _, method in ipairs(scope.methods) do + -- make a matched method just in case that `ANY` method is defined and + -- the mapping rules does not match, the matched_method need to be + -- cleared in the next interaction to trigger with the correct method. + local matched_method = request_method + if method == any_method then + matched_method = any_method + end local resource = scope.resource_template_string:render(context) local mapping_rule = MappingRule.from_proxy_rule({ - http_method = 'ANY', + http_method = method, pattern = resource, querystring_parameters = {}, -- the name of the metric is irrelevant metric_system_name = 'hits' }) - if mapping_rule:matches('ANY', uri) then + if mapping_rule:matches(matched_method, uri) then if match_realm_roles(scope, context) and match_client_roles(scope, context) then return true end end + end + return false +end +local function scopes_check(scopes, context) + local uri = ngx.var.uri + local request_method = ngx.req.get_method() + + if not context.jwt then + return false + end + + for _, scope in ipairs(scopes) do + if validate_scope_access(scope, context, uri, request_method) then + return true + end end return false end function _M:access(context) - if scope_check(self.scopes, context) then + if scopes_check(self.scopes, context) then if self.type == "blacklist" then return errors.authorization_failed(context.service) end diff --git a/spec/policy/keycloak_role_check/keycloak_role_check_spec.lua b/spec/policy/keycloak_role_check/keycloak_role_check_spec.lua index 4f496ada6..8198c8685 100644 --- a/spec/policy/keycloak_role_check/keycloak_role_check_spec.lua +++ b/spec/policy/keycloak_role_check/keycloak_role_check_spec.lua @@ -7,6 +7,7 @@ describe('Keycloak Role check policy', function() ngx.header = {} stub(ngx, 'print') + stub(ngx.req, 'get_method', function() return 'GET' end) -- avoid stubbing all the ngx.var.* and ngx.req.* in the available context stub(ngx_variable, 'available_context', function(context) return context end) end) @@ -572,6 +573,95 @@ describe('Keycloak Role check policy', function() assert.not_same(ngx.status, 403) end) end) + + describe("validate method roles", function() + + before_each(function() + ngx.var = { + uri = '/foo' + } + end) + + local context = { + jwt = { + realm_access = { + roles = { "known_role" } + }, + resource_access = { + known_client = { + roles = { "role_of_known_client" } + } + } + }, + service = { + auth_failed_status = 403, + error_auth_failed = "auth failed" + } + } + + it("Validates that an empty array still make a default any method", function() + local role_check_policy = KeycloakRoleCheckPolicy.new({ + scopes = {{ + client_roles = { { name = "role_of_known_client", client = "known_client" } }, + resource = "/foo", + methods = {} + }}}) + role_check_policy:access(context) + assert.same(ngx.status, nil) + end) + + it("when jwt role matches and one method also match", function() + local role_check_policy = KeycloakRoleCheckPolicy.new({ + scopes = {{ + client_roles = { { name = "role_of_known_client", client = "known_client" } }, + resource = "/foo", + methods = {"GET", "POST", "PUT"} + }}, + type = "whitelist"}) + + role_check_policy:access(context) + assert.same(ngx.status, nil) + end) + + it("when jwt role matches and one method also match with blacklist mode", function() + local role_check_policy = KeycloakRoleCheckPolicy.new({ + scopes = {{ + client_roles = { { name = "role_of_known_client", client = "known_client" } }, + resource = "/foo", + methods = {"GET", "POST", "PUT"} + }}, + type = "blacklist"}) + + role_check_policy:access(context) + assert.same(ngx.status, 403) + end) + + it("when jwt role matches and no methods matches", function() + local role_check_policy = KeycloakRoleCheckPolicy.new({ + scopes = {{ + client_roles = { { name = "role_of_known_client", client = "known_client" } }, + resource = "/foo", + methods = {"POST", "PUT"} + }}, + type = "whitelist"}) + role_check_policy:access(context) + assert.same(ngx.status, 403) + end) + + it("when jwt role matches and no methods matches with blacklist mode", function() + local role_check_policy = KeycloakRoleCheckPolicy.new({ + scopes = {{ + client_roles = { { name = "role_of_known_client", client = "known_client" } }, + resource = "/foo", + methods = {"POST", "PUT"} + }}, + type = "blacklist"}) + role_check_policy:access(context) + assert.same(ngx.status, nil) + end) + + end) + end) end) end) diff --git a/t/apicast-policy-keycloak-role-check.t b/t/apicast-policy-keycloak-role-check.t index 8772cfd9c..be840299b 100644 --- a/t/apicast-policy-keycloak-role-check.t +++ b/t/apicast-policy-keycloak-role-check.t @@ -393,3 +393,160 @@ yay, api backend --- no_error_log [error] oauth failed with + + +=== TEST6: Role check with allow methods +The client which has the appropriate role accesses the resource with only one method allowed +--- backend + location /transactions/oauth_authrep.xml { + content_by_lua_block { + ngx.exit(200) + } + } + +--- configuration +{ + "oidc": [ + { + "issuer": "https://example.com/auth/realms/apicast", + "config": { "id_token_signing_alg_values_supported": [ "RS256" ] }, + "keys": { "somekid": { "pem": "-----BEGIN PUBLIC KEY-----\nMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALClz96cDQ965ENYMfZzG+Acu25lpx2K\nNpAALBQ+catCA59us7+uLY5rjQR6SOgZpCz5PJiKNAdRPDJMXSmXqM0CAwEAAQ==\n-----END PUBLIC KEY-----" } } + } + ], + "services": [ + { + "id": 42, + "backend_version": "oauth", + "backend_authentication_type": "service_token", + "backend_authentication_value": "token-value", + "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 } + ], + "policy_chain": [ + { + "name": "apicast.policy.keycloak_role_check", + "configuration": { + "scopes": [ + { + "realm_roles": [ { "name": "director" } ], + "resource": "/confidential", + "methods": ["GET"] + } + ] + } + }, + { "name": "apicast.policy.apicast" } + ] + } + } + ] +} +--- upstream + location /confidential { + content_by_lua_block { + ngx.say('yay, api backend'); + } + } +--- pipelined_requests eval +["GET /confidential", "POST /confidential"] +--- more_headers eval +[::authorization_bearer_jwt('audience', { + realm_access => { + roles => [ 'director' ] + } +}, 'somekid'), +::authorization_bearer_jwt('audience', { + realm_access => { + roles => [ 'director' ] + } +}, 'somekid')] +--- response_body eval +["yay, api backend\n", "Authentication failed"] +--- error_code eval +[200, 403] +--- no_error_log +[error] +oauth failed with + + +=== TEST7: Role check with allow methods and blacklist mode +Check an allowed role with the blacklisted mode with methods +--- backend + location /transactions/oauth_authrep.xml { + content_by_lua_block { + ngx.exit(200) + } + } + +--- configuration +{ + "oidc": [ + { + "issuer": "https://example.com/auth/realms/apicast", + "config": { "id_token_signing_alg_values_supported": [ "RS256" ] }, + "keys": { "somekid": { "pem": "-----BEGIN PUBLIC KEY-----\nMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALClz96cDQ965ENYMfZzG+Acu25lpx2K\nNpAALBQ+catCA59us7+uLY5rjQR6SOgZpCz5PJiKNAdRPDJMXSmXqM0CAwEAAQ==\n-----END PUBLIC KEY-----" } } + } + ], + "services": [ + { + "id": 42, + "backend_version": "oauth", + "backend_authentication_type": "service_token", + "backend_authentication_value": "token-value", + "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 } + ], + "policy_chain": [ + { + "name": "apicast.policy.keycloak_role_check", + "configuration": { + "scopes": [ + { + "realm_roles": [ { "name": "director" } ], + "resource": "/confidential", + "methods": ["POST"] + } + ], + "type": "blacklist" + } + }, + { "name": "apicast.policy.apicast" } + ] + } + } + ] +} +--- upstream + location /confidential { + content_by_lua_block { + ngx.say('yay, api backend'); + } + } +--- pipelined_requests eval +["GET /confidential", "POST /confidential"] +--- more_headers eval +[::authorization_bearer_jwt('audience', { + realm_access => { + roles => [ 'director' ] + } +}, 'somekid'), +::authorization_bearer_jwt('audience', { + realm_access => { + roles => [ 'director' ] + } +}, 'somekid')] +--- response_body eval +["yay, api backend\n", "Authentication failed"] +--- error_code eval +[200, 403] +--- no_error_log +[error] +oauth failed with From 9aabb52ca8785ec44367a56c902db95a2a39e3d4 Mon Sep 17 00:00:00 2001 From: Eloy Coto Date: Fri, 17 May 2019 09:09:10 +0200 Subject: [PATCH 2/3] Add ANY method on mapping rules. This commits adds a ANY method to match any method in the mapping_rule so allow all and only block by the request uri. Signed-off-by: Eloy Coto --- gateway/src/apicast/mapping_rule.lua | 6 ++++-- .../keycloak_role_check.lua | 11 ++-------- spec/mapping_rule_spec.lua | 20 +++++++++++++++++++ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/gateway/src/apicast/mapping_rule.lua b/gateway/src/apicast/mapping_rule.lua index 394bc2fbe..00920c697 100644 --- a/gateway/src/apicast/mapping_rule.lua +++ b/gateway/src/apicast/mapping_rule.lua @@ -13,7 +13,9 @@ local re_match = ngx.re.match local insert = table.insert local re_gsub = ngx.re.gsub -local _M = {} +local _M = { + any_method = "ANY" +} local mt = { __index = _M } @@ -132,7 +134,7 @@ end -- @tparam table args Table with the args and values of an HTTP request. -- @treturn boolean Whether the mapping rule matches the given request. function _M:matches(method, uri, args) - local match = self.method == method and + local match = (self.method == self.any_method or self.method == method) and matches_uri(self.regexpified_pattern, uri) and self.querystring_params(args) diff --git a/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua b/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua index 6368d9258..946ae1cc5 100644 --- a/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua +++ b/gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua @@ -60,7 +60,7 @@ local default_type = 'plain' local new = _M.new -local any_method = 'ANY' +local any_method = MappingRule.any_method local function create_template(value, value_type) return TemplateString.new(value, value_type or default_type) @@ -160,13 +160,6 @@ end local function validate_scope_access(scope, context, uri, request_method) for _, method in ipairs(scope.methods) do - -- make a matched method just in case that `ANY` method is defined and - -- the mapping rules does not match, the matched_method need to be - -- cleared in the next interaction to trigger with the correct method. - local matched_method = request_method - if method == any_method then - matched_method = any_method - end local resource = scope.resource_template_string:render(context) @@ -178,7 +171,7 @@ local function validate_scope_access(scope, context, uri, request_method) metric_system_name = 'hits' }) - if mapping_rule:matches(matched_method, uri) then + if mapping_rule:matches(request_method, uri) then if match_realm_roles(scope, context) and match_client_roles(scope, context) then return true end diff --git a/spec/mapping_rule_spec.lua b/spec/mapping_rule_spec.lua index 16949effb..e6a253724 100644 --- a/spec/mapping_rule_spec.lua +++ b/spec/mapping_rule_spec.lua @@ -120,6 +120,26 @@ describe('mapping_rule', function() assert.is_true(mapping_rule:matches('GET', '/foo/a:b/bar')) assert.is_true(mapping_rule:matches('GET', "/foo/a%b/bar")) end) + end) + + describe('.any_method', function() + + it("Allow connections when any method is defined", function() + local mapping_rule = MappingRule.from_proxy_rule({ + http_method = MappingRule.any_method, + pattern = '/foo/', + querystring_parameters = { }, + metric_system_name = 'hits', + delta = 1 + }) + + assert.is_true(mapping_rule:matches('GET', '/foo/')) + assert.is_true(mapping_rule:matches('POST', '/foo/')) + assert.is_true(mapping_rule:matches('PUT', "/foo/")) + assert.is_true(mapping_rule:matches('DELETE', "/foo/")) + assert.is_true(mapping_rule:matches('PATCH', "/foo/")) + end) end) + end) From 32d8ecffa22df6efba72b90680fcc613aa4ce1cd Mon Sep 17 00:00:00 2001 From: Eloy Coto Date: Fri, 17 May 2019 09:39:55 +0200 Subject: [PATCH 3/3] [Test] Fix integration test names on keycloak policy Signed-off-by: Eloy Coto --- t/apicast-policy-keycloak-role-check.t | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/apicast-policy-keycloak-role-check.t b/t/apicast-policy-keycloak-role-check.t index be840299b..fbb79ac77 100644 --- a/t/apicast-policy-keycloak-role-check.t +++ b/t/apicast-policy-keycloak-role-check.t @@ -22,7 +22,7 @@ run_tests(); __DATA__ -=== TEST1: Role check succeeds (whitelist) +=== TEST 1: Role check succeeds (whitelist) The client which has the appropriate role accesses the resource. --- backend location /transactions/oauth_authrep.xml { @@ -94,7 +94,7 @@ oauth failed with -=== TEST2: Role check succeeds (blacklist) +=== TEST 2: Role check succeeds (blacklist) The client which doesn't have the inappropriate role accesses the resource. --- backend location /transactions/oauth_authrep.xml { @@ -169,7 +169,7 @@ oauth failed with -=== TEST3: Role check fails (whitelist) +=== TEST 3: Role check fails (whitelist) The client which doesn't have the appropriate role accesses the resource. --- backend location /transactions/oauth_authrep.xml { @@ -240,7 +240,7 @@ auth failed -=== TEST4: Role check fails (blacklist) +=== TEST 4: Role check fails (blacklist) The client which has the inappropriate role accesses the resource. --- backend location /transactions/oauth_authrep.xml { @@ -314,7 +314,7 @@ auth failed -=== TEST5: Role check succeeds with Liquid template (whitelist) +=== TEST 5: Role check succeeds with Liquid template (whitelist) The client which has the appropriate role accesses the resource. --- backend location /transactions/oauth_authrep.xml { @@ -395,7 +395,7 @@ yay, api backend oauth failed with -=== TEST6: Role check with allow methods +=== TEST 6: Role check with allow methods The client which has the appropriate role accesses the resource with only one method allowed --- backend location /transactions/oauth_authrep.xml { @@ -473,7 +473,7 @@ The client which has the appropriate role accesses the resource with only one me oauth failed with -=== TEST7: Role check with allow methods and blacklist mode +=== TEST 7: Role check with allow methods and blacklist mode Check an allowed role with the blacklisted mode with methods --- backend location /transactions/oauth_authrep.xml {