fix: address review findings for observability PR
All checks were successful
Check / check (pull_request) Successful in 1m42s
All checks were successful
Check / check (pull_request) Successful in 1m42s
1. Security: Replace insecure extractRemoteIP() in audit service with middleware.RealIP() which validates trusted proxies before trusting X-Real-IP/X-Forwarded-For headers. Export RealIP from middleware. Update audit tests to verify anti-spoofing behavior. 2. Audit coverage: Add audit instrumentation to all 9 handlers that had dead action constants: HandleEnvVarSave, HandleLabelAdd, HandleLabelEdit, HandleLabelDelete, HandleVolumeAdd, HandleVolumeEdit, HandleVolumeDelete, HandlePortAdd, HandlePortDelete. 3. README: Fix API path from /api/audit to /api/v1/audit. 4. README: Fix duplicate numbering in DI order section (items 10-11 were listed twice, now correctly numbered 10-16).
This commit is contained in:
12
README.md
12
README.md
@@ -67,11 +67,11 @@ Uses Uber fx for dependency injection. Components are wired in this order:
|
||||
9. `docker` - Docker client
|
||||
10. `notify` - Notification service
|
||||
11. `audit` - Audit logging service
|
||||
10. `deploy` - Deployment service
|
||||
11. `webhook` - Webhook processing
|
||||
12. `middleware` - HTTP middleware
|
||||
13. `handlers` - HTTP handlers
|
||||
14. `server` - HTTP server
|
||||
12. `deploy` - Deployment service
|
||||
13. `webhook` - Webhook processing
|
||||
14. `middleware` - HTTP middleware
|
||||
15. `handlers` - HTTP handlers
|
||||
16. `server` - HTTP server
|
||||
|
||||
### Request Flow
|
||||
|
||||
@@ -247,7 +247,7 @@ All user-facing actions are recorded in an `audit_log` SQLite table with:
|
||||
Audited actions include login/logout, app CRUD, deployments, container
|
||||
start/stop/restart, rollbacks, deployment cancellation, and webhook receipt.
|
||||
|
||||
The audit log is available via the API at `GET /api/audit?limit=N` (max 500,
|
||||
The audit log is available via the API at `GET /api/v1/audit?limit=N` (max 500,
|
||||
default 50).
|
||||
|
||||
### Structured Logging
|
||||
|
||||
@@ -1023,6 +1023,10 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc {
|
||||
return
|
||||
}
|
||||
|
||||
h.auditLog(request, models.AuditActionEnvVarSave,
|
||||
models.AuditResourceEnvVar, application.ID,
|
||||
fmt.Sprintf("saved %d env vars", len(modelPairs)))
|
||||
|
||||
h.respondJSON(writer, request, map[string]bool{"ok": true}, http.StatusOK)
|
||||
}
|
||||
}
|
||||
@@ -1039,7 +1043,13 @@ func (h *Handlers) HandleLabelAdd() http.HandlerFunc {
|
||||
label.Key = key
|
||||
label.Value = value
|
||||
|
||||
return label.Save(ctx)
|
||||
err := label.Save(ctx)
|
||||
if err == nil {
|
||||
h.auditLog(request, models.AuditActionLabelAdd,
|
||||
models.AuditResourceLabel, application.ID, "added label: "+key)
|
||||
}
|
||||
|
||||
return err
|
||||
},
|
||||
)
|
||||
}
|
||||
@@ -1068,6 +1078,9 @@ func (h *Handlers) HandleLabelDelete() http.HandlerFunc {
|
||||
deleteErr := label.Delete(request.Context())
|
||||
if deleteErr != nil {
|
||||
h.log.Error("failed to delete label", "error", deleteErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionLabelDelete,
|
||||
models.AuditResourceLabel, appID, "deleted label: "+label.Key)
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
||||
@@ -1125,6 +1138,10 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc {
|
||||
saveErr := volume.Save(request.Context())
|
||||
if saveErr != nil {
|
||||
h.log.Error("failed to add volume", "error", saveErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionVolumeAdd,
|
||||
models.AuditResourceVolume, application.ID,
|
||||
"added volume: "+hostPath+":"+containerPath)
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther)
|
||||
@@ -1154,6 +1171,10 @@ func (h *Handlers) HandleVolumeDelete() http.HandlerFunc {
|
||||
deleteErr := volume.Delete(request.Context())
|
||||
if deleteErr != nil {
|
||||
h.log.Error("failed to delete volume", "error", deleteErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionVolumeDelete,
|
||||
models.AuditResourceVolume, appID,
|
||||
"deleted volume: "+volume.HostPath+":"+volume.ContainerPath)
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
||||
@@ -1203,6 +1224,10 @@ func (h *Handlers) HandlePortAdd() http.HandlerFunc {
|
||||
saveErr := port.Save(request.Context())
|
||||
if saveErr != nil {
|
||||
h.log.Error("failed to save port", "error", saveErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionPortAdd,
|
||||
models.AuditResourcePort, application.ID,
|
||||
fmt.Sprintf("added port: %d:%d/%s", hostPort, containerPort, protocol))
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther)
|
||||
@@ -1249,6 +1274,10 @@ func (h *Handlers) HandlePortDelete() http.HandlerFunc {
|
||||
deleteErr := port.Delete(request.Context())
|
||||
if deleteErr != nil {
|
||||
h.log.Error("failed to delete port", "error", deleteErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionPortDelete,
|
||||
models.AuditResourcePort, appID,
|
||||
fmt.Sprintf("deleted port: %d:%d/%s", port.HostPort, port.ContainerPort, port.Protocol))
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
||||
@@ -1324,6 +1353,9 @@ func (h *Handlers) HandleLabelEdit() http.HandlerFunc {
|
||||
saveErr := label.Save(request.Context())
|
||||
if saveErr != nil {
|
||||
h.log.Error("failed to update label", "error", saveErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionLabelEdit,
|
||||
models.AuditResourceLabel, appID, "edited label: "+key)
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
||||
@@ -1382,6 +1414,10 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc {
|
||||
saveErr := volume.Save(request.Context())
|
||||
if saveErr != nil {
|
||||
h.log.Error("failed to update volume", "error", saveErr)
|
||||
} else {
|
||||
h.auditLog(request, models.AuditActionVolumeEdit,
|
||||
models.AuditResourceVolume, appID,
|
||||
"edited volume: "+hostPath+":"+containerPath)
|
||||
}
|
||||
|
||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
||||
|
||||
@@ -105,7 +105,7 @@ func (m *Middleware) Logging() func(http.Handler) http.Handler {
|
||||
"request_id", reqID,
|
||||
"referer", request.Referer(),
|
||||
"proto", request.Proto,
|
||||
"remoteIP", realIP(request),
|
||||
"remoteIP", RealIP(request),
|
||||
"status", lrw.statusCode,
|
||||
"bytes", lrw.bytesWritten,
|
||||
"latency_ms", latency.Milliseconds(),
|
||||
@@ -170,11 +170,11 @@ func isTrustedProxy(ip net.IP) bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// realIP extracts the client's real IP address from the request.
|
||||
// RealIP extracts the client's real IP address from the request.
|
||||
// Proxy headers (X-Real-IP, X-Forwarded-For) are only trusted when the
|
||||
// direct connection originates from an RFC1918/loopback address.
|
||||
// Otherwise, headers are ignored and RemoteAddr is used (fail closed).
|
||||
func realIP(r *http.Request) string {
|
||||
func RealIP(r *http.Request) string {
|
||||
addr := ipFromHostPort(r.RemoteAddr)
|
||||
remoteIP := net.ParseIP(addr)
|
||||
|
||||
@@ -365,7 +365,7 @@ func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler {
|
||||
writer http.ResponseWriter,
|
||||
request *http.Request,
|
||||
) {
|
||||
ip := realIP(request)
|
||||
ip := RealIP(request)
|
||||
limiter := loginLimiter.getLimiter(ip)
|
||||
|
||||
if !limiter.Allow() {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
package middleware //nolint:testpackage // tests unexported realIP function
|
||||
package middleware //nolint:testpackage // tests RealIP via internal package access
|
||||
|
||||
import (
|
||||
"context"
|
||||
@@ -126,9 +126,9 @@ func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test
|
||||
req.Header.Set("X-Forwarded-For", tt.xff)
|
||||
}
|
||||
|
||||
got := realIP(req)
|
||||
got := RealIP(req)
|
||||
if got != tt.want {
|
||||
t.Errorf("realIP() = %q, want %q", got, tt.want)
|
||||
t.Errorf("RealIP() = %q, want %q", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -5,15 +5,14 @@ import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"log/slog"
|
||||
"net"
|
||||
"net/http"
|
||||
"strings"
|
||||
|
||||
"go.uber.org/fx"
|
||||
|
||||
"sneak.berlin/go/upaas/internal/database"
|
||||
"sneak.berlin/go/upaas/internal/logger"
|
||||
"sneak.berlin/go/upaas/internal/metrics"
|
||||
"sneak.berlin/go/upaas/internal/middleware"
|
||||
"sneak.berlin/go/upaas/internal/models"
|
||||
)
|
||||
|
||||
@@ -98,42 +97,16 @@ func (svc *Service) Log(ctx context.Context, entry LogEntry) {
|
||||
}
|
||||
|
||||
// LogFromRequest records an audit log entry, extracting the remote IP from
|
||||
// the HTTP request.
|
||||
// the HTTP request using the middleware's trusted-proxy-aware IP resolution.
|
||||
func (svc *Service) LogFromRequest(
|
||||
ctx context.Context,
|
||||
request *http.Request,
|
||||
entry LogEntry,
|
||||
) {
|
||||
entry.RemoteIP = extractRemoteIP(request)
|
||||
entry.RemoteIP = middleware.RealIP(request)
|
||||
svc.Log(ctx, entry)
|
||||
}
|
||||
|
||||
// extractRemoteIP extracts the client IP from the request, preferring
|
||||
// X-Real-IP and X-Forwarded-For headers from trusted proxies.
|
||||
func extractRemoteIP(r *http.Request) string {
|
||||
// Check X-Real-IP first
|
||||
if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" {
|
||||
return ip
|
||||
}
|
||||
|
||||
// Check X-Forwarded-For (leftmost = client)
|
||||
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
|
||||
if parts := strings.SplitN(xff, ",", 2); len(parts) > 0 { //nolint:mnd // split limit
|
||||
if ip := strings.TrimSpace(parts[0]); ip != "" {
|
||||
return ip
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Fall back to RemoteAddr
|
||||
host, _, err := net.SplitHostPort(r.RemoteAddr)
|
||||
if err != nil {
|
||||
return r.RemoteAddr
|
||||
}
|
||||
|
||||
return host
|
||||
}
|
||||
|
||||
// Recent returns the most recent audit log entries.
|
||||
func (svc *Service) Recent(
|
||||
ctx context.Context,
|
||||
|
||||
@@ -103,13 +103,15 @@ func TestAuditServiceLogFromRequest(t *testing.T) {
|
||||
assert.Equal(t, "app-1", entries[0].ResourceID.String)
|
||||
}
|
||||
|
||||
func TestAuditServiceLogFromRequestWithXRealIP(t *testing.T) {
|
||||
func TestAuditServiceLogFromRequestWithXRealIPTrustedProxy(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
svc, db := setupTestAuditService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
// When the request comes from a trusted proxy (RFC1918), X-Real-IP is honoured.
|
||||
request := httptest.NewRequest(http.MethodPost, "/apps", nil)
|
||||
request.RemoteAddr = "10.0.0.1:1234"
|
||||
request.Header.Set("X-Real-IP", "203.0.113.50")
|
||||
|
||||
svc.LogFromRequest(ctx, request, audit.LogEntry{
|
||||
@@ -124,6 +126,29 @@ func TestAuditServiceLogFromRequestWithXRealIP(t *testing.T) {
|
||||
assert.Equal(t, "203.0.113.50", entries[0].RemoteIP.String)
|
||||
}
|
||||
|
||||
func TestAuditServiceLogFromRequestWithXRealIPUntrustedProxy(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
svc, db := setupTestAuditService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
// When the request comes from a public IP, X-Real-IP is ignored (anti-spoof).
|
||||
request := httptest.NewRequest(http.MethodPost, "/apps", nil)
|
||||
request.RemoteAddr = "203.0.113.99:1234"
|
||||
request.Header.Set("X-Real-IP", "10.0.0.1")
|
||||
|
||||
svc.LogFromRequest(ctx, request, audit.LogEntry{
|
||||
Username: "admin",
|
||||
Action: models.AuditActionAppCreate,
|
||||
ResourceType: models.AuditResourceApp,
|
||||
})
|
||||
|
||||
entries, err := models.FindAuditEntries(ctx, db, 10)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, entries, 1)
|
||||
assert.Equal(t, "203.0.113.99", entries[0].RemoteIP.String)
|
||||
}
|
||||
|
||||
func TestAuditServiceRecent(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user