[WIP] feat: add audit logging for security events#448
Conversation
Implements structured JSON audit logging across satellite and
ground-control for authentication, satellite lifecycle, user management,
and configuration changes. Events are written one-per-line with
size/age/backup rotation via lumberjack, suitable for SIEM ingestion
(Filebeat/ELK, Wazuh, Splunk).
Event schema (one per line):
{"event_id":"<uuid>","timestamp":"<RFC3339Z>","event_type":"<dotted>",
"actor":"<who>","source_ip":"<ip>","details":{...}}
Event types implemented:
- user.login.success / user.login.failure
- user.created / user.deleted / user.password_changed
- satellite.registered / satellite.deregistered / satellite.auth.failure
- config.changed (GC API + satellite hot-reload)
Reserved (constants defined, not yet emitted; see roadmap section of
docs/guides/audit-logging.md):
- satellite.revoked / satellite.unrevoked - pending revoke handlers in GC
- policy.pull_blocked - pending registry-level policy hooks
Configuration:
- Satellite: app_config.audit.{enabled, file_path, max_size_mb,
max_backups, max_age_days}
- Ground Control: AUDIT_LOG_ENABLED / AUDIT_LOG_PATH / AUDIT_LOG_MAX_*
env vars
- Defaults: disabled, ./audit.log, 100 MB, 7 backups, 30 days
Verified end-to-end:
- Unit tests green in both modules (go test ./...)
- E2E run against real Harbor v2.11.1 + Postgres + GC + satellite
- 11 events captured live across 8 distinct event types
Fixes #238
Signed-off-by: aloui ikram <ikram@container-registry.com>
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a structured, file-backed audit logger (with rotation and runtime reconfigure) and instruments Satellite and Ground Control to emit typed audit events for auth, user, satellite registration/deregistration, and config changes; includes config types, validation, tests, docs, and example env/config updates. ChangesAudit Logging Implementation
Sequence DiagramssequenceDiagram
participant Client
participant AuthHandler
participant Server
participant AuditLogger
participant File
Client->>AuthHandler: POST /login (username,password)
AuthHandler->>AuthHandler: validate credentials
alt success
AuthHandler->>Server: s.auditEvent(EventUserLoginSuccess,...)
Server->>AuditLogger: Log(EventUserLoginSuccess, actor, source_ip, details)
AuditLogger->>File: write JSON line (event_id,timestamp,...)
AuthHandler->>Client: 200
else failure
AuthHandler->>Server: s.auditEvent(EventUserLoginFailure,{reason})
Server->>AuditLogger: Log(EventUserLoginFailure, actor, source_ip, details)
AuditLogger->>File: write JSON line
AuthHandler->>Client: 401
end
sequenceDiagram
participant SatelliteHandler
participant RegistrationDB
participant Server
participant AuditLogger
participant File
SatelliteHandler->>SatelliteHandler: validate token/ZTR/SPIFFE
alt validation fails
SatelliteHandler->>Server: auditEvent(EventSatelliteAuthFailure,{reason,masked})
Server->>AuditLogger: Log(EventSatelliteAuthFailure, actor, source_ip, details)
AuditLogger->>File: write JSON line
else registration succeeds
SatelliteHandler->>RegistrationDB: persist registration
RegistrationDB-->>SatelliteHandler: commit
SatelliteHandler->>Server: auditEvent(EventSatelliteRegistered,{flow,config})
Server->>AuditLogger: Log(EventSatelliteRegistered, actor, source_ip, details)
AuditLogger->>File: write JSON line
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 142 |
| Duplication | 6 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Aloui-Ikram
added
enhancement
![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
🧹 Nitpick comments (2)
ground-control/internal/logger/audit.go (1)
73-81: ⚡ Quick winAvoid writing empty optional fields.
Line 77 and Line 78 always emit
actorandsource_ip. For optional schema fields, omit them when empty to keep event payloads semantically clean.Proposed patch
evt := a.log.Log(). Str("event_id", uuid.NewString()). Time("timestamp", time.Now().UTC()). - Str("event_type", string(eventType)). - Str("actor", actor). - Str("source_ip", sourceIP) + Str("event_type", string(eventType)) + if actor != "" { + evt = evt.Str("actor", actor) + } + if sourceIP != "" { + evt = evt.Str("source_ip", sourceIP) + } if len(details) > 0 { evt = evt.Interface("details", details) }🤖 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 `@ground-control/internal/logger/audit.go` around lines 73 - 81, The current audit event builder always emits the optional fields "actor" and "source_ip"; update the event construction around the evt variable returned from a.log.Log() so that you only call Str("actor", actor) and Str("source_ip", sourceIP) when those values are non-empty (similar to the existing conditional for details), e.g., check actor != "" and sourceIP != "" before adding each field to evt; keep the uuid, timestamp and event_type fields as-is.internal/logger/audit.go (1)
73-81: ⚡ Quick winOnly emit optional fields when present.
Line 77 and Line 78 currently always write
actorandsource_ip. Since these are optional fields, prefer omitting them when empty instead of logging empty strings.Proposed patch
evt := a.log.Log(). Str("event_id", uuid.NewString()). Time("timestamp", time.Now().UTC()). - Str("event_type", string(eventType)). - Str("actor", actor). - Str("source_ip", sourceIP) + Str("event_type", string(eventType)) + if actor != "" { + evt = evt.Str("actor", actor) + } + if sourceIP != "" { + evt = evt.Str("source_ip", sourceIP) + } if len(details) > 0 { evt = evt.Interface("details", details) }🤖 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 `@internal/logger/audit.go` around lines 73 - 81, The code always emits actor and source_ip as empty strings; update the audit logger construction in internal/logger/audit.go so that after creating evt := a.log.Log().Str("event_id", uuid.NewString()).Time("timestamp", time.Now().UTC()).Str("event_type", string(eventType)), you only call evt = evt.Str("actor", actor) when actor != "" and only call evt = evt.Str("source_ip", sourceIP) when sourceIP != "", keeping the existing conditional for details untouched; reference the evt variable and the a.log.Log() call to locate where to add these conditional Str(...) calls.
🤖 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 `@ground-control/.env.example`:
- Around line 33-36: Reorder the audit-related environment variable keys in the
.env.example so they are alphabetically ordered to satisfy dotenv-linter
UnorderedKey warnings: arrange AUDIT_LOG_MAX_AGE_DAYS, AUDIT_LOG_MAX_BACKUPS,
AUDIT_LOG_MAX_SIZE_MB, AUDIT_LOG_PATH (or whichever alphabetical order your
linter expects) replacing the current block containing AUDIT_LOG_PATH,
AUDIT_LOG_MAX_SIZE_MB, AUDIT_LOG_MAX_BACKUPS, AUDIT_LOG_MAX_AGE_DAYS to remove
the lint noise.
In `@ground-control/internal/server/middleware.go`:
- Around line 114-127: The clientIP function currently trusts X-Forwarded-For
unconditionally; change it so it only uses the first X-Forwarded-For entry when
the immediate peer (from net.SplitHostPort(r.RemoteAddr)) is a trusted proxy
(implement or call an isTrustedProxy(host) helper that checks against your
configured proxy CIDRs), and validate the candidate IP with net.ParseIP before
returning it; otherwise ignore X-Forwarded-For and return the parsed RemoteAddr
host (or RemoteAddr raw if SplitHostPort fails). Ensure to reference and update
clientIP, add or use isTrustedProxy, and use net.ParseIP to validate extracted
IPs.
In `@ground-control/internal/server/server.go`:
- Around line 302-318: loadAuditConfig currently calls parseIntEnv which
silently falls back to defaults, so when AUDIT_LOG_ENABLED is true you must
validate numeric env vars and fail startup on invalid values. Update either
parseIntEnv to return (int, error) or add a new helper (e.g., parseIntEnvOrFail)
and in loadAuditConfig call that for AUDIT_LOG_MAX_SIZE_MB,
AUDIT_LOG_MAX_BACKUPS and AUDIT_LOG_MAX_AGE_DAYS; if parsing fails, log a clear
error and exit (e.g., log.Fatalf or os.Exit(1)) instead of using defaults, then
populate and return auditlog.AuditConfig with the validated ints.
In `@internal/state/registration_process.go`:
- Around line 71-74: The audit log call is writing raw err.Error() (and gcURL)
which may contain the satellite token and leak secrets; replace the direct use
of err.Error() (and/or raw gcURL) in the audit.Log invocation with a sanitized
value. Locate the audit.Log call (using identifiers audit.Log and
logger.EventSatelliteAuthFailure in registration_process.go) and change the
"reason" to a redacted/sanitized string (e.g., a short generic message plus safe
error type or a call to a sanitize/redact helper that strips URL token
segments), and ensure any logged URLs (gcURL) are likewise sanitized before
being passed to audit.Log. Ensure you do not write the full original err.Error()
or unredacted gcURL to the audit map.
---
Nitpick comments:
In `@ground-control/internal/logger/audit.go`:
- Around line 73-81: The current audit event builder always emits the optional
fields "actor" and "source_ip"; update the event construction around the evt
variable returned from a.log.Log() so that you only call Str("actor", actor) and
Str("source_ip", sourceIP) when those values are non-empty (similar to the
existing conditional for details), e.g., check actor != "" and sourceIP != ""
before adding each field to evt; keep the uuid, timestamp and event_type fields
as-is.
In `@internal/logger/audit.go`:
- Around line 73-81: The code always emits actor and source_ip as empty strings;
update the audit logger construction in internal/logger/audit.go so that after
creating evt := a.log.Log().Str("event_id", uuid.NewString()).Time("timestamp",
time.Now().UTC()).Str("event_type", string(eventType)), you only call evt =
evt.Str("actor", actor) when actor != "" and only call evt =
evt.Str("source_ip", sourceIP) when sourceIP != "", keeping the existing
conditional for details untouched; reference the evt variable and the
a.log.Log() call to locate where to add these conditional Str(...) calls.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 019c88ad-23c5-40e6-ba9e-b903402ace63
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumground-control/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
cmd/main.goconfig.example.jsondocs/guides/audit-logging.mdgo.modground-control/.env.exampleground-control/go.modground-control/internal/logger/audit.goground-control/internal/logger/audit_test.goground-control/internal/server/auth_handlers.goground-control/internal/server/config_handlers.goground-control/internal/server/middleware.goground-control/internal/server/satellite_handlers.goground-control/internal/server/server.goground-control/internal/server/user_handlers.gointernal/logger/audit.gointernal/logger/audit_test.gointernal/state/registration_process.gopkg/config/config.gopkg/config/constants.gopkg/config/getters.gopkg/config/validate.gopkg/config/validate_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ground-control/internal/server/middleware.go">
<violation number="1" location="ground-control/internal/server/middleware.go:117">
P1: `clientIP` unconditionally trusts `X-Forwarded-For`, allowing any client to spoof `source_ip` in audit records. Only honor forwarded headers when `RemoteAddr` belongs to a trusted proxy; otherwise fall back to `RemoteAddr` directly. Without this, the audit trail's source-IP field is unreliable for incident investigation.</violation>
<violation number="2" location="ground-control/internal/server/middleware.go:123">
P2: Add `X-Real-IP` fallback before `RemoteAddr` so audit logs capture client IP correctly when `X-Forwarded-For` is absent.</violation>
</file>
<file name="ground-control/internal/server/server.go">
<violation number="1" location="ground-control/internal/server/server.go:314">
P2: Audit rotation env values are not validated; negative values are accepted and propagated to the logger config.</violation>
</file>
<file name="internal/state/registration_process.go">
<violation number="1" location="internal/state/registration_process.go:72">
P1: Raw `err.Error()` is written to the audit log as `reason`. If the HTTP call to the ZTR endpoint fails, the error may include the full request URL which contains the satellite token as a path segment. Redact or sanitize the error string before emitting it — the GC side already masks tokens in its auth-failure events.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
- Sanitize errors before emitting to the audit log so the satellite token cannot leak via wrapped HTTP error messages from the ZTR call (internal/state/registration_process.go). - Validate AUDIT_LOG_MAX_* env values on Ground Control; fail startup on negative or empty values instead of silently passing them through to lumberjack (ground-control/internal/server/server.go). - Gate X-Forwarded-For / X-Real-IP behind a new opt-in env var AUDIT_TRUST_FORWARDED_HEADERS so audit source_ip cannot be spoofed by clients in default deployments. When the flag is set, the first X-Forwarded-For entry is preferred, then X-Real-IP, then RemoteAddr (ground-control/internal/server/middleware.go). - Alphabetize audit keys in ground-control/.env.example. - Document the new env var and validation behaviour in the guide. Addresses bot review comments on PR #448. Signed-off-by: aloui ikram <ikram@container-registry.com>
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ground-control/internal/server/server.go">
<violation number="1" location="ground-control/internal/server/server.go:324">
P2: Invalid numeric audit env values are still silently accepted as defaults because `parseIntEnv` ignores parse errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
If an operator sets one of the audit rotation env vars to a non-integer (e.g. AUDIT_LOG_MAX_SIZE_MB=abc), GC used to silently fall back to the default. It now exits with a clear error instead. Addresses cubic re-review on PR #448. Signed-off-by: aloui ikram <ikram@container-registry.com>
|
Thanks for the reviews. Summary of what changed since Commit
Commit
Not addressed (deliberately): the nit about omitting empty @coderabbitai review |
@Aloui-Ikram I have started the AI code review. It will take a few minutes to complete. |
|
The reasoning on keeping Triggering a fresh review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
1 issue found across 25 files
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
getEnvOrDefault returns the default whenever the env var is unset OR set to empty, so the path == "" check that followed it could never fire. An operator who wrote AUDIT_LOG_PATH= in their .env (typo, blank value) silently got ./audit.log instead of the intended fatal error. Switch to os.LookupEnv so we can tell the two cases apart: unset falls back to ./audit.log, set-but-empty is a fatal misconfiguration. Addresses cubic re-review on PR #448. Signed-off-by: aloui ikram <ikram@container-registry.com>
Aloui-Ikram
changed the title
|
@bupd @Vad1mo ,thank you for the feedback at the standup last week. Based on your input and the Harbor audit-logging gist, I reworked the AuditEvent schema into 17 fields. Before I start writing code, I would like to get your opinion on the shape. You both know Harbor's conventions better than I do, so your feedback would help a lot here. If the design works for you, the plan is:
The 17 fields and where each one comes from
Mapping each field to OTel / syslog / CloudEventsUsing registered semantic conventions where they exist, custom attributes where they do not. No internal field renames will be needed when transports are added later.
Quick note: the CloudEvents Severity and Auth Context extensions are listed in the spec repo but are not part of the v1.0.2 core. i can use them, or put those fields in Could you let me know if this shape works, or if there are fields you would add, drop, or rename? Thank you for your time. |
|
it is not clear if all fields are meant to be mandatory? |
|
oh i should have made that clearer , sorry. Out of the 17, only 8 are always there: The other 9 are optional and omitted when we don't have a value for them. The plan is to omit optional fields when unknown rather than emit empty strings, because OTel, CloudEvents and syslog all do it that way , keeps the internal shape aligned with the transports later on. |
|
Review findings:
Verification run locally from the PR head worktree: go test ./... -count=1
go test ./... -count=1 # from ground-control/ |
Audit logger (satellite + ground-control): - NewAuditLogger returns (*AuditLogger, error) and checks the destination is writable up front, so an unwritable path fails fast at startup instead of looking enabled while silently dropping every event. - Add in-place Reconfigure so the satellite rebuilds the logger on hot reload when audit settings change, instead of ignoring them until restart. Satellite rotation config: - MaxSizeMB/MaxBackups/MaxAgeDays become *int so an omitted field differs from an explicit 0. Omitted -> default (100/7/30), matching GC; explicit 0 -> retain all; negative -> warn + default. Adds helpers to read these safely (defaulting when unset) and to compare two audit configs by value, so hot reload only rebuilds the logger on a real change. Docs and deps: - Fix the source_ip row in the audit-logging guide and document omit=default / 0=unlimited rotation. - go mod tidy both modules. Refs #238 Signed-off-by: aloui ikram <ikram@container-registry.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ground-control/internal/logger/audit.go (1)
137-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame nanosecond-precision gap as
internal/logger/audit.go.
zerolog's defaultTimeFieldFormatistime.RFC3339, so the emittedtimestampdrops nanoseconds while the schema documents ns precision. Apply the same explicit-format fix here.🛠️ Proposed fix
evt := a.log.Log(). Str("event_id", uuid.NewString()). - Time("timestamp", time.Now().UTC()). + Str("timestamp", time.Now().UTC().Format(time.RFC3339Nano)). Str("event_type", string(eventType)).🤖 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 `@ground-control/internal/logger/audit.go` around lines 137 - 142, The timestamp field currently uses Time("timestamp", time.Now().UTC()) which loses nanoseconds under zerolog's default format; replace that Time call on the evt builder returned by a.log.Log() with an explicit string timestamp formatted with time.RFC3339Nano (e.g., format time.Now().UTC() to RFC3339Nano and pass it via Str("timestamp", ...)) so the emitted "timestamp" preserves nanosecond precision consistent with the schema.internal/logger/audit.go (1)
137-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix audit
timestampto preserve nanosecond precision
internal/logger/audit.goemitstimestampviazerolog’s.Time("timestamp", time.Now().UTC()), which useszerolog’s defaultTimeFieldFormat(time.RFC3339, second precision) unless the global format is overridden. The repo has noTimeFieldFormat = ...override, so the output conflicts withdocs/guides/audit-logging.md’s promise of “RFC 3339 with nanoseconds”.internal/logger/audit_test.godoesn’t fail becausetime.Parse(time.RFC3339Nano, ...)accepts second-precision RFC3339 timestamps too.🛠️ Proposed fix
evt := a.log.Log(). Str("event_id", uuid.NewString()). - Time("timestamp", time.Now().UTC()). + Str("timestamp", time.Now().UTC().Format(time.RFC3339Nano)). Str("event_type", string(eventType)). Str("actor", actor). Str("source_ip", sourceIP)This avoids relying on any global
zerolog.TimeFieldFormatconfiguration.🤖 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 `@internal/logger/audit.go` around lines 137 - 142, Replace the zerolog .Time call so timestamps are emitted with RFC3339 nanosecond precision: instead of using a.log.Log().Time("timestamp", time.Now().UTC()) in the evt builder, format the time explicitly with time.RFC3339Nano (e.g., use time.Now().UTC().Format(time.RFC3339Nano)) and set it via .Str("timestamp", ...); update any imports if necessary and keep the rest of the evt chain (Str("event_id", ...), Str("event_type", ...), etc.) unchanged.
🧹 Nitpick comments (1)
ground-control/internal/logger/audit.go (1)
1-176: 🏗️ Heavy liftDeduplicate the audit logger to avoid drift between root and ground-control
ground-control/internal/logger/audit.gois not a byte-for-byte copy ofinternal/logger/audit.go(it already differs by addingtype contextKey stringnearauditLoggerKey), so future fixes risk landing in only one place.- Since
ground-controldepends on the root module and its import paths are undergithub.com/container-registry/harbor-satellite, consider importing the rootinternal/loggerimplementation fromground-controlinstead of maintaining a second copy (or keep duplication intentional, but treat sync as required).🤖 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 `@ground-control/internal/logger/audit.go` around lines 1 - 176, This file duplicates the root internal/logger implementation and should be removed in favor of importing the canonical package; delete ground-control/internal/logger/audit.go and update callers in ground-control to import github.com/container-registry/harbor-satellite/internal/logger and use the existing types and functions (AuditLogger, AuditConfig, NewAuditLogger, (*AuditLogger).Reconfigure, (*AuditLogger).Log, (*AuditLogger).Enabled, WithAuditLogger, AuditFromContext and the auditLoggerKey/contextKey handling) so there is a single source of truth and no drift between root and ground-control.
🤖 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.
Outside diff comments:
In `@ground-control/internal/logger/audit.go`:
- Around line 137-142: The timestamp field currently uses Time("timestamp",
time.Now().UTC()) which loses nanoseconds under zerolog's default format;
replace that Time call on the evt builder returned by a.log.Log() with an
explicit string timestamp formatted with time.RFC3339Nano (e.g., format
time.Now().UTC() to RFC3339Nano and pass it via Str("timestamp", ...)) so the
emitted "timestamp" preserves nanosecond precision consistent with the schema.
In `@internal/logger/audit.go`:
- Around line 137-142: Replace the zerolog .Time call so timestamps are emitted
with RFC3339 nanosecond precision: instead of using
a.log.Log().Time("timestamp", time.Now().UTC()) in the evt builder, format the
time explicitly with time.RFC3339Nano (e.g., use
time.Now().UTC().Format(time.RFC3339Nano)) and set it via .Str("timestamp",
...); update any imports if necessary and keep the rest of the evt chain
(Str("event_id", ...), Str("event_type", ...), etc.) unchanged.
---
Nitpick comments:
In `@ground-control/internal/logger/audit.go`:
- Around line 1-176: This file duplicates the root internal/logger
implementation and should be removed in favor of importing the canonical
package; delete ground-control/internal/logger/audit.go and update callers in
ground-control to import
github.com/container-registry/harbor-satellite/internal/logger and use the
existing types and functions (AuditLogger, AuditConfig, NewAuditLogger,
(*AuditLogger).Reconfigure, (*AuditLogger).Log, (*AuditLogger).Enabled,
WithAuditLogger, AuditFromContext and the auditLoggerKey/contextKey handling) so
there is a single source of truth and no drift between root and ground-control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c007b15-bb83-4021-b27a-6f70e58bd999
⛔ Files ignored due to path filters (1)
ground-control/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/main.godocs/guides/audit-logging.mdgo.modground-control/go.modground-control/internal/logger/audit.goground-control/internal/logger/audit_test.goground-control/internal/server/server.gointernal/logger/audit.gointernal/logger/audit_test.gopkg/config/config.gopkg/config/validate.gopkg/config/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/validate.go
- docs/guides/audit-logging.md
- ground-control/internal/server/server.go
There was a problem hiding this comment.
3 issues found across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Record config.changed before disabling the audit logger on hot reload, so turning audit off is itself audited (it was emitted through the new no-op logger and lost). Reload handling moves into reconfigureAuditOnReload. - Detect audit-section changes in ConfigManager.detectChanges, so an audit-only hot reload (enable/disable or path change) actually triggers a reconfigure instead of being ignored unless a zot/log/interval change happened to ride along. - Create the audit log directory as 0o750 instead of 0o755; audit logs hold usernames/IPs/failure reasons and should not be world-listable. - Format the audit timestamp explicitly as RFC3339Nano so precision is deterministic and matches the documented contract, instead of depending on zerolog's global TimeFieldFormat. - Split audit rotation defaulting into enforceAuditRotation to keep validateAndEnforceAuditConfig under the cyclomatic-complexity limit. Refs #238 Signed-off-by: aloui ikram <ikram@container-registry.com>
Deploying harbor-satellite with
|
| Latest commit: |
09c7b87
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e2242505.harbor-satellite.pages.dev |
| Branch Preview URL: | https://feat-audit-logging-238.harbor-satellite.pages.dev |
Code Review ✅ Approved 3 resolved / 3 findingsImplements structured audit logging across Ground Control and Satellite for security-relevant events, including configurable rotation and SIEM integration. AuditFromContext performs an unnecessary allocation on every context miss, which should be optimized to avoid overhead. ✅ 3 resolved✅ Security: Audit log directory created with world-readable permissions (0755)
✅ Quality: Timestamp lacks nanosecond precision despite docs claiming it
✅ Performance: AuditFromContext allocates a new AuditLogger on every miss
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Important Your trial ends in 2 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
|
Follow-up review, focused on the audit event model and transport compatibility:
The implementation follow-ups from the previous review look substantially improved: startup now probes audit log writability, Satellite audit settings are detected during hot reload, zero retention values are preserved, Verification run locally at PR head go test ./... -count=1
(cd ground-control && go test ./... -count=1) |
3 participants
Closes #238.
Summary
Adds a dedicated structured audit log on both Ground Control and Satellite for security-relevant events. Output is one JSON document per line with size-, age, and count-based rotation, intended for direct ingestion by SIEM
tools (Filebeat → Elasticsearch, Wazuh, Splunk).
The two sides ship as separate Go modules and therefore each carry their own copy of the logger; the shared contract is the event schema documented in
docs/guides/audit-logging.md.Event schema
{ "event_id": "<uuid v4>", "timestamp": "<RFC3339 UTC, nanosecond precision>", "event_type": "<dotted lowercase name>", "actor": "<who, or empty>", "source_ip": "<ip, or empty>", "details": { "...": "event-specific" } }event_id,timestamp, andevent_typeare always present.detailsis omitted when empty.Implementation overview
internal/logger(Satellite) andground-control/internal/logger(Ground Control). Identical surface; intentional duplication justified by the two-module layout.AuditLoggeris a thin wrapper overzerologwriting to agopkg.in/natefinch/lumberjack.v2rotator. When disabled it writes toio.Discard, makingaudit.Log(...)safe to call unconditionally from every emit site.WithAuditLogger/AuditFromContexthelpers carry the logger through request-scoped contexts in the Ground Control HTTP stack.ground-control/internal/server/middleware.go) captures the client IP (X-Forwarded-For→X-Real-IP→RemoteAddr) and attaches both the logger and IP to the request context.Files
internal/logger/audit.go,internal/logger/audit_test.goground-control/internal/logger/audit.go,ground-control/internal/logger/audit_test.gopkg/config/{config,constants,getters,validate,validate_test}.go,config.example.jsonground-control/.env.examplecmd/main.go,ground-control/internal/server/{server,middleware}.go,internal/state/registration_process.goground-control/internal/server/{auth,user,satellite,config}_handlers.godocs/guides/audit-logging.mdgo.mod,go.sum(both modules)24 files, ~887 lines added, one new dependency (
lumberjack.v2).Event types
user.login.successuser.login.failureuser.createduser.deleteduser.password_changedsatellite.registeredsatellite.deregisteredsatellite.auth.failureconfig.changedsatellite.revokedsatellite.unrevokedpolicy.pull_blockedReserved constants are defined in the logger package so follow-up PRs (revoke handlers, registry-level policy hooks) can wire them up without changing the logger surface. See the roadmap section of
docs/guides/audit-logging.md.Configuration
Satellite :
app_config.auditblock inconfig.json:Ground Control : environment variables:
Both sides default to disabled. Defaults for path, rotation size, backup count, and retention are identical on both sides.
Validation: when
enabledis true,file_pathmust be non-empty and the rotation values must be non-negative. The process refuses to start on invalid config rather than silently dropping events.Verification
Unit tests
go test ./...passes in both modules. 14 new tests across the two loggerpackages cover: JSON output shape, required-field presence, RFC3339 timestamp
format, UUID uniqueness across calls, no-op behaviour when disabled and when
called on a nil receiver, and context round-trip via
WithAuditLogger/AuditFromContext.End-to-end run
Executed against a local stack: Harbor v2.11.1, Ground Control (this branch),
Satellite (this branch), Postgres 16. Audit enabled on both sides, paths set to
/tmp/audit-logs/gc-audit.logand/tmp/audit-logs/satellite-audit.log.Each event type was exercised via a real API call (no synthetic generators).
Captured Ground Control events
{ "event_id": "86ecf39e-40e4-46de-90d0-80e9900e77eb", "timestamp": "2026-05-17T15:02:01Z", "event_type": "user.login.success", "actor": "admin", "source_ip": "::1" } { "event_id": "77524874-81bb-4885-944e-9e754fb21a0a", "timestamp": "2026-05-17T15:12:57Z", "event_type": "user.login.failure", "actor": "admin", "source_ip": "::1", "details": { "reason": "bad_password" } } { "event_id": "261473a3-4445-4469-b277-4500b433877e", "timestamp": "2026-05-17T15:18:19Z", "event_type": "user.created", "actor": "admin", "source_ip": "::1", "details": { "role": "admin", "target_user": "demo" } } { "event_id": "8bb311ce-170a-4787-ab1d-6ef7b4bdcdef", "timestamp": "2026-05-17T15:24:22Z", "event_type": "user.password_changed", "actor": "admin", "source_ip": "::1", "details": { "flow": "admin_reset", "target_user": "demo" } } { "event_id": "2f612367-85e0-4c89-9156-da0a719a16d1", "timestamp": "2026-05-17T15:31:41Z", "event_type": "config.changed", "actor": "admin", "source_ip": "::1", "details": { "action": "create", "config_name": "default" } } { "event_id": "7c61a5df-a2a8-41d0-8fec-7cd6e14c2a67", "timestamp": "2026-05-17T15:32:00Z", "event_type": "satellite.registered", "actor": "sat-demo-01", "source_ip": "::1", "details": { "config_name": "default", "flow": "token", "groups": null } } { "event_id": "9483ecb9-e686-4f24-960a-7455ef803054", "timestamp": "2026-05-17T15:44:31Z", "event_type": "user.deleted", "actor": "admin", "source_ip": "::1", "details": { "target_user": "demo" } } { "event_id": "8a4a5e08-5382-4f6f-82e6-ddff9924cae4", "timestamp": "2026-05-17T15:45:42Z", "event_type": "satellite.auth.failure", "actor": "", "source_ip": "::1", "details": { "masked_token": "this...oken", "reason": "invalid_token" } } { "event_id": "4162ba9d-d66e-4fd2-ad1a-76f2e0d63518", "timestamp": "2026-05-17T15:55:22Z", "event_type": "satellite.registered", "actor": "sat-demo-02", "source_ip": "127.0.0.1", "details": { "flow": "ztr" } }Sensitive material is redacted before emission:
satellite.auth.failurerecords
masked_token(first four and last four characters only), never thefull invalid token.
Captured Satellite events
{ "event_id": "1b94cad2-18b2-47e6-8091-3f3fbe32227c", "timestamp": "2026-05-17T15:55:22.38776313Z", "event_type": "satellite.registered", "actor": "http://localhost:8080", "source_ip": "", "details": { "flow": "ztr" } } { "event_id": "7e5b0b00-3c32-4b3a-94c4-dbb7dd4dbe40", "timestamp": "2026-05-17T15:55:22.387929188Z", "event_type": "config.changed", "actor": "satellite", "source_ip": "", "details": { "changed_keys": ["zot_config"], "source": "hot_reload" } }Coverage matrix
user.login.successuser.login.failureuser.createduser.deleteduser.password_changedsatellite.registeredsatellite.auth.failureconfig.changedsatellite.deregisteredKnown limitation
satellite.deregisteredcould not be exercised in the local test environment.The deregister handler attempts to delete the satellite's Harbor repository
before the audit emit fires; when the repository was never written to, Harbor
returns 404 and the handler exits early with HTTP 500. This is a pre-existing
behaviour of the deregister handler, not a defect introduced by this PR; the
audit emit on the success path is wired identically to the other satellite
events.
Documentation
docs/guides/audit-logging.mddocuments the schema, every event type and itsdetailspayload, both configuration mechanisms, and an integration recipe forFilebeat → Elasticsearch.
Summary by CodeRabbit