Skip to content

Commit c8bcc4a

Browse files
authored
fix: weiger opstart bij onveilige SECRET_KEY in productie (#73)
refactor: rotating SECRET_KEY default + strict length check
1 parent 13451b3 commit c8bcc4a

5 files changed

Lines changed: 214 additions & 3 deletions

File tree

operations-manager/python/opi/core/config.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
1+
from __future__ import annotations
2+
13
import logging
24
import os
35
import pathlib
46

7+
from pydantic import Field, model_validator
58
from pydantic_settings import BaseSettings
69

710
# Initialize logging early to ensure it's available during config loading
811
from opi.core.early_logging import initialize_logging # noqa: F401 (side-effect import)
12+
from opi.core.secret_key import generate_secret_key, validate_secret_key
913
from opi.utils.logging_config import setup_logging
1014

1115
logger = logging.getLogger(__name__)
@@ -137,7 +141,7 @@ class Settings(BaseSettings):
137141
OWN_DOMAIN: str = "operations-manager.kind"
138142
ADDITIONAL_DOMAINS: str = "" # Comma-separated list of additional domains for redirect URIs
139143

140-
SECRET_KEY: str = "default-secret-key-for-development-change-in-production"
144+
SECRET_KEY: str = Field(default_factory=generate_secret_key)
141145
ENVIRONMENT: str = "local"
142146
DEBUG: bool = False
143147
CLUSTER_MANAGER: str = "local"
@@ -372,6 +376,12 @@ class Settings(BaseSettings):
372376
# a typical PVC+DB+bucket run to finish.
373377
BACKUP_LOCK_WAIT_SECONDS: int = 1800
374378

379+
@model_validator(mode="after")
380+
def _enforce_secure_secret_key(self) -> Settings:
381+
"""Fail closed when an explicitly-set SECRET_KEY is too short."""
382+
validate_secret_key(self.SECRET_KEY)
383+
return self
384+
375385

376386
def parse_sops_age_key_content(content: str) -> tuple[str | None, str | None]:
377387
"""
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
"""
2+
SECRET_KEY safety validation.
3+
4+
SECRET_KEY signs the session cookie (Starlette SessionMiddleware) and the
5+
websocket log-stream session. A weak or publicly known key lets an attacker
6+
forge a session for an arbitrary user, bypassing both HTTP and websocket auth.
7+
8+
Design: no hard-coded default in source. If SECRET_KEY is not set in env, the
9+
default factory generates a fresh cryptographically random key per process.
10+
Sessions then invalidate on restart -- acceptable for any deployment that has
11+
not explicitly opted in to a stable key. Operators who want session stability
12+
across reboots set SECRET_KEY in env (production already does this via the
13+
SOPS-encrypted operations-manager env secret).
14+
15+
When SECRET_KEY is set, it must be at least MIN_SECRET_KEY_LENGTH characters
16+
or startup fails closed. No cluster gate, no tolerated weak values.
17+
"""
18+
19+
import logging
20+
import secrets
21+
22+
logger = logging.getLogger(__name__)
23+
24+
# Minimum acceptable SECRET_KEY length. itsdangerous accepts any non-empty
25+
# key, so we enforce a floor that makes brute-forcing infeasible.
26+
MIN_SECRET_KEY_LENGTH = 32
27+
28+
29+
class InsecureSecretKeyError(RuntimeError):
30+
"""Raised at startup when SECRET_KEY is set but too short to be safe."""
31+
32+
33+
def generate_secret_key() -> str:
34+
"""
35+
Default factory: fresh per-process random key.
36+
37+
Emits a single WARNING so operators see in logs that they are running
38+
without a persistent key (sessions die on restart, and a multi-replica
39+
deployment cannot share sessions across pods).
40+
"""
41+
logger.warning(
42+
"SECRET_KEY not set; using a fresh random key for this process. "
43+
"Sessions will invalidate on restart and cannot be shared across "
44+
"replicas. Set SECRET_KEY in env for stable sessions."
45+
)
46+
return secrets.token_urlsafe(MIN_SECRET_KEY_LENGTH)
47+
48+
49+
def validate_secret_key(secret_key: str) -> None:
50+
"""
51+
Fail closed when SECRET_KEY is shorter than MIN_SECRET_KEY_LENGTH.
52+
53+
The default factory produces a key well above this threshold, so this
54+
validator only fires when an operator explicitly set a too-short value.
55+
56+
Raises:
57+
InsecureSecretKeyError: If the key is shorter than the minimum.
58+
"""
59+
if len(secret_key) < MIN_SECRET_KEY_LENGTH:
60+
raise InsecureSecretKeyError(
61+
f"Refusing to start: SECRET_KEY must be at least {MIN_SECRET_KEY_LENGTH} "
62+
"characters. The session cookie and websocket log-stream auth are signed "
63+
"with SECRET_KEY; a weak key lets an attacker forge a session for any "
64+
"user. Either unset SECRET_KEY (a fresh random key is generated per "
65+
"process; sessions invalidate on restart) or provide a strong value via "
66+
"the cluster's operations-manager env secret."
67+
)

operations-manager/python/tests/e2e/conftest.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@
3131

3232
# Sandbox config - override via environment variables
3333
E2E_BASE_URL = os.environ.get("E2E_BASE_URL", "")
34-
E2E_SECRET_KEY = os.environ.get("E2E_SECRET_KEY", "default-secret-key-for-development-change-in-production")
34+
# Sandbox tests sign cookies for an already-running cluster, so the secret
35+
# must match what that cluster uses. The default is only useful when no
36+
# sandbox tests are actually run.
37+
E2E_SECRET_KEY = os.environ.get("E2E_SECRET_KEY", "sandbox-e2e-test-secret-key-min32chars")
3538

3639
TEST_USER = {
3740
"sub": "e2e-user",

operations-manager/python/tests/e2e/testserver.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
logger = logging.getLogger(__name__)
2626

27-
SECRET_KEY = "e2e-test-secret-key"
27+
SECRET_KEY = "e2e-test-secret-key-padded-to-32-chars-minimum"
2828

2929
# Fixed test AGE keypair for E2E testing (DO NOT use in production)
3030
TEST_AGE_PUBLIC_KEY = "age10uegg2n4sxnsmpd00xjqh8e80hhrs9983yhy673gp8k0aevn4dtsn9d8xj"
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
"""
2+
Tests for the SECRET_KEY safety design.
3+
4+
Two layers:
5+
6+
1. The pure ``validate_secret_key`` / ``generate_secret_key`` logic.
7+
2. The real ``Settings`` model_validator wiring, so a regression in how the
8+
validator is hooked up (or a production-shaped environment) is actually
9+
caught here rather than silently booting with a forgeable key.
10+
11+
Design: no hard-coded dev default. If SECRET_KEY is unset, a fresh random key
12+
is generated per process (sessions invalidate on restart). If SECRET_KEY is
13+
set, it must be at least MIN_SECRET_KEY_LENGTH characters.
14+
15+
Run with:
16+
17+
uv run pytest --noconftest tests/test_secret_key_failclosed.py
18+
"""
19+
20+
import importlib
21+
22+
import pytest
23+
from opi.core.secret_key import (
24+
MIN_SECRET_KEY_LENGTH,
25+
InsecureSecretKeyError,
26+
generate_secret_key,
27+
validate_secret_key,
28+
)
29+
30+
STRONG_KEY = "x" * MIN_SECRET_KEY_LENGTH
31+
32+
33+
class TestGenerateSecretKey:
34+
"""The default factory must produce a key that passes its own validator."""
35+
36+
def test_generated_key_meets_minimum_length(self) -> None:
37+
key = generate_secret_key()
38+
assert len(key) >= MIN_SECRET_KEY_LENGTH
39+
40+
def test_generated_keys_are_unique(self) -> None:
41+
# Two calls must not collide -- this is the whole point of secrets.token_urlsafe.
42+
assert generate_secret_key() != generate_secret_key()
43+
44+
def test_generated_key_passes_validator(self) -> None:
45+
# The factory output must never fail the validator -- otherwise the
46+
# default code path raises at startup, which would be a regression.
47+
validate_secret_key(generate_secret_key())
48+
49+
def test_generate_logs_warning(self, caplog: pytest.LogCaptureFixture) -> None:
50+
with caplog.at_level("WARNING"):
51+
generate_secret_key()
52+
assert any("SECRET_KEY not set" in record.message for record in caplog.records)
53+
54+
55+
class TestValidateSecretKey:
56+
"""A short or missing key must raise; a sufficiently long key must pass."""
57+
58+
def test_empty_raises(self) -> None:
59+
with pytest.raises(InsecureSecretKeyError, match="at least"):
60+
validate_secret_key("")
61+
62+
def test_short_key_raises(self) -> None:
63+
short_key = "a" * (MIN_SECRET_KEY_LENGTH - 1)
64+
with pytest.raises(InsecureSecretKeyError, match="at least"):
65+
validate_secret_key(short_key)
66+
67+
def test_strong_key_passes(self) -> None:
68+
validate_secret_key(STRONG_KEY)
69+
70+
def test_key_at_exact_minimum_length_passes(self) -> None:
71+
validate_secret_key("k" * MIN_SECRET_KEY_LENGTH)
72+
73+
74+
def _load_settings_class(monkeypatch: pytest.MonkeyPatch):
75+
"""
76+
Import opi.core.config and return its Settings class.
77+
78+
config.py instantiates a module-level ``settings = Settings()`` on import,
79+
so we make sure no SECRET_KEY is set first -- the factory will then run
80+
and the module-level instantiation succeeds.
81+
82+
If the import fails for a reason unrelated to this fix (a stale installed
83+
package mismatch such as ``setup_logging() got an unexpected keyword
84+
argument`` that also breaks origin/main in this environment), the test is
85+
skipped rather than reported as a SECRET_KEY regression.
86+
"""
87+
monkeypatch.delenv("SECRET_KEY", raising=False)
88+
try:
89+
config = importlib.import_module("opi.core.config")
90+
config = importlib.reload(config)
91+
except InsecureSecretKeyError:
92+
raise
93+
except (TypeError, ImportError) as exc: # pre-existing unrelated env breakage
94+
pytest.skip(f"opi.core.config import broken by unrelated environment issue: {exc}")
95+
return config.Settings
96+
97+
98+
class TestSettingsModelValidatorWiring:
99+
"""
100+
Exercise the real Settings model_validator so a wiring regression (or a
101+
production-shaped env) fails the test instead of silently booting with a
102+
forgeable key. Also guards the `-> Settings` class-body NameError.
103+
"""
104+
105+
def test_config_module_imports(self, monkeypatch: pytest.MonkeyPatch) -> None:
106+
# Regression guard for the `-> Settings` NameError at class-body eval.
107+
# Reaching this line means the class body evaluated and the module-level
108+
# Settings() succeeded with the factory-generated key.
109+
_load_settings_class(monkeypatch)
110+
111+
def test_unset_env_uses_random_factory_key(self, monkeypatch: pytest.MonkeyPatch) -> None:
112+
settings_cls = _load_settings_class(monkeypatch)
113+
monkeypatch.delenv("SECRET_KEY", raising=False)
114+
settings = settings_cls(_env_file=None)
115+
assert len(settings.SECRET_KEY) >= MIN_SECRET_KEY_LENGTH
116+
# Second instantiation must produce a different key -- proves the
117+
# factory ran fresh and we are not pinned to a constant default.
118+
other = settings_cls(_env_file=None)
119+
assert settings.SECRET_KEY != other.SECRET_KEY
120+
121+
def test_short_env_key_refuses_to_boot(self, monkeypatch: pytest.MonkeyPatch) -> None:
122+
settings_cls = _load_settings_class(monkeypatch)
123+
monkeypatch.setenv("SECRET_KEY", "short")
124+
with pytest.raises(InsecureSecretKeyError):
125+
settings_cls(_env_file=None)
126+
127+
def test_strong_env_key_boots(self, monkeypatch: pytest.MonkeyPatch) -> None:
128+
settings_cls = _load_settings_class(monkeypatch)
129+
monkeypatch.setenv("SECRET_KEY", STRONG_KEY)
130+
settings = settings_cls(_env_file=None)
131+
assert settings.SECRET_KEY == STRONG_KEY

0 commit comments

Comments
 (0)