From c8aafb4ab38015f7d5eb12763171b93d08cfa897 Mon Sep 17 00:00:00 2001 From: Matthieu Huin Date: Fri, 31 Jan 2020 13:34:03 +0100 Subject: [PATCH] Authorization rules: add templating Operators can use the "{tenant.name}" special word when setting conditions' values. This special word will be replaced at evaluation time by the name of the tenant for which the authorization check is being done. Change-Id: I6f1cf14ad29e775d9090e54b4a633384eef61085 --- doc/source/reference/tenants.rst | 54 +++++ .../playbooks/nonvoting-project-merge.yaml | 2 + .../playbooks/nonvoting-project-test1.yaml | 2 + .../playbooks/nonvoting-project-test2.yaml | 2 + .../playbooks/project-merge.yaml | 2 + .../common-config/playbooks/project-post.yaml | 2 + .../playbooks/project-test1.yaml | 2 + .../playbooks/project-test2.yaml | 2 + .../playbooks/project-testfile.yaml | 2 + .../project1-project2-integration.yaml | 2 + .../git/common-config/zuul.yaml | 197 ++++++++++++++++++ .../rules-templating/git/org_project/README | 1 + .../rules-templating/git/org_project1/README | 1 + .../rules-templating/git/org_project2/README | 1 + .../authorization/rules-templating/main.yaml | 34 +++ .../authorizations-templating.yaml | 29 +++ tests/unit/test_configloader.py | 56 +++++ tests/unit/test_scheduler.py | 26 +++ zuul/model.py | 41 ++-- zuul/rpclistener.py | 3 +- 20 files changed, 444 insertions(+), 17 deletions(-) create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-merge.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test1.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test2.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-merge.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-post.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test1.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test2.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-testfile.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project1-project2-integration.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/common-config/zuul.yaml create mode 100644 tests/fixtures/config/authorization/rules-templating/git/org_project/README create mode 100644 tests/fixtures/config/authorization/rules-templating/git/org_project1/README create mode 100644 tests/fixtures/config/authorization/rules-templating/git/org_project2/README create mode 100644 tests/fixtures/config/authorization/rules-templating/main.yaml create mode 100644 tests/fixtures/config/tenant-parser/authorizations-templating.yaml diff --git a/doc/source/reference/tenants.rst b/doc/source/reference/tenants.rst index bab684eaa0..a3e84a6095 100644 --- a/doc/source/reference/tenants.rst +++ b/doc/source/reference/tenants.rst @@ -460,3 +460,57 @@ Below are some examples of how access rules can be defined: } }, } + +Access Rule Templating +---------------------- + +The special word "{tenant.name}" can be used in conditions' values. It will be automatically +substituted for the relevant tenant when evaluating authorizations for a given +set of claims. For example, consider the following rule: + +.. code-block:: yaml + + - admin-rule: + name: tenant_in_groups + conditions: + - groups: "{tenant.name}" + +If applied to the following tenants: + +.. code-block:: yaml + + - tenant: + name: tenant-one + admin-rules: + - tenant_in_groups + - tenant: + name: tenant-two + admin-rules: + - tenant_in_groups + +Then this set of claims will be allowed to perform protected actions on **tenant-one**: + +.. code-block:: javascript + + { + 'iss': 'some_other_institution', + 'aud': 'my_zuul_deployment', + 'exp': 1234567890, + 'sub': 'carol', + 'iat': 1234556780, + 'groups': ['tenant-one', 'some-other-group'], + } + +And this set of claims will be allowed to perform protected actions on **tenant-one** +and **tenant-two**: + +.. code-block:: javascript + + { + 'iss': 'some_other_institution', + 'aud': 'my_zuul_deployment', + 'exp': 1234567890, + 'sub': 'carol', + 'iat': 1234556780, + 'groups': ['tenant-one', 'tenant-two'], + } diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-merge.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-merge.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-merge.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test1.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test1.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test1.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test2.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test2.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/nonvoting-project-test2.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-merge.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-merge.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-merge.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-post.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-post.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-post.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test1.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test1.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test1.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test2.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test2.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-test2.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-testfile.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-testfile.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project-testfile.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project1-project2-integration.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project1-project2-integration.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/playbooks/project1-project2-integration.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/authorization/rules-templating/git/common-config/zuul.yaml b/tests/fixtures/config/authorization/rules-templating/git/common-config/zuul.yaml new file mode 100644 index 0000000000..750d578ec0 --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/common-config/zuul.yaml @@ -0,0 +1,197 @@ +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- pipeline: + name: gate + manager: dependent + success-message: Build succeeded (gate). + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + success: + gerrit: + Verified: 2 + submit: true + failure: + gerrit: + Verified: -2 + start: + gerrit: + Verified: 0 + precedence: high + +- pipeline: + name: post + manager: independent + trigger: + gerrit: + - event: ref-updated + ref: ^(?!refs/).*$ + precedence: low + +- job: + name: base + parent: null + +- job: + name: project-merge + hold-following-changes: true + nodeset: + nodes: + - name: controller + label: label1 + run: playbooks/project-merge.yaml + +- job: + name: project-test1 + attempts: 4 + nodeset: + nodes: + - name: controller + label: label1 + run: playbooks/project-test1.yaml + +- job: + name: project-test1 + branches: stable + nodeset: + nodes: + - name: controller + label: label2 + run: playbooks/project-test1.yaml + +- job: + name: project-post + nodeset: + nodes: + - name: static + label: ubuntu-xenial + run: playbooks/project-post.yaml + +- job: + name: project-test2 + nodeset: + nodes: + - name: controller + label: label1 + run: playbooks/project-test2.yaml + +- job: + name: project1-project2-integration + nodeset: + nodes: + - name: controller + label: label1 + run: playbooks/project1-project2-integration.yaml + +- job: + name: project-testfile + files: + - .*-requires + run: playbooks/project-testfile.yaml + +- project: + name: org/project + check: + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + gate: + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project-testfile: + dependencies: project-merge + post: + jobs: + - project-post + +- project: + name: org/project1 + check: + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project1-project2-integration: + dependencies: project-merge + gate: + queue: integrated + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project1-project2-integration: + dependencies: project-merge + +- project: + name: org/project2 + check: + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project1-project2-integration: + dependencies: project-merge + gate: + queue: integrated + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project1-project2-integration: + dependencies: project-merge + +- project: + name: common-config + check: + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project1-project2-integration: + dependencies: project-merge + gate: + queue: integrated + jobs: + - project-merge + - project-test1: + dependencies: project-merge + - project-test2: + dependencies: project-merge + - project1-project2-integration: + dependencies: project-merge + +- job: + name: test-job + run: playbooks/project-merge.yaml + required-projects: + - org/project diff --git a/tests/fixtures/config/authorization/rules-templating/git/org_project/README b/tests/fixtures/config/authorization/rules-templating/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/authorization/rules-templating/git/org_project1/README b/tests/fixtures/config/authorization/rules-templating/git/org_project1/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/org_project1/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/authorization/rules-templating/git/org_project2/README b/tests/fixtures/config/authorization/rules-templating/git/org_project2/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/git/org_project2/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/authorization/rules-templating/main.yaml b/tests/fixtures/config/authorization/rules-templating/main.yaml new file mode 100644 index 0000000000..3e6b007657 --- /dev/null +++ b/tests/fixtures/config/authorization/rules-templating/main.yaml @@ -0,0 +1,34 @@ +- admin-rule: + name: tenant-admin + conditions: + - groups: "{tenant.name}" +- tenant: + name: tenant-zero + admin-rules: + - tenant-admin + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project +- tenant: + name: tenant-one + admin-rules: + - tenant-admin + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project1 +- tenant: + name: tenant-two + admin-rules: + - tenant-admin + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project2 diff --git a/tests/fixtures/config/tenant-parser/authorizations-templating.yaml b/tests/fixtures/config/tenant-parser/authorizations-templating.yaml new file mode 100644 index 0000000000..2b30ea5abe --- /dev/null +++ b/tests/fixtures/config/tenant-parser/authorizations-templating.yaml @@ -0,0 +1,29 @@ +- admin-rule: + name: tenant-admin + conditions: + - group: "{tenant.name}-admin" +- admin-rule: + name: tenant-admin-complex + conditions: + - path.to.group: "{tenant.name}-admin" +- tenant: + name: tenant-one + admin-rules: + - tenant-admin + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project1 +- tenant: + name: tenant-two + admin-rules: + - tenant-admin + - tenant-admin-complex + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project2 diff --git a/tests/unit/test_configloader.py b/tests/unit/test_configloader.py index 4521ac7110..e2f8de4035 100644 --- a/tests/unit/test_configloader.py +++ b/tests/unit/test_configloader.py @@ -554,6 +554,62 @@ class TestAuthorizationRuleParser(ZuulTestCase): self.assertTrue(rule(claims)) +class TestAuthorizationRuleParserWithTemplating(ZuulTestCase): + tenant_config_file = 'config/tenant-parser/authorizations-templating.yaml' + + def test_rules_are_loaded(self): + rules = self.sched.abide.admin_rules + self.assertTrue('tenant-admin' in rules, self.sched.abide) + self.assertTrue('tenant-admin-complex' in rules, self.sched.abide) + + def test_tenant_substitution(self): + claims_1 = {'group': 'tenant-one-admin'} + claims_2 = {'group': 'tenant-two-admin'} + rules = self.sched.abide.admin_rules + tenant_one = self.sched.abide.tenants.get('tenant-one') + tenant_two = self.sched.abide.tenants.get('tenant-two') + self.assertTrue(rules['tenant-admin'](claims_1, + tenant_one)) + self.assertTrue(rules['tenant-admin'](claims_2, + tenant_two)) + self.assertTrue(not rules['tenant-admin'](claims_1, + tenant_two)) + self.assertTrue(not rules['tenant-admin'](claims_2, + tenant_one)) + + def test_tenant_substitution_in_list(self): + claims_1 = {'group': ['tenant-one-admin', 'some-other-tenant']} + claims_2 = {'group': ['tenant-two-admin', 'some-other-tenant']} + rules = self.sched.abide.admin_rules + tenant_one = self.sched.abide.tenants.get('tenant-one') + tenant_two = self.sched.abide.tenants.get('tenant-two') + self.assertTrue(rules['tenant-admin'](claims_1, + tenant_one)) + self.assertTrue(rules['tenant-admin'](claims_2, + tenant_two)) + self.assertTrue(not rules['tenant-admin'](claims_1, + tenant_two)) + self.assertTrue(not rules['tenant-admin'](claims_2, + tenant_one)) + + def test_tenant_substitution_in_dict(self): + claims_2 = { + 'path': { + 'to': { + 'group': 'tenant-two-admin' + } + } + } + rules = self.sched.abide.admin_rules + tenant_one = self.sched.abide.tenants.get('tenant-one') + tenant_two = self.sched.abide.tenants.get('tenant-two') + self.assertTrue( + not rules['tenant-admin-complex'](claims_2, + tenant_one)) + self.assertTrue( + rules['tenant-admin-complex'](claims_2, tenant_two)) + + class TestTenantExtra(TenantParserTestCase): tenant_config_file = 'config/tenant-parser/extra.yaml' diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index f3b8718a49..8fbeb06c74 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -138,6 +138,32 @@ class TestAuthorizeViaRPC(ZuulTestCase): self.assertTrue(json.loads(authorized)) +class TestAuthorizeWithTemplatingViaRPC(ZuulTestCase): + tenant_config_file = 'config/authorization/rules-templating/main.yaml' + + def test_authorize_via_rpc(self): + client = zuul.rpcclient.RPCClient('127.0.0.1', + self.gearman_server.port) + self.addCleanup(client.shutdown) + tenants = ['tenant-zero', 'tenant-one', 'tenant-two'] + for t_claim in tenants: + claims = {'groups': [t_claim, ]} + for tenant in tenants: + authorized = client.submitJob('zuul:authorize_user', + {'tenant': tenant, + 'claims': claims}).data[0] + if t_claim == tenant: + self.assertTrue( + json.loads(authorized), + "Failed for t_claim: %s, tenant: %s" % (t_claim, + tenant)) + else: + self.assertTrue( + not json.loads(authorized), + "Failed for t_claim: %s, tenant: %s" % (t_claim, + tenant)) + + class TestSchedulerAutoholdHoldExpiration(ZuulTestCase): ''' This class of tests validates the autohold node expiration values diff --git a/zuul/model.py b/zuul/model.py index 4e2f71b6b2..81772c718f 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -4839,15 +4839,22 @@ class ClaimRule(AuthZRule): self.claim = claim or 'sub' self.value = value - def _match_jsonpath(self, claims): + def templated(self, value, tenant=None): + template_dict = {} + if tenant is not None: + template_dict['tenant'] = tenant.getSafeAttributes() + return value.format(**template_dict) + + def _match_jsonpath(self, claims, tenant): matches = [match.value for match in jsonpath_rw.parse(self.claim).find(claims)] + t_value = self.templated(self.value, tenant) if len(matches) == 1: match = matches[0] if isinstance(match, list): - return self.value in match + return t_value in match elif isinstance(match, str): - return self.value == match + return t_value == match else: # unsupported type - don't raise, but this should be notified return False @@ -4855,15 +4862,16 @@ class ClaimRule(AuthZRule): # TODO we should differentiate no match and 2+ matches return False - def _match_dict(self, claims): + def _match_dict(self, claims, tenant): def _compare(value, claim): if isinstance(value, list): + t_value = map(self.templated, value, [tenant] * len(value)) if isinstance(claim, list): # if the claim is empty, the value must be empty too: if claim == []: - return value == [] + return t_value == [] else: - return (set(claim) <= set(value)) + return (set(claim) <= set(t_value)) else: return claim in value elif isinstance(value, dict): @@ -4875,15 +4883,16 @@ class ClaimRule(AuthZRule): return all(_compare(value[x], claim.get(x, {})) for x in value.keys()) else: - return value == claim + t_value = self.templated(value, tenant) + return t_value == claim return _compare(self.value, claims.get(self.claim, {})) - def __call__(self, claims): + def __call__(self, claims, tenant=None): if isinstance(self.value, dict): - return self._match_dict(claims) + return self._match_dict(claims, tenant) else: - return self._match_jsonpath(claims) + return self._match_jsonpath(claims, tenant) def __eq__(self, other): if not isinstance(other, ClaimRule): @@ -4903,8 +4912,8 @@ class OrRule(AuthZRule): super(OrRule, self).__init__() self.rules = set(subrules) - def __call__(self, claims): - return any(rule(claims) for rule in self.rules) + def __call__(self, claims, tenant=None): + return any(rule(claims, tenant) for rule in self.rules) def __eq__(self, other): if not isinstance(other, OrRule): @@ -4924,8 +4933,8 @@ class AndRule(AuthZRule): super(AndRule, self).__init__() self.rules = set(subrules) - def __call__(self, claims): - return all(rule(claims) for rule in self.rules) + def __call__(self, claims, tenant=None): + return all(rule(claims, tenant) for rule in self.rules) def __eq__(self, other): if not isinstance(other, AndRule): @@ -4946,8 +4955,8 @@ class AuthZRuleTree(object): # initialize actions as unauthorized self.ruletree = None - def __call__(self, claims): - return self.ruletree(claims) + def __call__(self, claims, tenant=None): + return self.ruletree(claims, tenant) def __eq__(self, other): if not isinstance(other, AuthZRuleTree): diff --git a/zuul/rpclistener.py b/zuul/rpclistener.py index f781fc05bf..9028145b74 100644 --- a/zuul/rpclistener.py +++ b/zuul/rpclistener.py @@ -303,7 +303,8 @@ class RPCListener(object): '"%s" to claims %s') self.log.debug( debug_msg % (rule, tenant, json.dumps(claims))) - authorized = self.sched.abide.admin_rules[rule](claims) + authorized = self.sched.abide.admin_rules[rule](claims, + tenant) if authorized: if '__zuul_uid_claim' in claims: uid = claims['__zuul_uid_claim']