From 893fbbc2434f4037b4c326d91be744fba43c199b Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Fri, 10 Apr 2026 11:42:43 +1000 Subject: [PATCH] fix: remediate trivial security audit findings (#124) --- spinifex/gateway/auth.go | 5 +---- spinifex/gateway/auth_test.go | 9 +++++--- spinifex/gateway/gateway.go | 5 +---- spinifex/gateway/gateway_test.go | 5 +++-- spinifex/handlers/ec2/vpc/security_group.go | 11 ++++++++++ spinifex/handlers/iam/service_impl.go | 10 +++++---- spinifex/utils/nats.go | 23 ++++++++++++++++++++- 7 files changed, 50 insertions(+), 18 deletions(-) diff --git a/spinifex/gateway/auth.go b/spinifex/gateway/auth.go index 6e01d7470..11c901e5f 100644 --- a/spinifex/gateway/auth.go +++ b/spinifex/gateway/auth.go @@ -308,10 +308,7 @@ func canonicalHeaderName(header string) string { // writeSigV4Error writes an EC2-compatible XML error response for authentication failures. func (gw *GatewayConfig) writeSigV4Error(w http.ResponseWriter, r *http.Request, errorCode string) { - requestID := r.Header.Get("X-Amz-Request-Id") - if requestID == "" { - requestID = uuid.NewString() - } + requestID := uuid.NewString() errorMsg, exists := awserrors.ErrorLookup[errorCode] if !exists { diff --git a/spinifex/gateway/auth_test.go b/spinifex/gateway/auth_test.go index 68dffd944..28fc8292e 100644 --- a/spinifex/gateway/auth_test.go +++ b/spinifex/gateway/auth_test.go @@ -1012,7 +1012,7 @@ func TestWriteSigV4Error_ResponseFormat(t *testing.T) { } } -func TestWriteSigV4Error_PreservesRequestID(t *testing.T) { +func TestWriteSigV4Error_IgnoresClientRequestID(t *testing.T) { gw := &GatewayConfig{DisableLogging: true, Region: testRegion} w := httptest.NewRecorder() @@ -1022,8 +1022,11 @@ func TestWriteSigV4Error_PreservesRequestID(t *testing.T) { gw.writeSigV4Error(w, req, awserrors.ErrorIncompleteSignature) body, _ := io.ReadAll(w.Result().Body) - if !strings.Contains(string(body), "test-request-id-123") { - t.Errorf("Expected preserved request ID, got: %s", string(body)) + if strings.Contains(string(body), "test-request-id-123") { + t.Errorf("Expected server-generated request ID, but client ID was used: %s", string(body)) + } + if !strings.Contains(string(body), "") { + t.Errorf("Expected RequestID in response, got: %s", string(body)) } } diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 8548c0ced..50ce6780e 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -231,11 +231,8 @@ func (gw *GatewayConfig) ErrorHandler(w http.ResponseWriter, r *http.Request, er svc, _ := gw.GetService(r) slog.Debug("ErrorHandler", "service", svc, "error", err.Error()) - // Get the request ID + // Generate a server-side request ID — never trust client-provided values var requestId = uuid.NewString() - if rid := r.Header.Get("X-Amz-Request-Id"); rid != "" { - requestId = rid - } var errorMsg = awserrors.ErrorMessage{} diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index 0d0768f79..3ef77901f 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -230,7 +230,7 @@ func TestErrorHandler_EC2Service(t *testing.T) { assert.NotContains(t, xmlStr, "") } -func TestErrorHandler_UsesRequestIDHeader(t *testing.T) { +func TestErrorHandler_IgnoresClientRequestID(t *testing.T) { gw := &GatewayConfig{DisableLogging: true} handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -244,7 +244,8 @@ func TestErrorHandler_UsesRequestIDHeader(t *testing.T) { resp := doRequest(handler, req) body, _ := io.ReadAll(resp.Body) - assert.Contains(t, string(body), "custom-req-id-123") + assert.NotContains(t, string(body), "custom-req-id-123") + assert.Contains(t, string(body), "") } func TestErrorHandler_ContentTypeXML(t *testing.T) { diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 759a76933..6c91aa116 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "slices" "strings" "time" @@ -325,6 +326,11 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(input *ec2.AuthorizeSecur // Convert AWS IpPermissions to SGRules newRules := ipPermissionsToSGRules(input.IpPermissions) + for _, nr := range newRules { + if slices.Contains(record.IngressRules, nr) { + return nil, errors.New(awserrors.ErrorInvalidPermissionDuplicate) + } + } record.IngressRules = append(record.IngressRules, newRules...) data, err := json.Marshal(record) @@ -370,6 +376,11 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(input *ec2.AuthorizeSecuri } newRules := ipPermissionsToSGRules(input.IpPermissions) + for _, nr := range newRules { + if slices.Contains(record.EgressRules, nr) { + return nil, errors.New(awserrors.ErrorInvalidPermissionDuplicate) + } + } record.EgressRules = append(record.EgressRules, newRules...) data, err := json.Marshal(record) diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index e31769d7f..615655ce0 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -463,10 +463,8 @@ func (s *IAMServiceImpl) DeleteAccessKey(accountID string, input *iam.DeleteAcce return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) } - if err := s.accessKeysBucket.Delete(accessKeyID); err != nil { - return nil, fmt.Errorf("delete access key: %w", err) - } - + // Update user record first, then delete the key. This avoids orphaning the + // reference if a crash occurs between the two operations. user.AccessKeys = remaining userData, err := json.Marshal(user) if err != nil { @@ -477,6 +475,10 @@ func (s *IAMServiceImpl) DeleteAccessKey(accountID string, input *iam.DeleteAcce return nil, fmt.Errorf("update user: %w", err) } + if err := s.accessKeysBucket.Delete(accessKeyID); err != nil { + return nil, fmt.Errorf("delete access key: %w", err) + } + slog.Info("IAM access key deleted", "accountID", accountID, "userName", userName, "accessKeyID", accessKeyID) return &iam.DeleteAccessKeyOutput{}, nil } diff --git a/spinifex/utils/nats.go b/spinifex/utils/nats.go index 9c90031b9..06f62057d 100644 --- a/spinifex/utils/nats.go +++ b/spinifex/utils/nats.go @@ -77,6 +77,17 @@ func NATSRequest[Out any](conn *nats.Conn, subject string, input any, timeout ti return &output, nil } +const ( + // maxScatterGatherResponseSize is the maximum allowed size for a single + // scatter-gather response payload (10 MB). Responses exceeding this are + // skipped to prevent OOM from buggy nodes. + maxScatterGatherResponseSize = 10 * 1024 * 1024 + + // maxScatterGatherUnboundedResponses is the hard cap on responses collected + // when expectedNodes is 0 (unbounded fan-out). + maxScatterGatherUnboundedResponses = 256 +) + // NATSScatterGather publishes a fan-out request and collects responses from // multiple nodes. It returns the first successful (non-error) response. Error // payloads from individual nodes are skipped. If all responses are errors, the @@ -108,8 +119,13 @@ func NATSScatterGather[Out any](conn *nats.Conn, subject string, input any, time responsesReceived := 0 var lastErr error + maxResponses := maxScatterGatherUnboundedResponses + if expectedNodes > 0 { + maxResponses = expectedNodes + } + for time.Now().Before(deadline) { - if expectedNodes > 0 && responsesReceived >= expectedNodes { + if responsesReceived >= maxResponses { break } @@ -126,6 +142,11 @@ func NATSScatterGather[Out any](conn *nats.Conn, subject string, input any, time return nil, fmt.Errorf("scatter-gather receive error: %w", err) } + if len(msg.Data) > maxScatterGatherResponseSize { + slog.Warn("ScatterGather: skipping oversized response", "subject", subject, "size", len(msg.Data)) + continue + } + responsesReceived++ responseError, err := ValidateErrorPayload(msg.Data)