[codex] fix notary audit scopes#179
Conversation
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
jeremi
force-pushed
the
codex/notary-audit-scopes-178
branch
from
9f1190e to
fad6409
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f1190e676
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| event_id: Ulid::new().to_string(), | ||
| occurred_at, | ||
| principal_id_hash: None, | ||
| scopes_used: Vec::new(), |
There was a problem hiding this comment.
For successful /federation/v1/evaluations calls, the handler builds an EvidencePrincipal from peer.config.source_scopes and uses those scopes to evaluate and return claim results, but this audit constructor always emits an empty scopes_used list. That makes every successful federated evaluation look unscoped in the audit trail even though source scopes were exercised, undermining the required reconstruction of which permission authorized the claim result.
Useful? React with 👍 / 👎.
| principal_id_hash: principal.map(|principal| { | ||
| Hashed::<PrincipalIdentifier>::from_hash(hasher.hash(&principal.principal_id)) | ||
| }), | ||
| scopes_used: principal.map_or_else(Vec::new, |principal| principal.scopes.clone()), |
There was a problem hiding this comment.
When a credential or OIDC token carries extra scopes beyond the one this route actually checks, this copies every granted scope into scopes_used, including unrelated scopes on successes and denials. For example, claim access is checked against per-claim required scopes and OID4VCI issuance checks the credential-configuration scope, so auditing the whole principal scope set can overstate which permission authorized returned data; record the checked/required scopes for the request instead of the full token/API-key scope list.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #178.
scopes_usedtoEvidenceAuditEvent, defaulting legacy deserialization to an empty listEvidencePrincipal.scopesSecurity-sensitive review notes
This changes the Notary audit record schema and audit envelope contents for REQ-SEC-G-008. The new field records scope names only, not raw bearer tokens, API keys, principal IDs, civil identifiers, or claim values. Existing privacy assertions continue to verify that raw tokens, subject identifiers, and principal IDs are not emitted in the covered audit records. Legacy audit events without
scopes_usedremain deserializable via#[serde(default)].Validation
cargo fmtcargo fmt --checkcargo test --locked -p registry-notary-core audit_cargo test --locked -p registry-notary-server --test standalone_http oidc_mode_verifies_token_from_fixture_idp -- --exactcargo test --locked -p registry-notary-server --test standalone_http oidc_self_attestation_evaluates_renders_and_audits_access_mode -- --exactcargo check --locked -p registry-notary-server --all-targetsgit diff --checkSubagent review: Echo found no issues. Residual gap noted: no direct assertion that background/federation/public audit constructors serialize
scopes_used: []; the constructors are direct and covered byregistry-notary-server --all-targetscompile checking.