Add audit logging #121
Add audit logging #121
Conversation
|
Warning Rate limit exceeded@wksantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA comprehensive audit logging system is being introduced to keep-cli and keep-core, featuring encrypted log persistence with hash-chain verification, retention policies, JSON export, and five new CLI subcommands for managing and inspecting vault audit events. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI
participant Keep
participant AuditLog
participant File
User->>CLI: unlock vault
CLI->>Keep: unlock(password)
Keep->>AuditLog: open(path, data_key)
AuditLog->>File: read last_hash
Keep->>AuditLog: log(VaultUnlock event)
AuditLog->>AuditLog: compute_hash(entry + prev_hash)
AuditLog->>File: encrypt & append entry
Keep->>CLI: success
CLI->>User: vault unlocked
sequenceDiagram
participant User
participant CLI
participant Keep
participant AuditLog
participant File
User->>CLI: audit list/export/verify
CLI->>Keep: audit_read_all() / audit_verify_chain() / audit_export()
Keep->>AuditLog: read_all(data_key) / verify_chain(data_key) / export(data_key)
AuditLog->>File: read encrypted lines
AuditLog->>AuditLog: decrypt & deserialize entries
alt verify_chain path
AuditLog->>AuditLog: validate hash chain integrity
else export path
AuditLog->>AuditLog: serialize entries to JSON
end
AuditLog->>Keep: entries / bool / JSON string
Keep->>CLI: result
CLI->>User: formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🤖 Fix all issues with AI agents
In @keep-core/src/lib.rs:
- Around line 296-304: The audit event for FrostShareImport is only emitted when
hex::decode and try_from succeed, leaving missing audit logs if group_pubkey
decoding fails; change the logic in the block that currently calls
self.audit_event(AuditEventType::FrostShareImport, ...) to always emit the event
by deriving a fallback group value when decoding/conversion fails (e.g., use a
zeroed [u8;32] or hash of export.group_pubkey) and then call
self.audit_event(AuditEventType::FrostShareImport) with
e.with_group(&group_or_fallback).with_participants(vec![export.identifier]) so
the audit is recorded regardless of hex decoding success.
🧹 Nitpick comments (4)
keep-core/src/audit.rs (2)
141-171: Consider using a stable discriminant forAuditEventTypein hash computation.The cast
self.event_type as u8(line 144) relies on implicit enum discriminants. If variants are reordered or inserted in the middle ofAuditEventType, the discriminant values change, which would silently break hash verification for existing audit logs.Consider either:
- Adding
#[repr(u8)]with explicit discriminants toAuditEventType- Using the
Displaystring representation instead (already implemented)Option 1: Add explicit discriminants
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[repr(u8)] pub enum AuditEventType { - KeyGenerate, - KeyImport, + KeyGenerate = 0, + KeyImport = 1, // ... assign explicit values to all variantsOption 2: Use Display string in hash
fn compute_hash(&self) -> [u8; 32] { let mut data = Vec::new(); data.extend_from_slice(&self.timestamp.to_le_bytes()); - data.extend_from_slice(&(self.event_type as u8).to_le_bytes()); + data.extend_from_slice(self.event_type.to_string().as_bytes());
233-241: Consider flushing/syncing file for durability.The file is written but not explicitly flushed or synced before returning. On system crash, the audit entry could be lost even though
log()returnedOk.Add explicit sync for durability
let mut file = OpenOptions::new() .create(true) .append(true) .open(&self.path)?; file.write_all(line.as_bytes())?; + file.sync_all()?; self.last_hash = entry.hash; Ok(())keep-core/src/lib.rs (1)
432-444: Audit logging failures are silently ignored.Line 442 uses
let _ = audit.log(...)which discards any errors. While this prevents audit failures from breaking primary operations, it means audit events could be silently lost.Consider at minimum logging a warning when audit fails, so operators can detect audit system issues.
Log warning on audit failure
fn audit_event<F>(&mut self, event_type: AuditEventType, builder: F) where F: FnOnce(AuditEntry) -> AuditEntry, { let data_key = match self.get_data_key() { Ok(k) => k, Err(_) => return, }; if let Some(ref mut audit) = self.audit { let entry = builder(AuditEntry::new(event_type, audit.last_hash())); - let _ = audit.log(entry, &data_key); + if let Err(e) = audit.log(entry, &data_key) { + tracing::warn!("Failed to write audit event: {}", e); + } } }keep-cli/src/commands/audit.rs (1)
182-199: Some event types are not included in statistics.The match statement doesn't count
FrostShareImport,FrostShareExport,FrostShareDelete, andVaultLockevents. These are silently ignored by the_ => {}catch-all.Consider adding counters for share operations and lock events, or at minimum displaying a count of "Other" events so users know the totals add up.
Add counters for missing event types
let mut auth_fail = 0; let mut unlock = 0; + let mut lock = 0; + let mut share_ops = 0; for entry in &entries { match entry.event_type { // ... existing matches ... AuditEventType::VaultUnlock => unlock += 1, + AuditEventType::VaultLock => lock += 1, + AuditEventType::FrostShareImport + | AuditEventType::FrostShareExport + | AuditEventType::FrostShareDelete => share_ops += 1, _ => {} } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
keep-cli/src/commands/audit.rskeep-cli/src/commands/mod.rskeep-cli/src/main.rskeep-core/Cargo.tomlkeep-core/src/audit.rskeep-core/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (4)
keep-cli/src/commands/audit.rs (2)
keep-core/src/audit.rs (2)
open(203-216)new(79-94)keep-core/src/lib.rs (2)
open(50-58)unlock(60-68)
keep-cli/src/main.rs (1)
keep-cli/src/commands/audit.rs (5)
cmd_audit_list(30-84)cmd_audit_export(86-106)cmd_audit_verify(108-123)cmd_audit_retention(125-156)cmd_audit_stats(158-235)
keep-core/src/lib.rs (1)
keep-core/src/audit.rs (3)
new(79-94)open(203-216)export(304-308)
keep-core/src/audit.rs (2)
keep-core/src/crypto.rs (2)
blake2b_256(355-363)from_slice(242-249)keep-core/src/lib.rs (2)
open(50-58)data_key(428-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
keep-core/Cargo.toml (1)
28-28: LGTM!The
base64 = "0.22"dependency is appropriately added to support the base64 encoding/decoding of encrypted audit log entries in the new audit module.keep-core/src/audit.rs (3)
13-60: LGTM!The
AuditEventTypeenum provides comprehensive coverage of auditable events across key operations, signing, FROST threshold operations, authentication, and vault lifecycle. TheDisplayimplementation produces consistent snake_case identifiers suitable for logging and export.
181-194: LGTM!Sensible default retention policy with 10,000 entries and 365 days. The optional fields provide flexibility for users who want only count-based or time-based retention.
370-455: LGTM - Tests cover core functionality.The tests validate hash chain integrity, persistence, chain verification, and entry-count retention. Consider adding tests for:
max_age_daysretention filtering- Detection of a broken/tampered chain (verify_chain returning false)
keep-core/src/lib.rs (2)
60-75: LGTM!The audit log lifecycle is properly managed: initialized on unlock with a
VaultUnlockevent, and cleared on lock after emittingVaultLock. The event emission order ensures the lock event is recorded before the audit log is closed.
446-474: LGTM!The public audit API methods correctly check for locked state and delegate to the audit log. The
audit_set_retentionsilently no-ops when locked, which is reasonable since there's no audit log to configure.keep-cli/src/commands/audit.rs (5)
15-28: LGTM!The helper functions are clean.
format_timestampcorrectly handles edge cases viatimestamp_opt().single()and provides a sensible fallback.
30-84: LGTM!The list command correctly displays the most recent entries (when limited) in chronological order. The truncation of hex identifiers to 8 characters provides a good balance between readability and identification.
86-106: LGTM!Simple and effective export implementation with stdout fallback when no output path is specified.
108-123: LGTM!The verify command correctly exits with a non-zero status code when chain verification fails, making it suitable for use in scripts and CI pipelines.
125-156: LGTM!The retention command properly separates policy configuration from application, requiring explicit
--applyflag to make changes. Good UX for a destructive operation.keep-cli/src/commands/mod.rs (1)
1-9: LGTM!The
auditmodule is properly exported and positioned alphabetically among the existing command modules.keep-cli/src/main.rs (3)
69-72: LGTM!The
Auditsubcommand is cleanly integrated following the existing pattern used forFrost,Bitcoin, andEnclavecommands.
99-119: LGTM!The
AuditCommandsenum is well-structured with clear help text for CLI arguments. The subcommands cover the essential audit operations: listing, export, verification, retention management, and statistics.
487-506: LGTM!The
dispatch_auditfunction correctly routes all audit subcommands to their respective handlers, following the established dispatch pattern used throughout the CLI.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.