From 1b4b1c4a1b3fc164e78b0f1f10ecf51e84fe3663 Mon Sep 17 00:00:00 2001
From: Meng Han <4225835+meng-han@users.noreply.github.com>
Date: Thu, 7 Nov 2024 15:01:24 -0800
Subject: [PATCH] loosen rbac for get cert - avoid checking every DNS in SAN
 (#444)

Previous rbac for get certificate:
* CN, and all DNS in SAN needs to match a single regex, and the outcome
of 'service_name' needs to be matched to the kms auth identity.

Problem: this limits a lot of the freedom of how DNS in SAN can be
defined, especially when 'service_name' does not need to be existent in
SAN DNS, even just missing in one of the entries.

Change: loosen the rbac of get cert to just match against CN. For DNS
list in SAN, we will skip checking against kms auth identity.
---
 confidant/authnz/rbac.py                 | 22 +++++++++-------------
 tests/unit/confidant/authnz/rbac_test.py | 20 +++-----------------
 2 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/confidant/authnz/rbac.py b/confidant/authnz/rbac.py
index 707844c8..122a1a98 100644
--- a/confidant/authnz/rbac.py
+++ b/confidant/authnz/rbac.py
@@ -44,19 +44,15 @@ def default_acl(*args, **kwargs):
             if not ca_object.settings['name_regex']:
                 return False
             cert_pattern = re.compile(ca_object.settings['name_regex'])
-            domains = [resource_id]
-            domains.extend(resource_kwargs.get('san', []))
-            # Ensure the CN and every value in the SAN is allowed for this
-            # user.
-            for domain in domains:
-                match = cert_pattern.match(domain)
-                if not match:
-                    return False
-                service_name = match.group('service_name')
-                if not service_name:
-                    return False
-                if not authnz.user_is_service(service_name):
-                    return False
+            domain = resource_id
+            match = cert_pattern.match(domain)
+            if not match:
+                return False
+            service_name = match.group('service_name')
+            if not service_name:
+                return False
+            if not authnz.user_is_service(service_name):
+                return False
             return True
         return False
     else:
diff --git a/tests/unit/confidant/authnz/rbac_test.py b/tests/unit/confidant/authnz/rbac_test.py
index 5b1c6256..63160682 100644
--- a/tests/unit/confidant/authnz/rbac_test.py
+++ b/tests/unit/confidant/authnz/rbac_test.py
@@ -85,30 +85,16 @@ def test_default_acl(mocker: MockerFixture):
             kwargs={'ca': 'development'},
         ) is False
         # Test for user type is service, with certificate resource and get
-        # action, with a valid CN, but an invalid SAN
-        assert rbac.default_acl(
-            resource_type='certificate',
-            action='get',
-            resource_id='test-service.example.com',
-            kwargs={
-                'ca': 'development',
-                'san': ['bad-service.example.com'],
-            },
-        ) is False
-        # Test for user type is service, with certificate resource and get
-        # action, with a valid CN, but a mix of valid and invalid SAN values
+        # action, with a valid CN
         assert rbac.default_acl(
             resource_type='certificate',
             action='get',
             resource_id='test-service.example.com',
             kwargs={
                 'ca': 'development',
-                'san': [
-                    'bad-service.example.com',
-                    'test-service.example.com',
-                ],
+                'san': ['test-service.sub.example.com'],
             },
-        ) is False
+        ) is True
         # Test for user type is service, and an allowed resource, with
         # disallowed fake action
         assert rbac.default_acl(resource_type='service', action='fake') is False