fix(security): authenticate persisted egress secrets

bot_bottle/orchestrator/service.py

@@ -366,15 +366,23 @@ class OrchestratorCore:
         value with *env_var_secret*, and restores ``_tokens[bottle_id]``.
         Returns True on success, False when no stored secrets exist for this
         bottle or decryption fails (wrong key / corrupt data)."""
-        from .store.secret_store import decrypt_value
+        from .store.secret_store import decrypt_value, encrypt_value, is_legacy_blob
         encrypted = self.registry.get_agent_secrets(bottle_id)
         if not encrypted:
             return False
         try:
-            self._tokens[bottle_id] = {k: decrypt_value(env_var_secret, v)
-                                       for k, v in encrypted.items()}
+            decrypted = {
+                k: decrypt_value(env_var_secret, v) for k, v in encrypted.items()
+            }
         except ValueError:
             return False
+        self._tokens[bottle_id] = decrypted
+        if any(is_legacy_blob(value) for value in encrypted.values()):
+            migrated = {
+                key: encrypt_value(env_var_secret, value)
+                for key, value in decrypted.items()
+            }
+            self.registry.store_agent_secrets(bottle_id, migrated)
         return True
 
     # --- consolidated gateway ----------------------------------------------

bot_bottle/orchestrator/store/secret_store.py

@@ -12,9 +12,15 @@ reattachment path reads ENV_VAR_SECRET from the running agent container via
 ``POST /bottles/<id>/reprovision_gateway``; the orchestrator decrypts the
 stored rows and re-populates ``_tokens``.
 
-Encryption scheme: HMAC-SHA256 used as a PRF in CTR mode (stdlib-only,
-no external deps). Each value is encrypted independently. The output blob is
-``nonce (16 bytes) || ciphertext`` encoded as URL-safe base64 (no padding).
+Encryption scheme: encrypt-then-MAC using independent HMAC-SHA256-derived
+encryption and authentication subkeys (stdlib-only, no external deps). Each
+value is encrypted independently. New output blobs are:
+
+``version || nonce (16 bytes) || ciphertext || tag (32 bytes)``
+
+encoded as URL-safe base64 (no padding). The version marker lets the reader
+accept legacy ``nonce || ciphertext`` rows long enough to rewrite them in the
+authenticated format after a successful reprovision.
 
     keystream_block_i = HMAC-SHA256(key, nonce || i.to_bytes(4, "big"))
     ciphertext_i      = plaintext_i XOR keystream_block_i[:len(plaintext_i)]
@@ -30,6 +36,8 @@ import secrets
 _KEY_BYTES = 32    # 256-bit key from ENV_VAR_SECRET
 _NONCE_BYTES = 16  # 128-bit random nonce per encrypt call
 _BLOCK = 32        # HMAC-SHA256 output width == one keystream block
+_TAG_BYTES = 32
+_VERSION = b"BBSE1"
 
 # Env-var name the agent container receives at startup.
 ENV_VAR_SECRET_NAME = "ENV_VAR_SECRET"
@@ -41,7 +49,13 @@ def new_env_var_secret() -> str:
 
 
 def _b64dec(s: str) -> bytes:
-    return base64.urlsafe_b64decode(s + "=" * (-len(s) % 4))
+    return base64.b64decode(
+        s + "=" * (-len(s) % 4), altchars=b"-_", validate=True,
+    )
+
+
+def _subkey(key: bytes, purpose: bytes) -> bytes:
+    return hmac.new(key, b"bot-bottle-secret-store:" + purpose, hashlib.sha256).digest()
 
 
 def _keystream(key: bytes, nonce: bytes, block_index: int) -> bytes:
@@ -53,42 +67,90 @@ def _keystream(key: bytes, nonce: bytes, block_index: int) -> bytes:
 def encrypt_value(secret_b64: str, plaintext: str) -> str:
     """Encrypt a single string value with *secret_b64* (the ENV_VAR_SECRET).
 
-    Returns a URL-safe base64 blob ``nonce || ciphertext`` suitable for
+    Returns a URL-safe base64 authenticated blob suitable for
     the ``bottled_agent_secrets.value`` column."""
     key = _b64dec(secret_b64)
+    encryption_key = _subkey(key, b"encryption")
+    authentication_key = _subkey(key, b"authentication")
     pt = plaintext.encode()
     nonce = secrets.token_bytes(_NONCE_BYTES)
     ct = bytearray()
     for i in range(0, len(pt), _BLOCK):
         chunk = pt[i : i + _BLOCK]
-        ks = _keystream(key, nonce, i)[: len(chunk)]
+        ks = _keystream(encryption_key, nonce, i // _BLOCK)[: len(chunk)]
         ct.extend(p ^ k for p, k in zip(chunk, ks))
-    return base64.urlsafe_b64encode(nonce + bytes(ct)).rstrip(b"=").decode()
+    authenticated = _VERSION + nonce + bytes(ct)
+    tag = hmac.new(authentication_key, authenticated, hashlib.sha256).digest()
+    return base64.urlsafe_b64encode(authenticated + tag).rstrip(b"=").decode()
 
 
-def decrypt_value(secret_b64: str, blob_b64: str) -> str:
-    """Decrypt a blob produced by :func:`encrypt_value`.
-
-    Returns the original plaintext string. Raises ``ValueError`` for malformed
-    input or a key mismatch (wrong key produces garbage, not an error, unless
-    the plaintext is non-UTF-8 — treat all such failures as wrong key)."""
-    key = _b64dec(secret_b64)
+def is_legacy_blob(blob_b64: str) -> bool:
+    """Whether *blob_b64* uses the pre-authentication storage format."""
     try:
-        blob = _b64dec(blob_b64)
-    except Exception as exc:
-        raise ValueError(f"invalid ciphertext blob: {exc}") from exc
+        return not _b64dec(blob_b64).startswith(_VERSION)
+    except (ValueError, TypeError):
+        return False
+
+
+def _decrypt_legacy(key: bytes, blob: bytes) -> str:
+    """Read the original ``nonce || ciphertext`` format for migration only."""
     if len(blob) < _NONCE_BYTES:
         raise ValueError("ciphertext blob too short")
     nonce, ciphertext = blob[:_NONCE_BYTES], blob[_NONCE_BYTES:]
     pt = bytearray()
+    # The legacy format used the byte offset as the PRF counter.
     for i in range(0, len(ciphertext), _BLOCK):
         chunk = ciphertext[i : i + _BLOCK]
         ks = _keystream(key, nonce, i)[: len(chunk)]
         pt.extend(c ^ k for c, k in zip(chunk, ks))
     try:
         return bytes(pt).decode()
+    except UnicodeDecodeError as exc:
+        raise ValueError(f"decryption produced non-UTF-8 output: {exc}") from exc
+
+
+def decrypt_value(secret_b64: str, blob_b64: str) -> str:
+    """Decrypt a blob produced by :func:`encrypt_value`.
+
+    Returns the original plaintext string. Raises ``ValueError`` for malformed
+    input, authentication failure, or a key mismatch. Legacy unauthenticated
+    rows remain readable so callers can migrate them immediately."""
+    key = _b64dec(secret_b64)
+    try:
+        blob = _b64dec(blob_b64)
+    except (ValueError, TypeError) as exc:
+        raise ValueError(f"invalid ciphertext blob: {exc}") from exc
+    if not blob.startswith(_VERSION):
+        return _decrypt_legacy(key, blob)
+    minimum = len(_VERSION) + _NONCE_BYTES + _TAG_BYTES
+    if len(blob) < minimum:
+        raise ValueError("ciphertext blob too short")
+    authenticated, supplied_tag = blob[:-_TAG_BYTES], blob[-_TAG_BYTES:]
+    authentication_key = _subkey(key, b"authentication")
+    expected_tag = hmac.new(
+        authentication_key, authenticated, hashlib.sha256,
+    ).digest()
+    if not hmac.compare_digest(supplied_tag, expected_tag):
+        raise ValueError("ciphertext authentication failed")
+    nonce_start = len(_VERSION)
+    nonce = blob[nonce_start : nonce_start + _NONCE_BYTES]
+    ciphertext = blob[nonce_start + _NONCE_BYTES : -_TAG_BYTES]
+    encryption_key = _subkey(key, b"encryption")
+    pt = bytearray()
+    for i in range(0, len(ciphertext), _BLOCK):
+        chunk = ciphertext[i : i + _BLOCK]
+        ks = _keystream(encryption_key, nonce, i // _BLOCK)[: len(chunk)]
+        pt.extend(c ^ k for c, k in zip(chunk, ks))
+    try:
+        return bytes(pt).decode()
     except UnicodeDecodeError as exc:
         raise ValueError(f"decryption produced non-UTF-8 output (wrong key?): {exc}") from exc
 
 
-__all__ = ["ENV_VAR_SECRET_NAME", "new_env_var_secret", "encrypt_value", "decrypt_value"]
+__all__ = [
+    "ENV_VAR_SECRET_NAME",
+    "new_env_var_secret",
+    "encrypt_value",
+    "decrypt_value",
+    "is_legacy_blob",
+]

tests/unit/test_orchestrator_secret_store.py

@@ -2,6 +2,9 @@
 
 from __future__ import annotations
 
+import base64
+import hashlib
+import hmac
 import unittest
 
 from bot_bottle.orchestrator.store.secret_store import (
@@ -68,15 +71,28 @@ class TestDecryptErrors(unittest.TestCase):
     def test_wrong_key_raises_value_error(self) -> None:
         ct = encrypt_value(self.secret, "secret-token")
         other_key = new_env_var_secret()
-        # Wrong key produces garbage bytes; decrypt_value raises ValueError
-        # when the result is non-UTF-8 (which is very likely for 12-char data).
-        # We allow it to succeed only if garbage happens to be valid UTF-8, but
-        # the plaintext must not match.
-        try:
-            result = decrypt_value(other_key, ct)
-            self.assertNotEqual("secret-token", result)
-        except ValueError:
-            pass
+        with self.assertRaisesRegex(ValueError, "authentication failed"):
+            decrypt_value(other_key, ct)
+
+    def test_tampered_ciphertext_raises_value_error(self) -> None:
+        raw = bytearray(base64.urlsafe_b64decode(
+            encrypt_value(self.secret, "secret-token") + "=="
+        ))
+        raw[22] ^= 1
+        tampered = base64.urlsafe_b64encode(raw).rstrip(b"=").decode()
+        with self.assertRaisesRegex(ValueError, "authentication failed"):
+            decrypt_value(self.secret, tampered)
+
+    def test_reads_legacy_ciphertext_for_migration(self) -> None:
+        key = base64.urlsafe_b64decode(self.secret + "==")
+        nonce = b"0123456789abcdef"
+        plaintext = b"legacy-token"
+        stream = hmac.new(
+            key, nonce + (0).to_bytes(4, "big"), hashlib.sha256,
+        ).digest()
+        ciphertext = bytes(p ^ k for p, k in zip(plaintext, stream))
+        legacy = base64.urlsafe_b64encode(nonce + ciphertext).rstrip(b"=").decode()
+        self.assertEqual("legacy-token", decrypt_value(self.secret, legacy))
 
     def test_truncated_blob_raises_value_error(self) -> None:
         with self.assertRaises(ValueError):