diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ab64c96..2d928dd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,7 @@ jobs: persist-credentials: false - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: ${{ matrix.python-version }} @@ -51,7 +51,7 @@ jobs: persist-credentials: false - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.12" @@ -67,11 +67,12 @@ jobs: run: pip-audit -r requirements.txt || pip-audit --desc - name: Check for secrets - uses: trufflesecurity/trufflehog@30d5bb91af1a771378349dbbb0c82129392acf70 # v3.95.6 + uses: trufflesecurity/trufflehog@6f3c981e7b77f235fd2702dd74af25fc4b72bf11 # v3.96.0 with: path: ./ base: "" head: ${{ github.sha }} + extra_args: --exclude-paths=.trufflehogignore build: runs-on: ubuntu-latest @@ -82,7 +83,7 @@ jobs: persist-credentials: false - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.12" diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index cebc5ec..8d65a0c 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -19,7 +19,7 @@ jobs: persist-credentials: false - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.12" @@ -38,4 +38,4 @@ jobs: run: pip install twine && twine check dist/* - name: Publish to PyPI - uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b + uses: pypa/gh-action-pypi-publish@a892a5a61159132606e93a2fa6f4358831b04d26 # v1.14.2 diff --git a/.gitignore b/.gitignore index 2139fe5..6ba980f 100644 --- a/.gitignore +++ b/.gitignore @@ -29,4 +29,6 @@ node_modules # Operational state (not for commit) LEARNING/ -_cowork_ops/ \ No newline at end of file +_cowork_ops/ +# Windows reserved device name artifact +nul diff --git a/.trufflehogignore b/.trufflehogignore new file mode 100644 index 0000000..b198d17 --- /dev/null +++ b/.trufflehogignore @@ -0,0 +1 @@ +^tests/.* diff --git a/conftest.py b/conftest.py index c9861f1..d52023f 100644 --- a/conftest.py +++ b/conftest.py @@ -1,4 +1,5 @@ """pytest configuration — add project src to Python path and skip rate limits.""" + import os import sys from pathlib import Path diff --git a/src/apiauth/cli.py b/src/apiauth/cli.py index 8a3468a..78619ef 100644 --- a/src/apiauth/cli.py +++ b/src/apiauth/cli.py @@ -25,11 +25,14 @@ try: from revenueholdings_license import require_license except ImportError: + def require_license(tool): def decorator(func): return func + return decorator + console = Console() err_console = Console(stderr=True) @@ -339,9 +342,7 @@ def import_key( now = _timestamp() expiry = None if expiry_days: - expiry = ( - dt.datetime.now(dt.timezone.utc) + dt.timedelta(days=expiry_days) - ).isoformat()[:23] + "Z" + expiry = (dt.datetime.now(dt.timezone.utc) + dt.timedelta(days=expiry_days)).isoformat()[:23] + "Z" entry = { "type": "api_key", @@ -436,18 +437,18 @@ def _export_github_actions(active: list[dict]) -> None: console.print("# GitHub Actions: Add these as repository secrets or use with actions/env") for k in active: prefix = _make_env_prefix(k) - console.print(f"echo \"{prefix}_ID={k['id']}\" >> $GITHUB_ENV") - console.print(f"echo \"{prefix}_SERVICE={k.get('service', '')}\" >> $GITHUB_ENV") - console.print(f"echo \"{prefix}_CREATED={k.get('created_at', '')}\" >> $GITHUB_ENV") + console.print(f'echo "{prefix}_ID={k["id"]}" >> $GITHUB_ENV') + console.print(f'echo "{prefix}_SERVICE={k.get("service", "")}" >> $GITHUB_ENV') + console.print(f'echo "{prefix}_CREATED={k.get("created_at", "")}" >> $GITHUB_ENV') if k.get("expires_at"): - console.print(f"echo \"{prefix}_EXPIRES={k['expires_at']}\" >> $GITHUB_ENV") + console.print(f'echo "{prefix}_EXPIRES={k["expires_at"]}" >> $GITHUB_ENV') console.print() console.print("# Or add to .github/workflows/*.yml env: block:") console.print("env:") for k in active: prefix = _make_env_prefix(k) - console.print(f" {prefix}_ID: \"{k['id']}\"") - console.print(f" {prefix}_SERVICE: \"{k.get('service', '')}\"") + console.print(f' {prefix}_ID: "{k["id"]}"') + console.print(f' {prefix}_SERVICE: "{k.get("service", "")}"') # ── audit ───────────────────────────────────────────────────────────── @@ -499,9 +500,7 @@ def audit(ctx: click.Context, exit_on_expired: bool, exit_on_revoked: bool) -> N console.print(f"[yellow]⚠ {len(expiring)} EXPIRING key(s) (within 7 days):[/yellow]") for k in expiring: console.print( - f" [yellow]{k['id']}[/yellow] " - f"{k.get('name', '')} — expires " - f"{_short_ts(k.get('expires_at', ''))}" + f" [yellow]{k['id']}[/yellow] {k.get('name', '')} — expires {_short_ts(k.get('expires_at', ''))}" ) console.print() diff --git a/src/apiauth/keygen.py b/src/apiauth/keygen.py index 27e6424..663e060 100644 --- a/src/apiauth/keygen.py +++ b/src/apiauth/keygen.py @@ -25,6 +25,7 @@ def generate_api_key(prefix: str = "ak", byte_length: int = 32) -> str: def _base64url_no_pad(data: bytes) -> str: import base64 + return base64.urlsafe_b64encode(data).rstrip(b"=").decode("ascii") @@ -57,9 +58,9 @@ def create_api_key_entry( now = _timestamp() expiry = None if expiry_days: - expiry = ( - datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days) - ).isoformat(timespec="milliseconds")[:23] + "Z" + expiry = (datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days)).isoformat(timespec="milliseconds")[ + :23 + ] + "Z" entry = { "type": "api_key", @@ -111,14 +112,15 @@ def create_jwt_entry( # Create the JWT import jwt as pyjwt + token = pyjwt.encode(payload, signing_secret, algorithm="HS256") now_str = _timestamp() expiry = None if expiry_days: - expiry = ( - datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days) - ).isoformat(timespec="milliseconds")[:23] + "Z" + expiry = (datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days)).isoformat(timespec="milliseconds")[ + :23 + ] + "Z" entry = { "type": "jwt", @@ -158,9 +160,9 @@ def rotate_key( now = _timestamp() expiry = None if expiry_days: - expiry = ( - datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days) - ).isoformat(timespec="milliseconds")[:23] + "Z" + expiry = (datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days)).isoformat(timespec="milliseconds")[ + :23 + ] + "Z" updated = dict(entry) updated["previous_hash"] = entry.get("key_hash") @@ -288,9 +290,9 @@ def rotate_jwt( now = _timestamp() expiry = None if expiry_days: - expiry = ( - datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days) - ).isoformat(timespec="milliseconds")[:23] + "Z" + expiry = (datetime.datetime.now(UTC) + datetime.timedelta(days=expiry_days)).isoformat(timespec="milliseconds")[ + :23 + ] + "Z" updated = dict(entry) updated["previous_hash"] = entry.get("signing_secret_hash") diff --git a/src/apiauth/keystore.py b/src/apiauth/keystore.py index b0cbaf0..41d958b 100644 --- a/src/apiauth/keystore.py +++ b/src/apiauth/keystore.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import json import os from cryptography.hazmat.primitives.ciphers.aead import AESGCM @@ -65,11 +66,27 @@ def _load(self) -> None: ) from exc def _save(self) -> None: + """Atomically write the keystore to disk. + + Uses a temp file + os.replace so that a crash or disk-full mid-write + never truncates the existing store. The previous file remains intact + until the replacement is fully written. + """ plaintext = json.dumps(self._entries, indent=2, default=str).encode("utf-8") nonce = os.urandom(12) ciphertext = self._aesgcm.encrypt(nonce, plaintext, None) - self._store_path.write_bytes(nonce + ciphertext) - os.chmod(str(self._store_path), 0o600) + data = nonce + ciphertext + + tmp_path = self._store_path.with_suffix(self._store_path.suffix + ".tmp") + try: + tmp_path.write_bytes(data) + os.chmod(str(tmp_path), 0o600) + os.replace(str(tmp_path), str(self._store_path)) + except BaseException: + # Clean up the temp file on any failure so we don't leak .tmp files. + with contextlib.suppress(OSError): + tmp_path.unlink(missing_ok=True) + raise def get_all(self) -> dict[str, dict[str, Any]]: """Return all stored entries.""" diff --git a/tests/conftest.py b/tests/conftest.py index f1a3b2b..aa4370c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ """Mock revenueholdings_license for tests so CLI commands don't hit the paywall.""" + import sys from unittest.mock import MagicMock diff --git a/tests/test_cli.py b/tests/test_cli.py index 56b9f9a..c9f872b 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -24,6 +24,7 @@ def runner(): """Provide a Click CliRunner.""" from click.testing import CliRunner + return CliRunner() @@ -139,9 +140,7 @@ def test_create_jwt(self, tmp_keystore): assert result["token"].count(".") == 2 # JWT has 3 parts def test_custom_claims(self, tmp_keystore): - result = create_jwt_entry( - tmp_keystore, "Claims", "api", claims={"role": "admin", "scope": "read:users"} - ) + result = create_jwt_entry(tmp_keystore, "Claims", "api", claims={"role": "admin", "scope": "read:users"}) stored = tmp_keystore.get(result["id"]) assert stored["claims"]["role"] == "admin" assert stored["claims"]["scope"] == "read:users" @@ -274,6 +273,7 @@ def test_not_expired(self): def test_expiring_soon(self): import datetime + soon = (datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(days=3)).isoformat()[:23] + "Z" result = check_expiry({"expires_at": soon}) assert result == "expiring" @@ -307,8 +307,18 @@ def test_help(self, runner): def test_generate_api_key(self, runner, tmp_keystore): result = runner.invoke( cli, - ["--key-dir", str(tmp_keystore.key_dir), "generate", "api-key", - "--name", "TestKey", "--service", "api-gateway", "--expiry-days", "90"], + [ + "--key-dir", + str(tmp_keystore.key_dir), + "generate", + "api-key", + "--name", + "TestKey", + "--service", + "api-gateway", + "--expiry-days", + "90", + ], ) assert result.exit_code == 0 assert "TestKey" in result.output @@ -318,8 +328,18 @@ def test_generate_api_key(self, runner, tmp_keystore): def test_generate_jwt(self, runner, tmp_keystore): result = runner.invoke( cli, - ["--key-dir", str(tmp_keystore.key_dir), "generate", "jwt", - "--name", "MyJWT", "--service", "auth", "--expiry-days", "30"], + [ + "--key-dir", + str(tmp_keystore.key_dir), + "generate", + "jwt", + "--name", + "MyJWT", + "--service", + "auth", + "--expiry-days", + "30", + ], ) assert result.exit_code == 0 assert "MyJWT" in result.output @@ -338,9 +358,7 @@ def test_list_filter_by_service(self, runner, tmp_keystore): create_api_key_entry(tmp_keystore, "Key1", "svc1") create_api_key_entry(tmp_keystore, "Key2", "svc2") - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "list", "--service", "svc1"] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "list", "--service", "svc1"]) assert result.exit_code == 0 assert "Key1" in result.output assert "Key2" not in result.output @@ -369,9 +387,7 @@ def test_rotate_key(self, runner, tmp_keystore): entry = create_api_key_entry(tmp_keystore, "RotateMe", "api") key_id = entry["id"] - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "rotate", key_id] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "rotate", key_id]) assert result.exit_code == 0 assert "Rotated" in result.output or "v2" in result.output or "New" in result.output @@ -379,9 +395,7 @@ def test_revoke_key(self, runner, tmp_keystore): entry = create_api_key_entry(tmp_keystore, "RevokeMe", "api") key_id = entry["id"] - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "revoke", key_id] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "revoke", key_id]) assert result.exit_code == 0 assert "Revoked" in result.output @@ -409,8 +423,17 @@ def test_verify_json_output(self, runner, tmp_keystore): def test_import_key(self, runner, tmp_keystore): result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "import", "ak_myimportedkey123", - "--name", "Imported", "--service", "api"] + cli, + [ + "--key-dir", + str(tmp_keystore.key_dir), + "import", + "ak_myimportedkey123", + "--name", + "Imported", + "--service", + "api", + ], ) assert result.exit_code == 0 assert "Imported" in result.output @@ -418,8 +441,7 @@ def test_import_key(self, runner, tmp_keystore): def test_import_key_stores_hash(self, runner, tmp_keystore): api_key = "ak_testimportkey123abc" result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "import", api_key, - "--name", "HashTest", "--service", "api"] + cli, ["--key-dir", str(tmp_keystore.key_dir), "import", api_key, "--name", "HashTest", "--service", "api"] ) assert result.exit_code == 0 @@ -431,34 +453,26 @@ def test_import_key_stores_hash(self, runner, tmp_keystore): def test_export_env(self, runner, tmp_keystore): create_api_key_entry(tmp_keystore, "ExportKey", "api-gateway") - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "env"] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "env"]) assert result.exit_code == 0 assert "export" in result.output def test_export_dotenv(self, runner, tmp_keystore): create_api_key_entry(tmp_keystore, "DotenvKey", "api") - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "dotenv"] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "dotenv"]) assert result.exit_code == 0 assert "export" not in result.output # dotenv has no export prefix assert "DOTENVKEY" in result.output def test_export_github_actions(self, runner, tmp_keystore): create_api_key_entry(tmp_keystore, "GHKey", "api") - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "github-actions"] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "github-actions"]) assert result.exit_code == 0 assert "GITHUB_ENV" in result.output def test_export_json(self, runner, tmp_keystore): create_api_key_entry(tmp_keystore, "JsonExport", "api") - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "json"] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "export", "--format", "json"]) assert result.exit_code == 0 data = json.loads(result.output) assert isinstance(data, list) @@ -518,9 +532,7 @@ def test_verify_revoked_jwt_via_cli(self, runner, tmp_keystore): def test_verify_jwt_json_output(self, runner, tmp_keystore): entry = create_jwt_entry(tmp_keystore, "CLIJsonJWT", "auth") - result = runner.invoke( - cli, ["--key-dir", str(tmp_keystore.key_dir), "verify", "--json-output", entry["token"]] - ) + result = runner.invoke(cli, ["--key-dir", str(tmp_keystore.key_dir), "verify", "--json-output", entry["token"]]) assert result.exit_code == 0 data = json.loads(result.output) assert data["status"] == "valid" @@ -528,6 +540,7 @@ def test_verify_jwt_json_output(self, runner, tmp_keystore): def test_verify_unknown_jwt_via_cli(self, runner, tmp_keystore): # A well-formed JWT that doesn't exist in the keystore import base64 + header = base64.urlsafe_b64encode(b'{"alg":"HS256"}').rstrip(b"=").decode() payload = base64.urlsafe_b64encode(b'{"jti":"nosuchid"}').rstrip(b"=").decode() fake_jwt = f"{header}.{payload}.fakesig" diff --git a/tests/test_keystore_atomic.py b/tests/test_keystore_atomic.py new file mode 100644 index 0000000..4c42385 --- /dev/null +++ b/tests/test_keystore_atomic.py @@ -0,0 +1,64 @@ +"""Test atomic write behavior for keystore.""" + +import os +from apiauth.keystore import Keystore +from pathlib import Path + + +def test_keystore_atomic_write_no_temp_files(tmp_path: Path) -> None: + """Verify that keystore save leaves no temporary files behind.""" + key_dir = tmp_path / "keystore" + key_dir.mkdir() + + ks = Keystore(key_dir=key_dir) + ks.put("test-key", {"id": "test-key", "type": "api_key", "value": "test123"}) + + # Check no temp files remain + files = list(key_dir.iterdir()) + filenames = [f.name for f in files] + + # Should only have master.key and keys.json + assert "master.key" in filenames + assert "keys.json" in filenames + assert len(filenames) == 2, f"Unexpected files left behind: {filenames}" + + +def test_keystore_atomic_write_preserves_data(tmp_path: Path) -> None: + """Verify that atomic write preserves valid data.""" + key_dir = tmp_path / "keystore" + key_dir.mkdir() + + # Write initial data + ks1 = Keystore(key_dir=key_dir) + ks1.put("key1", {"id": "key1", "type": "api_key", "value": "value1"}) + ks1.put("key2", {"id": "key2", "type": "jwt", "value": "value2"}) + + # Reload and verify + ks2 = Keystore(key_dir=key_dir) + entries = ks2.get_all() + + assert len(entries) == 2 + assert "key1" in entries + assert "key2" in entries + assert entries["key1"]["value"] == "value1" + assert entries["key2"]["value"] == "value2" + + +def test_keystore_atomic_write_file_permissions(tmp_path: Path) -> None: + """Verify that atomic write maintains restrictive permissions.""" + key_dir = tmp_path / "keystore" + key_dir.mkdir() + + ks = Keystore(key_dir=key_dir) + ks.put("test-key", {"id": "test-key", "type": "api_key", "value": "test"}) + + store_path = key_dir / "keys.json" + key_path = key_dir / "master.key" + + # Check permissions (on Unix-like systems) + if os.name != "nt": # Skip on Windows + store_mode = store_path.stat().st_mode & 0o777 + key_mode = key_path.stat().st_mode & 0o777 + + assert store_mode == 0o600, f"keys.json permissions: {oct(store_mode)}" + assert key_mode == 0o600, f"master.key permissions: {oct(key_mode)}" diff --git a/tests/test_keystore_atomic_failure.py b/tests/test_keystore_atomic_failure.py new file mode 100644 index 0000000..3b34577 --- /dev/null +++ b/tests/test_keystore_atomic_failure.py @@ -0,0 +1,43 @@ +"""Test that keystore survives write failures.""" + +import contextlib +from apiauth.keystore import Keystore +from pathlib import Path +from unittest.mock import patch + + +def test_keystore_survives_write_failure(tmp_path: Path) -> None: + """If _save() fails mid-write, existing data must remain intact.""" + key_dir = tmp_path / "keystore" + key_dir.mkdir() + + # Write initial data + ks1 = Keystore(key_dir=key_dir) + ks1.put("original-key", {"id": "original-key", "value": "original-value"}) + + # Verify initial state + store_path = key_dir / "keys.json" + assert store_path.exists() + original_size = store_path.stat().st_size + + # Try to add new data, but make the write fail + ks2 = Keystore(key_dir=key_dir) + + # Mock write to raise an exception after opening file + with patch("pathlib.Path.write_bytes") as mock_write: + mock_write.side_effect = OSError("Disk full") + + # This should fail, but original data must survive + with contextlib.suppress(OSError): + ks2.put("new-key", {"id": "new-key", "value": "new-value"}) + + # Reload and verify original data is intact + ks3 = Keystore(key_dir=key_dir) + entries = ks3.get_all() + + assert "original-key" in entries, "Original key was lost during failed write!" + assert entries["original-key"]["value"] == "original-value" + + # File size should be unchanged (no partial write) + current_size = store_path.stat().st_size + assert current_size == original_size, f"File size changed: {original_size} -> {current_size}" diff --git a/tests/test_keystore_atomic_real_failure.py b/tests/test_keystore_atomic_real_failure.py new file mode 100644 index 0000000..2a0e4ec --- /dev/null +++ b/tests/test_keystore_atomic_real_failure.py @@ -0,0 +1,52 @@ +"""Test that keystore survives realistic write failures (file truncation).""" + +from apiauth.keystore import Keystore +from pathlib import Path +from unittest.mock import patch + + +def test_keystore_survives_truncating_write_failure(tmp_path: Path) -> None: + """If write_bytes truncates the file but fails to write content, + the original data must still be recoverable. + + This tests the REAL failure mode: open('wb') truncates immediately, + then write() fails (disk full). Without atomic write, data is lost. + """ + key_dir = tmp_path / "keystore" + key_dir.mkdir() + + # Write initial data + ks1 = Keystore(key_dir=key_dir) + ks1.put("original-key", {"id": "original-key", "value": "original-value"}) + + store_path = key_dir / "keys.json" + assert store_path.exists() + + # Simulate realistic failure: file gets truncated but write fails + original_write_bytes = Path.write_bytes + tmp_store_path = store_path.with_suffix(store_path.suffix + ".tmp") + + def truncating_write_bytes(self_path, data): + # _save() writes to .tmp first; truncate THAT to simulate real failure + if self_path == tmp_store_path: + with open(self_path, "wb"): + pass # truncates temp file to zero bytes + raise OSError("Disk full after truncation") + return original_write_bytes(self_path, data) + + ks2 = Keystore(key_dir=key_dir) + try: + with patch.object(Path, "write_bytes", truncating_write_bytes): + ks2.put("new-key", {"id": "new-key", "value": "new-value"}) + except OSError: + pass # Expected + + # After atomic write implementation, original data must survive + # even though the target file was truncated + ks3 = Keystore(key_dir=key_dir) + entries = ks3.get_all() + + assert "original-key" in entries, ( + "Original key was lost! Implementation is not atomic. Use temp file + os.replace for crash-safe writes." + ) + assert entries["original-key"]["value"] == "original-value"