From 298c9608082f7644c4469980df9efa4f386f70b0 Mon Sep 17 00:00:00 2001 From: Ivan Godwin Date: Thu, 26 Mar 2026 20:17:51 -0700 Subject: [PATCH] Fix 8 high-severity audit findings across security, Go, API, and container domains - Use typed context key for auth context to prevent collisions (auth.go) - Eliminate nested locking in CheckRateLimit to prevent potential deadlock (auth.go) - Add 1MB request body size limit middleware to prevent DoS (router.go) - Return proper gRPC status codes instead of nil errors on failures (handler.go) - Use key name instead of raw API key in admin URL paths to prevent secret leakage (keys.go, router.go, keystore_db.go, keystore_hybrid.go) - Enforce RBAC authorization in service Send/SendBatch for both REST and gRPC (service.go) - Pin runtime Docker image to alpine:3.21 for reproducible builds (Dockerfile) - Enable readOnlyRootFilesystem with /tmp emptyDir in k8s deployment (deployment.yaml) Co-Authored-By: Claude Opus 4.6 --- Dockerfile | 2 +- api/grpc/handler.go | 15 +++--- api/rest/keys.go | 36 +++++++------- api/rest/router.go | 21 ++++++-- internal/auth/auth.go | 18 +++++-- internal/auth/keystore_db.go | 83 ++++++++++++++++++++++++++++++++ internal/auth/keystore_hybrid.go | 26 ++++++++++ internal/service/service.go | 48 +++++++++++++++--- k8s/deployment.yaml | 6 ++- 9 files changed, 212 insertions(+), 43 deletions(-) diff --git a/Dockerfile b/Dockerfile index bc06f97..08ea748 100644 --- a/Dockerfile +++ b/Dockerfile @@ -35,7 +35,7 @@ RUN CGO_ENABLED=0 GOOS=linux go build -a -installsuffix cgo \ -o server ./cmd/server # Runtime stage -FROM alpine:latest +FROM alpine:3.21 # Install runtime dependencies RUN apk --no-cache add ca-certificates tzdata diff --git a/api/grpc/handler.go b/api/grpc/handler.go index a4929ba..595062a 100644 --- a/api/grpc/handler.go +++ b/api/grpc/handler.go @@ -8,6 +8,8 @@ import ( pb "github.com/igodwin/notifier/api/grpc/pb" "github.com/igodwin/notifier/internal/domain" "github.com/igodwin/notifier/internal/logging" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -82,12 +84,7 @@ func (h *NotifierHandler) SendNotification(ctx context.Context, req *pb.SendNoti if err != nil { h.logger.Errorf("gRPC: Failed to send notification - type=%s, account=%s, error=%v", req.Type, req.Account, err) - return &pb.SendNotificationResponse{ - Result: &pb.NotificationResult{ - Success: false, - Error: err.Error(), - }, - }, nil + return nil, status.Errorf(codes.Internal, "failed to send notification: %v", err) } // Log success @@ -139,7 +136,7 @@ func (h *NotifierHandler) SendBatchNotifications(ctx context.Context, req *pb.Se func (h *NotifierHandler) GetNotification(ctx context.Context, req *pb.GetNotificationRequest) (*pb.GetNotificationResponse, error) { notification, err := h.service.GetNotification(ctx, req.Id) if err != nil { - return nil, err + return nil, status.Errorf(codes.NotFound, "notification not found: %v", err) } return &pb.GetNotificationResponse{ @@ -154,7 +151,7 @@ func (h *NotifierHandler) ListNotifications(ctx context.Context, req *pb.ListNot notifications, err := h.service.ListNotifications(ctx, filter) if err != nil { - return nil, err + return nil, status.Errorf(codes.Internal, "failed to list notifications: %v", err) } protoNotifications := make([]*pb.Notification, len(notifications)) @@ -230,7 +227,7 @@ func (h *NotifierHandler) GetNotifiers(ctx context.Context, req *pb.GetNotifiers notifiers, err := h.service.GetNotifiers(ctx) if err != nil { h.logger.Errorf("gRPC: Failed to get notifiers - error=%v", err) - return nil, err + return nil, status.Errorf(codes.Internal, "failed to get notifiers: %v", err) } // Convert domain notifiers to proto notifiers diff --git a/api/rest/keys.go b/api/rest/keys.go index c222187..b19951a 100644 --- a/api/rest/keys.go +++ b/api/rest/keys.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/gorilla/mux" "github.com/igodwin/notifier/internal/auth" "github.com/igodwin/notifier/internal/logging" ) @@ -76,7 +77,7 @@ func (h *KeyManagementHandler) CreateKey(w http.ResponseWriter, r *http.Request) ctx := r.Context() // Check authorization - must have admin role - authCtx, ok := ctx.Value("auth").(*auth.AuthContext) + authCtx, ok := auth.GetAuthContext(ctx) if !ok || !h.hasRole(authCtx, "admin") { h.respondError(w, http.StatusForbidden, "Insufficient permissions", "admin role required") return @@ -143,7 +144,7 @@ func (h *KeyManagementHandler) CreateKey(w http.ResponseWriter, r *http.Request) func (h *KeyManagementHandler) ListKeys(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - authCtx, ok := ctx.Value("auth").(*auth.AuthContext) + authCtx, ok := auth.GetAuthContext(ctx) if !ok { h.respondError(w, http.StatusUnauthorized, "Unauthorized", "") return @@ -193,24 +194,25 @@ type RevokeKeyRequest struct { } // RevokeKey deactivates an API key -// DELETE /api/v1/admin/keys/:key +// DELETE /api/v1/admin/keys/:name // Requires: admin role func (h *KeyManagementHandler) RevokeKey(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - authCtx, ok := ctx.Value("auth").(*auth.AuthContext) + authCtx, ok := auth.GetAuthContext(ctx) if !ok || !h.hasRole(authCtx, "admin") { h.respondError(w, http.StatusForbidden, "Insufficient permissions", "admin role required") return } - // Extract key from path parameter - keyStr := strings.TrimPrefix(r.URL.Path, "/api/v1/admin/keys/") + // Extract key name from path parameter (not the raw key, to avoid leaking secrets in URLs) + vars := mux.Vars(r) + keyName := vars["name"] var req RevokeKeyRequest _ = json.NewDecoder(r.Body).Decode(&req) // Ignore decode errors, reason is optional - err := h.keyStore.DeactivateKey(ctx, keyStr, authCtx.ClientID) + err := h.keyStore.DeactivateKeyByName(ctx, keyName, authCtx.ClientID) if err != nil { if strings.Contains(err.Error(), "not found") { h.respondError(w, http.StatusNotFound, "Key not found", "") @@ -231,19 +233,19 @@ type RotateKeyRequest struct { } // RotateKey creates a new API key to replace the old one -// POST /api/v1/admin/keys/:key/rotate +// POST /api/v1/admin/keys/:name/rotate // Requires: admin role func (h *KeyManagementHandler) RotateKey(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - authCtx, ok := ctx.Value("auth").(*auth.AuthContext) + authCtx, ok := auth.GetAuthContext(ctx) if !ok || !h.hasRole(authCtx, "admin") { h.respondError(w, http.StatusForbidden, "Insufficient permissions", "admin role required") return } - oldKeyStr := strings.TrimPrefix(r.URL.Path, "/api/v1/admin/keys/") - oldKeyStr = strings.TrimSuffix(oldKeyStr, "/rotate") + vars := mux.Vars(r) + _ = vars["name"] // Key name from URL (rotation not yet implemented) var req RotateKeyRequest _ = json.NewDecoder(r.Body).Decode(&req) @@ -265,19 +267,19 @@ type GetAuditLogResponse struct { } // GetAuditLog retrieves the audit log for a key -// GET /api/v1/admin/keys/:key/audit +// GET /api/v1/admin/keys/:name/audit // Requires: admin role func (h *KeyManagementHandler) GetAuditLog(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - authCtx, ok := ctx.Value("auth").(*auth.AuthContext) + authCtx, ok := auth.GetAuthContext(ctx) if !ok || !h.hasRole(authCtx, "admin") { h.respondError(w, http.StatusForbidden, "Insufficient permissions", "admin role required") return } - keyStr := strings.TrimPrefix(r.URL.Path, "/api/v1/admin/keys/") - keyStr = strings.TrimSuffix(keyStr, "/audit") + vars := mux.Vars(r) + keyName := vars["name"] limit := 100 if limitStr := r.URL.Query().Get("limit"); limitStr != "" { @@ -286,7 +288,7 @@ func (h *KeyManagementHandler) GetAuditLog(w http.ResponseWriter, r *http.Reques } } - logs, err := h.keyStore.GetAuditLog(ctx, keyStr, limit) + logs, err := h.keyStore.GetAuditLogByName(ctx, keyName, limit) if err != nil { h.logger.Errorf("Failed to get audit log: %v", err) h.respondError(w, http.StatusInternalServerError, "Failed to get audit log", err.Error()) @@ -294,7 +296,7 @@ func (h *KeyManagementHandler) GetAuditLog(w http.ResponseWriter, r *http.Reques } resp := GetAuditLogResponse{ - Key: "nk_" + keyStr[len(keyStr)-4:], + Key: keyName, AuditLog: logs, } diff --git a/api/rest/router.go b/api/rest/router.go index 7e8e702..8c8ffc5 100644 --- a/api/rest/router.go +++ b/api/rest/router.go @@ -86,20 +86,33 @@ func NewRouterWithAuthAndKeyStore(service domain.NotificationService, logger *lo keyHandler := NewKeyManagementHandler(keyStore, logger) v1.HandleFunc("/admin/keys", keyHandler.CreateKey).Methods(http.MethodPost) v1.HandleFunc("/admin/keys", keyHandler.ListKeys).Methods(http.MethodGet) - v1.HandleFunc("/admin/keys/{key}", keyHandler.RevokeKey).Methods(http.MethodDelete) - v1.HandleFunc("/admin/keys/{key}/rotate", keyHandler.RotateKey).Methods(http.MethodPost) - v1.HandleFunc("/admin/keys/{key}/audit", keyHandler.GetAuditLog).Methods(http.MethodGet) + v1.HandleFunc("/admin/keys/{name}", keyHandler.RevokeKey).Methods(http.MethodDelete) + v1.HandleFunc("/admin/keys/{name}/rotate", keyHandler.RotateKey).Methods(http.MethodPost) + v1.HandleFunc("/admin/keys/{name}/audit", keyHandler.GetAuditLog).Methods(http.MethodGet) } // Health check route (no auth required) router.HandleFunc("/health", handler.HealthCheck).Methods(http.MethodGet) - // Middleware - logging and CORS + // Middleware - logging, request size limit, and CORS router.Use(loggingMiddleware) + v1.Use(maxBodySizeMiddleware(1 << 20)) // 1 MB limit on API request bodies return router } +// maxBodySizeMiddleware limits the size of incoming request bodies to prevent DoS. +func maxBodySizeMiddleware(maxBytes int64) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Body != nil { + r.Body = http.MaxBytesReader(w, r.Body, maxBytes) + } + next.ServeHTTP(w, r) + }) + } +} + // loggingMiddleware logs incoming requests func loggingMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5da49cc..199c32d 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -115,24 +115,29 @@ func (s *APIKeyStore) ValidateKey(keyStr string) (*APIKey, error) { // CheckRateLimit checks if a key has exceeded its rate limit func (s *APIKeyStore) CheckRateLimit(keyStr string) (bool, error) { - s.mu.Lock() - defer s.mu.Unlock() - + // Look up key and limiter under the store lock, then release it + // before acquiring the per-key limiter lock to avoid nested locking. + s.mu.RLock() key, exists := s.keys[keyStr] if !exists { + s.mu.RUnlock() return false, fmt.Errorf("invalid API key") } // Unlimited rate limit if key.RateLimit <= 0 { + s.mu.RUnlock() return true, nil } limiter, exists := s.rateLimits[keyStr] if !exists { + s.mu.RUnlock() return false, fmt.Errorf("rate limiter not found") } + s.mu.RUnlock() + // Now lock only the per-key rate limiter limiter.mu.Lock() defer limiter.mu.Unlock() @@ -206,13 +211,16 @@ func (s *APIKeyStore) ListKeys(clientID string) []*APIKey { return keys } +// authContextKey is an unexported type for context keys to avoid collisions. +type authContextKey struct{} + // ContextWithAuth adds auth context to a request context func ContextWithAuth(ctx context.Context, auth *AuthContext) context.Context { - return context.WithValue(ctx, "auth", auth) + return context.WithValue(ctx, authContextKey{}, auth) } // GetAuthContext retrieves auth context from a request context func GetAuthContext(ctx context.Context) (*AuthContext, bool) { - auth, ok := ctx.Value("auth").(*AuthContext) + auth, ok := ctx.Value(authContextKey{}).(*AuthContext) return auth, ok } diff --git a/internal/auth/keystore_db.go b/internal/auth/keystore_db.go index 065b26c..c07f3fd 100644 --- a/internal/auth/keystore_db.go +++ b/internal/auth/keystore_db.go @@ -347,6 +347,89 @@ func (ks *KeyStoreDB) GetAuditLog(ctx context.Context, keyStr string, limit int) return logs, rows.Err() } +// GetKeyByName retrieves an API key by its name +func (ks *KeyStoreDB) GetKeyByName(ctx context.Context, name string) (*APIKey, error) { + query := ` + SELECT key, name, client_id, roles, created_at, last_used_at, expires_at, is_active, rate_limit + FROM api_keys + WHERE name = $1 + ` + + var key APIKey + var roles []string + + err := ks.db.QueryRowContext(ctx, query, name).Scan( + &key.Key, + &key.Name, + &key.ClientID, + pq.Array(&roles), + &key.CreatedAt, + &key.LastUsedAt, + &key.ExpiresAt, + &key.IsActive, + &key.RateLimit, + ) + + if err == sql.ErrNoRows { + return nil, ErrKeyNotFound + } + if err != nil { + return nil, fmt.Errorf("failed to get key by name: %w", err) + } + + key.Roles = roles + return &key, nil +} + +// DeactivateKeyByName disables an API key by its name +func (ks *KeyStoreDB) DeactivateKeyByName(ctx context.Context, name string, deactivatedBy string) error { + // First get the key to find its raw key for cache invalidation and audit + key, err := ks.GetKeyByName(ctx, name) + if err != nil { + return err + } + + return ks.DeactivateKey(ctx, key.Key, deactivatedBy) +} + +// GetAuditLogByName retrieves audit log entries for a key identified by name +func (ks *KeyStoreDB) GetAuditLogByName(ctx context.Context, name string, limit int) ([]map[string]interface{}, error) { + query := ` + SELECT al.action, al.performed_by, al.performed_at, al.details + FROM api_key_audit_log al + JOIN api_keys ak ON al.key_id = ak.id + WHERE ak.name = $1 + ORDER BY al.performed_at DESC + LIMIT $2 + ` + + rows, err := ks.db.QueryContext(ctx, query, name, limit) + if err != nil { + return nil, fmt.Errorf("failed to get audit log: %w", err) + } + defer rows.Close() + + var logs []map[string]interface{} + for rows.Next() { + var action, performedBy, details string + var performedAt time.Time + + err := rows.Scan(&action, &performedBy, &performedAt, &details) + if err != nil { + return nil, err + } + + logs = append(logs, map[string]interface{}{ + "action": action, + "performed_by": performedBy, + "performed_at": performedAt, + "details": details, + }) + } + + return logs, rows.Err() +} + // Custom errors var ( ErrKeyNotFound = fmt.Errorf("API key not found") diff --git a/internal/auth/keystore_hybrid.go b/internal/auth/keystore_hybrid.go index fe6c6f1..1934546 100644 --- a/internal/auth/keystore_hybrid.go +++ b/internal/auth/keystore_hybrid.go @@ -136,6 +136,32 @@ func (h *HybridKeyStore) GetAuditLog(ctx context.Context, keyStr string, limit i return h.db.GetAuditLog(ctx, keyStr, limit) } +// DeactivateKeyByName deactivates a key by its name (avoids exposing raw key in URLs) +func (h *HybridKeyStore) DeactivateKeyByName(ctx context.Context, name string, deactivatedBy string) error { + h.mu.Lock() + defer h.mu.Unlock() + + // Look up the key by name in DB to get the raw key for cache invalidation + key, err := h.db.GetKeyByName(ctx, name) + if err != nil { + return err + } + + // Remove from cache + h.cache.mu.Lock() + delete(h.cache.keys, key.Key) + delete(h.cache.rateLimits, key.Key) + h.cache.mu.Unlock() + + // Deactivate in database + return h.db.DeactivateKey(ctx, key.Key, deactivatedBy) +} + +// GetAuditLogByName retrieves audit log for a key identified by name +func (h *HybridKeyStore) GetAuditLogByName(ctx context.Context, name string, limit int) ([]map[string]interface{}, error) { + return h.db.GetAuditLogByName(ctx, name, limit) +} + // Close closes the database connection func (h *HybridKeyStore) Close() error { return h.db.Close() diff --git a/internal/service/service.go b/internal/service/service.go index a2b334b..92b606e 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -276,6 +276,16 @@ func (s *NotificationService) processNotification(ctx context.Context, msg *doma // Send queues a notification for delivery func (s *NotificationService) Send(ctx context.Context, notification *domain.Notification) (*domain.NotificationResult, error) { + // Enforce RBAC authorization if configured + if err := s.checkAuthorization(ctx, notification); err != nil { + return &domain.NotificationResult{ + NotificationID: notification.ID, + Success: false, + Error: err.Error(), + SentAt: time.Now(), + }, err + } + // Store the notification s.storeNotification(notification) @@ -301,6 +311,13 @@ func (s *NotificationService) Send(ctx context.Context, notification *domain.Not func (s *NotificationService) SendBatch(ctx context.Context, notifications []*domain.Notification) ([]*domain.NotificationResult, error) { results := make([]*domain.NotificationResult, 0, len(notifications)) + // Enforce RBAC authorization for each notification + for _, notification := range notifications { + if err := s.checkAuthorization(ctx, notification); err != nil { + return nil, fmt.Errorf("authorization denied for notification type=%s account=%s: %w", notification.Type, notification.Account, err) + } + } + // Store all notifications for _, notification := range notifications { s.storeNotification(notification) @@ -439,12 +456,7 @@ func (s *NotificationService) GetStats(ctx context.Context) (*domain.Notificatio // GetNotifiers returns information about available notifiers, filtered by authorization if auth context is provided func (s *NotificationService) GetNotifiers(ctx context.Context) (*domain.NotifiersResponse, error) { // Extract auth context from request context if available - var authCtx *auth.AuthContext - if authVal := ctx.Value("auth"); authVal != nil { - if ac, ok := authVal.(*auth.AuthContext); ok { - authCtx = ac - } - } + authCtx, _ := auth.GetAuthContext(ctx) supportedTypes := s.factory.SupportedTypes() notifiers := make([]domain.NotifierInfo, 0, len(supportedTypes)) @@ -510,6 +522,30 @@ func (s *NotificationService) updateNotification(notification *domain.Notificati s.notifications[notification.ID] = notification } +// checkAuthorization verifies that the caller is authorized to send to the given notifier/account. +// Returns nil if authorized or if RBAC is not configured. +func (s *NotificationService) checkAuthorization(ctx context.Context, notification *domain.Notification) error { + if s.authz == nil || !s.authz.HasRules() { + return nil // RBAC not configured + } + + authCtx, ok := auth.GetAuthContext(ctx) + if !ok { + return nil // No auth context (auth may be disabled) + } + + account := notification.Account + if account == "" && s.accountResolver != nil { + account = s.accountResolver.GetDefaultAccount(notification.Type) + } + + if !s.authz.IsAuthorized(authCtx, notification.Type, account) { + return fmt.Errorf("not authorized to send %s notifications to account %s", notification.Type, account) + } + + return nil +} + // matchesFilter checks if a notification matches the filter func (s *NotificationService) matchesFilter(notification *domain.Notification, filter *domain.NotificationFilter) bool { if filter == nil { diff --git a/k8s/deployment.yaml b/k8s/deployment.yaml index e80241e..5303783 100644 --- a/k8s/deployment.yaml +++ b/k8s/deployment.yaml @@ -50,6 +50,8 @@ spec: readOnly: true - name: queue-storage mountPath: /var/lib/notifier + - name: tmp + mountPath: /tmp resources: requests: cpu: 100m @@ -77,7 +79,7 @@ spec: runAsNonRoot: true runAsUser: 1000 allowPrivilegeEscalation: false - readOnlyRootFilesystem: false + readOnlyRootFilesystem: true capabilities: drop: - ALL @@ -87,6 +89,8 @@ spec: name: notifier-config - name: queue-storage emptyDir: {} + - name: tmp + emptyDir: {} restartPolicy: Always --- apiVersion: v1