From 51337de07233efda776c2e521c5eb576be80fde7 Mon Sep 17 00:00:00 2001 From: PietroPasotti Date: Tue, 3 Sep 2024 10:58:55 +0200 Subject: [PATCH] basic auth support (#394) * basic auth implemented * static * simplified retry wrapper * simplified retry some more * refactored get url assertions * lint * fixed itest --- config.yaml | 9 ++ src/charm.py | 63 ++++++++----- src/traefik.py | 17 +++- tests/integration/test_basic_auth.py | 133 +++++++++++++++++++++++++++ tests/scenario/test_middlewares.py | 45 ++++++++- 5 files changed, 241 insertions(+), 26 deletions(-) create mode 100644 tests/integration/test_basic_auth.py diff --git a/config.yaml b/config.yaml index eff857a9..1e643be6 100644 --- a/config.yaml +++ b/config.yaml @@ -8,6 +8,15 @@ options: This feature is experimental and may be unstable. type: boolean default: False + basic_auth_user: + description: | + Enables the `basicAuth` middleware for **all** routes on this proxy. + The format of this string must be: `name:hashed-password`, generated with e.g. htpasswd. + Supported hashing algorithms are: MD5, SHA1, BCrypt. + For more documentation see https://doc.traefik.io/traefik/middlewares/http/basicauth/ + Once this config option is set, the username/password pair will be required to authenticate + http requests on all routes proxied by this traefik app. + type: string external_hostname: description: | The DNS name to be used by Traefik ingress. diff --git a/src/charm.py b/src/charm.py index 604ac01b..59aa5cd0 100755 --- a/src/charm.py +++ b/src/charm.py @@ -127,9 +127,7 @@ def __init__(self, *args): super().__init__(*args) self._stored.set_default( - current_external_host=None, - current_routing_mode=None, - current_forward_auth_mode=self.config["enable_experimental_forward_auth"], + config_hash=None, ) self.container = self.unit.get_container(_TRAEFIK_CONTAINER_NAME) @@ -173,6 +171,7 @@ def __init__(self, *args): tls_enabled=self._is_tls_enabled(), experimental_forward_auth_enabled=self._is_forward_auth_enabled, traefik_route_static_configs=self._traefik_route_static_configs(), + basic_auth_user=self._basic_auth_user, ) self.service_patch = KubernetesServicePatch( @@ -299,6 +298,14 @@ def _is_forward_auth_enabled(self) -> bool: return True return False + @property + def _basic_auth_user(self) -> Optional[str]: + """A single user for the global basic auth configuration. + + As we can't reject it, we assume it's correctly formatted. + """ + return cast(Optional[str], self.config.get("basic_auth_user", None)) + def _on_forward_auth_config_changed(self, event: AuthConfigChangedEvent): if self._is_forward_auth_enabled: if self.forward_auth.is_ready(): @@ -535,29 +542,41 @@ def _on_update_status(self, _: UpdateStatusEvent): self._process_status_and_configurations() self._set_workload_version() + @property + def _config_hash(self) -> int: + """A hash of the config of this application. + + Only include here the config options that, should they change, should trigger a recalculation of + the traefik config files. + The main goal of this logic is to avoid recalculating status and configs on each event, + since it can be quite expensive. + """ + return hash( + ( + self._external_host, + self.config["routing_mode"], + self._is_forward_auth_enabled, + self._basic_auth_user, + self._is_tls_enabled(), + ) + ) + def _on_config_changed(self, _: ConfigChangedEvent): - # If the external hostname is changed since we last processed it, we need to - # reconsider all data sent over the relations and all configs - new_external_host = self._external_host - new_routing_mode = self.config["routing_mode"] - new_forward_auth_mode = self._is_forward_auth_enabled + """Handle the ops.ConfigChanged event.""" + # that we're processing a config-changed event, doesn't necessarily mean that our config has changed (duh!) - # TODO set BlockedStatus here when compound_status is introduced - # https://github.com/canonical/operator/issues/665 + # If the config hash has changed since we last calculated it, we need to + # recompute our state from scratch, based on all data sent over the relations and all configs + new_config_hash = self._config_hash + if self._stored.config_hash != new_config_hash: + self._stored.config_hash = new_config_hash - if ( - self._stored.current_external_host != new_external_host # type: ignore - or self._stored.current_routing_mode != new_routing_mode # type: ignore - or self._stored.current_forward_auth_mode != new_forward_auth_mode # type: ignore - ): - self._process_status_and_configurations() - self._stored.current_external_host = new_external_host # type: ignore - self._stored.current_routing_mode = new_routing_mode # type: ignore - self._stored.current_forward_auth_mode = new_forward_auth_mode # type: ignore + if self._is_tls_enabled(): + # we keep this nested under the hash-check because, unless the tls config has + # changed, we don't need to redo this. + self._update_cert_configs() + self._configure_traefik() - if self._is_tls_enabled(): - self._update_cert_configs() - self._configure_traefik() self._process_status_and_configurations() def _process_status_and_configurations(self): diff --git a/src/traefik.py b/src/traefik.py index b1ce72c3..0d59efe7 100644 --- a/src/traefik.py +++ b/src/traefik.py @@ -104,6 +104,7 @@ def __init__( experimental_forward_auth_enabled: bool, tcp_entrypoints: Dict[str, int], traefik_route_static_configs: Iterable[Dict[str, Any]], + basic_auth_user: Optional[str] = None, ): self._container = container self._tcp_entrypoints = tcp_entrypoints @@ -111,6 +112,7 @@ def __init__( self._routing_mode = routing_mode self._tls_enabled = tls_enabled self._experimental_forward_auth_enabled = experimental_forward_auth_enabled + self._basic_auth_user = basic_auth_user @property def scrape_jobs(self) -> list: @@ -536,6 +538,14 @@ def _generate_middleware_config( "cannot create middleware: multi-types middleware not supported, consider declaring two different pieces of middleware instead" """ + basicauth_middleware = {} + if basicauth_user := self._basic_auth_user: + basicauth_middleware[f"juju-basic-auth-{prefix}"] = { + "basicAuth": { + "users": [basicauth_user], + } + } + forwardauth_middleware = {} if self._experimental_forward_auth_enabled: if forward_auth_app: @@ -560,7 +570,12 @@ def _generate_middleware_config( "redirectScheme": {"scheme": "https", "port": 443, "permanent": True} } - return {**forwardauth_middleware, **no_prefix_middleware, **redir_scheme_middleware} + return { + **forwardauth_middleware, + **no_prefix_middleware, + **redir_scheme_middleware, + **basicauth_middleware, + } @staticmethod def generate_tls_config_for_route( diff --git a/tests/integration/test_basic_auth.py b/tests/integration/test_basic_auth.py new file mode 100644 index 00000000..c578bd65 --- /dev/null +++ b/tests/integration/test_basic_auth.py @@ -0,0 +1,133 @@ +# Copyright 2022 Canonical Ltd. +# See LICENSE file for licensing details. +import asyncio +import subprocess +import time +import urllib.request +from urllib.error import HTTPError + +import pytest +import yaml +from pytest_operator.plugin import OpsTest +from tenacity import retry, stop_after_delay + +from tests.integration.conftest import ( + get_relation_data, + trfk_resources, +) + +USERNAME = "admin" +PASSWORD = "admin" + +# we don't expect a 200 because ipa-tester has no real server listening +SUCCESS_EXIT_CODE = 502 + +# user:hashed-password pair generated via https://www.transip.nl/htpasswd/ +TEST_AUTH_USER = r"admin:$2a$13$XOHdzKdVS4mPKT0LvOfXru4LqyLbwcEvFlssXGS3laC6d/i6cKrLS" +APP_NAME = "traefik" +IPA = "ipa-tester" + + +@pytest.mark.abort_on_fail +@pytest.mark.skip_on_deployed +async def test_deployment(ops_test: OpsTest, traefik_charm, ipa_tester_charm): + await asyncio.gather( + ops_test.model.deploy(traefik_charm, application_name=APP_NAME, resources=trfk_resources), + ops_test.model.deploy(ipa_tester_charm, IPA), + ) + + await ops_test.model.wait_for_idle([APP_NAME, IPA], status="active", timeout=1000) + + +@pytest.mark.abort_on_fail +@pytest.mark.skip_on_deployed +async def test_relate(ops_test: OpsTest): + await ops_test.model.add_relation("ipa-tester:ingress", f"{APP_NAME}:ingress") + await ops_test.model.wait_for_idle([APP_NAME, IPA]) + + +def get_tester_url(model): + data = get_relation_data( + requirer_endpoint=f"{IPA}/0:ingress", + provider_endpoint=f"{APP_NAME}/0:ingress", + model=model, + ) + provider_app_data = yaml.safe_load(data.provider.application_data["ingress"]) + return provider_app_data["url"] + + +@retry(stop=stop_after_delay(60 * 1)) # 5 minutes +def assert_get_url_returns(url: str, expected: int, auth: str = None): + print(f"attempting to curl {url} (with auth? {'yes' if auth else 'no'})") + if auth: + passman = urllib.request.HTTPPasswordMgrWithDefaultRealm() + passman.add_password(None, url, USERNAME, PASSWORD) + authhandler = urllib.request.HTTPBasicAuthHandler(passman) + opener = urllib.request.build_opener(authhandler) + urllib.request.install_opener(opener) + + try: + urllib.request.urlopen(url, timeout=1) + except HTTPError as e: + if e.code == expected: + return True + + print(f"unexpected exit code {e.code}") + time.sleep(0.1) + raise AssertionError + + if expected == 200: + return True + + print("unexpected 200") + time.sleep(0.1) + raise AssertionError + + +@pytest.fixture +def model(ops_test): + return ops_test.model_full_name + + +def set_basic_auth(model: str, user: str): + print(f"setting basic auth to {user!r}") + option = f"basic_auth_user={user}" if user else "basic_auth_user=" + subprocess.run(["juju", "config", "-m", model, APP_NAME, option]) + + +def test_ipa_charm_ingress_noauth(model): + # GIVEN basic auth is disabled (initial condition) + set_basic_auth(model, "") + tester_url = get_tester_url(model) + + # WHEN we GET the tester url + # THEN we get it fine + assert_get_url_returns(tester_url, expected=SUCCESS_EXIT_CODE) + + +def test_ipa_charm_ingress_auth(model): + # GIVEN basic auth is disabled (previous test) + tester_url = get_tester_url(model) + + # WHEN we enable basic auth + set_basic_auth(model, TEST_AUTH_USER) + + # THEN we can't GET the tester url + # might take a little bit to apply the new config + # 401 unauthorized + assert_get_url_returns(tester_url, expected=401) + + # UNLESS we use auth + assert_get_url_returns(tester_url, expected=SUCCESS_EXIT_CODE, auth=TEST_AUTH_USER) + + +def test_ipa_charm_ingress_auth_disable(model): + # GIVEN auth is enabled (previous test) + tester_url = get_tester_url(model) + + # WHEN we disable it again + set_basic_auth(model, "") + + # THEN we eventually can GET the endpoint without auth + # might take a little bit to apply the new config + assert_get_url_returns(tester_url, expected=SUCCESS_EXIT_CODE) diff --git a/tests/scenario/test_middlewares.py b/tests/scenario/test_middlewares.py index 7e17f5c8..de52eaed 100644 --- a/tests/scenario/test_middlewares.py +++ b/tests/scenario/test_middlewares.py @@ -2,15 +2,17 @@ # See LICENSE file for licensing details. import tempfile -import unittest +from pathlib import Path from unittest.mock import PropertyMock, patch import ops import pytest +import scenario import yaml from scenario import Container, ExecOutput, Mount, Relation, State from tests.scenario._utils import _render_config, create_ingress_relation +from traefik import DYNAMIC_CONFIG_DIR def _create_relation( @@ -127,5 +129,42 @@ def test_middleware_config( assert yaml.safe_load(config_file) == expected -if __name__ == "__main__": - unittest.main() +@patch("charm.TraefikIngressCharm.version", PropertyMock(return_value="0.0.0")) +def test_basicauth_config(traefik_ctx: scenario.Context): + # GIVEN traefik is configured with a sample basicauth user + ingress = create_ingress_relation() + basicauth_user = "user:hashed-password" + state = State( + config={"basic_auth_user": basicauth_user}, + relations=[ingress], + containers=[ + Container( + name="traefik", + can_connect=True, + exec_mock={ + ("find", "/opt/traefik/juju", "-name", "*.yaml", "-delete"): ExecOutput() + }, + layers={ + "traefik": ops.pebble.Layer({"services": {"traefik": {"startup": "enabled"}}}) + }, + service_status={"traefik": ops.pebble.ServiceStatus.ACTIVE}, + ) + ], + ) + + # WHEN we process a config-changed event + state_out = traefik_ctx.run("config_changed", state) + + # THEN traefik writes a dynamic config file with the expected basicauth middleware + traefik_fs = state_out.get_container("traefik").get_filesystem(traefik_ctx) + dynamic_config_path = ( + Path(str(traefik_fs) + DYNAMIC_CONFIG_DIR) + / f"juju_ingress_{ingress.endpoint}_{ingress.relation_id}_{ingress.remote_app_name}.yaml" + ) + assert dynamic_config_path.exists() + http_cfg = yaml.safe_load(dynamic_config_path.read_text())["http"] + assert http_cfg["middlewares"]["juju-basic-auth-test-model-remote-0"] == { + "basicAuth": {"users": [basicauth_user]} + } + for router in http_cfg["routers"].values(): + assert "juju-basic-auth-test-model-remote-0" in router["middlewares"]