From 405caaeb4cb946333bddf2b1fbe1c44684966daf Mon Sep 17 00:00:00 2001 From: Ibrahim Awwad Date: Wed, 18 Dec 2024 15:49:29 +0200 Subject: [PATCH] Create an Authorization Policy for any ingress relation (#31) * add l4 ingress auth policies * adding scenario and itests * add AuthorizationPolicy CRD to itests * refactor policy name * fix model name in itest --- src/charm.py | 113 ++++++++++++++++++++++++---- src/models.py | 68 ++++++++++++++++- tests/integration/helpers.py | 29 +++++++ tests/integration/test_charm_ipa.py | 44 +++++++++++ tests/scenario/test_ingress.py | 73 +++++++++++++++--- 5 files changed, 302 insertions(+), 25 deletions(-) diff --git a/src/charm.py b/src/charm.py index 0b96a93..9f4e394 100755 --- a/src/charm.py +++ b/src/charm.py @@ -24,16 +24,19 @@ from lightkube.resources.core_v1 import Secret, Service from lightkube.types import PatchType from lightkube_extensions.batch import KubernetesResourceManager, create_charm_default_labels -from ops import BlockedStatus, EventBase +from ops import BlockedStatus, EventBase, main from ops.charm import CharmBase -from ops.main import main from ops.model import ActiveStatus, MaintenanceStatus from ops.pebble import ChangeError, Layer # from lightkube.models.meta_v1 import Patch from models import ( AllowedRoutes, + AuthorizationPolicyResource, + AuthorizationPolicySpec, + AuthRule, BackendRef, + From, GatewayTLSConfig, HTTPRequestRedirectFilter, HTTPRouteFilter, @@ -46,10 +49,14 @@ Listener, Match, Metadata, + Operation, ParentRef, PathMatch, Rule, SecretObjectReference, + Source, + To, + WorkloadSelector, ) logger = logging.getLogger(__name__) @@ -68,6 +75,12 @@ "ReferenceGrant": create_namespaced_resource( "gateway.networking.k8s.io", "v1beta1", "ReferenceGrant", "referencegrants" ), + "AuthorizationPolicy": create_namespaced_resource( + "security.istio.io", + "v1", + "AuthorizationPolicy", + "authorizationpolicies", + ), } GATEWAY_RESOURCE_TYPES = {RESOURCE_TYPES["Gateway"], Secret} @@ -76,8 +89,10 @@ RESOURCE_TYPES["ReferenceGrant"], RESOURCE_TYPES["HTTPRoute"], } -GATEWAY_LABEL = "istio-gateway" -INGRESS_LABEL = "istio-ingress" +AUTHORIZATION_POLICY_RESOURCE_TYPES = {RESOURCE_TYPES["AuthorizationPolicy"]} +GATEWAY_SCOPE = "istio-gateway" +INGRESS_SCOPE = "istio-ingress" +AUTHORIZATION_POLICY_SCOPE = "istio-ingress-authorization-policy" class DataValidationError(RuntimeError): @@ -207,7 +222,7 @@ def _setup_proxy_pebble_service(self): def _get_gateway_resource_manager(self): return KubernetesResourceManager( labels=create_charm_default_labels( - self.app.name, self.model.name, scope=GATEWAY_LABEL + self.app.name, self.model.name, scope=GATEWAY_SCOPE ), resource_types=GATEWAY_RESOURCE_TYPES, # pyright: ignore lightkube_client=self.lightkube_client, @@ -217,13 +232,23 @@ def _get_gateway_resource_manager(self): def _get_ingress_resource_manager(self): return KubernetesResourceManager( labels=create_charm_default_labels( - self.app.name, self.model.name, scope=INGRESS_LABEL + self.app.name, self.model.name, scope=INGRESS_SCOPE ), resource_types=INGRESS_RESOURCE_TYPES, # pyright: ignore lightkube_client=self.lightkube_client, logger=logger, ) + def _get_authorization_policy_resource_manager(self): + return KubernetesResourceManager( + labels=create_charm_default_labels( + self.app.name, self.model.name, scope=AUTHORIZATION_POLICY_SCOPE + ), + resource_types=AUTHORIZATION_POLICY_RESOURCE_TYPES, # pyright: ignore + lightkube_client=self.lightkube_client, + logger=logger, + ) + def _on_cert_handler_cert_changed(self, _): """Event handler for when tls certificates have changed.""" self._sync_all_resources() @@ -239,11 +264,14 @@ def _metrics_proxy_pebble_ready(self, _): def _on_remove(self, _): """Event handler for remove.""" # Removing tailing ingresses - krm = self._get_ingress_resource_manager() - krm.delete() + kim = self._get_ingress_resource_manager() + kim.delete() - krm = self._get_gateway_resource_manager() - krm.delete() + kgm = self._get_gateway_resource_manager() + kgm.delete() + + kam = self._get_authorization_policy_resource_manager() + kam.delete() def _on_ingress_data_provided(self, _): """Handle a unit providing data requesting IPU.""" @@ -439,6 +467,42 @@ def _construct_httproute(self, data: IngressRequirerData, prefix: str, section_n spec=http_route.spec.model_dump(exclude_none=True), ) + def _construct_ingress_auth_policy(self, data: IngressRequirerData): + + auth_policy = AuthorizationPolicyResource( + metadata=Metadata( + name=data.app.name + "-" + self.app.name + "-" + data.app.model + "-l4", + namespace=data.app.model, + ), + spec=AuthorizationPolicySpec( + rules=[ + AuthRule( + to=[To(operation=Operation(ports=[str(data.app.port)]))], + from_=[ # type: ignore # this is accessible via an alias + From( + source=Source( + principals=[ + _get_peer_identity_for_juju_application( + self.managed_name, self.model.name + ) + ] + ) + ) + ], + ) + ], + selector=WorkloadSelector(matchLabels={"app.kubernetes.io/name": data.app.name}), + ), + ) + auth_resource = RESOURCE_TYPES["AuthorizationPolicy"] + return auth_resource( + metadata=ObjectMeta.from_dict(auth_policy.metadata.model_dump()), + # by_alias=True because the model includes an alias for the `from` field + # exclude_unset=True because unset fields will be treated as their default values in Kubernetes + # exclude_none=True because null values in this data always mean the Kubernetes default + spec=auth_policy.spec.model_dump(by_alias=True, exclude_unset=True, exclude_none=True), + ) + def _construct_redirect_to_https_httproute( self, data: IngressRequirerData, prefix: str, section_name: str ): @@ -555,11 +619,13 @@ def _remove_hostname_if_present(self): def _sync_ingress_resources(self): current_ingresses = [] + current_policies = [] relation_mappings = {} if not self.unit.is_leader(): raise RuntimeError("Ingress can only be provided on the leader unit.") krm = self._get_ingress_resource_manager() + kam = self._get_authorization_policy_resource_manager() # If we can construct a gateway Secret, TLS is enabled so we should configure the routes accordingly is_tls_enabled = self._construct_gateway_tls_secret() is not None @@ -572,28 +638,33 @@ def _sync_ingress_resources(self): data = self.ingress_per_appv2.get_data(rel) prefix = self._generate_prefix(data.app.model_dump(by_alias=True)) - resources_to_append = [] + ingress_resources_to_append = [] + ingress_policies_to_append = [] if is_tls_enabled: # TLS is configured, so we enable HTTPS route and redirect HTTP to HTTPS - resources_to_append.append( + ingress_resources_to_append.append( self._construct_redirect_to_https_httproute(data, prefix, section_name="http") ) - resources_to_append.append( + ingress_resources_to_append.append( self._construct_httproute(data, prefix, section_name="https") ) else: # Else, we enable only an HTTP route - resources_to_append.append( + ingress_resources_to_append.append( self._construct_httproute(data, prefix, section_name="http") ) + ingress_policies_to_append.append(self._construct_ingress_auth_policy(data)) + if rel.active: - current_ingresses.extend(resources_to_append) + current_ingresses.extend(ingress_resources_to_append) + current_policies.extend(ingress_policies_to_append) external_url = self._generate_external_url(prefix) relation_mappings[rel] = external_url try: krm.reconcile(current_ingresses) + kam.reconcile(current_policies) for relation, url in relation_mappings.items(): self.ingress_per_appv2.wipe_ingress_data(relation) logger.debug(f"Publishing external URL for {relation.app.name}: {url}") @@ -697,5 +768,17 @@ def format_labels(label_dict: Dict[str, str]) -> str: return ",".join(f"{key}={value}" for key, value in label_dict.items()) +def _get_peer_identity_for_juju_application(app_name, namespace): + """Return a Juju application's peer identity. + + Format returned is defined by `principals` in + [this reference](https://istio.io/latest/docs/reference/config/security/authorization-policy/#Source): + + This function relies on the Juju convention that each application gets a ServiceAccount of the same name in the same + namespace. + """ + return f"cluster.local/ns/{namespace}/sa/{app_name}" + + if __name__ == "__main__": main(IstioIngressCharm) diff --git a/src/models.py b/src/models.py index 05605e7..fe618a6 100644 --- a/src/models.py +++ b/src/models.py @@ -6,7 +6,7 @@ from enum import Enum from typing import Dict, List, Optional -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict, Field # Global metadata schema @@ -166,3 +166,69 @@ class HTTPRouteResource(BaseModel): metadata: Metadata spec: HTTPRouteResourceSpec + + +# Authrization Policy schema +# Below is stripped down to cater for only L4 needed policies for ingress + + +class Action(str, Enum): + """Action is a type that represents the action to take when a rule matches.""" + + allow = "ALLOW" + + +class WorkloadSelector(BaseModel): + """WorkloadSelector defines the selector for the policy.""" + + matchLabels: Dict[str, str] + + +class Source(BaseModel): + """Source defines the source of the policy.""" + + principals: Optional[List[str]] = None + + +class From(BaseModel): + """From defines the source of the policy.""" + + source: Source + + +class Operation(BaseModel): + """Operation defines the operation of the To model.""" + + ports: Optional[List[str]] = None + paths: Optional[List[str]] = None + + +class To(BaseModel): + """To defines the destination of the policy.""" + + operation: Optional[Operation] = None + + +class AuthRule(BaseModel): + """AuthRule defines a policy rule.""" + + from_: Optional[List[From]] = Field(default=None, alias="from") + to: Optional[List[To]] = None + # Allows us to populate with `Rule(from_=[From()])`. Without this, we can only use they alias `from`, which is + # protected, meaning we could only build rules from a dict like `Rule(**{"from": [From()]})`. + model_config = ConfigDict(populate_by_name=True) + + +class AuthorizationPolicySpec(BaseModel): + """AuthorizationPolicySpec defines the spec of an Istio AuthorizationPolicy Kubernetes resource.""" + + action: Action = Action.allow + rules: List[AuthRule] + selector: WorkloadSelector + + +class AuthorizationPolicyResource(BaseModel): + """AuthorizationPolicyResource defines the structure of an Istio AuthorizationPolicy Kubernetes resource.""" + + metadata: Metadata + spec: AuthorizationPolicySpec diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 500b0a3..c339ecd 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -20,6 +20,12 @@ "HTTPRoute": create_namespaced_resource( "gateway.networking.k8s.io", "v1", "HTTPRoute", "httproutes" ), + "AuthorizationPolicy": create_namespaced_resource( + "security.istio.io", + "v1", + "AuthorizationPolicy", + "authorizationpolicies", + ), } @@ -107,6 +113,29 @@ async def get_route_spec(ops_test: OpsTest, route_name: str) -> Optional[Dict[st return None +async def get_auth_policy_spec(ops_test: OpsTest, policy_name: str) -> Optional[Dict[str, Any]]: + """Retrieve and check the spec of the AuthorizationPolicy resource. + + Args: + ops_test: pytest-operator plugin + policy_name: Name of the AuthorizationPolicy resource. + + Returns: + A dictionary representing the spec of the policy, or None if not found. + """ + model = ops_test.model.info + try: + c = lightkube.Client() + policy = c.get( + RESOURCE_TYPES["AuthorizationPolicy"], namespace=model.name, name=policy_name + ) + return policy.spec + + except Exception as e: + logger.error("Error retrieving AuthorizationPolicy condition: %s", e, exc_info=1) + return None + + async def get_route_condition(ops_test: OpsTest, route_name: str) -> Optional[Dict[str, Any]]: """Retrieve and check the condition from the HTTPRoute resource. diff --git a/tests/integration/test_charm_ipa.py b/tests/integration/test_charm_ipa.py index b3f3ad0..f33a4a1 100644 --- a/tests/integration/test_charm_ipa.py +++ b/tests/integration/test_charm_ipa.py @@ -14,6 +14,7 @@ ) from helpers import ( dequote, + get_auth_policy_spec, get_k8s_service_address, get_listener_condition, get_listener_spec, @@ -119,6 +120,49 @@ async def test_ipa_charm_has_ingress(ops_test: OpsTest): assert url == f"http://{istio_ingress_address}/{model}-ipa-tester" +@pytest.mark.abort_on_fail +async def test_auth_policy_validity(ops_test: OpsTest): + policy_name = f"{IPA_TESTER}-{APP_NAME}-{ops_test.model.name}-l4" + + # Retrieve the AuthorizationPolicy spec + policy_spec = await get_auth_policy_spec(ops_test, policy_name) + + # Ensure the policy spec is not None + assert policy_spec is not None, f"AuthorizationPolicy '{policy_name}' not found." + + # Validate the 'rules' structure + assert "rules" in policy_spec, "'rules' field is missing in the AuthorizationPolicy spec." + rules = policy_spec["rules"] + assert len(rules) == 1, "Expected exactly one rule in AuthorizationPolicy spec." + + # Validate the 'to' field inside the rule + to_rules = rules[0].get("to", []) + assert len(to_rules) == 1, "'to' field should contain exactly one operation." + assert "operation" in to_rules[0], "Missing 'operation' in the 'to' field." + assert to_rules[0]["operation"]["ports"] == [ + "8080" + ], "Port mismatch in the AuthorizationPolicy." + + # Validate the 'from' field inside the rule + from_rules = rules[0].get("from", []) + assert len(from_rules) == 1, "'from' field should contain exactly one source." + assert "source" in from_rules[0], "Missing 'source' in the 'from' field." + principals = from_rules[0]["source"].get("principals", []) + assert len(principals) == 1, "Expected exactly one principal in the 'source' field." + assert ( + principals[0] == f"cluster.local/ns/{ops_test.model.name}/sa/istio-ingress-k8s-istio" + ), "Principal does not match expected format." + + # Validate 'selector' field + assert ( + "selector" in policy_spec + ), "'selector' field is missing in the AuthorizationPolicy spec." + match_labels = policy_spec["selector"].get("matchLabels", {}) + assert ( + match_labels.get("app.kubernetes.io/name") == "ipa-tester" + ), "AuthorizationPolicy selector does not match the expected app name." + + @pytest.mark.abort_on_fail @pytest.mark.parametrize( "external_hostname, expected_hostname", diff --git a/tests/scenario/test_ingress.py b/tests/scenario/test_ingress.py index b752e4f..afbbfce 100644 --- a/tests/scenario/test_ingress.py +++ b/tests/scenario/test_ingress.py @@ -90,6 +90,41 @@ def test_construct_httproute( ) +def test_construct_ingress_auth_policy( + istio_ingress_charm, istio_ingress_context, mock_ingress_requirer_data +): + """Test that the _construct_ingress_auth_policy method constructs an Authorization Policy object correctly.""" + with istio_ingress_context( + istio_ingress_context.on.update_status(), + state=scenario.State(), + ) as manager: + charm: IstioIngressCharm = manager.charm + auth_policy = charm._construct_ingress_auth_policy( + data=mock_ingress_requirer_data, + ) + + # Verify the AuthorizationPolicy resource + assert auth_policy.metadata.name == f"app-name-{charm.app.name}-app-namespace-l4" + assert auth_policy.metadata.namespace == "app-namespace" + + # Check spec rules + assert len(auth_policy.spec["rules"]) == 1 + rule = auth_policy.spec["rules"][0] + + # Verify `to` field + assert rule["to"] == [{"operation": {"ports": ["80"]}}] + + # Verify `from` field (principals) + principals = rule["from"][0]["source"]["principals"] + expected_principal = f"cluster.local/ns/{charm.model.name}/sa/{charm.managed_name}" + assert principals == [expected_principal] + + # Verify workload selector + assert auth_policy.spec["selector"] == { + "matchLabels": {"app.kubernetes.io/name": "app-name"} + } + + def test_construct_redirect_to_https_httproute( istio_ingress_charm, istio_ingress_context, mock_ingress_requirer_data ): @@ -181,9 +216,13 @@ def test_sync_ingress_resources( istio_ingress_context, ): """Test that the _sync_ingress_resources constructs HTTP routes when TLS is not configured.""" - mock_krm = MagicMock() - mock_krm_factory = MagicMock(return_value=mock_krm) + # Mock Kubernetes Resource Managers + mock_ingress_manager = MagicMock() + mock_auth_manager = MagicMock() + mock_ingress_manager_factory = MagicMock(return_value=mock_ingress_manager) + mock_auth_manager_factory = MagicMock(return_value=mock_auth_manager) + # Initialize charm in test scenario with istio_ingress_context( istio_ingress_context.on.update_status(), state=scenario.State( @@ -192,20 +231,36 @@ def test_sync_ingress_resources( ), ) as manager: charm: IstioIngressCharm = manager.charm - charm._get_ingress_resource_manager = mock_krm_factory + + # Patch the managers into the charm + charm._get_ingress_resource_manager = mock_ingress_manager_factory + charm._get_authorization_policy_resource_manager = mock_auth_manager_factory + + # Call the method under test charm._sync_ingress_resources() - # Assert that we've tried to reconcile the kubernetes resources - charm._get_ingress_resource_manager().reconcile.assert_called_once() + # Assertions: Managers' reconcile methods are called once + mock_ingress_manager.reconcile.assert_called_once() + mock_auth_manager.reconcile.assert_called_once() - # Assert that _+_______________________ - resources = charm._get_ingress_resource_manager().reconcile.call_args[0][0] + # Retrieve the resources passed to reconcile + ingress_resources = mock_ingress_manager.reconcile.call_args[0][0] + auth_resources = mock_auth_manager.reconcile.call_args[0][0] - assert len(resources) == n_routes_expected - for route in resources: + # Assertions: Check resource counts + assert len(ingress_resources) == n_routes_expected + assert len(auth_resources) == n_routes_expected + + # Assertions: Verify each ingress resource's structure + for route in ingress_resources: assert len(route.spec["parentRefs"]) == 1 assert route.spec["parentRefs"][0]["sectionName"] == "http" + # Assertions: Verify authorization resources + for auth in auth_resources: + assert auth.metadata.name is not None + assert auth.metadata.namespace is not None + @pytest.mark.parametrize( "ingress_relations, n_routes_expected",