From e5c61059a77a3912bb0270df394cef3a0380cda4 Mon Sep 17 00:00:00 2001 From: stacknil <219124580+stacknil@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:24:07 +0800 Subject: [PATCH] fix(signer): sample authorization time inside transaction --- docs/design-decisions/SECURITY-INCREMENT.md | 145 ++++++++++++++++++ .../design-decisions/THREAT-BOUNDARY-AUDIT.md | 103 +++++++++++++ signer/repo_sentinel_signer/service.py | 4 +- tests/test_repo_sentinel_signer_time.py | 133 ++++++++++++++++ 4 files changed, 383 insertions(+), 2 deletions(-) create mode 100644 docs/design-decisions/SECURITY-INCREMENT.md create mode 100644 docs/design-decisions/THREAT-BOUNDARY-AUDIT.md create mode 100644 tests/test_repo_sentinel_signer_time.py diff --git a/docs/design-decisions/SECURITY-INCREMENT.md b/docs/design-decisions/SECURITY-INCREMENT.md new file mode 100644 index 0000000..a8e4cbb --- /dev/null +++ b/docs/design-decisions/SECURITY-INCREMENT.md @@ -0,0 +1,145 @@ +# Signer Transaction-Time Freshness + +## Problem + +An authorized request could wait for the signer lock across its token or +evaluation expiry, then be accepted using time sampled before that wait. +The bounded reproduction is documented in +[THREAT-BOUNDARY-AUDIT.md](THREAT-BOUNDARY-AUDIT.md). + +## Invariant + +Admission and finalization must sample authorization time after acquiring the +shared transaction lock. Expired OIDC is refused before admission or publication; +an expired evaluation with valid execution identity is refused after consuming +its finalization JTI, as required by the existing replay contract. + +## Design decision + +Move the existing `now = self._now()` statement inside `store.transaction()` in +the two service entrypoints. This changes two statement locations and introduces +no dependency, new error code, schema, authority source, or policy identity. +Ticket issuance time and TTL now begin at transaction entry. An existing +idempotent ticket retains its original frozen timestamps. + +## Threat and failure model + +The adversarial schedule is: request arrives with valid identity, waits behind +another lock holder, expires, then acquires the lock. Both new admission and +idempotent admission must refuse an expired identity. Finalization must refuse +expired identity or evaluation without reserving a slot or calling the publisher. +The local reproduction uses synthetic claims and a controlled clock; it assumes +neither stolen real credentials nor control over production server time. + +## Rejected alternatives + +1. Extend token/evaluation lifetimes or add queue-time grace. Rejected because + this would weaken the frozen lifetime contract instead of checking it correctly. +2. Replace the shared lock with a new transaction/outbox architecture. Rejected + because production persistence is a separate, unauthorized implementation scope. +3. Change `P_v(H)`, status contexts, or the candidate policy bundle. Rejected + because none is responsible for this operational authorization-time defect. +4. Change equality-at-expiry semantics. Rejected because the existing inclusive + contract is separate from the stale-time defect and remains explicitly tested. + +## Compatibility + +Existing valid requests, frozen registry selection, exact H, canonical payloads, +publisher source binding, JTI consumption order, and UNKNOWN transitions retain +their contracts. A request whose identity expires in the queue now fails closed. +The worker semantic digest contains no new operational timestamp. No workflow, +protected manifest, baseline, scanner policy, golden, or v1 bundle is changed. + +## Validation + +Environment: Windows, CPython 3.14.3, Git 2.53.0.windows.3. Tests use synthetic +fixtures and D-drive temporary storage. Ruff 0.16.7 uses `--isolated` so unrelated +parent-directory configuration cannot affect this repository's validation. + +| Check | Result | +| --- | --- | +| Original signer suite before changes | 76 tests, OK. | +| New regression against original service | 4 methods run; FAILED (failures=5): three OIDC subcases, expired evaluation, and issuance timestamp. Inclusive-expiry control passed. | +| New regression after correction | 4 methods, OK. | +| All signer tests after correction | 80 tests, OK. | +| Full repository suite | 258 tests run: 253 passed, 5 platform skips. | +| Changed Python Ruff lint and format | Passed; two files already formatted. | +| Tags, README snapshot, pattern library | Passed. | +| Markdown/front matter | 157 files passed. | +| Pre-commit on the four changed files | Passed; out-of-scope hooks skipped. | +| Scanner integration | Synthetic PASS, FAIL, redaction, and import isolation passed. | +| Changed-file scanner without baseline | No findings. | +| Design-document markdownlint | 2 files, 0 errors. | +| Diff and manual privacy review | Passed; no credentials or machine identifiers added. | + +The full-suite skips are four unavailable symlink cases and the POSIX-only +bootstrap test. They are not signer test skips. The placeholder script excludes +these design documents and reported no applicable Markdown; it is not a privacy +certification for these files. Manual review and the changed-file scanner cover +the new artifacts instead. + +Initial tooling attempts found an unusable default Python launcher and a broken +virtualenv seed cache. Validation used an explicit CPython installation, a fresh +D-drive validation environment, and a separate virtualenv cache; no repository +test was weakened. An initial Ruff invocation inherited unrelated parent config; +the reported checks above use isolated configuration. Scanner false positives +on long public references and a test identifier were resolved with concise +references/naming, without baseline or suppression changes. + +Exact commands, run from the repository with the validation Python environment +on PATH (all test fixtures are local): + +```powershell +python -m unittest tests.test_repo_sentinel_signer_time -v +python -m unittest discover -s tests -p 'test_repo_sentinel_signer*.py' +python -m unittest discover -s tests -p 'test_*.py' +python -m ruff check --isolated --select E4,E7,E9,F,B signer/repo_sentinel_signer/service.py tests/test_repo_sentinel_signer_time.py +python -m ruff format --isolated --check signer/repo_sentinel_signer/service.py tests/test_repo_sentinel_signer_time.py +python scripts/render_tags_doc.py --check +python scripts/render_readme_snapshot.py --check +python scripts/check_pattern_library.py +python scripts/check_markdown.py +python scripts/test_repo_sentinel_integration.py +git diff --check +``` + +Pre-commit, the no-baseline changed-file scan, and markdownlint were passed the +exact four-file list below (markdownlint received only the two Markdown files). +No standalone build or static type-check configuration applies to this change. + +To reproduce the original failure independently, create a disposable worktree +at base `d35e5a6` on the D drive, copy only +`tests/test_repo_sentinel_signer_time.py` into it, and run the first command. +Alternatively, in a disposable copy of this increment, move the two clock +reads back above their transaction blocks. Do not alter a dirty user checkout. + +The tests observe externally meaningful decisions, timestamps, JTI replay, +slot creation, and publisher calls. They do not merely assert source placement. +Event synchronization makes the schedule deterministic; a five-second timeout +only detects a broken test setup and is not an expiry threshold. + +## Precise changed-file list + +- `signer/repo_sentinel_signer/service.py`: sample time after lock acquisition. +- `tests/test_repo_sentinel_signer_time.py`: contention regression and controls. + +Two documents under `docs/design-decisions/`: + +- `THREAT-BOUNDARY-AUDIT.md`: baseline, classifications, assumptions, severity, + and evidence boundaries. +- `SECURITY-INCREMENT.md`: correction and reproduction. + +## Rollback and release boundary + +Revert this local correction and its regression/documentation together if review +rejects it; doing so restores the demonstrated stale-time weakness. No remote +status, deployment, credential, schema, or policy migration is needed. + +This increment is local only. A later authorized security-boundary PR must be +draft and receive next-day review before merge. Production adapters and +publication-time expiry semantics require their own evidence and authorization. + +Recommendation: **ACCEPT** the bounded local correction. A local commit attempt +was blocked because the configured GPG signer had no available secret key. The +four-file change remains staged on its dedicated branch. Signing was not +disabled, no key material was collected, and nothing was pushed or published. diff --git a/docs/design-decisions/THREAT-BOUNDARY-AUDIT.md b/docs/design-decisions/THREAT-BOUNDARY-AUDIT.md new file mode 100644 index 0000000..64b9080 --- /dev/null +++ b/docs/design-decisions/THREAT-BOUNDARY-AUDIT.md @@ -0,0 +1,103 @@ +# Trusted Authority Boundary Audit + +Audit date: 2026-10-15. Scope: one local engineering increment, no deployment. + +## Baseline and scope + +The audited source is `stacknil/sec-writeups-public` main at +`d35e5a6`, fetched before creating the +dedicated local branch `authority-boundary-increment` under `stacknil/`. +The latest merge is PR #23, the mock signer core. PRs #19, #20, and #21 +already delivered commit authority, the controller, and external policy input. +Historical notes describing those changes as unmerged are superseded by this +read-only remote inspection. + +Two older main checkouts had unrelated document edits; neither was modified. +Existing authority branches and an unrelated dirty mutation checkout were +preserved. PR #22 remains a draft candidate-policy construction at `7c9e513`; +its candidate bundle is neither production policy nor this increment's base. +Issue #10 remains open and describes the larger control-plane boundary. Its +historical prerequisite list is not evidence that merged components are absent. + +Reviewed source includes the acquisition/reader contracts, worker/controller +design records, policy contracts, signer service/store/registry, and signer +tests. This is a bounded boundary audit, not an exhaustive security certification. +Research materials outside these repositories were excluded. + +## Threat assumptions and attacker capabilities + +- PR authors control proposed content, filenames, scripts, and workflow changes. + Those bytes must remain data to trusted scanning code. +- Only a trusted verifier may supply `VerifiedOidcClaims`. No real token, + private key, installation credential, or authenticated provider was needed. +- The selected weakness requires an otherwise authorized execution's request + to wait for the shared transaction lock across an expiry boundary. Queue + contention may be benign; a caller able to submit concurrent authorized + requests could influence it. Production reachability is unproven. +- The registry, store, process, injected clock, and adapters are trusted in this + reference model. A PR author is not assumed able to forge verified claims, + alter registry records, or control server time. + +## Boundary classification + +| Boundary | Classification and evidence | Decision | +| --- | --- | --- | +| PR content versus trusted execution | Existing local contracts cover isolated scanner imports, raw object identity, unsupported-entry refusal, inert target code, and exact pull-ref acquisition. | Preserve; no speculative rewrite. | +| Head H versus test-merge M | Historical canary evidence records M regeneration during merge with fixed B/H. The commit worker defines only `P_v(H)`. | Already rejected design; no new M-only experiment. | +| Protected name and App source | Historical canary evidence shows an ordinary Actions App name can be spoofed; dedicated expected-App binding rejects the other source's success. Same-name failure can affect availability. | Already characterized; do not change remote contexts or claim availability isolation. | +| Policy integrity and version binding | Explicit selector registry, external expected digest, frozen epoch identity, exact mirror/coverage controls, and v1 compatibility tests already exist. PR #22 is candidate evidence. | Preserve all bundle and policy bytes. | +| Replay and authority downgrade | Existing tests cover fresh finalization JTI, malformed-evidence consumption, frozen revision, revocation, same-H slot conflicts, publisher-bound receipts, and UNKNOWN reconciliation. | Preserve these invariants. | +| Expiry under transaction contention | Confirmed reproducible weakness: both service entrypoints sample time before waiting for the transaction lock. Stale time can authorize after token/evaluation expiry. | **Selected: transaction-time freshness.** | +| Missing or malformed input | Exact types/keys, independent evidence binding, fixed refusal vocabulary, and no publication for infrastructure refusal have existing tests. | Preserve fail-closed handling. | +| Real OIDC, durable storage, App publication | Explicitly absent production adapters. Hypothetical deployment risks are not confirmed deployed vulnerabilities. | Outside this increment; no credentials or activation. | + +## Selected finding and severity + +**Medium, reference implementation only:** stale authorization time across +transaction lock acquisition. It defeats an explicit lifetime boundary and can +reach the mock publisher after expiry. It does not establish token forgery, +arbitrary verdict injection, a head mismatch, or a deployed GitHub bypass. + +`SignerService.issue_evaluation()` and `finalize_evaluation()` originally read +`now` before entering `store.transaction()`. The same snapshot drives OIDC +validity and, respectively, ticket timestamps or evaluation expiry. A request +queued at time T can therefore use T after the lock becomes available at T2. + +The regression holds the real shared RLock, queues a background service call, +waits for the transaction-entry signal, advances an injected logical clock, +then releases the lock. No expiry sleeps, remote writes, scanner execution, +forged token bytes, or modified registry policy are involved. The original +source produced five failing assertions; the corrected source passes all four +test methods, including their admission/finalization subcases. + +The precise result and reproduction commands are in +[SECURITY-INCREMENT.md](SECURITY-INCREMENT.md). + +## Remaining uncertainty + +This correction establishes freshness at transaction entry. It does not provide +an atomic wall-clock deadline at remote publication: a future blocking reader, +publisher, durable transaction, or outbox needs a separately reviewed timing +contract. Clock trust and clock rollback are also outside this change. Frozen +inclusive expiry comparisons remain unchanged. + +No live canary was rerun and no present-day branch-rule configuration is +asserted. Existing canary evidence is historical, scoped evidence. The Windows +test suite does not certify the fixed Linux controller runtime or a production +OIDC verifier. Independent evidence here means a reproducible regression using +a real lock and synthetic inputs; no separate human review is claimed. + +## Sources + +- [Signer frozen contract](repo-sentinel-signer-v1.md). +- [Commit-intrinsic authority](repo-sentinel-commit-authority-v1.md). +- [External policy root](repo-sentinel-external-policy-root-v1.md). +- Canary repository: `stacknil/repo-sentinel-authority-canary`, inspected + commit `d34538c`. Source-binding evidence: `evidence/canary-results.md`. + Test-merge rejection: `evidence/test-merge-authority-gate-1.md`. +- [GitHub OIDC reference](https://docs.github.com/en/actions/reference/security/oidc) + documents token time, execution, repository, and workflow claims; it does not + prove that this mock core cryptographically verifies them. +- GitHub Docs, "About protected branches", section "Require status checks + before merging", describes expected-App status sources. Canary conclusions + above come from the recorded experiments, not a new settings inspection. diff --git a/signer/repo_sentinel_signer/service.py b/signer/repo_sentinel_signer/service.py index 4f25bda..c3975a9 100644 --- a/signer/repo_sentinel_signer/service.py +++ b/signer/repo_sentinel_signer/service.py @@ -281,8 +281,8 @@ def issue_evaluation( ) -> TicketResponse: if type(request) is not TicketRequest: raise SignerRefused("invalid_ticket_request") - now = self._now() with self._store.transaction(): + now = self._now() if type(verified_oidc) is not VerifiedOidcClaims: raise SignerRefused("unverified_oidc_claims") record = self._registry.resolve_active( @@ -489,8 +489,8 @@ def finalize_evaluation( evaluation_id: str, controller_result: object, ) -> FinalizationResult: - now = self._now() with self._store.transaction(): + now = self._now() evaluation = self._store.get(evaluation_id) record = self._registry.get(evaluation.registry_key) if self._registry.is_revoked(record.key): diff --git a/tests/test_repo_sentinel_signer_time.py b/tests/test_repo_sentinel_signer_time.py new file mode 100644 index 0000000..ece68cd --- /dev/null +++ b/tests/test_repo_sentinel_signer_time.py @@ -0,0 +1,133 @@ +"""Authorization time must be sampled after waiting for the signer transaction.""" + +from __future__ import annotations + +import threading +import unittest +from collections.abc import Callable, Iterator +from concurrent.futures import ThreadPoolExecutor +from contextlib import contextmanager +from functools import partial +from unittest.mock import patch + +from tests.signer_test_support import NOW, PULL_NUMBER, Harness, controller_result + +from repo_sentinel_signer import SignerRefused, TicketRequest # noqa: E402 + + +class TransactionTimeTests(unittest.TestCase): + def after_lock_wait( + self, harness: Harness, operation: Callable[[], object], resume_at: int + ) -> object: + """Use the real shared lock and a logical clock; never sleep for expiry.""" + attempted = threading.Event() + transaction = harness.store.transaction + + @contextmanager + def observed_transaction() -> Iterator[None]: + attempted.set() + with transaction(): + yield + + with ( + patch.object(harness.store, "transaction", observed_transaction), + ThreadPoolExecutor(max_workers=1) as executor, + ): + with harness.lock: + future = executor.submit(operation) + self.assertTrue(attempted.wait(timeout=5), "transaction not reached") + harness.clock.now = resume_at + return future.result(timeout=5) + + def test_oidc_expiring_while_queued_is_rejected(self) -> None: + for operation in ("admission", "admission-retry", "finalization"): + with self.subTest(operation=operation): + harness = Harness() + ticket = harness.issue() + claims = harness.final_claims( + "queued-identity", + exp=NOW + 1, + run_id=7002 if operation == "admission" else 7001, + ) + evidence = controller_result(ticket, harness.record) + + if operation != "finalization": + request = partial( + harness.service.issue_evaluation, + claims, + TicketRequest(PULL_NUMBER), + ) + else: + request = partial( + harness.service.finalize_evaluation, + claims, + ticket.evaluation_id, + evidence, + ) + + with self.assertRaisesRegex(SignerRefused, "^oidc_expired$"): + self.after_lock_wait(harness, request, NOW + 2) + + self.assertEqual(harness.store.evaluation_count(), 1) + self.assertEqual(harness.store.slot_count(), 0) + self.assertEqual(harness.publisher.calls, ()) + self.assertEqual( + harness.store.get(ticket.evaluation_id).finalization_state.value, + "ISSUED", + ) + + def test_queued_expiry_consumes_jti(self) -> None: + harness = Harness() + ticket = harness.issue() + claims = harness.final_claims("queued-evaluation", exp=ticket.expires_at + 100) + evidence = controller_result(ticket, harness.record) + + def finalize() -> object: + return harness.service.finalize_evaluation( + claims, ticket.evaluation_id, evidence + ) + + with self.assertRaisesRegex(SignerRefused, "^evaluation_expired$"): + self.after_lock_wait(harness, finalize, ticket.expires_at + 1) + with self.assertRaisesRegex(SignerRefused, "^finalization_jti_reused$"): + finalize() + self.assertEqual(harness.store.slot_count(), 0) + self.assertEqual(harness.publisher.calls, ()) + + def test_valid_admission_uses_time_after_wait_for_issued_at_and_ttl(self) -> None: + harness = Harness() + claims = harness.final_claims("queued-admission") + + ticket = self.after_lock_wait( + harness, + lambda: harness.service.issue_evaluation( + claims, TicketRequest(PULL_NUMBER) + ), + NOW + 10, + ) + + evaluation = harness.store.get(ticket.evaluation_id) + self.assertEqual(evaluation.issued_at, NOW + 10) + self.assertEqual(ticket.expires_at, NOW + 10 + 900) + self.assertEqual(harness.store.evaluation_count(), 1) + + def test_finalization_at_exact_expiry_retains_inclusive_contract(self) -> None: + harness = Harness() + ticket = harness.issue() + claims = harness.final_claims("queued-at-boundary", exp=ticket.expires_at) + evidence = controller_result(ticket, harness.record) + + result = self.after_lock_wait( + harness, + lambda: harness.service.finalize_evaluation( + claims, ticket.evaluation_id, evidence + ), + ticket.expires_at, + ) + + self.assertEqual(result.evaluation_state.value, "PUBLISHED") + self.assertEqual(len(harness.publisher.calls), 1) + + +if __name__ == "__main__": + unittest.main()