Add Audit Logging for Security Events #304
Add Audit Logging for Security Events #304iqbalcodes6602 wants to merge 2 commits intocontainer-registry:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements a structured audit logging facility for security events across the ground-control and satellite applications. It introduces new audit logger packages that emit JSON-formatted events with unique IDs and timestamps, adds configuration for audit log file paths, and instruments authentication, user management, and satellite lifecycle handlers with audit logging calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ground-control/internal/server/middleware.go (1)
29-69:⚠️ Potential issue | 🟡 MinorCapture attempted username on failed Basic auth.
actoris only set after successful authentication, so failed Basic-auth attempts log an empty actor even when a username was supplied. Setactorimmediately after parsing Basic auth so the audit log captures the attempted username.🛠️ Suggested change
if !authenticated { if username, password, ok := extractBasicAuth(r); ok { + actor = username dbUser, err := s.dbQueries.GetUserByUsername(r.Context(), username) if err == nil { valid, err := auth.VerifyPassword(password, dbUser.PasswordHash) if err == nil && valid {
🤖 Fix all issues with AI agents
In `@ground-control/internal/logger/audit.go`:
- Around line 31-49: The getAuditLogger initialization currently returns early
on os.OpenFile failure which leaves auditLogger nil and auditLoggerOnce
preventing retries; update getAuditLogger so that when OpenFile(path, ...)
returns an error it logs that error (including the error value) and falls back
to using os.Stdout for w instead of returning, then continue to create and
assign auditLogger (so auditLogger is never left nil); reference the
getAuditLogger function, auditLoggerOnce, AUDIT_LOG_PATH env var and the
auditLogger variable when making this change.
- Around line 24-27: The package-level globals auditLogger and auditLoggerOnce
must be removed and replaced with an instance-based logger: create an
AuditLogger type (or add a field to the existing Server struct) that holds a
zerolog.Logger and any initialization state, provide a constructor like
NewAuditLogger(ctx, cfg...) to initialize and return the instance, update all
call sites that referenced the globals to use the instance (e.g.,
server.AuditLogger or pass *AuditLogger into functions), and eliminate sync.Once
usage by ensuring the constructor handles safe initialization; reference the
symbols auditLogger and auditLoggerOnce to locate and replace the globals with
the new instance-based pattern.
In `@internal/logger/audit.go`:
- Around line 23-26: Remove the package-level globals auditLogger and
auditLoggerOnce and refactor to an instance-based logger: create an AuditLogger
struct that holds a *zerolog.Logger, add a constructor NewAuditLogger(logger
*zerolog.Logger) (or accept a factory) and convert any package-level functions
that used auditLogger to methods on *AuditLogger so callers inject or create the
instance. Replace singleton lazy-init logic (auditLoggerOnce) by requiring the
caller to provide a configured *zerolog.Logger (or by making NewAuditLogger
initialize it once per instance), update all call sites to use the AuditLogger
receiver instead of the globals, and ensure thread-safety is preserved by
avoiding shared mutable package state.
In `@pkg/config/config.go`:
- Line 34: Add validation for the new AppConfig.AuditLogPath by updating
pkg/config/validate.go: inside the config validation function that validates
AppConfig (e.g., Validate or ValidateAppConfig), check that AuditLogPath is
non-empty when provided, that the path exists and is a directory or file as
expected, and that the process has required permissions (read/write as
appropriate); return a clear error if checks fail. Also update the example
configuration (config.example.json) to include the AuditLogPath key with a
descriptive sample value and brief comment so users can discover and configure
it.
🧹 Nitpick comments (1)
internal/logger/audit.go (1)
74-85: Return a WriteCloser (or document closing) to avoid file descriptor leaks.Since this function opens a file, it’s safer to return
io.WriteCloserso callers can close it on shutdown.♻️ Proposed signature tweak
-func NewFileAuditWriter(path string) (io.Writer, error) { +func NewFileAuditWriter(path string) (io.WriteCloser, error) { if path == "" { return nil, nil } f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) if err != nil { return nil, err } return f, nil }
| var ( | ||
| auditLogger *zerolog.Logger | ||
| auditLoggerOnce sync.Once | ||
| ) |
There was a problem hiding this comment.
Replace package-level audit logger globals to satisfy lint rules.
Package-level state violates the “no globals” lint rule and will fail strict golangci-lint. Please refactor to an instance-based logger (e.g., attach an AuditLogger to Server or pass it explicitly).
As per coding guidelines: no global variables (gochecknoglobals).
🤖 Prompt for AI Agents
In `@ground-control/internal/logger/audit.go` around lines 24 - 27, The
package-level globals auditLogger and auditLoggerOnce must be removed and
replaced with an instance-based logger: create an AuditLogger type (or add a
field to the existing Server struct) that holds a zerolog.Logger and any
initialization state, provide a constructor like NewAuditLogger(ctx, cfg...) to
initialize and return the instance, update all call sites that referenced the
globals to use the instance (e.g., server.AuditLogger or pass *AuditLogger into
functions), and eliminate sync.Once usage by ensuring the constructor handles
safe initialization; reference the symbols auditLogger and auditLoggerOnce to
locate and replace the globals with the new instance-based pattern.
| func getAuditLogger() *zerolog.Logger { | ||
| auditLoggerOnce.Do(func() { | ||
| path := os.Getenv("AUDIT_LOG_PATH") | ||
| var w *os.File | ||
| if path == "" { | ||
| w = os.Stdout | ||
| } else { | ||
| f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) | ||
| if err != nil { | ||
| return | ||
| } | ||
| w = f | ||
| } | ||
|
|
||
| l := zerolog.New(w).With().Timestamp().Logger() | ||
| auditLogger = &l | ||
| }) | ||
|
|
||
| return auditLogger |
There was a problem hiding this comment.
Silent init failure permanently disables audit logging.
If the file open fails, the logger stays nil and sync.Once prevents any retry or fallback. Consider logging the error and allowing a retry or a fallback to stdout to avoid silent loss of audit events.
🤖 Prompt for AI Agents
In `@ground-control/internal/logger/audit.go` around lines 31 - 49, The
getAuditLogger initialization currently returns early on os.OpenFile failure
which leaves auditLogger nil and auditLoggerOnce preventing retries; update
getAuditLogger so that when OpenFile(path, ...) returns an error it logs that
error (including the error value) and falls back to using os.Stdout for w
instead of returning, then continue to create and assign auditLogger (so
auditLogger is never left nil); reference the getAuditLogger function,
auditLoggerOnce, AUDIT_LOG_PATH env var and the auditLogger variable when making
this change.
| var ( | ||
| auditLogger *zerolog.Logger | ||
| auditLoggerOnce sync.Once | ||
| ) |
There was a problem hiding this comment.
Remove package-level audit logger globals (lint violation).
Strict golangci-lint disallows package-level globals. Please refactor to an injected/instance-based logger.
As per coding guidelines: no global variables (gochecknoglobals).
🤖 Prompt for AI Agents
In `@internal/logger/audit.go` around lines 23 - 26, Remove the package-level
globals auditLogger and auditLoggerOnce and refactor to an instance-based
logger: create an AuditLogger struct that holds a *zerolog.Logger, add a
constructor NewAuditLogger(logger *zerolog.Logger) (or accept a factory) and
convert any package-level functions that used auditLogger to methods on
*AuditLogger so callers inject or create the instance. Replace singleton
lazy-init logic (auditLoggerOnce) by requiring the caller to provide a
configured *zerolog.Logger (or by making NewAuditLogger initialize it once per
instance), update all call sites to use the AuditLogger receiver instead of the
globals, and ensure thread-safety is preserved by avoiding shared mutable
package state.
| DisableHeartbeat bool `json:"disable_heartbeat,omitempty"` | ||
| HeartbeatInterval string `json:"heartbeat_interval,omitempty"` | ||
| Metrics MetricsConfig `json:"metrics,omitempty"` | ||
| AuditLogPath string `json:"audit_log_path,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for AuditLogPath references across the codebase
echo "=== Searching for AuditLogPath references ==="
rg -n "AuditLogPath|audit_log_path" -S
echo -e "\n=== Checking config.example.json ==="
fd -a 'config.example.json' -x cat {}
echo -e "\n=== Checking pkg/config/validate.go ==="
fd -a 'validate.go' pkg -x cat {}Repository: container-registry/harbor-satellite
Length of output: 5460
Add AuditLogPath to validation and config example.
The AuditLogPath field added to AppConfig lacks validation in pkg/config/validate.go and is not documented in config.example.json. Add validation logic (path existence, permissions if needed) and include it in the example config so users can discover and configure this setting.
🤖 Prompt for AI Agents
In `@pkg/config/config.go` at line 34, Add validation for the new
AppConfig.AuditLogPath by updating pkg/config/validate.go: inside the config
validation function that validates AppConfig (e.g., Validate or
ValidateAppConfig), check that AuditLogPath is non-empty when provided, that the
path exists and is a directory or file as expected, and that the process has
required permissions (read/write as appropriate); return a clear error if checks
fail. Also update the example configuration (config.example.json) to include the
AuditLogPath key with a descriptive sample value and brief comment so users can
discover and configure it.
Signed-off-by: Anas Iqbal <mohd.abd.6602@gmail.com>
Signed-off-by: Anas Iqbal <mohd.abd.6602@gmail.com>
iqbalcodes6602
force-pushed
the
audit-logger
branch
from
3fdebdc to
5632549
Compare
1 participant
Description
This PR introduces audit logging support and improves server-side request handling across the Ground Control and shared components. The changes focus on better traceability, observability, and consistency in how authentication, user actions, and satellite operations are processed.
Key Changes
Summary by cubic
Adds structured audit logging for security events across Ground Control to improve traceability of auth flows, user actions, and satellite operations. Events include IDs, timestamps, actor, source IP, and details, and are written to stdout or a file.
New Features
Migration
Written for commit 5632549. Summary will update on new commits.
Summary by CodeRabbit
Release Notes