From c7550cbf0957e8fdbb7de23090f4a9144e8d950c Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Mon, 8 Jun 2026 15:21:27 +0200 Subject: [PATCH] fix(security): lock down signing-key file permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Private JWK files were written under the default umask (observed 0664 — group and world readable). Create the key directory 0700, chmod private key files (private_jwks.json, token_jwks.json) to 0600 after they are written, and refuse to start if a pre-existing private key is group/world accessible. Tests now use an isolated per-test key directory. Refs: porchlight-91i Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/oidc/provider.py | 27 ++++++++++++++++++++++++++- tests/conftest.py | 10 ++++++++-- tests/test_client_registration.py | 8 +++++--- tests/test_oidc/test_provider.py | 28 ++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/porchlight/oidc/provider.py b/src/porchlight/oidc/provider.py index aae64d3..bcf1b71 100644 --- a/src/porchlight/oidc/provider.py +++ b/src/porchlight/oidc/provider.py @@ -1,5 +1,6 @@ """idpyoidc Server initialization.""" +import os from pathlib import Path from idpyoidc.server import Server @@ -144,8 +145,32 @@ def _build_server_config(settings: Settings) -> dict: } +_PRIVATE_KEY_FILES = ("private_jwks.json", "token_jwks.json") + + def create_oidc_server(settings: Settings) -> Server: - """Create and configure an idpyoidc Server instance.""" + """Create and configure an idpyoidc Server instance. + + Private signing keys are written to ``signing_key_path``; lock the directory + to 0700 and the private key files to 0600, and refuse to start if a + pre-existing private key is group/world accessible (a key disclosure). + """ + key_path = Path(settings.signing_key_path) + key_path.mkdir(parents=True, exist_ok=True) + os.chmod(key_path, 0o700) + + # Fail on pre-existing keys with loose permissions (left by a prior run). + for name in _PRIVATE_KEY_FILES: + f = key_path / name + if f.exists() and (f.stat().st_mode & 0o077): + raise RuntimeError(f"Insecure permission on {f}: private key is group/world accessible") + config = _build_server_config(settings) server = Server(conf=config) + + # Lock down any keys idpyoidc just wrote (umask may have left them 0644). + for name in _PRIVATE_KEY_FILES: + f = key_path / name + if f.exists(): + os.chmod(f, 0o600) return server diff --git a/tests/conftest.py b/tests/conftest.py index cc9eee3..72256b3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import re from collections.abc import AsyncIterator +from pathlib import Path import pytest from httpx import ASGITransport, AsyncClient @@ -10,8 +11,13 @@ from porchlight.rate_limit import limiter @pytest.fixture -def settings() -> Settings: - return Settings(issuer="http://localhost:8000", sqlite_path=":memory:", session_https_only=False) +def settings(tmp_path: Path) -> Settings: + return Settings( + issuer="http://localhost:8000", + sqlite_path=":memory:", + session_https_only=False, + signing_key_path=str(tmp_path / "keys"), + ) @pytest.fixture diff --git a/tests/test_client_registration.py b/tests/test_client_registration.py index ea6ba37..acfe41f 100644 --- a/tests/test_client_registration.py +++ b/tests/test_client_registration.py @@ -19,7 +19,7 @@ scope = ["openid", "profile"] toml_file = tmp_path / "test.toml" toml_file.write_text(toml_content) - settings = Settings(_toml_file=str(toml_file), session_secret="x" * 32) + settings = Settings(_toml_file=str(toml_file), session_secret="x" * 32, signing_key_path=str(tmp_path / "keys")) app = create_app(settings) async with ( @@ -39,9 +39,11 @@ scope = ["openid", "profile"] assert cdb_entry["allowed_scopes"] == ["openid", "profile"] -async def test_manage_app_always_registered() -> None: +async def test_manage_app_always_registered(tmp_path: Path) -> None: """The internal manage-app client is always registered, even without config file clients.""" - settings = Settings(issuer="https://test.example.com", session_secret="x" * 32) + settings = Settings( + issuer="https://test.example.com", session_secret="x" * 32, signing_key_path=str(tmp_path / "keys") + ) app = create_app(settings) async with ( diff --git a/tests/test_oidc/test_provider.py b/tests/test_oidc/test_provider.py index ebfbf97..e57eb92 100644 --- a/tests/test_oidc/test_provider.py +++ b/tests/test_oidc/test_provider.py @@ -1,6 +1,9 @@ +import os import shutil from pathlib import Path +import pytest + from porchlight.config import Settings from porchlight.oidc.claims import PorchlightUserInfo from porchlight.oidc.provider import create_oidc_server @@ -53,3 +56,28 @@ def test_create_server_userinfo_is_porchlight() -> None: assert isinstance(server.context.userinfo, PorchlightUserInfo) finally: shutil.rmtree(key_path, ignore_errors=True) + + +def test_signing_key_files_have_strict_permissions(tmp_path: Path) -> None: + key_path = tmp_path / "keys" + settings = Settings(issuer="http://localhost:8000", sqlite_path=":memory:", signing_key_path=str(key_path)) + create_oidc_server(settings) + + assert (key_path.stat().st_mode & 0o077) == 0, "key directory must not be group/world accessible" + for name in ("private_jwks.json", "token_jwks.json"): + f = key_path / name + assert f.exists() + assert (f.stat().st_mode & 0o077) == 0, f"{name} must be 0600" + + +def test_startup_fails_on_world_readable_private_key(tmp_path: Path) -> None: + key_path = tmp_path / "keys" + key_path.mkdir() + # Simulate a pre-existing private key left group/world readable. + leaked = key_path / "private_jwks.json" + leaked.write_text("{}") + os.chmod(leaked, 0o644) + + settings = Settings(issuer="http://localhost:8000", sqlite_path=":memory:", signing_key_path=str(key_path)) + with pytest.raises(RuntimeError, match="permission"): + create_oidc_server(settings)