Repository navigation
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
|
@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. |
|
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
left a comment
There was a problem hiding this comment.
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.
|
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
left a comment
There was a problem hiding this comment.
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.
|
@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. |
|
@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>
fa4c936 to
166798d
Compare
|
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
carloshvp
left a comment
There was a problem hiding this comment.
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.
|
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. |
|
@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
left a comment
There was a problem hiding this comment.
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>
|
@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
left a comment
There was a problem hiding this comment.
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 undertests\uniton this head; the paths directly undertestsdo 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.
There was a problem hiding this comment.
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.
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.
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.