Skip to content

fix: reject future revocation snapshots under freshness policy - #269

Open
dinakarjs wants to merge 6 commits into
agentrust-io:mainfrom
dinakarjs:fix/future-revocation-freshness
Open

dinakarjs wants to merge 6 commits into
agentrust-io:mainfrom
dinakarjs:fix/future-revocation-freshness

Conversation

@dinakarjs

@dinakarjs dinakarjs commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Problem and behavior

A future-dated revocation snapshot could satisfy the maximum-staleness policy. The fix refuses future snapshots as REVOCATION_STATUS_UNKNOWN when freshness is enabled, while effective signed revocations still take precedence. With no freshness policy, the existing behavior remains.

The 2 November follow-up adds the missing live-clock regression with at_time omitted, a recent-snapshot valid twin, and specification/error-registry wording for the inclusive accepted interval [evaluation_time - max_revocation_staleness, evaluation_time], zero future tolerance, and live versus historical evaluation time. No runtime code changed in this follow-up.

Validation

Current head: 6130e78; tree: f94abc45580c8d1ad62ca218f6238b4741745b72.

  • Fresh hash-locked Python 3.12 development environment, with the exact checkout installed: 130 focused revocation/peer/offline/delegation tests pass, including the new live-clock test. Restricting the future guard to explicit at_time makes that new test fail by assertion; restored source passes.
  • Strict mypy passes for 47 source files; configured Bandit passes. Ruff passes with tests/ca2a_runtime/ca2a_verify explicitly declared first-party for this materialized checkout. Default auto-detection reports 26 existing test import-order findings, including the unchanged baseline imports; no import cleanup is included. CRLF-aware whitespace check passes.
  • Broader local unit run: 800 passed, 2 skipped, one hardware-fixture signature failure. The transferred quote.sig does not match its repository blob size/hash, so this is not evidence of a source regression or a successful full-suite run. No fixture changes are published. Current-head hosted validation remains required.

The original runtime change is security-sensitive and still requires two eligible independent maintainer approvals of the current head. Prior approvals apply to their stated revisions. No hardware run, merge, release or independent acceptance is claimed. Prepared with OpenAI Codex; DCO-signed.

Dependency follow-through — 2 November 2026

The first hosted run of this follow-up stopped at pip-audit because the branch still pinned pyasn1 0.6.4. The current head carries the exact reviewed #303 input/lock and SDK/dev-extra floors (pyasn1 >=0.6.5), with both changelog entries preserved. This is a dependency-only follow-up; this PR's runtime/test implementation is unchanged. Hash-required installation, pip check and local pip-audit (editable checkout explicitly skipped by the dependency scanner) pass; the 130 focused revocation/peer/offline/delegation tests pass in that updated environment. Hosted checks are running on the current head. No audit bypass or upstream merge occurred.

The subsequent format-only commit applies the repository formatter to the new assertion; all 46 revocation tests pass locally after formatting. Hosted validation applies to the current head above.

@dinakarjs
dinakarjs requested review from a team, carloshvp and zohebk8s as code owners October 23, 2026 07:25
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

@carloshvp @zohebk8s — this focused fix touches a delegation security path, so the repository policy requires two current-head maintainer approvals. Could you review the timestamp policy and preserved revocation precedence? The formal review-request API returned 403 for this integration, so I am requesting the reviews here. Exact head: ddd533c. Local full unit suite: 797 passed, 2 existing optional hardware skips; Ruff, mypy and Bandit passed.

Copy link
Copy Markdown
Contributor Author

Hosted CI is currently blocked before tests by the inherited certifi 2026.7.22 audit finding (PYSEC-2026-4320; job 120294976449). The dependency repair is already isolated in #265; I am keeping this timestamp fix separate rather than duplicating its lock changes. Local full-suite/check results in the description are passing, but do not replace a green hosted matrix after #265 lands. CodeQL passed; fuzz is still running.

carloshvp
carloshvp previously approved these changes Oct 23, 2026

@carloshvp carloshvp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ddd533ced1b5c103d0a86aea66cceb22fbeeec0f in an isolated checkout.

Approving the bounded freshness-policy fix. A future as_of now yields REVOCATION_STATUS_UNKNOWN when max_revocation_staleness is requested. Effective signed revocations are evaluated first and still yield CREDENTIAL_REVOKED; exact-time acceptance and historical verification without a freshness policy retain their existing contracts.

Independent macOS/Python 3.12 validation: the full unit selection passes 783 tests with 16 skips; pip check, Ruff, mypy and configured Bandit pass. Removing the new future-time guard in an isolated process produces exactly 5 failed and 40 passed revocation tests, including the offline verifier propagation case.

This does not authenticate supplier completeness or make as_of trusted provenance. This security path needs two current-head maintainer approvals, so my approval supplies one. The failed hosted security scan currently reports both inherited certifi and opentelemetry-api findings; #265 fixes certifi, while the OpenTelemetry lock repair remains separate. The cancelled matrix jobs still need a successful run. The commit carries DCO sign-off.

Reviewed with OpenAI Codex.

Copy link
Copy Markdown
Contributor Author

The separately requested OpenTelemetry audit follow-up is now #292. Its two universal locks retain unrelated pins/markers; the dev environment passes 843 unit/conformance tests plus Ruff, mypy, configured Bandit and pip check. The targeted audit finding is removed; #265/#280/#281 retain the other dependency repairs. This PR still needs its second current-head maintainer approval and successful refreshed hosted matrix after dependencies are integrated. No self-approval or merge performed.

zohebk8s
zohebk8s previously approved these changes Oct 26, 2026

@zohebk8s zohebk8s left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Verified on ddd533c.

The precedence is the part I checked hardest and it is right: an effective signed revocation still raises CredentialRevoked ahead of the freshness check, which test_future_snapshot_still_proves_effective_revocation pins down precisely. A revocation is true whatever the snapshot's date, and this keeps that. Gating the new check on max_staleness is not None leaves offline verification untouched, and the docstring update says what the function now does.

One question, which I would rather see answered than changed. The policy is asymmetric about time: with max_revocation_staleness=300, a snapshot 300 seconds old is accepted, while one a single second in the future is REVOCATION_STATUS_UNKNOWN. Your parametrized test fixes that as intended behaviour at future_offset=1.

Where the snapshot publisher and the verifier are different hosts, ordinary NTP skew will put snapshots a second or two ahead often enough to matter, and the result is a spurious unknown rather than a clean answer. Two ways to settle it: allow a small future tolerance, or say in the docstring that zero future tolerance is deliberate and that publishers must not post ahead of their own clocks. Either is fine. I mention it because this test will become the thing people read to learn the rule.

@dinakarjs
dinakarjs dismissed stale reviews from zohebk8s and carloshvp via fa4c936 October 27, 2026 10:34

Copy link
Copy Markdown
Contributor Author

@zohebk8s clarified the deliberate zero future tolerance in verify_chain's docstring at fa4c936. With the freshness policy enabled, the accepted as_of interval is [at_time - max_revocation_staleness, at_time]; even one second ahead produces unknown. The documentation now explicitly states that publisher/verifier clock skew can cause this and deployments must account for clock synchronization. Effective signed revocations retain precedence. This is a documentation clarification, not a new tolerance policy or runtime change. All 45 existing revocation tests pass with imports pinned to this checkout; changed-file Ruff and whitespace pass. Prepared with OpenAI Codex.

@rajnisht7

Copy link
Copy Markdown
Member

@dinakarjs Kindly resolve conflicts and rebase on main

Address the future-snapshot freshness case reported in agentrust-io#267.
Effective signed revocations retain precedence; no-policy offline replay is unchanged.

Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
Prepared with OpenAI Codex.

Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
@dinakarjs
dinakarjs force-pushed the fix/future-revocation-freshness branch from fa4c936 to 166798d Compare October 27, 2026 13:12

Copy link
Copy Markdown
Contributor Author

Resolved the requested rebase/conflict action at 166798d, based on main d170dc9. The only conflict was CHANGELOG.md; both entries are preserved under Fixed. The runtime/test patch and existing zero-future-tolerance clarification are unchanged. Both commits retain DCO sign-offs.

Fresh Python 3.12 validation with hash-locked dev dependencies: 853 unit/conformance tests passed, 2 skipped; Ruff, mypy (47 files), configured Bandit, pip check and whitespace checks passed. Published tree 55e351e53cbbdef3d4ff00c2d5231186dacd4666 matches the tested tree exactly. GitHub now reports mergeable=true (conflict-free); hosted checks are running, so this is not a claim that merge gates have passed.

Please refresh the two maintainer approvals against this current head. Prepared with OpenAI Codex.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

carloshvp
carloshvp previously approved these changes Oct 29, 2026

@carloshvp carloshvp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 166798d03edefe3df6de43adabcac92a3dac13df.

The future-snapshot check runs after effective revocations are considered, so a future publication cannot prove freshness while an effective signed revocation still refuses the credential. The explicit accepted interval and zero clock-skew tolerance are documented. Against the rebased current checkout, all 45 revocation tests, changed-file Ruff, mypy (47 files), DCO trailers and whitespace checks pass. The development environment used the hash-locked dependencies already validated for #300; no new hardware run is claimed. This changes the delegation security path, so a second eligible maintainer approval of this head remains required.

Prepared with OpenAI Codex.

rajnisht7
rajnisht7 previously approved these changes Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Closure follow-up at current head 364243f: Carlos and Rajnish have approved this head, but the base maintainer roster counts Imran/Carlos/Zoheb for this security path, and the latest gate reports one eligible approval of two. @zohebk8s could you re-review this head (or @imran-siddique supply the second eligible review)? The zero-future-tolerance policy is documented; effective signed revocations retain precedence. No code or roster change is proposed to bypass the gate. The formal reviewer-request API returned 403, so this is a conversation request, not a successful formal assignment. Prepared with OpenAI Codex.

Copy link
Copy Markdown
Contributor Author

@zohebk8s requesting your re-review of current head 364243f; @imran-siddique could alternatively supply the second eligible maintainer review. Zoheb's formal review request is already pending.

I rechecked the hosted jobs: the Python 3.11–3.13 Linux/Windows test matrix, governance, fuzzing, build-pr and CodeQL pass. The remaining gate explicitly reports: 'Awaiting 2 independent maintainer approval(s) of current head 364243f; found 1.' Carlos and Rajnish have submitted approvals, but the base policy counts Imran/Carlos/Zoheb for this security path.

The fix preserves effective signed-revocation precedence and documents deliberate zero future tolerance when freshness is enabled. Please review this head and provide your disposition, or identify any remaining changes. The existing local validation is recorded in the PR; this follow-up does not claim a fresh suite run. Prepared with OpenAI Codex.

@drk-ram drk-ram left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head 364243f. Merge-base f00060a; 1 commit behind main.

The runtime fix is right. I reported it (#267 item 4), so I went at it harder than usual. Windows, Python 3.11.9.

  • ruff check src tests: clean. ruff format --check: 107 files already formatted. mypy src: 47 files, no issues.
  • test_revocation.py, test_peer.py, test_verify.py and test_delegation.py together: 129 passed.
  • Full tests/: 707 passed, 17 skipped, 14 failed, 5 collection errors. At f00060a: 700 passed, 17 skipped, 14 failed, 5 collection errors. The 14 and 5 are the same on both and come from this box (optional packages not installed, no installed ca2a metadata), not from revocation. +7 passed is exactly the 7 new cases.

Mutations on _check_revocation, 7:

  • future check removed: 5 fail.
  • > to >=: 2 fail (the exact-time zero-staleness case and the peer-node test).
  • one second of tolerance (> now + 1): 3 fail.
  • check applied with no freshness policy set: 9 fail.
  • check moved ahead of the revocation lookup: 1 fail (test_future_snapshot_still_proves_effective_revocation).
  • staleness bound > to >=: 2 fail.
  • check made conditional on at_time is not None (revocations.as_of > at_time): survives. All 129 pass.

That survivor is the first change I am asking for. Every new test passes at_time=NOW. The live path, at_time omitted so now = int(time.time()), is the one an attacker uses, and nothing runs it. I ran it by hand on this head: as_of = time.time()+3600 and +1 with max_revocation_staleness=300 both raise REVOCATION_STATUS_UNKNOWN, and as_of 10 seconds in the past is accepted. The code is correct; the test is missing. A snapshot with as_of = int(time.time()) + 3600 and no at_time needs no clock mocking.

The second is the spec. docs/spec/delegation-chain.md still states the old rule: the checks table row at line 61 ("A snapshot no older than max_revocation_staleness was supplied") and the max_revocation_staleness paragraph at lines 150-153 ("one whose as_of is more than that many seconds before the evaluation time"). docs/spec/error-codes.md:24 defines REVOCATION_STATUS_UNKNOWN as a snapshot whose as_of "is older than that bound". None of them says a future-dated snapshot is unknown. This adds a case to a documented error code, so the spec text has to carry it, along with the zero-tolerance sentence that is now only in the docstring: the accepted interval is [at_time - max_revocation_staleness, at_time].

With both in I will approve.

Exercise verify_chain without at_time and retain a recent-snapshot valid twin. Document the inclusive accepted freshness interval, zero future tolerance and effective-revocation precedence in the specification and error registry. Runtime behavior is unchanged.

Prepared with OpenAI Codex.

Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com>
@dinakarjs
dinakarjs dismissed stale reviews from rajnisht7 and carloshvp via f28dcd4 November 2, 2026 10:20

dinakarjs commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor Author

@drk-ram Both requested follow-ups are published at current head 6130e78: a live-clock future-snapshot refusal with at_time omitted and a recent-snapshot valid twin; plus the checks table, freshness paragraph and error definition now state future-unknown, the inclusive accepted interval, zero tolerance and revocation precedence. All 130 focused tests pass; adding at_time-is-not-None to the future guard makes the new test fail. The head also carries the exact reviewed #303 pyasn1 dependency repair and repository formatting; local pip-audit is clean and the current Linux 3.11–3.13 CI jobs pass. Windows/fuzz follow-through and two eligible independent maintainer approvals remain separate. Please re-review this head. Formal reviewer assignment returned 403, so this conversation is the re-review request. The PR description records the local binary-fixture transfer limitation; no full local suite or new hardware success is claimed. Prepared with OpenAI Codex.

Carry the exact reviewed agentrust-io#303 dependency input, lock and SDK/dev-extra floors into this branch to clear the current hosted security audit failure. Preserve this PR's own runtime/tests and changelog entry. No security gate is bypassed.

Prepared with OpenAI Codex.

Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com>
Apply the repository formatter to the new valid-twin assertion. No behavior changes.

Prepared with OpenAI Codex.

Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com>

@drk-ram drk-ram left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up verification of exact head 6130e782595ca577c82198c794571b00d5ff2a80, including the three commits since my review of 364243f.

Both requested changes are present and effective.

test_live_clock_rejects_future_snapshot_without_at_time calls verification with at_time omitted, using an actual future snapshot (int(time.time()) + 3600) under a 300-second freshness policy. It requires REVOCATION_STATUS_UNKNOWN and pairs that refusal with acceptance of a snapshot ten seconds in the past. This exercises the live path rather than another explicit-time control.

I independently repeated the mutation that previously survived: make the future guard conditional on at_time is not None. With that in-memory mutation, the four-file suite produces 1 failed, 129 passed; the sole failure is the new live-clock test, which reports that RevocationStatusUnknown was not raised. The unmodified head produces 130 passed. No tracked source was modified for that check.

The checks table, freshness paragraph and error definition now agree on the inclusive accepted interval [evaluation_time - max_revocation_staleness, evaluation_time], current Unix time when at_time is omitted, zero tolerance for future timestamps, and effective signed revocation taking precedence over freshness. The existing exact-boundary, zero-staleness and revocation-precedence controls still pass.

Local validation, Windows/Python 3.11:

  • python -m pytest tests\unit\test_revocation.py tests\unit\test_peer.py tests\unit\test_verify.py tests\unit\test_delegation.py -q --tb=short -o addopts= --basetemp ..\.followup-review-20261102\pytest-ca2a: 130 passed. The requested filenames live under tests\unit on this head; the paths directly under tests do not exist.
  • python -m ruff check src tests: passed.
  • python -m ruff format --check src tests: 107 files already formatted.

One nonblocking hygiene note: git diff --check 364243f..HEAD still reports trailing whitespace at docs/spec/error-codes.md:24; the file has mixed line endings. I am not treating that as a freshness-policy defect or withholding approval over it. I have not claimed a full-suite or hardware-attestation rerun.

Approving this exact head: the formerly surviving mutation is now killed, and the normative freshness/error text matches the runtime.

@carloshvp carloshvp left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 6130e782595ca577c82198c794571b00d5ff2a80.

The current implementation refuses future snapshots when freshness is required, including live verification with no at_time, while effective signed revocations still take precedence. The documentation states the inclusive accepted interval and zero future tolerance. All 46 revocation tests, Ruff/format, mypy and configured Bandit pass. The added dependency repair matches #303. This approval covers the delegation-security change; the second eligible maintainer review and required hosted checks still apply.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:MEDIUM Contributor check flagged MEDIUM risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants