feat(governance): tamper-evident audit log for RBAC + policy changes (#395)#450
feat(governance): tamper-evident audit log for RBAC + policy changes (#395)#450Prateeks16 wants to merge 2 commits into
Conversation
sreerevanth#395) CMP-005 already ships roles, permissions, and SAML/RBAC enforcement (sreerevanth#387). The remaining acceptance criterion — "any changes to RBAC or policies must be logged to a tamper-evident audit table" — was unaddressed. Add a hash-chained AuditLog where each record's hash covers its fields plus the previous record's hash, so altering or reordering any past entry breaks the chain and AuditLog.verify() returns False. RBACEngine now records every add_user, set_role, and set_team_policy change (with optional actor) to this log; the mutating methods stay backward compatible. set_role is added so role changes are captured explicitly with before/after.
|
Warning Review limit reached
More reviews will be available in 42 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new ChangesCMP-005 Hash-Chained Audit Log and RBAC Integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant RBACEngine
participant AuditLog
participant _digest
rect rgba(70, 130, 180, 0.5)
note over Client,RBACEngine: RBAC Mutation
Client->>RBACEngine: add_user(user, actor="admin")
RBACEngine->>AuditLog: append("user.add", user_id, actor="admin", details={role})
AuditLog->>_digest: SHA-256(fields + prev_hash)
_digest-->>AuditLog: record_hash
AuditLog-->>RBACEngine: AuditRecord
RBACEngine-->>Client: None
end
rect rgba(60, 179, 113, 0.5)
note over Client,RBACEngine: Role Change
Client->>RBACEngine: set_role(user_id, new_role, actor="admin")
RBACEngine->>AuditLog: append("role.change", user_id, details={from, to})
AuditLog->>_digest: SHA-256(fields + prev_hash)
_digest-->>AuditLog: record_hash
AuditLog-->>RBACEngine: AuditRecord
RBACEngine-->>Client: True
end
rect rgba(210, 105, 30, 0.5)
note over Client,AuditLog: Chain Verification
Client->>AuditLog: verify()
loop each AuditRecord
AuditLog->>_digest: recompute expected_hash
_digest-->>AuditLog: expected_hash
AuditLog->>AuditLog: check prev_hash linkage and record_hash match
end
AuditLog-->>Client: True / False
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 PR Test Results
Python 3.12 · commit f07d1ff |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
agentwatch/governance/audit_log.py (1)
117-135: ⚡ Quick winAlso verify sequence continuity.
verify()currently does not enforceseqmonotonic continuity. Adding an index check strengthens integrity semantics and forensic reliability.Suggested verification check
- for record in self._records: + for expected_seq, record in enumerate(self._records): + if record.seq != expected_seq: + return False if record.prev_hash != prev_hash: return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/governance/audit_log.py` around lines 117 - 135, The verify() method in the audit log currently checks hash integrity but does not enforce sequence number continuity. Add a check to verify that each record's seq field is monotonically increasing by one from the previous record, starting from sequence 0 or 1 after the GENESIS_HASH. Initialize an expected_seq variable before the loop over self._records, and for each record, verify that record.seq equals the expected_seq value before proceeding with the existing hash checks. If the sequence is not continuous, return False. Increment expected_seq after each successful record verification to ensure the next record has the correct sequence number.tests/test_rbac_audit.py (1)
36-36: ⚡ Quick winAvoid making public mutability part of the test contract.
Line 36 tampering through
records()implicitly requires that public accessor to expose mutable internals. Since Line 45 already uses private-state tampering for simulation, prefer the same pattern here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_rbac_audit.py` at line 36, The test on line 36 is mutating log records through the public records() accessor method, which violates encapsulation by requiring public methods to expose mutable internals. To fix this, replace the public accessor mutation pattern on line 36 with the same private-state tampering approach already being used on line 45, directly accessing the private internal state instead of going through the public records() method. This maintains test isolation without exposing mutable internals through the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/governance/audit_log.py`:
- Around line 63-73: The to_dict() method exposes the live details dictionary
and the code returning AuditRecord objects at line 113 expose mutable internal
state that callers can modify, breaking the append-only contract. Fix this by
creating a deep copy of the details dictionary in the to_dict() method before
returning it, and by returning copies of AuditRecord objects instead of live
references where they are returned to callers. This ensures that external
mutations cannot corrupt the in-memory audit log.
- Around line 79-111: The AuditLog class has race conditions where concurrent
calls to append() and property accesses to head_hash can interleave, causing
sequence numbers and hash linkage to become inconsistent. Fix this by adding a
threading.RLock instance variable to the __init__ method, then wrap all access
to the _records attribute with the lock in the append() method, the head_hash
property, and any other methods that read or write _records (such as records()
and verify() if they exist) to ensure atomic operations and maintain chain
integrity.
---
Nitpick comments:
In `@agentwatch/governance/audit_log.py`:
- Around line 117-135: The verify() method in the audit log currently checks
hash integrity but does not enforce sequence number continuity. Add a check to
verify that each record's seq field is monotonically increasing by one from the
previous record, starting from sequence 0 or 1 after the GENESIS_HASH.
Initialize an expected_seq variable before the loop over self._records, and for
each record, verify that record.seq equals the expected_seq value before
proceeding with the existing hash checks. If the sequence is not continuous,
return False. Increment expected_seq after each successful record verification
to ensure the next record has the correct sequence number.
In `@tests/test_rbac_audit.py`:
- Line 36: The test on line 36 is mutating log records through the public
records() accessor method, which violates encapsulation by requiring public
methods to expose mutable internals. To fix this, replace the public accessor
mutation pattern on line 36 with the same private-state tampering approach
already being used on line 45, directly accessing the private internal state
instead of going through the public records() method. This maintains test
isolation without exposing mutable internals through the public API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 722530f0-cf8c-4e92-a09a-1c6fd1d45947
📒 Files selected for processing (3)
agentwatch/governance/audit_log.pyagentwatch/governance/rbac.pytests/test_rbac_audit.py
…y check Address review feedback on the tamper-evident audit log: - records() and append() now return detached copies, and to_dict() copies the details dict, so callers cannot mutate stored records and bypass the append-only / tamper-evident contract. - Guard all _records access with a threading.RLock so concurrent appends keep sequence numbers and hash linkage consistent. - verify() now also enforces monotonic seq continuity, not just hash linkage. - Test tampering goes through private state; added a test that mutating a returned record leaves the stored chain intact.
Addresses the outstanding acceptance criterion of #395 (CMP-005).
Context
The RBAC role model, permission map, deny-by-default checks, and SAML/RBAC API enforcement already shipped in #387 (Closes #377). The one #395 criterion left unmet was "any changes to RBAC or policies must be logged to a tamper-evident audit table" —
RBACEnginerecorded nothing, and the existinggovernance/engine.pyaudit log is append-only (not cryptographically tamper-evident).Change
governance/audit_log.py— a hash-chainedAuditLog. EachAuditRecord.record_hashisSHA-256over its own fields and the previous record's hash, seeded from a genesis hash. Altering or reordering any past record breaks every subsequent hash, soAuditLog.verify()detects tampering even in memory.RBACEnginenow owns anauditlog and records everyadd_user,set_role, andset_team_policychange (with an optionalactor). Addedset_roleso role changes are captured explicitly with before/after values. Mutating methods stay backward compatible (actoris keyword-only and optional).Acceptance criteria (#395)
Test
Scope kept to the audit-log gap to avoid duplicating the merged #387 work. Full-fidelity OIDC/SAML (replacing the HMAC token stand-in) would be a separate, larger PR.
Summary by CodeRabbit
New Features
Tests