From df2be47bb1c5c117ec679ca1f58d2ed2727af68d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 20 Apr 2026 20:09:36 +0200 Subject: [PATCH 1/5] Fix Hashicorp Vault 2.0 non-canonical path rejection Vault 2.0 rejects KV paths with leading or double slashes that Vault 1.x silently normalized. VaultKeyPrefixWrapper emitted `/{prefix}/{key}`, producing `//galaxy/...` on the wire. Strip outer slashes from the configured prefix and validate it at construction, and fall back to the legacy double-slash key once on read so pre-upgrade secrets survive, rewriting under the canonical path. Integration test pins hashicorp/vault:2.0, passes dev-mode settings via env vars (the entrypoint duplicates CLI flags), and sets SKIP_SETCAP=1 so the non-root image doesn't abort on cap_ipc_lock. --- lib/galaxy/security/vault.py | 36 +++++++++++-- test/integration/test_hashicorp_vault.py | 55 ++++++++++++++------ test/unit/data/security/test_vault.py | 64 ++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 19 deletions(-) diff --git a/lib/galaxy/security/vault.py b/lib/galaxy/security/vault.py index fcdfbb041fc..26a2eb0fde1 100644 --- a/lib/galaxy/security/vault.py +++ b/lib/galaxy/security/vault.py @@ -147,8 +147,7 @@ class HashicorpVault(Vault): response = self.client.secrets.kv.read_secret_version(path=key) return response["data"]["data"].get("value") except hvac.exceptions.InvalidPath: - log.exception(f"Failed to read secret from Hashicorp Vault at key: {key}") - return None + return self._read_legacy_and_migrate(key) except hvac.exceptions.Forbidden: log.error( "Permission denied reading secret at key: %s. " @@ -157,6 +156,24 @@ class HashicorpVault(Vault): ) return None + def _read_legacy_and_migrate(self, key: str) -> Optional[str]: + # Galaxy < 26.x emitted a leading slash in Vault paths, which hvac's + # format_url turned into a double-slash KV v2 key. Vault 1.x accepted + # it silently; Vault 2.0 rejects it. Fall back to reading the legacy + # form and rewrite under the canonical key so the secret survives the + # Galaxy upgrade. This fallback can be removed after a deprecation + # window once operators have migrated. + legacy = f"/{key}" + try: + response = self.client.secrets.kv.read_secret_version(path=legacy) + except (hvac.exceptions.InvalidPath, hvac.exceptions.InvalidRequest): + return None + value = response["data"]["data"].get("value") + if value is not None: + log.warning("Migrating legacy non-canonical Vault secret to canonical path: %s", key) + self.client.secrets.kv.v2.create_or_update_secret(path=key, secret={"value": value}) + return value + def write_secret(self, key: str, value: str) -> None: try: self.client.secrets.kv.v2.create_or_update_secret(path=key, secret={"value": value}) @@ -284,13 +301,22 @@ class VaultKeyPrefixWrapper(Vault): def __init__(self, vault: Vault, prefix: str): self.vault = vault - self.prefix = prefix.strip("/") + # Strip conventional outer slashes so admins can write `/galaxy`, + # `galaxy`, or `/galaxy/` interchangeably in config. Reject anything + # that would still produce a non-canonical Vault path after stripping. + stripped = prefix.strip("/") + if not stripped or VAULT_KEY_INVALID_REGEX.search(stripped): + raise InvalidVaultConfigException( + f"Vault path_prefix {prefix!r} is invalid: must be non-empty and must not contain " + "double slashes or whitespace adjacent to a slash." + ) + self.prefix = stripped def read_secret(self, key: str) -> Optional[str]: - return self.vault.read_secret(f"/{self.prefix}/{key}") + return self.vault.read_secret(f"{self.prefix}/{key}") def write_secret(self, key: str, value: str) -> None: - return self.vault.write_secret(f"/{self.prefix}/{key}", value) + return self.vault.write_secret(f"{self.prefix}/{key}", value) def list_secrets(self, key: str) -> list[str]: raise NotImplementedError() diff --git a/test/integration/test_hashicorp_vault.py b/test/integration/test_hashicorp_vault.py index dc0c6dae041..64775da32ca 100644 --- a/test/integration/test_hashicorp_vault.py +++ b/test/integration/test_hashicorp_vault.py @@ -1,7 +1,7 @@ """Integration tests for Hashicorp Vault token renewal using a real Vault Docker container. -Requires Docker to be available. The test starts a hashicorp/vault container in dev mode, -creates a renewable token, and verifies the renewal logic works end-to-end. +Requires Docker to be available. The test starts a hashicorp/vault container in dev mode +(Vault 2.0+), creates a renewable token, and verifies the renewal logic works end-to-end. """ import os @@ -11,7 +11,9 @@ import threading import time import requests +from celery.beat import Service as BeatService +from galaxy.celery import celery_app from galaxy.security.vault import ( _unwrap_vault, HashicorpVault, @@ -27,7 +29,7 @@ from galaxy_test.driver.integration_util import ( VAULT_DEV_ROOT_TOKEN = "vault-integration-test-token" VAULT_PORT = 18200 -VAULT_IMAGE = "hashicorp/vault" +VAULT_IMAGE = "hashicorp/vault:2.0" CREDENTIALS_TOOL = "secret_tool" CREDENTIALS_VARIABLES = [{"name": "server", "value": "http://localhost:8080"}] CREDENTIALS_SECRETS = [{"name": "username", "value": "user"}, {"name": "password", "value": "pass"}] @@ -89,19 +91,43 @@ def _start_vault_container(container_name): docker_rm(container_name) except subprocess.CalledProcessError: pass + # Pre-pull so wait_ready only covers container boot, not a ~180 MB cold pull. + subprocess.check_call(["docker", "pull", VAULT_IMAGE]) + # Pass dev-mode settings via env vars consumed by the image's + # docker-entrypoint.sh. Passing them as CLI flags causes the entrypoint to + # inject empty `-dev-root-token-id=` / `-dev-listen-address=` duplicates + # ahead of our values, which Vault 2.0 handles less forgivingly than 1.x. + # SKIP_SETCAP=1 disables the entrypoint's `setcap cap_ipc_lock=+ep`, which + # aborts the container (under `set -e`) when the image's default non-root + # `vault` user lacks CAP_SETFCAP. mlock is irrelevant in dev mode. docker_run( VAULT_IMAGE, container_name, "server", "-dev", - f"-dev-root-token-id={VAULT_DEV_ROOT_TOKEN}", - "-dev-listen-address=0.0.0.0:8200", ports=[(VAULT_PORT, 8200)], - env_vars={"VAULT_ADDR": "http://0.0.0.0:8200"}, + env_vars={ + "VAULT_DEV_ROOT_TOKEN_ID": VAULT_DEV_ROOT_TOKEN, + "VAULT_DEV_LISTEN_ADDRESS": "0.0.0.0:8200", + "VAULT_ADDR": "http://0.0.0.0:8200", + "SKIP_SETCAP": "1", + }, ) vault_addr = f"http://127.0.0.1:{VAULT_PORT}" client = VaultClient(vault_addr, VAULT_DEV_ROOT_TOKEN) - client.wait_ready() + try: + client.wait_ready() + except Exception: + # Dump container logs so the reason (image pull, bind failure, crash) + # is visible when the health check doesn't come up. + try: + logs = subprocess.check_output(["docker", "logs", container_name], stderr=subprocess.STDOUT).decode( + "utf-8", errors="replace" + ) + print(f"=== docker logs {container_name} ===\n{logs}\n=== end logs ===") + except subprocess.CalledProcessError as log_err: + print(f"Could not fetch docker logs for {container_name}: {log_err}") + raise client.create_policy( "galaxy", r'path "secret/*" { capabilities = ["create","read","update","delete","list"] }', @@ -170,18 +196,21 @@ class TestHashicorpVaultRenewalGalaxyIntegration(integration_util.IntegrationTes @classmethod def _start_beat(cls): - from celery.beat import Service as BeatService - - from galaxy.celery import celery_app - module_name = celery_app.trim_module_name("galaxy.celery.tasks") + task_name = f"{module_name}.renew_vault_token" schedule = dict(celery_app.conf.beat_schedule or {}) schedule["renew-vault-token"] = { - "task": f"{module_name}.renew_vault_token", + "task": task_name, "schedule": cls.RENEWAL_INTERVAL, } celery_app.conf.beat_schedule = schedule celery_app.conf.task_always_eager = True + # Under task_always_eager, Celery validates the no-arg Beat invocation + # against the task's `(vault: Vault)` signature before our galaxy_task + # wrapper can inject `vault` from the DI container, and aborts with + # SchedulingError. Real workers deserialize args from AMQP and skip + # this check, so the typing assertion is test-only noise. + celery_app.tasks[task_name].typing = False cls._beat_service = BeatService(celery_app, max_interval=cls.RENEWAL_INTERVAL) cls._beat_thread = threading.Thread(target=cls._beat_service.start, daemon=True) @@ -194,8 +223,6 @@ class TestHashicorpVaultRenewalGalaxyIntegration(integration_util.IntegrationTes cls._beat_service.stop() if cls._beat_thread: cls._beat_thread.join(timeout=5) - from galaxy.celery import celery_app - celery_app.conf.task_always_eager = False # ---- helpers ------------------------------------------------------------- diff --git a/test/unit/data/security/test_vault.py b/test/unit/data/security/test_vault.py index 0b467e1523b..87aa86fc437 100644 --- a/test/unit/data/security/test_vault.py +++ b/test/unit/data/security/test_vault.py @@ -1,7 +1,9 @@ import os import string import tempfile +from unittest.mock import MagicMock +import hvac import pytest from cryptography.fernet import InvalidToken @@ -12,10 +14,13 @@ from galaxy.model.unittest_utils.data_app import ( from galaxy.security.vault import ( _unwrap_vault, HashicorpVault, + InvalidVaultConfigException, InvalidVaultKeyException, renew_vault_token_if_needed, Vault, VaultFactory, + VaultKeyPrefixWrapper, + VaultKeyValidationWrapper, ) from galaxy.util.unittest import TestCase @@ -123,3 +128,62 @@ class TestDatabaseVault(AbstractTestCases.VaultTestBase): vault = VaultFactory.from_app(app) with self.assertRaises(InvalidToken): vault.read_secret("my/incorrect/secret") + + +def _make_mocked_hashicorp_vault() -> HashicorpVault: + inner = HashicorpVault.__new__(HashicorpVault) + inner.client = MagicMock() + return inner + + +@pytest.mark.parametrize("prefix", ["/galaxy", "galaxy", "/galaxy/"]) +def test_vault_key_prefix_wrapper_emits_canonical_path(prefix): + # Admins may write path_prefix with or without surrounding slashes; all + # canonical spellings must produce the same canonical Vault path. Vault 2.0 + # rejects leading or double slashes in URLs. + inner = _make_mocked_hashicorp_vault() + inner.client.secrets.kv.read_secret_version.return_value = {"data": {"data": {"value": "v"}}} + vault = VaultKeyValidationWrapper(VaultKeyPrefixWrapper(inner, prefix=prefix)) + + vault.write_secret("user/1/preferences/editor", "vscode") + inner.client.secrets.kv.v2.create_or_update_secret.assert_called_once_with( + path="galaxy/user/1/preferences/editor", secret={"value": "vscode"} + ) + + assert vault.read_secret("user/1/preferences/editor") == "v" + inner.client.secrets.kv.read_secret_version.assert_called_once_with(path="galaxy/user/1/preferences/editor") + + +@pytest.mark.parametrize("prefix", ["", "/", "gal//axy", "gal /axy", "gal/ axy"]) +def test_vault_key_prefix_wrapper_rejects_invalid_prefix(prefix): + # Anything that would still produce a non-canonical Vault path after + # stripping outer slashes must raise at construction time rather than + # silently getting normalized into something hvac sends on the wire. + with pytest.raises(InvalidVaultConfigException): + VaultKeyPrefixWrapper(_make_mocked_hashicorp_vault(), prefix=prefix) + + +def test_hashicorp_vault_read_migrates_legacy_double_slash_secret(): + inner = _make_mocked_hashicorp_vault() + # First canonical read raises InvalidPath; legacy read returns a value. + inner.client.secrets.kv.read_secret_version.side_effect = [ + hvac.exceptions.InvalidPath(), + {"data": {"data": {"value": "legacy-value"}}}, + ] + + value = inner.read_secret("galaxy/user/1/x") + + assert value == "legacy-value" + assert inner.client.secrets.kv.read_secret_version.call_args_list[0].kwargs == {"path": "galaxy/user/1/x"} + assert inner.client.secrets.kv.read_secret_version.call_args_list[1].kwargs == {"path": "/galaxy/user/1/x"} + inner.client.secrets.kv.v2.create_or_update_secret.assert_called_once_with( + path="galaxy/user/1/x", secret={"value": "legacy-value"} + ) + + +def test_hashicorp_vault_read_missing_returns_none_without_rewrite(): + inner = _make_mocked_hashicorp_vault() + inner.client.secrets.kv.read_secret_version.side_effect = hvac.exceptions.InvalidPath() + + assert inner.read_secret("galaxy/user/1/missing") is None + inner.client.secrets.kv.v2.create_or_update_secret.assert_not_called() From b8cb218c4c00a61132715b417e0dec29d989cf40 Mon Sep 17 00:00:00 2001 From: Marius van den Beek Date: Tue, 21 Apr 2026 11:23:04 +0200 Subject: [PATCH 2/5] Simplify subprocess call, drop unused env var Co-authored-by: Nicola Soranzo --- test/integration/test_hashicorp_vault.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/integration/test_hashicorp_vault.py b/test/integration/test_hashicorp_vault.py index 64775da32ca..95a0a728ca3 100644 --- a/test/integration/test_hashicorp_vault.py +++ b/test/integration/test_hashicorp_vault.py @@ -109,7 +109,6 @@ def _start_vault_container(container_name): env_vars={ "VAULT_DEV_ROOT_TOKEN_ID": VAULT_DEV_ROOT_TOKEN, "VAULT_DEV_LISTEN_ADDRESS": "0.0.0.0:8200", - "VAULT_ADDR": "http://0.0.0.0:8200", "SKIP_SETCAP": "1", }, ) @@ -121,9 +120,7 @@ def _start_vault_container(container_name): # Dump container logs so the reason (image pull, bind failure, crash) # is visible when the health check doesn't come up. try: - logs = subprocess.check_output(["docker", "logs", container_name], stderr=subprocess.STDOUT).decode( - "utf-8", errors="replace" - ) + logs = subprocess.check_output(["docker", "logs", container_name], stderr=subprocess.STDOUT, text=True) print(f"=== docker logs {container_name} ===\n{logs}\n=== end logs ===") except subprocess.CalledProcessError as log_err: print(f"Could not fetch docker logs for {container_name}: {log_err}") From 8e5801c95f1becf77bde176b4ed80c9266786b91 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 21 Apr 2026 11:13:45 +0200 Subject: [PATCH 3/5] Add hvac to galaxy-data test extras The packaged galaxy-data test suite imports `hvac` (via the vault tests), so installing the `test` extra needs to pull it in for the package tests to pass in CI. --- packages/data/setup.cfg | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/data/setup.cfg b/packages/data/setup.cfg index 15ae09a6a85..3cfb6230e60 100644 --- a/packages/data/setup.cfg +++ b/packages/data/setup.cfg @@ -70,8 +70,9 @@ packages = find_namespace: python_requires = >=3.10 [options.extras_require] -test = +test = galaxy-config + hvac pytest roc-validator From 098e5bf4e30f239b7ea0a7ede941d74ab429cc28 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 21 Apr 2026 11:20:07 +0200 Subject: [PATCH 4/5] Do log exception on legacy fallback --- lib/galaxy/security/vault.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/galaxy/security/vault.py b/lib/galaxy/security/vault.py index 26a2eb0fde1..2fe24df4e2a 100644 --- a/lib/galaxy/security/vault.py +++ b/lib/galaxy/security/vault.py @@ -167,6 +167,7 @@ class HashicorpVault(Vault): try: response = self.client.secrets.kv.read_secret_version(path=legacy) except (hvac.exceptions.InvalidPath, hvac.exceptions.InvalidRequest): + log.exception(f"Failed to read secret from Hashicorp Vault at key: {key}") return None value = response["data"]["data"].get("value") if value is not None: From 16fb151cbb5084b7844d25d96de9704c1eb385bc Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 21 Apr 2026 15:18:50 +0200 Subject: [PATCH 5/5] Address review: clarify version comment and swap test order --- lib/galaxy/security/vault.py | 2 +- test/unit/data/security/test_vault.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/security/vault.py b/lib/galaxy/security/vault.py index 2fe24df4e2a..af481ef4743 100644 --- a/lib/galaxy/security/vault.py +++ b/lib/galaxy/security/vault.py @@ -157,7 +157,7 @@ class HashicorpVault(Vault): return None def _read_legacy_and_migrate(self, key: str) -> Optional[str]: - # Galaxy < 26.x emitted a leading slash in Vault paths, which hvac's + # Galaxy <= 26.0 emitted a leading slash in Vault paths, which hvac's # format_url turned into a double-slash KV v2 key. Vault 1.x accepted # it silently; Vault 2.0 rejects it. Fall back to reading the legacy # form and rewrite under the canonical key so the secret survives the diff --git a/test/unit/data/security/test_vault.py b/test/unit/data/security/test_vault.py index 87aa86fc437..26023fe01f6 100644 --- a/test/unit/data/security/test_vault.py +++ b/test/unit/data/security/test_vault.py @@ -145,14 +145,14 @@ def test_vault_key_prefix_wrapper_emits_canonical_path(prefix): inner.client.secrets.kv.read_secret_version.return_value = {"data": {"data": {"value": "v"}}} vault = VaultKeyValidationWrapper(VaultKeyPrefixWrapper(inner, prefix=prefix)) + assert vault.read_secret("user/1/preferences/editor") == "v" + inner.client.secrets.kv.read_secret_version.assert_called_once_with(path="galaxy/user/1/preferences/editor") + vault.write_secret("user/1/preferences/editor", "vscode") inner.client.secrets.kv.v2.create_or_update_secret.assert_called_once_with( path="galaxy/user/1/preferences/editor", secret={"value": "vscode"} ) - assert vault.read_secret("user/1/preferences/editor") == "v" - inner.client.secrets.kv.read_secret_version.assert_called_once_with(path="galaxy/user/1/preferences/editor") - @pytest.mark.parametrize("prefix", ["", "/", "gal//axy", "gal /axy", "gal/ axy"]) def test_vault_key_prefix_wrapper_rejects_invalid_prefix(prefix):