security: implement audit logging and rate limiting

[notsointresting]

📝 Description

This PR implements audit logging for security events and rate limiting for API/tool usage.

Audit Logging (pkg/audit/audit.go):

• Logs tool executions, auth events, config changes
• HMAC hash chain for tamper-evident logs
• Configurable retention policy
• Convenience functions for common events

Rate Limiting (pkg/ratelimit/limiter.go):

• Token bucket algorithm implementation
• Global and per-user rate limiting
• Separate limits for tool executions
• Non-blocking and blocking wait modes

🗣️ Type of Change

• 🐞 Bug fix (non-breaking change which fixes an issue)
• ✨ New feature (non-breaking change which adds functionality)
• 📖 Documentation update
• ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

• 🤖 Fully AI-generated (100% AI, 0% Human)
• 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
• 👨💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

Part of #782

📚 Technical Context (Skip for Docs)

• Reference URL: OWASP API Security Top 10
• Reasoning: Audit logging provides accountability and forensic capabilities. Rate limiting prevents abuse and resource exhaustion.

🧪 Test Environment

• Hardware: PC
• OS: Windows 11
• Model/Provider: N/A
• Channels: All channels

☑️ Checklist

• My code/docs follow the style of this project.
• I have performed a self-review of my own changes.
• I have updated the documentation accordingly.

[yinwm]

Code Review: Audit Logging and Rate Limiting

Thanks for this comprehensive PR implementing audit logging and rate limiting! These are valuable security features. However, I found several critical issues that need to be addressed before merging.

---

🔴 Critical Issues

1. Code Overlap with PR #813, #814 - Will Cause Merge Conflicts

Same configuration changes as the previous security PRs:

• SecurityConfig definition
• Default value changes
• DMScope change
• Removal of RequestTimeout fields

Recommendation: Depend on PR #813 being merged first, then rebase.

---

2. Secret Key Generation is Severely Insecure

func generateSecretKey() []byte {
    key := make([]byte, 32)
    // Use timestamp as a simple seed (in production, use crypto/rand)
    for i := range key {
        key[i] = byte(time.Now().UnixNano() % 256)
    }
    return key
}

This implementation has severe security problems:

• Uses timestamp as random source - completely predictable
• Loop executes too fast, all bytes are nearly identical (same nanosecond)
• Comment says "in production, use crypto/rand" but it's not implemented

An attacker can easily predict the key and forge audit logs.

Fix:

import "crypto/rand"

func generateSecretKey() []byte {
    key := make([]byte, 32)
    if _, err := rand.Read(key); err != nil {
        panic("failed to generate secret key: " + err.Error())
    }
    return key
}

---

3. HMAC Hash Does Not Include Complete Event Data - Can Be Tampered

func (l *Logger) computeHash(event Event) string {
    signData := fmt.Sprintf("%s|%s|%s|%s|%v",
        event.Timestamp.Format(time.RFC3339Nano),
        event.EventType,
        event.Action,
        event.Resource,
        event.Success,
    )

The hash only includes partial fields! Actor, Details, Source, SessionID, Error are NOT signed.

An attacker can modify these fields without detection, breaking audit log integrity.

Fix: Sign the complete event JSON:

func (l *Logger) computeHash(event Event) string {
    // Create a copy without hash fields for signing
    signEvent := event
    signEvent.Hash = ""
    signEvent.PreviousHash = ""
    
    data, _ := json.Marshal(signEvent)
    h := hmac.New(sha256.New, l.config.SecretKey)
    h.Write(data)
    return fmt.Sprintf("%x", h.Sum(nil))
}

---

4. CleanupOldLogs Breaks Hash Chain Integrity

func (l *Logger) CleanupOldLogs() error {
    // ...
    // Rewrite the file with kept lines
    newData := ""
    for _, line := range keptLines {
        newData += line + "\n"
    }
    return os.WriteFile(l.config.LogFilePath, []byte(newData), 0o600)
}

After cleanup, the first retained log's PreviousHash still points to a deleted log entry, breaking the hash chain.

Fix: Recalculate the hash chain after cleanup, or clearly document that the chain is reset.

---

Summary

Category	Count
🔴 Critical Issues	4

Verdict: Please address these critical issues before we can proceed:

1. Must Fix:

   • Secret key generation must use crypto/rand
   • HMAC hash must include complete event data
   • Resolve merge conflicts with other PRs

2. Recommended:

   • Fix hash chain integrity after cleanup
   • Consider rate limiter double-deduction issue (global bucket not refunded when user limit fails)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[yinwm]

⚠️ Compatibility Check Required

This PR needs to be checked for compatibility with #1239 (refactor logger to zerolog) which has been merged.

The logger package has been significantly refactored to use zerolog. Please verify your audit logging implementation is compatible.

📌 Related PR

Also please review #1383 (feat: add request logging and statistics features) as there may be overlapping functionality between:

• Your audit logging feature
• Their request logging/statistics feature

It would be good to coordinate to avoid duplicate efforts.

---

🤖 Detected by Claude Code