Merge pull request #22530 from mvdbeek/issue-22515-vault-test

Fix Hashicorp Vault 2.0 non-canonical path rejection

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,25 @@ class HashicorpVault(Vault):
             )
             return None
 
+    def _read_legacy_and_migrate(self, key: str) -> Optional[str]:
+        # 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
+        # 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):
+            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:
+            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 +302,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()

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
 

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,40 @@ 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",
+            "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, 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}")
+        raise
     client.create_policy(
         "galaxy",
         r'path "secret/*" { capabilities = ["create","read","update","delete","list"] }',
@@ -170,18 +193,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 +220,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 -------------------------------------------------------------

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))
+
+    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"}
+    )
+
+
+@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()