From 4262ba3e34b520735492a5a2e79d02a64183748d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 13:18:12 +0000 Subject: [PATCH] fix(security): land leftover IAM, decrypt, and migrate hardenings Re-implement the 2026-08-31 audit remediations that never merged, plus fd cleanup on encrypt/decrypt temp paths. Scope S3 IAM to encrypted/*, allow DisableKey for IR, encrypt the ops SNS topic, refuse decrypt overwrites without --force, and confine migrate inputs to the import directory. Co-authored-by: Specter099 --- .env.example | 34 +- .github/workflows/ci.yml | 2 + .github/workflows/publish.yml | 2 + CHANGELOG.md | 16 + README.md | 2 +- SECURITY_AUDIT.md | 790 ++++-------------------------- infra/cdk/stacks/envault_stack.py | 37 +- src/envault/cli.py | 155 +++++- src/envault/config.py | 29 +- src/envault/crypto.py | 95 ++-- src/envault/s3.py | 17 + src/envault/state.py | 41 +- tests/unit/test_cli.py | 317 +++++++++++- tests/unit/test_exec.py | 29 +- tests/unit/test_s3.py | 26 + tests/unit/test_state.py | 18 + 16 files changed, 790 insertions(+), 820 deletions(-) diff --git a/.env.example b/.env.example index a9bb560..7e91558 100644 --- a/.env.example +++ b/.env.example @@ -1,19 +1,23 @@ -# AWS Encryption Configuration -# Copy this file to .env and fill in your values +# envault CLI configuration +# Copy this file to .env and fill in your values. +# The Python CLI reads ENVAULT_* variables only. -# AWS Configuration -AWS_PROFILE=default -AWS_REGION=us-east-1 +# Required +ENVAULT_KEY_ID=alias/envault +ENVAULT_BUCKET=my-encrypted-files-bucket +ENVAULT_TABLE=envault-state -# S3 Bucket for encrypted file storage -S3_BUCKET=sensitive-docs-XXXXXXXXXXXX +# Required for decrypt, exec, and rotate-key +ENVAULT_ALLOWED_ACCOUNT_IDS=123456789012 -# KMS Key Configuration -# For encryption, use the alias -KMS_KEY_ALIAS=alias/s3_key +# Optional +ENVAULT_REGION=us-east-1 +ENVAULT_AUDIT_TTL_DAYS=365 -# For decryption, use the full ARN -KMS_KEY_ARN=arn:aws:kms:us-east-1:XXXXXXXXXXXX:key/your-key-id-here - -# Encryption context (must match on decrypt) -ENCRYPTION_CONTEXT=purpose=backup +# --------------------------------------------------------------------------- +# Legacy shell scripts (code/encrypt.sh, code/decrypt.sh) — deprecated. +# These are NOT read by the envault CLI. +# --------------------------------------------------------------------------- +# S3_BUCKET=sensitive-docs-XXXXXXXXXXXX +# KMS_ACCOUNT_ID=123456789012 +# KMS_KEY_ALIAS=alias/s3_key diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 83ea691..b779306 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,6 +70,8 @@ jobs: run: | pip install --upgrade pip pip install pip-audit + # CVE-2026-4539: pygments (dev/docs highlighter, not a runtime dep) + # CVE-2026-3219: pip itself (CI installer), not shipped in the wheel pip-audit --ignore-vuln CVE-2026-4539 --ignore-vuln CVE-2026-3219 test: diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 790652e..abf2af3 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -47,6 +47,8 @@ jobs: run: | pip install --upgrade pip pip install pip-audit + # CVE-2026-4539: pygments (dev/docs highlighter, not a runtime dep) + # CVE-2026-3219: pip itself (CI installer), not shipped in the wheel pip-audit --ignore-vuln CVE-2026-4539 --ignore-vuln CVE-2026-3219 build: diff --git a/CHANGELOG.md b/CHANGELOG.md index a229f9d..5c00bb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,22 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- KMS key policy no longer denies `DisableKey`, so a compromised CMK can be frozen during incident response without a CloudFormation change. +- IAM policy no longer grants unused `dynamodb:UpdateItem` / `s3:ListBucket`; S3 object actions are scoped to `encrypted/*`; `sts:GetCallerIdentity` is granted for audit attribution. +- Ops SNS topic is encrypted with the envault CMK. +- `decrypt` refuses to overwrite an existing destination unless `--force` is passed, and checks before creating temp files. +- S3 downloads require a content-addressed key (`encrypted/{aa}/{sha256}/{name}.encrypted`) so a poisoned DynamoDB `s3_key` cannot fetch an arbitrary object. +- Directory-symlink trees are skipped by `os.walk(followlinks=False)` during encrypt. +- `migrate` confines input paths to the import directory and rejects per-component symlinks. +- `ENVAULT_AUDIT_TTL_DAYS` is applied on encrypt, decrypt, exec, rotate-key, and migrate event writes. +- `rotate-key` calls `DescribeKey` on the target CMK before downloading or decrypting anything. +- `last_updated` CAS tokens use microsecond timestamps. +- Dashboard `last_activity` pages the state-index until a CURRENT item survives the filter. +- `exec` warns when the child will inherit `AWS_*` credentials; `--clean-env` remains opt-in. +- Encrypt closes the output fd if the SDK stream fails before `fdopen`. + ## [0.2.0] - 2026-07-26 ### Added diff --git a/README.md b/README.md index 88891a9..4bbe26a 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ envault encrypt INPUT_PATH [--tag KEY=VALUE]... [--force] envault exec -s IDENTIFIER=VAR [-f IDENTIFIER=VAR]... [--clean-env] -- COMMAND [ARGS]... # Decrypt by filename or SHA256 hash -envault decrypt IDENTIFIER [-o OUTPUT_DIR] [--version N] +envault decrypt IDENTIFIER [-o OUTPUT_DIR] [--version N] [--force] # List all encrypted/decrypted files envault status [--state encrypted|decrypted|all] diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index adfd032..795013c 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -1,778 +1,182 @@ -# CISO Final Security Review — envault-cli +# Weekly Security Audit — envault-cli -**Reviewer:** CISO Final Security Review -**Date:** 2026-03-04 -**Codebase version:** commit `5b575e9` on `main` (post-remediation of prior audit findings) -**Scope:** Full codebase — Python package (`src/envault/`), CDK infrastructure (`infra/cdk/`), CI/CD workflows (`.github/workflows/`), legacy shell scripts (`code/`), tests (`tests/`), dependencies, pre-commit configuration. -**Prior audit:** v1 dated 2026-03-03 +**Reviewer:** Automated weekly static security review +**Date:** 2026-09-07 +**Codebase:** `main` at start of this run, remediations on `cursor/repository-security-audit-*` +**Scope:** `src/envault/`, `infra/cdk/`, `.github/workflows/`, `code/`, tests, dependency manifests, git tree. Static analysis only. --- -## Executive Summary +## Executive summary -The envault-cli codebase demonstrates strong security fundamentals: client-side envelope encryption with AES-256-GCM, KMS commitment policy enforcement (`REQUIRE_ENCRYPT_REQUIRE_DECRYPT`), discovery filters with mandatory account ID restrictions, least-privilege IAM, CMK encryption on all data stores, SHA-pinned CI actions with OIDC Trusted Publisher for PyPI, and optimistic locking on DynamoDB state transitions. +Cryptographic foundations remain sound: streaming AES-256-GCM via the AWS Encryption SDK, `REQUIRE_ENCRYPT_REQUIRE_DECRYPT`, mandatory `DiscoveryFilter` with 12-digit account IDs, checksum-before-rename decrypt, and SHA-pinned GitHub Actions with OIDC PyPI publish. -The prior security audit (2026-03-03) identified 4 critical, 5 high, and 8 medium findings. All critical and high findings from that audit have been remediated in the current codebase (see [Prior Audit Remediation Status](#prior-audit-remediation-status) below). +This week's scan found **0 Critical**, **8 High**, **7 Medium**, and **6 Low** issues still present on `main`. The High items were leftover from the 2026-08-31 audit branch that never merged. This PR remediates the High findings that are safe to land without a DynamoDB GSI replacement or a breaking CLI default. -However, this final review identifies **3 critical, 8 high, 10 medium, and 7 low findings** that must be addressed before production deployment. The critical findings center on the **decrypt path**: plaintext is written to disk before integrity verification, the entire file is buffered in memory with no streaming support, and these code paths have zero test coverage. +**After this PR:** 0 Critical, 3 High (accepted / requires operator action), 5 Medium, 5 Low. --- -## Severity Definitions +## Remediations in this PR -| Severity | Meaning | -|----------|---------| -| **CRITICAL** | Exploitable in normal use; causes data loss, integrity failure, or unauthorized access | -| **HIGH** | Significant security or reliability risk; exploitable under plausible conditions | -| **MEDIUM** | Weakness that reduces defence-in-depth or can be chained with another finding | -| **LOW** | Best-practice gap with low standalone impact | +| ID | Severity | Fix | +|----|----------|-----| +| H-1 | High | Remove `kms:DisableKey` from the CMK deny (keep `ScheduleKeyDeletion`) | +| H-2 | High | Drop unused `UpdateItem` / `ListBucket`; scope S3 to `encrypted/*`; grant `sts:GetCallerIdentity` | +| H-3 | High | Encrypt the ops SNS topic with the envault CMK | +| H-4 | High | `decrypt --force`; refuse overwrite *before* `mkstemp` | +| H-5 | High | Reject DynamoDB `s3_key` values that are not `encrypted/{aa}/{sha256}/{name}.encrypted` | +| H-6 | High | `os.walk(followlinks=False)` so encrypt does not follow directory symlink trees | +| H-7 | High | `migrate` confines paths to the import directory; rejects `..` and per-component symlinks | +| H-8 | High | Wire `ENVAULT_AUDIT_TTL_DAYS` on encrypt/decrypt/exec/rotate-key/migrate event writes | +| H-9 | High | `rotate-key` `DescribeKey` preflight before any download/decrypt | +| H-10 | High | `last_updated` CAS tokens use microseconds | +| H-11 | High | Dashboard `last_activity` pages until a CURRENT GSI item survives the filter | +| M-1 | Medium | `exec` warns when inheriting `AWS_*` (default unchanged — breaking if flipped) | +| M-2 | Medium | Close encrypt output fd if the SDK stream fails before `fdopen` | +| M-3 | Medium | Document pip-audit CVE ignore rationale in CI | --- -## CRITICAL Findings +## Remaining findings (post-remediation) -### C-1 — Plaintext written to disk before checksum verification +### High -**File:** `src/envault/crypto.py:178-188` +#### H-A — `rotate-key` IAM still covers only the stack CMK -```python -output_path.parent.mkdir(parents=True, exist_ok=True) -with output_path.open("wb") as out: - out.write(plaintext) -os.chmod(output_path, 0o600) +**Location:** `infra/cdk/stacks/envault_stack.py` (KmsEnvelopeEncryption statement) -actual_sha256 = sha256_file(output_path) -file_size = output_path.stat().st_size +**Issue:** The managed policy grants `kms:GenerateDataKey` / `kms:Decrypt` / `kms:DescribeKey` only on the CMK this stack creates. `rotate-key --new-key-id alias/other` fails at the new DescribeKey preflight (no plaintext written). -if expected_sha256 and actual_sha256 != expected_sha256: - output_path.unlink(missing_ok=True) - raise ChecksumMismatchError(expected=expected_sha256, actual=actual_sha256) -``` +**Impact:** Operators cannot complete rotation to a second key with the stock policy. -**Description:** Decrypted plaintext is written to the output file, then re-read from disk for SHA256 verification. If the checksum fails (indicating tampering or corruption), the file is deleted — but it already existed on disk in the clear. On copy-on-write filesystems (APFS on macOS, ZFS), the data persists in filesystem snapshots even after `unlink()`. Additionally, between the write and the check, another process could read the potentially tampered plaintext. +**Fix:** Add a stack parameter for extra rotation-target key ARNs, or document the required policy amendment. Do not widen the default grant to `kms:*` / `Resource: *`. -**Attack scenario:** An attacker substitutes ciphertext in S3 with a different validly-encrypted file (encrypted under the same KMS key). The user decrypts it. Tampered plaintext hits disk at the user-specified path before the integrity check catches it. On APFS, it survives in a Time Machine snapshot. On any filesystem, a watching process can exfiltrate it during the verification window. +#### H-B — Rotation is not a revocation primitive -**Recommendation:** Compute SHA256 on the in-memory `plaintext` bytes before writing to disk. Only write if the hash matches: +**Location:** `infra/cdk/stacks/envault_stack.py` (noncurrent version expiration 365 days); `src/envault/cli.py` `rotate-key` re-uploads to the same S3 key -```python -actual_sha256 = hashlib.sha256(plaintext).hexdigest() -if expected_sha256 and actual_sha256 != expected_sha256: - raise ChecksumMismatchError(expected=expected_sha256, actual=actual_sha256) -# Only write after verification passes -fd = os.open(str(output_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) -with os.fdopen(fd, "wb") as out: - out.write(plaintext) -``` +**Issue:** Versioning retains ciphertext wrapped under the old key for 365 days. `s3:GetObjectVersion` is still granted on `encrypted/*`. -This also addresses H-3 (file permissions race) by creating the file with `0o600` atomically. +**Impact:** Anyone who still has `kms:Decrypt` on the old CMK can read prior versions for a year. ---- - -### C-2 — No streaming — entire plaintext/ciphertext held in memory - -**Files:** `src/envault/crypto.py:87-92` (encrypt), `src/envault/crypto.py:172-176` (decrypt) - -```python -# encrypt -with input_path.open("rb") as plaintext_file: - ciphertext, header = client.encrypt( - source=plaintext_file, key_provider=key_provider, - encryption_context=encryption_context, - ) - -# decrypt -with input_path.open("rb") as encrypted_file: - plaintext, header = client.decrypt( - source=encrypted_file, key_provider=key_provider, - ) -``` - -**Description:** Both `client.encrypt()` and `client.decrypt()` return the full content as a Python `bytes` object in memory. Two security consequences: - -1. **Memory exhaustion / DoS:** A maliciously large ciphertext causes OOM. There is no file size validation. -2. **Plaintext exposure in memory:** Sensitive plaintext in Python `bytes` cannot be securely zeroed — it persists in process memory (and potentially swap) until garbage collected. Python's memory allocator does not guarantee overwrite on deallocation. - -**Attack scenario:** (1) Attacker uploads a multi-GB ciphertext to S3; user's `envault decrypt` OOMs or causes system instability. (2) After decryption, plaintext bytes remain recoverable from process memory or swap via forensic tools. - -**Recommendation:** Use `client.stream(mode='e'/'d', ...)` for chunked I/O directly to output files. This also enables computing SHA256 during streaming, eliminating the TOCTOU in C-1 and M-8: - -```python -with client.stream(mode='d', source=encrypted_file, key_provider=key_provider) as decryptor: - hasher = hashlib.sha256() - for chunk in decryptor: - hasher.update(chunk) - out.write(chunk) -``` - ---- - -### C-3 — Zero test coverage on critical integrity checks - -**Files:** `tests/unit/test_crypto.py`, `tests/unit/test_cli.py` - -**Description:** The two most important security invariants in the system have **no test coverage**: - -1. **`ChecksumMismatchError` during decrypt** — The only test (`test_checksum_mismatch_error` at `test_crypto.py:102`) tests the exception's `__str__` method, never the actual `decrypt_file` code path that raises it. A regression in the checksum comparison, file cleanup, or retry exclusion would be invisible. - -2. **`EncryptionContextMismatchError`** — Never tested anywhere. The `decrypt` command (`cli.py:270`) and `rotate-key` command (`cli.py:601`) both check for context mismatches, but neither path is exercised by any test. - -3. **CLI `encrypt` and `decrypt` commands** — Never invoked via `CliRunner` except a single hash-format validation test for `decrypt`. The entire security-critical orchestration flow (tempfile creation, encryption, S3 upload, DynamoDB state write, tempfile cleanup, error handling) is untested. - -4. **`rotate-key` command** — The most complex operation in the system (download, decrypt, re-encrypt, upload, state update, 3 temp files) has zero tests. - -5. **Temp file cleanup on failure** — `finally` blocks that clean up temp files (including `_best_effort_delete` for plaintext) have zero test coverage. - -6. **File permission setting** — `os.chmod(output_path, 0o600)` after encrypt/decrypt is untested. - -**Impact:** Any regression in checksum verification, encryption context comparison, temp file cleanup, file permissions, or error handling would be completely invisible to CI. - -**Recommendation:** Add tests for each of the above. At minimum: -- Call `decrypt_file` with a mismatched `expected_sha256` and assert `ChecksumMismatchError` is raised and output file is deleted -- Mock `decrypt_file` to return a mismatched `encryption_context` and assert `EncryptionContextMismatchError` is raised -- Use `CliRunner` with mocked crypto and moto-backed AWS to test full encrypt/decrypt/rotate-key flows -- Simulate failures at each stage and verify temp file cleanup -- Verify output file permissions are `0o600` - ---- - -## HIGH Findings - -### H-1 — Path traversal via `file_name` from DynamoDB during decrypt - -**File:** `src/envault/cli.py:253` - -```python -output_path = (output if output.is_dir() else output.parent) / record.file_name -``` - -**Description:** `record.file_name` comes from DynamoDB and is used unsanitized in the output path construction. The `_sanitize_filename()` method exists in `S3Store` and is used for S3 key generation, but is NOT applied to the output path during decryption. The `file_name` stored in DynamoDB is the raw `file_path.name` from the original encryption. - -**Attack scenario:** An attacker with DynamoDB write access (or who corrupts the migration source) sets `file_name = "../../.ssh/authorized_keys"`. A user running `envault decrypt ` writes the decrypted file to an arbitrary filesystem path. - -**Recommendation:** Sanitize `record.file_name` before constructing the output path: - -```python -safe_name = Path(record.file_name).name # Strip directory components -if not safe_name or safe_name.startswith('.'): - safe_name = f"decrypted_{record.sha256_hash[:16]}" -output_path = (output if output.is_dir() else output.parent) / safe_name -``` - ---- - -### H-2 — Symlink traversal in `_collect_files` - -**File:** `src/envault/cli.py:650-653` - -```python -def _collect_files(path: Path) -> list[Path]: - if path.is_file(): - return [path] - return [p for p in path.rglob("*") if p.is_file()] -``` - -**Description:** `Path.rglob("*")` follows symbolic links by default. `path.is_file()` returns `True` for symlinks pointing to files. An attacker who can create symlinks within a target directory can cause envault to encrypt files outside the intended directory tree. - -**Attack scenario:** A shared directory contains `symlink -> /home/victim/.ssh/id_rsa`. User runs `envault encrypt shared_dir/`. The tool follows the symlink, encrypts the victim's SSH key, and uploads it to S3 where the attacker can decrypt it. - -**Recommendation:** Filter out symlinks: - -```python -def _collect_files(path: Path) -> list[Path]: - if path.is_symlink(): - return [] - if path.is_file(): - return [path] - return [p for p in path.rglob("*") if p.is_file() and not p.is_symlink()] -``` - ---- - -### H-3 — Output file permissions race (chmod after write) - -**Files:** `src/envault/crypto.py:95-97` (encrypt), `src/envault/crypto.py:179-181` (decrypt) - -```python -with output_path.open("wb") as out: - out.write(plaintext) -os.chmod(output_path, 0o600) -``` - -**Description:** Files are created with the process's default umask permissions (typically `0o644`), then restricted to `0o600` afterward. Between creation and chmod, other users on the system can read the file contents. This is particularly concerning for the decryption case where plaintext is being written. - -**Attack scenario:** On a multi-user system, another user reads the plaintext file during the window between `open("wb")` and `os.chmod()`. - -**Recommendation:** Use `os.open()` with explicit mode to create the file atomically with restricted permissions: - -```python -fd = os.open(str(output_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) -with os.fdopen(fd, "wb") as out: - out.write(plaintext) -``` - -Note: Temp files created with `mkstemp` already have `0o600` permissions — this issue only affects final output files. - ---- - -### H-4 — `encrypt_file` retry doesn't exclude non-retryable exceptions - -**File:** `src/envault/crypto.py:54-57` - -```python -@retry(stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=1, max=10)) -def encrypt_file(...) -> EncryptResult: -``` - -**Description:** The `@retry` decorator on `encrypt_file` retries ALL exceptions including `ConfigurationError` and other non-retryable errors. The `decrypt_file` decorator correctly uses `retry=retry_if_not_exception_type((ConfigurationError, ChecksumMismatchError))` to exclude non-retryable types. This inconsistency means a misconfigured KMS key causes 3 unnecessary KMS API calls, and each retry generates a different data encryption key producing different ciphertext. - -**Recommendation:** Add retry exclusion to match `decrypt_file`: - -```python -@retry( - stop=stop_after_attempt(3), - wait=wait_exponential(multiplier=1, min=1, max=10), - retry=retry_if_not_exception_type(ConfigurationError), -) -``` - ---- - -### H-5 — Publish workflow missing top-level `permissions` restriction - -**File:** `.github/workflows/publish.yml` - -**Description:** Unlike `ci.yml` which sets `permissions: read-all` at the top level, the publish workflow has no top-level permissions declaration. The `build` job inherits the default GITHUB_TOKEN permissions, which in many repository configurations is `write` for contents, packages, pull-requests, and more. - -**Attack scenario:** A compromised or future third-party action in the build job runs with elevated GITHUB_TOKEN permissions, potentially pushing malicious code, creating releases, or modifying PR reviews. - -**Recommendation:** Add `permissions: read-all` at the top level; scope per-job as needed: - -```yaml -permissions: read-all - -jobs: - build: - permissions: - contents: read - ... - publish-pypi: - permissions: - id-token: write - ... -``` - ---- - -### H-6 — PyPI publish has no CI quality gate - -**File:** `.github/workflows/publish.yml:3-6` - -```yaml -on: - push: - tags: - - "v*.*.*" -``` - -**Description:** Anyone with push access can create a tag matching `v*.*.*` and trigger a full PyPI release. The publish workflow does NOT depend on CI passing — no tests, linting, or type checking are required before publication. - -**Attack scenario:** A compromised contributor account pushes a tag on a commit containing malicious code. The package is built and published to PyPI without any quality checks, distributing a supply-chain attack to all `pip install envault-cli` users. - -**Recommendation:** Combine multiple defenses: -1. Add the CI workflow as a `needs:` dependency in the publish workflow -2. Configure the `pypi` GitHub Environment with required reviewers -3. Restrict tag creation to administrators via branch protection rules - -```yaml -jobs: - ci: - uses: ./.github/workflows/ci.yml - build: - needs: ci - ... -``` - ---- - -### H-7 — `decrypt.conf` shell variable not expanded by aws-encryption-cli - -**File:** `code/decrypt.conf:2` - -``` ---wrapping-keys key=${KMS_KEY_ARN} -``` - -**Description:** The `@filename` argument-file syntax in `aws-encryption-cli` reads literal text without shell variable expansion. The string `${KMS_KEY_ARN}` is passed literally as the key identifier, causing decryption to fail with an obscure KMS error or silently attempt to use a key named `${KMS_KEY_ARN}`. - -In contrast, `encrypt.conf` hardcodes `key=alias/s3_key`, creating an inconsistency between encrypt and decrypt configurations. - -**Recommendation:** Use `--wrapping-keys discovery=true` in the legacy scripts (matching the Python code's discovery pattern), or generate the conf file dynamically at runtime. - ---- - -### H-8 — Temp file cleanup on encryption/decryption failure is untested - -**Files:** `src/envault/cli.py:143-161` (encrypt), `src/envault/cli.py:250-268` (decrypt) +**Fix:** After rotation, disable the old CMK (now possible — H-1). Optionally shorten noncurrent expiration or write rotation to a new object key. Document this as the true revocation boundary. -**Description:** Both `_encrypt_one` and the `decrypt` command create temp files via `tempfile.mkstemp`. The `finally` blocks are responsible for cleaning up these files even on failure, including calling `_best_effort_delete` for plaintext files. No test verifies this behavior. Additionally, the `_best_effort_delete` test (`test_best_effort_delete_overwrites_before_removal`) only verifies the file no longer exists — it does not verify the file was zeroed before deletion. +#### H-C — `exec` inherits AWS credentials by default -**Attack scenario:** A transient AWS error causes encryption to fail. Temp files containing ciphertext or plaintext are left on disk. During key rotation, a decrypted plaintext temp file could remain indefinitely if the re-encryption step fails and the `finally` block has a regression. +**Location:** `src/envault/cli.py` (`exec`, `--clean-env`) -**Recommendation:** Add tests that simulate failures at various stages and verify all temp files are cleaned up. Modify `_best_effort_delete` test to verify zeroing occurs before unlinking. +**Issue:** Without `--clean-env`, the child receives the operator's `AWS_*` environment. A warning is now printed; flipping the default is a breaking change. ---- - -## MEDIUM Findings - -### M-1 — Broad `except Exception` in encrypt command hides programming errors - -**File:** `src/envault/cli.py:116-119` - -```python -except Exception as exc: - console.print(f"[red]✗[/red] {file_path.name}: {exc}") - logger.exception("Failed to encrypt %s", file_path) - errors += 1 -``` - -**Description:** The encrypt command catches `Exception` broadly. `TypeError`, `AttributeError`, and `NameError` (indicating bugs) are silently counted as "errors" and processing continues. A programming bug could cause every file in a batch to silently fail. The `rotate-key` command (`cli.py:630`) correctly narrowed this to `(EnvaultError, ClientError, BotoCoreError)`. - -**Recommendation:** Match the pattern used in `rotate-key`: - -```python -except (EnvaultError, ClientError, BotoCoreError) as exc: -``` - ---- - -### M-2 — SHA256 hash validation inconsistent across commands - -**Files:** `src/envault/cli.py:302` (status), `src/envault/cli.py:356` (audit) - -**Description:** The `decrypt` command validates the SHA256 hash with `re.fullmatch(r"[0-9a-f]{64}", sha256_hash)` at line 224, but the `status` and `audit` commands pass `sha256_hash` directly to DynamoDB queries without validation. A malformed input produces confusing "not found" errors. - -**Recommendation:** Extract the validation into a shared helper and apply consistently: - -```python -def _validate_sha256(value: str) -> str: - if not re.fullmatch(r"[0-9a-f]{64}", value): - raise click.BadParameter("Expected 64 lowercase hexadecimal characters") - return value -``` - ---- - -### M-3 — `_best_effort_delete` allocates full file size in memory - -**File:** `src/envault/cli.py:676-696` - -```python -size = path.stat().st_size -with path.open("r+b") as f: - f.write(b"\x00" * size) -``` - -**Description:** `b"\x00" * size` allocates a single bytes object equal to the file size. For a 1GB file, this allocates 1GB of zeros in memory. - -**Recommendation:** Write zeros in chunks: - -```python -CHUNK_SIZE = 65536 -remaining = size -with path.open("r+b") as f: - while remaining > 0: - to_write = min(CHUNK_SIZE, remaining) - f.write(b"\x00" * to_write) - remaining -= to_write - f.flush() - os.fsync(f.fileno()) -``` - ---- - -### M-4 — No format validation on `--allowed-account-ids` - -**Files:** `src/envault/cli.py:234`, `src/envault/cli.py:556` - -```python -account_ids = [a.strip() for a in allowed_account_ids.split(",") if a.strip()] -``` - -**Description:** AWS account IDs are 12-digit numeric strings. The code does not validate format before passing to `DiscoveryFilter`. A malformed account ID (e.g., `*`, empty string, non-numeric) could either cause a runtime error or weaken the discovery filter. - -**Recommendation:** Validate each account ID: - -```python -for account_id in account_ids: - if not re.fullmatch(r"\d{12}", account_id): - console.print(f"[red]Invalid AWS account ID: {account_id!r}. Must be 12 digits.[/red]") - sys.exit(1) -``` - ---- - -### M-5 — `EncryptionContextMismatchError` leaks full encryption context in error message - -**File:** `src/envault/exceptions.py:41-44` - -```python -super().__init__( - f"Encryption context mismatch: expected {expected!r}, got {actual!r}. " - "The ciphertext may have been tampered with or swapped." -) -``` - -**Description:** The full encryption context dictionaries (containing SHA256 hash, file name, and KMS key alias) are included in the exception message. If this exception propagates to logs or console output, it reveals metadata about the encrypted file. `ChecksumMismatchError` already truncates hashes to 16 chars, but this exception does not. - -**Recommendation:** Remove context details from the user-facing message: - -```python -super().__init__( - "Encryption context mismatch detected. " - "The ciphertext may have been tampered with or swapped." -) -``` - -Log the full context at DEBUG level for troubleshooting. - ---- - -### M-6 — S3 upload missing checksum verification - -**File:** `src/envault/s3.py:42-47` - -```python -response = self._s3.put_object( - Bucket=self._bucket, - Key=s3_key, - Body=f, - ServerSideEncryption="aws:kms", -) -``` +**Impact:** A compromised or buggy child command can use the operator's AWS credentials. -**Description:** The `put_object` call does not specify `ChecksumAlgorithm`. While AES-256-GCM provides authentication, a bit-flip during transit to S3 could corrupt the ciphertext before it's stored, leading to silent decryption failures later. - -**Recommendation:** Add `ChecksumAlgorithm="SHA256"` so S3 validates upload integrity server-side. +**Fix:** Prefer `--clean-env` in examples and runbooks. Consider defaulting it in a future major version. --- -### M-7 — `.secrets.baseline` referenced but does not exist - -**File:** `.pre-commit-config.yaml:6` - -```yaml -- id: detect-secrets - args: ["--baseline", ".secrets.baseline"] -``` - -**Description:** The `.secrets.baseline` file is referenced in pre-commit config and listed in `.gitignore` (line 62), but does not exist in the repository. Because it's in `.gitignore`, even if generated locally it won't be committed. This means the `detect-secrets` hook fails on every commit, causing developers to skip hooks with `--no-verify` or disable the hook. - -**Recommendation:** Remove `.secrets.baseline` from `.gitignore`, generate it with `detect-secrets scan > .secrets.baseline`, audit false positives, and commit it. - ---- +### Medium -### M-8 — TOCTOU: file read twice between hash and encrypt +#### M-A — `state-index` is a two-value partition that also stores events -**File:** `src/envault/cli.py:137-157` +**Location:** `infra/cdk/stacks/envault_stack.py` (GSI); `src/envault/state.py` `put_event` copies `current_state` -```python -sha256 = sha256_file(file_path) # First read -... -result = encrypt_file( - input_path=file_path, # Second read - ... -) -``` +**Issue:** Events inherit `current_state` via `asdict`, so they project into `state-index`. Queries filter them out. The partition key has two values (hot partition at scale). Changing the GSI requires table replacement. -**Description:** The file is read once to compute the SHA256 hash (which becomes the DynamoDB primary key) and again for encryption. If the file is modified between these two reads, the SHA256 stored in DynamoDB won't match the actual encrypted content. Upon decryption, the checksum verification would fail. +**Impact:** Read cost and latency grow with audit history; dashboard paging (H-11) mitigates emptiness but not cost. -**Recommendation:** Compute SHA256 as part of the encryption operation, or read the file once and use the same bytes for both. The streaming API fix (C-2) naturally resolves this. +**Fix:** Sparse-index marker (`gsi_state` only on CURRENT items) in a versioned migration. ---- +#### M-B — CDK L2 `kms.Key` default root `kms:*` policy -### M-9 — No dependency vulnerability scanning in CI +**Location:** `infra/cdk/stacks/envault_stack.py` `kms.Key(...)` -**File:** `.github/workflows/ci.yml` +**Issue:** CDK's L2 construct grants the account root `kms:*`, delegating to IAM. Standard CDK behavior; risky in shared accounts. -**Description:** The CI pipeline runs linting, type checking, and unit tests, but no dependency vulnerability scanner (`pip-audit`, `safety`). For a security-critical encryption tool depending on `cryptography`, `aws-encryption-sdk`, `cffi`, and `boto3`, CVE monitoring is essential. +**Fix:** For shared accounts, use an explicit key policy (CfnKey) scoped to envault principals. -**Recommendation:** Add a CI step: +#### M-C — No lockfile; pip-audit ignores two CVEs -```yaml -- name: Audit dependencies - run: pip install pip-audit && pip-audit -``` +**Location:** `pyproject.toml`; `.github/workflows/ci.yml`, `publish.yml` ---- +**Issue:** Production deps are ranged, not locked. CI ignores CVE-2026-4539 (pygments, not a runtime dep) and CVE-2026-3219 (pip, CI installer). Rationale is now commented. -### M-10 — CDK infrastructure requirements use open-ended version constraints +**Impact:** Reproducible builds and transitive CVE tracking are weaker than a lockfile. -**File:** `infra/cdk/requirements.txt` +**Fix:** Add `uv.lock` or `requirements.txt` from `pip-compile` for release artifacts. -``` -aws-cdk-lib>=2.100.0 -cdk-nag>=2.28.0 -constructs>=10.0.0 -``` +#### M-D — Filename lookup is O(all encrypted files) -**Description:** All three dependencies use `>=` with no upper bound. A `pip install` could pull in a future major version with breaking or security-behavioral changes. +**Location:** `src/envault/state.py` `list_by_file_name` -**Recommendation:** Add upper bounds: +**Issue:** Queries the whole `ENCRYPTED` GSI partition and filters `file_name` client-side. -``` -aws-cdk-lib>=2.100.0,<3 -cdk-nag>=2.28.0,<3 -constructs>=10.0.0,<11 -``` +**Impact:** `decrypt ` cost grows linearly with corpus size. ---- +**Fix:** `name-index` GSI (`file_name`, `encrypted_at`) in a future migration. -## LOW Findings +#### M-E — Empty `s3_version_id` fetches latest -### L-1 — S3 bucket missing `bucket_key_enabled` +**Location:** `src/envault/s3.py` `download_file` / `download_to_memory` -**File:** `infra/cdk/stacks/envault_stack.py:69-91` +**Issue:** Migrated records and any blank version ID download the latest object. A warning is logged. -**Description:** The S3 bucket uses KMS-SSE but does not set `bucket_key_enabled=True`. Without S3 Bucket Keys, every `PutObject`/`GetObject` makes a separate KMS request, increasing cost, latency, and CloudTrail noise (reducing signal-to-noise for security monitoring). +**Impact:** A silent overwrite of the S3 object could be decrypted instead of the recorded version. -**Recommendation:** Add `bucket_key_enabled=True` to the bucket construct. +**Fix:** Fail closed when `version_id` is empty except for an explicit `--latest` flag. --- -### L-2 — No MFA Delete on versioned S3 bucket +### Low -**File:** `infra/cdk/stacks/envault_stack.py:69-91` +#### L-1 — GSI projections use `ALL` -**Description:** Versioning is enabled but MFA Delete is not configured. An attacker who compromises IAM credentials with `s3:DeleteObjectVersion` could permanently destroy all encrypted file versions. The `EnvaultUserPolicy` does not grant delete permissions, but other principals might. +**Location:** `infra/cdk/stacks/envault_stack.py` -**Recommendation:** Document and enable MFA Delete post-deployment (requires root account credentials; cannot be configured via CDK). +Changing projection forces table replacement. Acceptable for current query patterns (`rotate-key` reads all attributes). For new stacks, consider `INCLUDE`. ---- +#### L-2 — Access logs bucket uses S3-managed encryption -### L-3 — Hardcoded table name and policy name prevent multi-environment deployment +**Location:** `infra/cdk/stacks/envault_stack.py` `EnvaultAccessLogsBucket` -**Files:** `infra/cdk/stacks/envault_stack.py:99` (`table_name="envault-state"`), `infra/cdk/stacks/envault_stack.py:135` (`managed_policy_name="EnvaultUserPolicy"`) +Log delivery to a CMK-encrypted bucket needs extra key-policy grants. S3-managed SSE plus `BLOCK_ALL` and TLS is acceptable for access logs. -**Description:** Hardcoded names prevent deploying multiple stack instances in the same account/region (e.g., staging and production). Hardcoded IAM policy names also risk CloudFormation replacement failures. +#### L-3 — No MFA Delete on the versioned data bucket -**Recommendation:** Parameterize names or let CDK generate unique names. +Requires root credentials; cannot be set via CDK. Document as a post-deploy step. ---- +#### L-4 — Fake AWS credentials in CI are job-scoped -### L-4 — GSI projections use `ALL`, increasing storage and exposure surface +**Location:** `.github/workflows/ci.yml` test job env. Intentional for moto. Keep them off later steps. -**File:** `infra/cdk/stacks/envault_stack.py:114-127` +#### L-5 — No object-delete / purge CLI path -**Description:** Both GSIs project ALL attributes, replicating `encryption_context`, `s3_key`, `kms_key_id`, and `tags` into each index. This increases the attack surface if a query against a GSI inadvertently exposes sensitive metadata. - -**Recommendation:** Use `KEYS_ONLY` or `INCLUDE` projection with only the attributes needed for each query pattern. +Storage grows monotonically. Honouring a deletion request requires a new command plus a tightly scoped `s3:DeleteObject` grant (not present today). --- -### L-5 — S3 `put_object` doesn't specify `SSEKMSKeyId` - -**File:** `src/envault/s3.py:42-47` - -**Description:** The upload specifies `ServerSideEncryption="aws:kms"` but not `SSEKMSKeyId`, so S3 uses the bucket's default KMS key. If the bucket default differs from the envault CMK, server-side encryption uses a different key than intended. +## Strengths verified this week (do not re-open) -**Recommendation:** Pass the KMS key ID to `put_object`. +- Streaming encrypt/decrypt; checksum and encryption context verified before plaintext is renamed into place +- `DiscoveryFilter` with mandatory 12-digit account IDs; partition derived from region +- `attribute_not_exists(SK)` on audit events; decrypt does not flip CURRENT to DECRYPTED +- SHA-pinned Actions, `permissions: read-all`, OIDC PyPI, gitleaks job +- `os.execvpe` has no shell; `--clean-env` exists +- Filename sanitization for decrypt output (`Path.name`); Rich markup escaped +- No `eval` / `pickle` / `subprocess` with `shell=True` in application code +- `.env` gitignored; `.secrets.baseline` committed; CODEOWNERS present --- -### L-6 — Fake AWS credentials in CI set at job level +## Prior reports -**File:** `.github/workflows/ci.yml:73-76` - -**Description:** Fake AWS credentials (`testing`) for moto mocking are set at the job level. Any future CI step added after tests would inherit these environment variables, normalizing credentials in workflow env blocks. - -**Recommendation:** Scope the env block to the specific `run:` step, or add a comment documenting the intent. +- `docs/reviews/2026-07-26-deep-review.md` — state-machine findings (mostly fixed in 0.2.0) +- `docs/plans/2026-03-03-security-audit-fixes.md` — original 25-item remediation plan +- This file previously held a 2026-03-04 CISO review of commit `5b575e9`; those Critical items (plaintext-before-checksum, no streaming) are fixed. Historical copy belongs in `docs/reviews/` if needed. --- -### L-7 — No `CODEOWNERS` file for security-critical paths - -**File:** (missing) - -**Description:** No `CODEOWNERS` file enforces review requirements for changes to `.github/workflows/`, `src/envault/crypto.py`, `infra/cdk/`, or `pyproject.toml`. Any contributor with write access can modify the publish pipeline or cryptographic code without mandatory security review. - -**Recommendation:** Add `.github/CODEOWNERS`: - -``` -/.github/workflows/ @security-team -/src/envault/crypto.py @security-team -/infra/cdk/ @security-team -/pyproject.toml @security-team -``` - ---- +## Summary count -## CDK Infrastructure Assessment - -The CDK infrastructure has no CRITICAL or HIGH findings. Key observations: - -| Area | Status | -|------|--------| -| KMS CMK with auto-rotation | Enabled | -| S3 encryption with CMK | Enabled | -| S3 public access blocked | `BLOCK_ALL` on both buckets | -| TLS enforced | `enforce_ssl=True` on both buckets | -| S3 versioning | Enabled | -| S3 access logging | Enabled (dedicated bucket) | -| DynamoDB CMK encryption | `CUSTOMER_MANAGED` | -| DynamoDB PITR | Enabled | -| DynamoDB deletion protection | Enabled | -| Removal policies | `RETAIN` on all stateful resources | -| IAM least privilege | No delete permissions; scoped to specific ARNs | -| cdk-nag integration | `AwsSolutionsChecks` with justified suppressions | - -**Notable CDK-specific items:** -- Default KMS key policy grants `kms:*` to account root (standard CDK behavior, delegates to IAM). For shared accounts, consider adding conditions. -- Access logs bucket uses legacy ACL-based delivery (`ObjectWriter`). `BLOCK_ALL` mitigates the primary risk. -- No CloudTrail data events provisioned (expected to be configured at account/org level). - ---- - -## Security Strengths - -These represent meaningful security engineering: - -1. **`CommitmentPolicy.REQUIRE_ENCRYPT_REQUIRE_DECRYPT`** — strongest setting, prevents key commitment attacks -2. **`DiscoveryFilter` with mandatory account IDs** — hard failure if `ENVAULT_ALLOWED_ACCOUNT_IDS` unset -3. **Per-file encryption context as AAD** — cryptographic binding prevents ciphertext substitution at the AEAD layer -4. **Optimistic locking** — `ConditionExpression` on DynamoDB writes prevents concurrent modification -5. **Zero shell invocations** — no `subprocess`, `os.system`, `shell=True` anywhere in Python code -6. **Temp files via `mkstemp`** with `_best_effort_delete` zero-overwrite -7. **SHA-pinned GitHub Actions** with OIDC Trusted Publisher for PyPI -8. **Least-privilege IAM** — no delete permissions, scoped to specific resource ARNs -9. **CMK encryption on all data stores** with key rotation enabled -10. **cdk-nag `AwsSolutionsChecks`** with properly scoped, justified suppressions -11. **S3 public access fully blocked**, TLS enforced, versioning enabled, access logging to dedicated bucket -12. **DynamoDB PITR + deletion protection** with `RemovalPolicy.RETAIN` -13. **Content-addressed S3 keys** — `encrypted/{sha256[:2]}/{sha256}/{filename}.encrypted` prevents collisions -14. **Paginated DynamoDB queries** — `_paginate_query()` follows `LastEvaluatedKey` until exhaustion -15. **Tag input validation** — strict regex for keys, length limits for values -16. **JSON structured logging** to stderr via `python-json-logger` - ---- - -## Prior Audit Remediation Status - -All findings from the v1 audit (2026-03-03) have been verified: - -| Prior ID | Severity | Finding | Status | Evidence | -|----------|----------|---------|--------|----------| -| C-1 | CRITICAL | `migrate` hashes file path, not content | **FIXED** | `cli.py:478-485` calls `sha256_file(plaintext_path)` | -| C-2 | CRITICAL | Predictable world-readable `/tmp` paths | **FIXED** | Uses `tempfile.mkstemp()` with `_best_effort_delete()` | -| C-3 | CRITICAL | Unrestricted `.env` sourcing in shell scripts | **FIXED** | `encrypt.sh:5-8`, `decrypt.sh:5-8` extract only `S3_BUCKET` via `grep` | -| C-4 | CRITICAL | Code synced to production S3 bucket | **FIXED** | `aws s3 sync ../code` lines removed | -| H-1 | HIGH | Non-atomic state transitions | **PARTIAL** | Optimistic locking added; compensating transactions not yet implemented | -| H-2 | HIGH | Predictable temp file names / TOCTOU | **FIXED** | `tempfile.mkstemp()` used throughout | -| H-3 | HIGH | DynamoDB queries not paginated | **FIXED** | `_paginate_query()` at `state.py:96-106` | -| H-4 | HIGH | `DiscoveryAwsKmsMasterKeyProvider` unconstrained | **FIXED** | `crypto.py:163-170` uses `DiscoveryFilter` | -| H-5 | HIGH | Version ID race (`upload_file` + `head_object`) | **FIXED** | `s3.py:42` uses `put_object` returning `VersionId` | -| M-1 | MEDIUM | Non-numeric `ENVAULT_AUDIT_TTL_DAYS` | **FIXED** | Validates with try/except raising `ConfigurationError` | -| M-2 | MEDIUM | S3 key from basename only (collisions) | **FIXED** | Content-addressed keys with SHA256 prefix | -| M-3 | MEDIUM | `date-index` returns CURRENT records | **FIXED** | Filters on `SK.begins_with(EVENT_PREFIX)` | -| M-4 | MEDIUM | Broad `except Exception` in rotate-key | **FIXED** | Now catches `(EnvaultError, ClientError, BotoCoreError)` | -| M-5 | MEDIUM | Tag inputs not validated | **FIXED** | Validates keys with regex, values with length limit | -| M-6 | MEDIUM | Actions pinned by floating tag | **FIXED** | All actions pinned to commit SHAs | -| M-7 | MEDIUM | Empty `version_id` downloads latest | **OPEN** | Still falls back silently | -| M-8 | MEDIUM | S3 upload integrity not verified | **OPEN** | See current M-6 | - ---- - -## Summary Table - -| ID | Severity | Component | Finding | -|----|----------|-----------|---------| -| C-1 | CRITICAL | `crypto.py:178-188` | Plaintext written to disk before checksum verification | -| C-2 | CRITICAL | `crypto.py:87-92, 172-176` | No streaming; entire file buffered in memory | -| C-3 | CRITICAL | `tests/` | Zero test coverage on critical integrity checks | -| H-1 | HIGH | `cli.py:253` | Path traversal via `file_name` from DynamoDB | -| H-2 | HIGH | `cli.py:650-653` | Symlink traversal in `_collect_files` | -| H-3 | HIGH | `crypto.py:95-97, 179-181` | File permissions race (chmod after write) | -| H-4 | HIGH | `crypto.py:54-57` | `encrypt_file` retry includes non-retryable exceptions | -| H-5 | HIGH | `publish.yml` | Missing top-level `permissions` restriction | -| H-6 | HIGH | `publish.yml:3-6` | PyPI publish has no CI quality gate | -| H-7 | HIGH | `code/decrypt.conf:2` | Shell variable not expanded by aws-encryption-cli | -| H-8 | HIGH | `cli.py:143-161, 250-268` | Temp file cleanup on failure is untested | -| M-1 | MEDIUM | `cli.py:116-119` | Broad `except Exception` hides programming errors | -| M-2 | MEDIUM | `cli.py:302, 356` | SHA256 validation inconsistent across commands | -| M-3 | MEDIUM | `cli.py:676-696` | `_best_effort_delete` allocates full file size in memory | -| M-4 | MEDIUM | `cli.py:234, 556` | No format validation on `--allowed-account-ids` | -| M-5 | MEDIUM | `exceptions.py:41-44` | Encryption context leaked in error message | -| M-6 | MEDIUM | `s3.py:42-47` | S3 upload missing checksum verification | -| M-7 | MEDIUM | `.pre-commit-config.yaml:6` | `.secrets.baseline` non-functional | -| M-8 | MEDIUM | `cli.py:137-157` | TOCTOU between hash computation and encryption | -| M-9 | MEDIUM | `ci.yml` | No dependency vulnerability scanning | -| M-10 | MEDIUM | `infra/cdk/requirements.txt` | Open-ended version constraints | -| L-1 | LOW | CDK stack | S3 bucket missing `bucket_key_enabled` | -| L-2 | LOW | CDK stack | No MFA Delete on versioned S3 bucket | -| L-3 | LOW | CDK stack | Hardcoded resource names prevent multi-env | -| L-4 | LOW | CDK stack | GSI projections use `ALL` | -| L-5 | LOW | `s3.py:42-47` | S3 upload doesn't specify `SSEKMSKeyId` | -| L-6 | LOW | `ci.yml:73-76` | Fake credentials at job level | -| L-7 | LOW | Missing | No `CODEOWNERS` for security-critical paths | - ---- - -## Remediation Priority - -### Block Release — Before Production - -| # | ID | Effort | Description | -|---|-----|--------|-------------| -| 1 | C-1 | 1h | Verify checksum in memory before writing plaintext to disk | -| 2 | H-1 | 15m | Sanitize `record.file_name` in decrypt output path | -| 3 | H-2 | 15m | Skip symlinks in `_collect_files` | -| 4 | H-3 | 30m | Atomic file creation with `os.open()` and `0o600` | -| 5 | H-5 | 5m | Add `permissions: read-all` to `publish.yml` | -| 6 | H-6 | 30m | Gate PyPI publish on CI passing | -| 7 | M-1 | 10m | Narrow `except Exception` to specific types | -| 8 | M-5 | 10m | Remove encryption context from error messages | - -### Next Release - -| # | ID | Effort | Description | -|---|-----|--------|-------------| -| 9 | C-2 | 4h | Switch to streaming encrypt/decrypt API | -| 10 | C-3 | 8h | Add test coverage for all integrity checks and CLI flows | -| 11 | H-4 | 10m | Add `retry_if_not_exception_type` to encrypt retry | -| 12 | H-8 | 2h | Add tests for temp file cleanup on failure paths | -| 13 | M-2 | 30m | Unify SHA256 validation across all commands | -| 14 | M-3 | 15m | Chunk zero-overwrite in `_best_effort_delete` | -| 15 | M-4 | 15m | Validate account ID format | -| 16 | M-6 | 10m | Add `ChecksumAlgorithm` to S3 upload | -| 17 | M-7 | 15m | Fix detect-secrets baseline | -| 18 | M-9 | 30m | Add `pip-audit` to CI | - -### Hardening - -| # | ID | Effort | Description | -|---|-----|--------|-------------| -| 19 | L-1 | 5m | Enable S3 Bucket Keys | -| 20 | L-7 | 10m | Add CODEOWNERS | -| 21 | M-10 | 10m | Add upper bounds to CDK requirements | -| 22 | L-2 | 30m | Document/enable MFA Delete post-deploy | -| 23 | L-5 | 10m | Pass KMS key ID to S3 `put_object` | - ---- - -## Compliance Assessment - -| Framework | Assessment | -|-----------|------------| -| **SOC 2 (CC6.1, CC6.7)** | Strong. Encryption at rest (CMK) and in transit (TLS enforced). Access logging provides audit. Gap: no CloudTrail data events for object-level auditing. | -| **PCI-DSS (Req 3, 7, 10)** | Partially met. Encryption and access control are solid. Req 10 needs CloudTrail data events. MFA Delete recommended for Req 3. | -| **HIPAA (164.312)** | Partially met. Encryption and access controls strong. Object Lock (WORM) would strengthen PHI storage compliance. | - ---- - -## Verdict - -The cryptographic foundations are sound and the prior audit remediations are solid. The critical gap is the **decrypt path** — plaintext hits disk before integrity verification, and this entire flow is untested. Items 1-8 above (Block Release) are release blockers. After those fixes plus test coverage (items 9-12), this codebase meets production-grade security standards for a client-side encryption tool. - ---- +**This scan (on `main` before remediations):** 0 Critical, 8 High, 7 Medium, 6 Low -*End of report.* +**After this PR:** 0 Critical, 3 High, 5 Medium, 5 Low diff --git a/infra/cdk/stacks/envault_stack.py b/infra/cdk/stacks/envault_stack.py index d339e40..7915d18 100644 --- a/infra/cdk/stacks/envault_stack.py +++ b/infra/cdk/stacks/envault_stack.py @@ -74,13 +74,15 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non ) # Deny key deletion for all principals — requires removing this - # policy statement first (break-glass procedure). + # policy statement first (break-glass procedure). DisableKey is + # intentionally allowed so operators can freeze a compromised CMK + # during incident response without a CloudFormation change. encryption_key.add_to_resource_policy( iam.PolicyStatement( sid="DenyScheduleKeyDeletion", effect=iam.Effect.DENY, principals=[iam.AnyPrincipal()], - actions=["kms:ScheduleKeyDeletion", "kms:DisableKey"], + actions=["kms:ScheduleKeyDeletion"], resources=["*"], ) ) @@ -196,9 +198,8 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non "s3:PutObject", "s3:GetObject", "s3:GetObjectVersion", - "s3:ListBucket", ], - resources=[bucket.bucket_arn, f"{bucket.bucket_arn}/*"], + resources=[f"{bucket.bucket_arn}/encrypted/*"], ), iam.PolicyStatement( sid="DynamoDBStateAccess", @@ -206,10 +207,14 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non "dynamodb:PutItem", "dynamodb:GetItem", "dynamodb:Query", - "dynamodb:UpdateItem", ], resources=[table.table_arn, f"{table.table_arn}/index/*"], ), + iam.PolicyStatement( + sid="StsCallerIdentity", + actions=["sts:GetCallerIdentity"], + resources=["*"], + ), ], ) @@ -222,12 +227,12 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non { "id": "AwsSolutions-IAM5", "reason": ( - "S3 object-level actions (PutObject, GetObject) require" - " bucket/* wildcard. Access is scoped to the single" - " envault bucket." + "S3 object-level actions require a key prefix wildcard." + " Access is scoped to encrypted/* on the single envault" + " bucket — the CLI never lists or reads other prefixes." ), "applies_to": [ - f"Resource::<{bucket.node.id}.Arn>/*", + f"Resource::<{bucket.node.id}.Arn>/encrypted/*", ], }, { @@ -235,12 +240,21 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non "reason": ( "DynamoDB GSI queries require table/index/* wildcard." " Access is scoped to the single envault table and" - " only allows read/write operations." + " only allows PutItem/GetItem/Query." ), "applies_to": [ f"Resource::<{table.node.id}.Arn>/index/*", ], }, + { + "id": "AwsSolutions-IAM5", + "reason": ( + "sts:GetCallerIdentity does not support resource-level" + " authorization; Resource * is required by the API." + " Used only to attribute audit events to the caller." + ), + "applies_to": ["Resource::*"], + }, ], ) @@ -252,6 +266,7 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non "EnvaultOpsTopic", display_name="envault operational alerts", enforce_ssl=True, + master_key=encryption_key, ) # DynamoDB throttle alarm @@ -269,7 +284,7 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: object) -> Non # operations envault actually calls to stay within the 10-metric # alarm limit imposed by CloudWatch. sys_err_metrics: dict[str, cloudwatch.IMetric] = {} - for op in ("PutItem", "GetItem", "Query", "UpdateItem"): + for op in ("PutItem", "GetItem", "Query"): sys_err_metrics[op.lower()] = cloudwatch.Metric( namespace="AWS/DynamoDB", metric_name="SystemErrors", diff --git a/src/envault/cli.py b/src/envault/cli.py index 6a651e7..ef2025e 100644 --- a/src/envault/cli.py +++ b/src/envault/cli.py @@ -13,6 +13,7 @@ from pathlib import Path from typing import Any +import boto3 import click from botocore.exceptions import BotoCoreError, ClientError from rich.console import Console @@ -20,7 +21,7 @@ from rich.progress import track from rich.table import Table -from envault.config import Config +from envault.config import Config, boto_config, parse_audit_ttl_days from envault.crypto import decrypt_file, decrypt_to_stream, encrypt_file from envault.exceptions import ( AlreadyEncryptedError, @@ -34,7 +35,7 @@ from envault.fileutils import best_effort_delete as _best_effort_delete from envault.identity import caller_arn from envault.isolation import CredentialFd, harden_process, wipe -from envault.s3 import S3Store +from envault.s3 import S3Store, assert_s3_key_matches_hash from envault.state import DECRYPTED, ENCRYPTED, FileRecord, StateStore console = Console() @@ -135,7 +136,13 @@ def encrypt( INPUT_PATH can be a single file or a directory (processed recursively). """ - config = Config(key_id=key_id, bucket=bucket, table_name=table, region=region) + config = Config( + key_id=key_id, + bucket=bucket, + table_name=table, + region=region, + audit_ttl_days=_audit_ttl_days(), + ) tags = _parse_tags(tag) store = StateStore(table_name=table, region=region) s3 = S3Store(bucket=bucket, region=region, kms_key_id=key_id) @@ -282,6 +289,7 @@ def _encrypt_one( default="", help="Comma-separated AWS account IDs to trust for decryption.", ) +@click.option("--force", is_flag=True, help="Overwrite an existing destination file.") @click.pass_context def decrypt( ctx: click.Context, @@ -292,6 +300,7 @@ def decrypt( region: str, version: int, allowed_account_ids: str, + force: bool, ) -> None: """Decrypt a file by SHA256 hash or filename. @@ -305,15 +314,30 @@ def decrypt( record = _resolve_identifier(identifier, version, store) sha256_hash = record.sha256_hash + try: + assert_s3_key_matches_hash(record.s3_key, sha256_hash) + except EnvaultError as exc: + console.print(f"[bold red]Decryption error:[/bold red] {escape(str(exc))}") + sys.exit(1) + audit_ttl_days = _audit_ttl_days() - _fd, _tmp = tempfile.mkstemp(suffix=".encrypted", prefix="envault_dl_") - os.close(_fd) - tmp_encrypted = Path(_tmp) safe_name = Path(record.file_name).name if not safe_name or safe_name.startswith("."): safe_name = f"decrypted_{sha256_hash[:16]}" output_path = (output if output.is_dir() else output.parent) / safe_name + # Refuse before creating temps so a no-op collision leaves no residue. + if output_path.exists() and not force: + console.print( + f"[red]Refusing to overwrite {escape(str(output_path))}. " + "Pass --force to replace the existing file.[/red]" + ) + sys.exit(1) + + _fd, _tmp = tempfile.mkstemp(suffix=".encrypted", prefix="envault_dl_") + os.close(_fd) + tmp_encrypted = Path(_tmp) + try: s3.download_file( s3_key=record.s3_key, local_path=tmp_encrypted, version_id=record.s3_version_id @@ -372,6 +396,7 @@ def decrypt( record, operation="DECRYPT", correlation_id=correlation_id, + audit_ttl_days=audit_ttl_days, principal_arn=caller_arn(region), ) except (ClientError, BotoCoreError, EnvaultError) as exc: @@ -572,6 +597,8 @@ def migrate(from_path: Path, table: str, region: str, dry_run: bool) -> None: """ store = StateStore(table_name=table, region=region) imported = skipped = errors = 0 + audit_ttl_days = _audit_ttl_days() + import_root = from_path.parent.resolve() lines = from_path.read_text().splitlines() for i, line in enumerate(track(lines, description="Migrating records..."), start=1): @@ -580,14 +607,18 @@ def migrate(from_path: Path, table: str, region: str, dry_run: bool) -> None: continue try: entry = json.loads(line) - record = _parse_output_json_entry(entry) + record = _parse_output_json_entry(entry, import_root=import_root) if record is None: skipped += 1 continue if not dry_run: store.put_current_state(record) store.put_event( - record, operation="ENCRYPT", correlation_id="migrated-from-output-json" + record, + operation="ENCRYPT", + correlation_id="migrated-from-output-json", + audit_ttl_days=audit_ttl_days, + principal_arn=caller_arn(region), ) imported += 1 except StateConflictError: @@ -603,7 +634,9 @@ def migrate(from_path: Path, table: str, region: str, dry_run: bool) -> None: ) -def _parse_output_json_entry(entry: dict[str, Any]) -> FileRecord | None: +def _parse_output_json_entry( + entry: dict[str, Any], *, import_root: Path | None = None +) -> FileRecord | None: """Convert an output.json record to a FileRecord. Returns None if not an encrypt record.""" if entry.get("mode") != "encrypt": return None @@ -613,11 +646,7 @@ def _parse_output_json_entry(entry: dict[str, Any]) -> FileRecord | None: if not input_path: return None - plaintext_path = Path(input_path) - if ".." in plaintext_path.parts: - raise MigrationError(f"Path traversal not allowed in migration input: {input_path!r}") - if plaintext_path.is_absolute(): - logger.warning("Absolute path in migration input: %s", input_path) + plaintext_path = _confine_migration_path(input_path, import_root) file_name = S3Store._sanitize_filename(plaintext_path.name) algorithm = _extract_algorithm(header) @@ -704,6 +733,9 @@ def rotate_key( correlation_id = str(uuid.uuid4()) account_ids = _validate_account_ids(allowed_account_ids) + if not dry_run: + _preflight_kms_key(new_key_id, region) + # Every tracked file still has ciphertext in S3 regardless of the state # recorded against it, so rotation must cover them all. Records written by # earlier versions were flipped to DECRYPTED on first read; skipping those @@ -729,6 +761,8 @@ def rotate_key( console.print(f" Would rotate: {escape(r.file_name)} ({r.sha256_hash[:16]}...)") return + audit_ttl_days = _audit_ttl_days() + console.print( "[dim yellow]Note: Temporary plaintext is overwritten with zeros before deletion, " "but secure erasure is not guaranteed on copy-on-write filesystems (APFS, Btrfs, " @@ -756,6 +790,7 @@ def rotate_key( os.close(_fd_enc) tmp_enc = Path(_tmp_enc) + assert_s3_key_matches_hash(record.s3_key, record.sha256_hash) s3.download_file(record.s3_key, tmp_dl, record.s3_version_id) decrypt_file( tmp_dl, @@ -791,6 +826,7 @@ def rotate_key( record, operation="ROTATE_KEY", correlation_id=correlation_id, + audit_ttl_days=audit_ttl_days, principal_arn=caller_arn(region), ) except Exception: @@ -931,6 +967,7 @@ def exec_( store = StateStore(table_name=table, region=region) s3 = S3Store(bucket=bucket, region=region) correlation_id = str(uuid.uuid4()) + audit_ttl_days = _audit_ttl_days() env_pairs = [_parse_secret_spec(spec, "--secret") for spec in env_specs] file_pairs = [_parse_secret_spec(spec, "--file") for spec in file_specs] @@ -941,6 +978,11 @@ def exec_( if clean_env else dict(os.environ) ) + if not clean_env and any(key.startswith("AWS_") for key in child_env): + console.print( + "[yellow]Warning:[/yellow] the child will inherit AWS credentials from this " + "environment. Pass --clean-env to start it from a minimal environment." + ) creds: list[CredentialFd] = [] sinks: list[_BufferSink] = [] @@ -979,6 +1021,7 @@ def exec_( record, operation="ACCESS", correlation_id=correlation_id, + audit_ttl_days=audit_ttl_days, principal_arn=principal, ) except EncryptionContextMismatchError: @@ -1043,6 +1086,7 @@ def _stream_secret( account_ids: list[str], ) -> None: """Fetch a record's ciphertext and decrypt it into ``out``, verifying first.""" + assert_s3_key_matches_hash(record.s3_key, record.sha256_hash) ciphertext = s3.download_to_memory(record.s3_key, record.s3_version_id) decrypt_to_stream( ciphertext, @@ -1127,6 +1171,77 @@ def _short_principal(arn: str) -> str: _TAG_VALUE_MAX_LEN = 256 +def _audit_ttl_days() -> int: + """Read ENVAULT_AUDIT_TTL_DAYS; exit on invalid values.""" + try: + return parse_audit_ttl_days() + except ConfigurationError as exc: + console.print(f"[bold red]Configuration error:[/bold red] {escape(str(exc))}") + sys.exit(1) + + +def _preflight_kms_key(key_id: str, region: str) -> None: + """Fail before any plaintext is written if the target CMK is unreachable.""" + kms = boto3.client("kms", region_name=region, config=boto_config) + try: + kms.describe_key(KeyId=key_id) + except ClientError as exc: + msg = exc.response.get("Error", {}).get("Message", str(exc)) + console.print( + f"[bold red]Cannot use KMS key {escape(key_id)}:[/bold red] {escape(msg)}\n" + "Rotation did not download or decrypt any files. Grant kms:DescribeKey " + "(and GenerateDataKey) on the target key, or pass a key this principal can use." + ) + sys.exit(1) + + +def _path_has_symlink_component(path: Path) -> bool: + """True if any existing component of ``path`` is a symlink.""" + parts = path.parts + if not parts: + return False + if path.is_absolute(): + current = Path(parts[0]) + rest = parts[1:] + else: + current = Path() + rest = parts + for part in rest: + current = current / part + try: + if current.is_symlink(): + return True + except OSError: + return True + return False + + +def _confine_migration_path(input_path: str, import_root: Path | None) -> Path: + """Resolve a migration input path, rejecting traversal and per-component symlinks.""" + candidate = Path(input_path) + if ".." in candidate.parts: + raise MigrationError(f"Path traversal not allowed in migration input: {input_path!r}") + + if import_root is None: + if _path_has_symlink_component(candidate): + raise MigrationError(f"Symlink not allowed in migration input: {input_path!r}") + return candidate + + root = import_root.resolve() + joined = candidate if candidate.is_absolute() else root / candidate + if _path_has_symlink_component(joined): + raise MigrationError(f"Symlink not allowed in migration input: {input_path!r}") + + resolved = joined.resolve() + try: + resolved.relative_to(root) + except ValueError as exc: + raise MigrationError( + f"Path {input_path!r} is outside the import directory {str(root)!r}" + ) from exc + return resolved + + def _validate_sha256(value: str) -> str: """Validate a SHA256 hash string. Exit with error if invalid.""" if not _SHA256_RE.fullmatch(value): @@ -1222,11 +1337,21 @@ def _validate_account_ids(raw: str) -> list[str]: def _collect_files(path: Path) -> list[Path]: + """Return regular files under ``path``, never following directory or file symlinks.""" if path.is_symlink(): return [] if path.is_file(): return [path] - return [p for p in path.rglob("*") if p.is_file() and not p.is_symlink()] + files: list[Path] = [] + for root, dirnames, filenames in os.walk(path, followlinks=False): + # os.walk(followlinks=False) still lists symlink directories; drop them + # so we never encrypt a tree the operator did not intend to include. + dirnames[:] = [d for d in dirnames if not Path(root, d).is_symlink()] + for name in filenames: + candidate = Path(root) / name + if candidate.is_file() and not candidate.is_symlink(): + files.append(candidate) + return files def _parse_tags(tag_strs: tuple[str, ...]) -> dict[str, str]: diff --git a/src/envault/config.py b/src/envault/config.py index 78cf263..3842097 100644 --- a/src/envault/config.py +++ b/src/envault/config.py @@ -29,6 +29,25 @@ ) +def parse_audit_ttl_days(raw: str | None = None) -> int: + """Parse ``ENVAULT_AUDIT_TTL_DAYS`` (or ``raw``) into a positive integer. + + Click commands construct :class:`Config` without going through + :meth:`Config.from_env`, so they must call this directly or audit TTL + silently stays at the dataclass default of 365. + """ + _ttl_raw = raw if raw is not None else os.environ.get("ENVAULT_AUDIT_TTL_DAYS", "365") + try: + audit_ttl_days = int(_ttl_raw) + if audit_ttl_days <= 0: + raise ValueError("must be positive") + except ValueError as exc: + raise ConfigurationError( + f"ENVAULT_AUDIT_TTL_DAYS must be a positive integer (days). Got: {_ttl_raw!r}" + ) from exc + return audit_ttl_days + + @dataclass class Config: """Runtime configuration loaded from environment variables.""" @@ -93,15 +112,7 @@ def from_env(cls) -> Config: ) region = os.environ.get("ENVAULT_REGION", "us-east-1") - _ttl_raw = os.environ.get("ENVAULT_AUDIT_TTL_DAYS", "365") - try: - audit_ttl_days = int(_ttl_raw) - if audit_ttl_days <= 0: - raise ValueError("must be positive") - except ValueError as exc: - raise ConfigurationError( - f"ENVAULT_AUDIT_TTL_DAYS must be a positive integer (days). Got: {_ttl_raw!r}" - ) from exc + audit_ttl_days = parse_audit_ttl_days() _account_ids_raw = os.environ.get("ENVAULT_ALLOWED_ACCOUNT_IDS", "") allowed_account_ids = [a.strip() for a in _account_ids_raw.split(",") if a.strip()] diff --git a/src/envault/crypto.py b/src/envault/crypto.py index 95d6a4f..2059448 100644 --- a/src/envault/crypto.py +++ b/src/envault/crypto.py @@ -166,49 +166,55 @@ def encrypt_file( output_path.parent.mkdir(parents=True, exist_ok=True) fd = os.open(str(output_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW, 0o600) - with input_path.open("rb") as raw_input: - hashing_reader = _HashingReader(raw_input) - with client.stream( - source=hashing_reader, - mode="e", - key_provider=key_provider, - encryption_context=encryption_context, - frame_length=4096, - ) as encryptor: - with os.fdopen(fd, "wb") as out: - while True: - chunk = encryptor.read(_CHUNK_SIZE) - if not chunk: - break - out.write(chunk) - header = encryptor.header - - sha256_hash = hashing_reader.hexdigest - - algorithm = ( - header.algorithm.name if hasattr(header.algorithm, "name") else str(header.algorithm) - ) - message_id = ( - header.message_id.hex() if isinstance(header.message_id, bytes) else str(header.message_id) - ) - - logger.info( - "Encryption complete", - extra={ - "sha256": sha256_hash[:16], - "output": str(output_path), - "algorithm": algorithm, - "message_id": message_id, - }, - ) - - return EncryptResult( - sha256_hash=sha256_hash, - file_size_bytes=file_size, - algorithm=algorithm, - message_id=message_id, - output_path=output_path, - ) + try: + with input_path.open("rb") as raw_input: + hashing_reader = _HashingReader(raw_input) + with client.stream( + source=hashing_reader, + mode="e", + key_provider=key_provider, + encryption_context=encryption_context, + frame_length=4096, + ) as encryptor: + with os.fdopen(fd, "wb") as out: + fd = -1 # ownership transferred; fdopen closes on exit + while True: + chunk = encryptor.read(_CHUNK_SIZE) + if not chunk: + break + out.write(chunk) + header = encryptor.header + + sha256_hash = hashing_reader.hexdigest + algorithm = ( + header.algorithm.name if hasattr(header.algorithm, "name") else str(header.algorithm) + ) + message_id = ( + header.message_id.hex() + if isinstance(header.message_id, bytes) + else str(header.message_id) + ) + logger.info( + "Encryption complete", + extra={ + "sha256": sha256_hash[:16], + "output": str(output_path), + "algorithm": algorithm, + "message_id": message_id, + }, + ) + return EncryptResult( + sha256_hash=sha256_hash, + file_size_bytes=file_size, + algorithm=algorithm, + message_id=message_id, + output_path=output_path, + ) + finally: + # If stream() (or anything before fdopen) fails, close the raw fd so + # we neither leak descriptors nor leave an unlinked 0600 file open. + if fd >= 0: + os.close(fd) def _discovery_key_provider( @@ -385,6 +391,7 @@ def decrypt_file( try: with os.fdopen(tmp_fd, "wb") as out, input_path.open("rb") as encrypted_file: + tmp_fd = -1 # ownership transferred; fdopen closes on exit result = decrypt_to_stream( encrypted_file, out, @@ -395,6 +402,8 @@ def decrypt_file( ) os.replace(tmp_path, output_path) finally: + if tmp_fd >= 0: + os.close(tmp_fd) # No-op after a successful rename; zero-overwrites partial plaintext # left behind by any failure above. best_effort_delete(tmp_path) diff --git a/src/envault/s3.py b/src/envault/s3.py index 45d5e40..d0d1fea 100644 --- a/src/envault/s3.py +++ b/src/envault/s3.py @@ -21,6 +21,23 @@ # belongs in the streaming `decrypt` path, not `exec`. MAX_IN_MEMORY_BYTES = 16 * 1024 * 1024 +# Content-addressed object keys: encrypted/{sha256[:2]}/{sha256}/{name}.encrypted +_S3_KEY_RE = re.compile(r"^encrypted/([0-9a-f]{2})/([0-9a-f]{64})/[^/]+\.encrypted$") + + +def assert_s3_key_matches_hash(s3_key: str, sha256_hash: str) -> None: + """Reject DynamoDB-sourced keys that are not this file's content-addressed path. + + A poisoned ``s3_key`` must not be able to make the CLI fetch an arbitrary + object from the bucket (or a different file's ciphertext). + """ + match = _S3_KEY_RE.fullmatch(s3_key) + if not match or match.group(1) != sha256_hash[:2] or match.group(2) != sha256_hash: + raise EnvaultError( + "Refusing to fetch S3 object: key is not the content-addressed " + f"path for hash {sha256_hash[:16]}...." + ) + class S3Store: """Handles upload and download of encrypted files to/from S3.""" diff --git a/src/envault/state.py b/src/envault/state.py index 6cb2e08..6ed71df 100644 --- a/src/envault/state.py +++ b/src/envault/state.py @@ -63,7 +63,9 @@ def to_dynamo_item(self, sk: str) -> dict[str, Any]: def _now_iso() -> str: - return datetime.now(timezone.utc).isoformat(timespec="seconds") + # Microseconds (not seconds) so last_updated can serve as a CAS token: + # two writers in the same wall-clock second must not share a token. + return datetime.now(timezone.utc).isoformat(timespec="microseconds") def _today_str() -> str: @@ -335,21 +337,32 @@ def _count_by_state(self, state: str) -> int: return count def _latest_record_timestamp(self, state: str) -> str | None: - """Return the last_updated timestamp of the most recent CURRENT record in a state.""" + """Return the last_updated timestamp of the most recent CURRENT record in a state. + + DynamoDB applies ``Limit`` *before* ``FilterExpression``. Event items + inherit ``current_state`` and land in this GSI, so a Limit=1 query + almost always fetches an EVENT, filters it out, and returns empty. + Page until a CURRENT item survives the filter. + """ from boto3.dynamodb.conditions import Attr - response = self._table.query( - IndexName="state-index", - KeyConditionExpression=Key("current_state").eq(state), - FilterExpression=Attr("SK").eq(CURRENT), - ScanIndexForward=False, - Limit=1, - ) - items = response.get("Items", []) - if not items: - return None - value = items[0].get("last_updated") - return str(value) if value is not None else None + query_kwargs: dict[str, Any] = { + "IndexName": "state-index", + "KeyConditionExpression": Key("current_state").eq(state), + "FilterExpression": Attr("SK").eq(CURRENT), + "ScanIndexForward": False, + "Limit": 25, + } + while True: + response = self._table.query(**query_kwargs) + items = response.get("Items", []) + if items: + value = items[0].get("last_updated") + return str(value) if value is not None else None + last_key = response.get("LastEvaluatedKey") + if not last_key: + return None + query_kwargs["ExclusiveStartKey"] = last_key def summary(self) -> dict[str, Any]: """Return aggregate counts and last activity timestamp for the dashboard.""" diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 6954d92..7f9785a 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -14,6 +14,7 @@ from envault.cli import ( _best_effort_delete, + _collect_files, _friendly_message, _parse_output_json_entry, _parse_tags, @@ -38,6 +39,13 @@ } +def _ensure_kms_alias(alias: str = "alias/new-key") -> None: + """Create a moto KMS alias so rotate-key's DescribeKey preflight succeeds.""" + kms = boto3.client("kms", region_name=REGION) + key = kms.create_key(Description="rotation-target") + kms.create_alias(AliasName=alias, TargetKeyId=key["KeyMetadata"]["KeyId"]) + + def _create_table() -> None: client = boto3.client("dynamodb", region_name=REGION) client.create_table( @@ -166,6 +174,42 @@ def test_parse_entry_rejects_path_traversal() -> None: _parse_output_json_entry(entry) +def test_parse_entry_rejects_path_outside_import_root(tmp_path: Path) -> None: + """Absolute paths outside the import directory must not be hashed.""" + outside = tmp_path / "outside" + outside.mkdir() + secret = outside / "secret.txt" + secret.write_bytes(b"x") + import_root = tmp_path / "import" + import_root.mkdir() + with pytest.raises(MigrationError, match="outside the import directory"): + _parse_output_json_entry(_make_entry(str(secret)), import_root=import_root) + + +def test_parse_entry_confined_relative_path(tmp_path: Path) -> None: + """Relative paths are resolved inside the import directory.""" + import_root = tmp_path / "import" + import_root.mkdir() + content = b"sensitive data\n" + (import_root / "secret.txt").write_bytes(content) + expected_hash = hashlib.sha256(content).hexdigest() + record = _parse_output_json_entry(_make_entry("secret.txt"), import_root=import_root) + assert record is not None + assert record.sha256_hash == expected_hash + + +def test_parse_entry_rejects_symlink_component(tmp_path: Path) -> None: + """A symlink anywhere in the migration path must be rejected before I/O.""" + import_root = tmp_path / "import" + import_root.mkdir() + real = tmp_path / "realdir" + real.mkdir() + (real / "secret.txt").write_bytes(b"x") + (import_root / "link").symlink_to(real) + with pytest.raises(MigrationError, match="Symlink"): + _parse_output_json_entry(_make_entry("link/secret.txt"), import_root=import_root) + + def test_parse_entry_records_file_size(tmp_path: Path) -> None: """Migrated records must store actual file size, not zero.""" plaintext = tmp_path / "sized.txt" @@ -683,6 +727,7 @@ def test_rotate_key_end_to_end(tmp_path: Path) -> None: """rotate-key: mocked decrypt + re-encrypt, real DynamoDB + S3.""" _create_table() _create_bucket() + _ensure_kms_alias() s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" version_id = _upload_fake_ciphertext(s3_key) store = StateStore(table_name=TABLE_NAME, region=REGION) @@ -908,6 +953,7 @@ def test_rotate_key_logs_recovery_info_on_state_write_failure( _create_table() _create_bucket() + _ensure_kms_alias() s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" version_id = _upload_fake_ciphertext(s3_key) store = StateStore(table_name=TABLE_NAME, region=REGION) @@ -959,6 +1005,7 @@ def test_rotate_key_recovery_log_records_old_key( _create_table() _create_bucket() + _ensure_kms_alias() s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" version_id = _upload_fake_ciphertext(s3_key) store = StateStore(table_name=TABLE_NAME, region=REGION) @@ -1006,6 +1053,7 @@ def test_rotate_key_mkstemp_failure_is_handled(tmp_path: Path) -> None: not an UnboundLocalError from the cleanup block referencing unset paths.""" _create_table() _create_bucket() + _ensure_kms_alias() s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" version_id = _upload_fake_ciphertext(s3_key) store = StateStore(table_name=TABLE_NAME, region=REGION) @@ -1181,24 +1229,48 @@ def test_decrypt_is_repeatable(tmp_path: Path) -> None: store = StateStore(table_name=TABLE_NAME, region=REGION) _seed_encrypted_record(store, s3_version_id=version_id) - args = [ - "decrypt", - FAKE_SHA, - "--output", - str(tmp_path), - "--table", - TABLE_NAME, - "--bucket", - BUCKET_NAME, - "--region", - REGION, - "--allowed-account-ids", - ACCOUNT_IDS, - ] + out1 = tmp_path / "first" + out2 = tmp_path / "second" + out1.mkdir() + out2.mkdir() runner = CliRunner() with patch("envault.cli.decrypt_file", side_effect=_mock_decrypt_file_ok): - first = runner.invoke(main, args, env=_CLI_ENV) - second = runner.invoke(main, args, env=_CLI_ENV) + first = runner.invoke( + main, + [ + "decrypt", + FAKE_SHA, + "--output", + str(out1), + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env=_CLI_ENV, + ) + second = runner.invoke( + main, + [ + "decrypt", + FAKE_SHA, + "--output", + str(out2), + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env=_CLI_ENV, + ) assert first.exit_code == 0, first.output assert second.exit_code == 0, second.output @@ -1248,6 +1320,7 @@ def test_rotate_key_covers_records_left_decrypted_by_old_versions(tmp_path: Path """C-2: a record stuck in DECRYPTED still has ciphertext in S3 and must rotate.""" _create_table() _create_bucket() + _ensure_kms_alias() s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" version_id = _upload_fake_ciphertext(s3_key) store = StateStore(table_name=TABLE_NAME, region=REGION) @@ -1340,7 +1413,9 @@ def test_status_escapes_markup_in_file_names() -> None: assert result.exit_code == 0, result.output # If the markup were interpreted, the tag text would be consumed as styling # and the displayed name would differ from the name actually stored. - assert "bold red" in result.output + # Rich may wrap the cell, so match the tag in pieces. + assert "[bold" in result.output + assert "not-my-name" in result.output @mock_aws @@ -1448,3 +1523,211 @@ def test_cli_entrypoint_escapes_markup_in_usage_errors() -> None: cli() assert exc_info.value.code == 2 assert "[/nope]" in output.getvalue() + + +def test_collect_files_skips_directory_symlinks(tmp_path: Path) -> None: + """os.walk must not follow a symlink into an unrelated tree.""" + root = tmp_path / "root" + root.mkdir() + (root / "ok.txt").write_bytes(b"ok") + outside = tmp_path / "outside" + outside.mkdir() + (outside / "id_rsa").write_bytes(b"secret-key") + (root / "link").symlink_to(outside) + + names = {p.name for p in _collect_files(root)} + assert names == {"ok.txt"} + + +def test_collect_files_skips_file_symlinks(tmp_path: Path) -> None: + root = tmp_path / "root" + root.mkdir() + target = tmp_path / "secret.env" + target.write_bytes(b"TOKEN=1") + (root / "alias.env").symlink_to(target) + (root / "real.txt").write_bytes(b"ok") + names = {p.name for p in _collect_files(root)} + assert names == {"real.txt"} + + +@mock_aws +def test_decrypt_refuses_to_overwrite_existing_file(tmp_path: Path) -> None: + _create_table() + _create_bucket() + s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" + version_id = _upload_fake_ciphertext(s3_key) + store = StateStore(table_name=TABLE_NAME, region=REGION) + _seed_encrypted_record(store, s3_version_id=version_id) + + dest = tmp_path / "test.txt" + dest.write_bytes(b"do not clobber") + + runner = CliRunner() + with patch("envault.cli.decrypt_file", side_effect=_mock_decrypt_file_ok): + result = runner.invoke( + main, + [ + "decrypt", + FAKE_SHA, + "--output", + str(tmp_path), + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env=_CLI_ENV, + ) + + assert result.exit_code != 0 + assert "overwrite" in result.output.lower() + assert dest.read_bytes() == b"do not clobber" + + +@mock_aws +def test_decrypt_force_overwrites_existing_file(tmp_path: Path) -> None: + _create_table() + _create_bucket() + s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" + version_id = _upload_fake_ciphertext(s3_key) + store = StateStore(table_name=TABLE_NAME, region=REGION) + _seed_encrypted_record(store, s3_version_id=version_id) + + dest = tmp_path / "test.txt" + dest.write_bytes(b"old") + + runner = CliRunner() + with patch("envault.cli.decrypt_file", side_effect=_mock_decrypt_file_ok): + result = runner.invoke( + main, + [ + "decrypt", + FAKE_SHA, + "--output", + str(tmp_path), + "--force", + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env=_CLI_ENV, + ) + + assert result.exit_code == 0, result.output + assert dest.read_bytes() == b"decrypted content" + + +@mock_aws +def test_decrypt_rejects_poisoned_s3_key(tmp_path: Path) -> None: + """A DynamoDB s3_key that is not content-addressed must not be fetched.""" + _create_table() + _create_bucket() + store = StateStore(table_name=TABLE_NAME, region=REGION) + record = _seed_encrypted_record(store) + table = boto3.resource("dynamodb", region_name=REGION).Table(TABLE_NAME) + table.update_item( + Key={"PK": f"FILE#{record.sha256_hash}", "SK": "CURRENT"}, + UpdateExpression="SET s3_key = :k", + ExpressionAttributeValues={":k": "other-prefix/not-ours"}, + ) + + runner = CliRunner() + result = runner.invoke( + main, + [ + "decrypt", + FAKE_SHA, + "--output", + str(tmp_path), + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env=_CLI_ENV, + ) + assert result.exit_code != 0 + assert "content-addressed" in result.output.lower() or "refusing" in result.output.lower() + + +@mock_aws +def test_decrypt_honours_audit_ttl_days(tmp_path: Path) -> None: + import time + + _create_table() + _create_bucket() + s3_key = f"encrypted/{FAKE_SHA[:2]}/{FAKE_SHA}/test.txt.encrypted" + version_id = _upload_fake_ciphertext(s3_key) + store = StateStore(table_name=TABLE_NAME, region=REGION) + _seed_encrypted_record(store, s3_version_id=version_id) + + runner = CliRunner() + with patch("envault.cli.decrypt_file", side_effect=_mock_decrypt_file_ok): + result = runner.invoke( + main, + [ + "decrypt", + FAKE_SHA, + "--output", + str(tmp_path), + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env={**_CLI_ENV, "ENVAULT_AUDIT_TTL_DAYS": "7"}, + ) + assert result.exit_code == 0, result.output + events = [e for e in store.list_events_for_file(FAKE_SHA) if e["operation"] == "DECRYPT"] + assert len(events) == 1 + expected = int(time.time()) + 7 * 86400 + assert abs(int(events[0]["ttl"]) - expected) < 15 + + +@mock_aws +def test_rotate_key_preflight_fails_before_download() -> None: + """DescribeKey on an unknown target key must fail closed with no decrypt.""" + _create_table() + _create_bucket() + store = StateStore(table_name=TABLE_NAME, region=REGION) + _seed_encrypted_record(store) + + runner = CliRunner() + with patch("envault.cli.decrypt_file") as decrypt_mock: + result = runner.invoke( + main, + [ + "rotate-key", + "--new-key-id", + "alias/does-not-exist", + "--table", + TABLE_NAME, + "--bucket", + BUCKET_NAME, + "--region", + REGION, + "--allowed-account-ids", + ACCOUNT_IDS, + ], + env=_CLI_ENV, + ) + assert result.exit_code != 0 + assert decrypt_mock.call_count == 0 + assert "cannot use kms key" in result.output.lower() diff --git a/tests/unit/test_exec.py b/tests/unit/test_exec.py index 29c8929..a9cef84 100644 --- a/tests/unit/test_exec.py +++ b/tests/unit/test_exec.py @@ -283,11 +283,20 @@ def test_exec_refuses_on_checksum_mismatch() -> None: _seed_secret(runner, env) store = StateStore(table_name=TABLE_NAME, region=REGION) record = store.list_by_state("ENCRYPTED")[0] + new_hash = "b" * 64 + new_key = f"encrypted/{new_hash[:2]}/{new_hash}/{record.file_name}.encrypted" + s3 = boto3.client("s3", region_name=REGION) + body = s3.get_object(Bucket=BUCKET_NAME, Key=record.s3_key)["Body"].read() + put = s3.put_object(Bucket=BUCKET_NAME, Key=new_key, Body=body) table = boto3.resource("dynamodb", region_name=REGION).Table(TABLE_NAME) table.update_item( Key={"PK": f"FILE#{record.sha256_hash}", "SK": "CURRENT"}, - UpdateExpression="SET sha256_hash = :h", - ExpressionAttributeValues={":h": "b" * 64}, + UpdateExpression="SET sha256_hash = :h, s3_key = :k, s3_version_id = :v", + ExpressionAttributeValues={ + ":h": new_hash, + ":k": new_key, + ":v": put.get("VersionId", ""), + }, ) rec = _ExecRecorder() with patch("os.execvpe", rec): @@ -422,3 +431,19 @@ def test_exec_file_mode_survives_markup_in_secret_name() -> None: ) assert rec.called, result.output assert rec.file_contents["TLS_CERT"] == SECRET_VALUE + + +@mock_aws +def test_exec_warns_when_inheriting_aws_credentials() -> None: + account = _provision() + env = _env(account) + runner = CliRunner() + with runner.isolated_filesystem(): + _seed_secret(runner, env) + rec = _ExecRecorder() + with patch("os.execvpe", rec): + result = runner.invoke( + main, ["exec", "-s", "db.env=DATABASE_URL", "--", "/bin/true"], env=env + ) + assert rec.called, result.output + assert "inherit AWS credentials" in result.output diff --git a/tests/unit/test_s3.py b/tests/unit/test_s3.py index c3de727..47260a1 100644 --- a/tests/unit/test_s3.py +++ b/tests/unit/test_s3.py @@ -261,3 +261,29 @@ def test_download_to_memory_does_not_touch_disk(tmp_path): store = S3Store(bucket=BUCKET, region=REGION) store.download_to_memory("enc/a") assert not list(tmp_path.iterdir()) + + +def test_assert_s3_key_matches_hash_accepts_content_addressed_key(): + from envault.s3 import assert_s3_key_matches_hash + + sha = "a" * 64 + assert_s3_key_matches_hash(f"encrypted/{sha[:2]}/{sha}/file.txt.encrypted", sha) + + +def test_assert_s3_key_matches_hash_rejects_wrong_prefix(): + from envault.exceptions import EnvaultError + from envault.s3 import assert_s3_key_matches_hash + + sha = "a" * 64 + with pytest.raises(EnvaultError, match="content-addressed"): + assert_s3_key_matches_hash("other/prefix/file.encrypted", sha) + + +def test_assert_s3_key_matches_hash_rejects_hash_mismatch(): + from envault.exceptions import EnvaultError + from envault.s3 import assert_s3_key_matches_hash + + sha = "a" * 64 + other = "b" * 64 + with pytest.raises(EnvaultError, match="content-addressed"): + assert_s3_key_matches_hash(f"encrypted/{other[:2]}/{other}/file.txt.encrypted", sha) diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index f6e09f4..10952ec 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -466,6 +466,24 @@ def test_summary_returns_last_activity_timestamp(): ) +@mock_aws +def test_summary_last_activity_pages_past_event_items(): + """Event items share the GSI; last_activity must still find the CURRENT record.""" + store = _create_table() + record = _make_record( + sha256_hash="a" * 64, + current_state=ENCRYPTED, + encrypted_at="2026-03-03T10:00:00+00:00", + ) + store.put_current_state(record) + for i in range(5): + store.put_event(record, operation="ENCRYPT", correlation_id=f"corr-{i}") + + summary = store.summary() + assert summary["last_activity"] != "\u2014" + assert "T" in summary["last_activity"] + + @mock_aws def test_summary_counts_exclude_events(): """summary() total/encrypted/decrypted counts must not double-count EVENT records."""