From 89810dcea383ef88bf5561d3d2c2f382670606ba Mon Sep 17 00:00:00 2001 From: chrislu Date: Tue, 26 Aug 2025 21:20:19 -0700 Subject: [PATCH] address comments --- test/s3/iam/STS_DISTRIBUTED.md | 3 +- weed/iam/sts/sts_service.go | 10 ++-- weed/s3api/s3_iam_middleware.go | 29 +++-------- weed/s3api/s3_token_differentiation_test.go | 57 +++++++++++++-------- 4 files changed, 48 insertions(+), 51 deletions(-) diff --git a/test/s3/iam/STS_DISTRIBUTED.md b/test/s3/iam/STS_DISTRIBUTED.md index 1d58e866e..8e138cec7 100644 --- a/test/s3/iam/STS_DISTRIBUTED.md +++ b/test/s3/iam/STS_DISTRIBUTED.md @@ -264,8 +264,7 @@ services: ↓ 6. STS generates temporary credentials - Creates temporary access key, secret key, session token - - Session token is signed JWT with session ID - - Stores session info in distributed session store + - Session token is signed JWT with all session information embedded (stateless) ↓ 7. User receives temporary credentials { diff --git a/weed/iam/sts/sts_service.go b/weed/iam/sts/sts_service.go index bff59f9e3..94e1e4478 100644 --- a/weed/iam/sts/sts_service.go +++ b/weed/iam/sts/sts_service.go @@ -15,7 +15,7 @@ import ( // in JWT tokens, eliminating the need for session storage and enabling true // distributed operation without shared state type STSService struct { - config *STSConfig + Config *STSConfig // Public for access by other components initialized bool providers map[string]providers.IdentityProvider issuerToProvider map[string]providers.IdentityProvider // Efficient issuer-based provider lookup @@ -199,7 +199,7 @@ func (s *STSService) Initialize(config *STSConfig) error { return fmt.Errorf("invalid STS configuration: %w", err) } - s.config = config + s.Config = config // Initialize token generator for stateless JWT operations s.tokenGenerator = NewTokenGenerator(config.SigningKey, config.Issuer) @@ -366,7 +366,7 @@ func (s *STSService) AssumeRoleWithWebIdentity(ctx context.Context, request *Ass } // Create rich JWT claims with all session information - sessionClaims := NewSTSSessionClaims(sessionId, s.config.Issuer, expiresAt). + sessionClaims := NewSTSSessionClaims(sessionId, s.Config.Issuer, expiresAt). WithSessionName(request.RoleSessionName). WithRoleInfo(request.RoleArn, assumedRoleUser.Arn, assumedRoleUser.Arn). WithIdentityProvider(provider.Name(), externalIdentity.UserID, ""). @@ -445,7 +445,7 @@ func (s *STSService) AssumeRoleWithCredentials(ctx context.Context, request *Ass } // Create rich JWT claims with all session information - sessionClaims := NewSTSSessionClaims(sessionId, s.config.Issuer, expiresAt). + sessionClaims := NewSTSSessionClaims(sessionId, s.Config.Issuer, expiresAt). WithSessionName(request.RoleSessionName). WithRoleInfo(request.RoleArn, assumedRoleUser.Arn, assumedRoleUser.Arn). WithIdentityProvider(provider.Name(), externalIdentity.UserID, ""). @@ -621,7 +621,7 @@ func (s *STSService) calculateSessionDuration(durationSeconds *int64) time.Durat } // Use default from config - return s.config.TokenDuration + return s.Config.TokenDuration } // extractSessionIdFromToken extracts session ID from JWT session token diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index 3afbbbf7c..50fa00db3 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -532,31 +532,14 @@ func getProviderNames(providers map[string]providers.IdentityProvider) []string } // isSTSIssuer determines if an issuer belongs to the STS service -// This provides more robust token type detection than checking for role claims +// Uses exact match against configured STS issuer for security and correctness func (s3iam *S3IAMIntegration) isSTSIssuer(issuer string) bool { - if s3iam.stsService == nil { + if s3iam.stsService == nil || s3iam.stsService.Config == nil { return false } - // Get the STS configuration to check the issuer - // For now, we'll use a simple heuristic - STS issuers typically contain "sts" or are internal - // In a full implementation, this could check against configured STS issuer values - - // Check if issuer contains common STS patterns - stsPatterns := []string{ - "sts", - "seaweedfs-sts", - "seaweed", - "localhost", // For development/testing - } - - lowerIssuer := strings.ToLower(issuer) - for _, pattern := range stsPatterns { - if strings.Contains(lowerIssuer, pattern) { - return true - } - } - - // If no pattern matches, assume it's an external OIDC issuer - return false + // Directly compare with the configured STS issuer for exact match + // This prevents false positives from external OIDC providers that might + // contain STS-related keywords in their issuer URLs + return issuer == s3iam.stsService.Config.Issuer } diff --git a/weed/s3api/s3_token_differentiation_test.go b/weed/s3api/s3_token_differentiation_test.go index 247353ee6..0a8642bda 100644 --- a/weed/s3api/s3_token_differentiation_test.go +++ b/weed/s3api/s3_token_differentiation_test.go @@ -1,7 +1,9 @@ package s3api import ( + "strings" "testing" + "time" "github.com/seaweedfs/seaweedfs/weed/iam/integration" "github.com/seaweedfs/seaweedfs/weed/iam/sts" @@ -9,10 +11,23 @@ import ( ) func TestS3IAMIntegration_isSTSIssuer(t *testing.T) { - // Create test STS service + // Create test STS service with configuration stsService := sts.NewSTSService() - // Create S3IAM integration with STS service + // Set up STS configuration with a specific issuer + testIssuer := "https://seaweedfs-prod.company.com/sts" + stsConfig := &sts.STSConfig{ + Issuer: testIssuer, + SigningKey: []byte("test-signing-key-32-characters-long"), + TokenDuration: time.Hour, + MaxSessionLength: 12 * time.Hour, // Required field + } + + // Initialize STS service with config (this sets the Config field) + err := stsService.Initialize(stsConfig) + assert.NoError(t, err) + + // Create S3IAM integration with configured STS service s3iam := &S3IAMIntegration{ iamManager: &integration.IAMManager{}, // Mock stsService: stsService, @@ -25,33 +40,33 @@ func TestS3IAMIntegration_isSTSIssuer(t *testing.T) { issuer string expected bool }{ - // STS issuers (should return true) + // Only exact match should return true { - name: "explicit STS issuer", - issuer: "seaweedfs-sts", + name: "exact match with configured issuer", + issuer: testIssuer, expected: true, }, + // All other issuers should return false (exact matching) { - name: "STS in issuer name", - issuer: "https://mycompany-sts.example.com", - expected: true, + name: "similar but not exact issuer", + issuer: "https://seaweedfs-prod.company.com/sts2", + expected: false, }, { - name: "seaweed in issuer name", - issuer: "https://seaweed-prod.company.com", - expected: true, + name: "substring of configured issuer", + issuer: "seaweedfs-prod.company.com", + expected: false, }, { - name: "localhost for development", - issuer: "http://localhost:9333/sts", - expected: true, + name: "contains configured issuer as substring", + issuer: "prefix-" + testIssuer + "-suffix", + expected: false, }, { - name: "case insensitive STS", - issuer: "SEAWEEDFS-STS-PROD", - expected: true, + name: "case sensitive - different case", + issuer: strings.ToUpper(testIssuer), + expected: false, }, - // External OIDC issuers (should return false) { name: "Google OIDC", issuer: "https://accounts.google.com", @@ -73,8 +88,8 @@ func TestS3IAMIntegration_isSTSIssuer(t *testing.T) { expected: false, }, { - name: "Generic OIDC provider", - issuer: "https://oidc.provider.com", + name: "Empty string", + issuer: "", expected: false, }, } @@ -82,7 +97,7 @@ func TestS3IAMIntegration_isSTSIssuer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := s3iam.isSTSIssuer(tt.issuer) - assert.Equal(t, tt.expected, result, "isSTSIssuer should correctly identify issuer type") + assert.Equal(t, tt.expected, result, "isSTSIssuer should use exact matching against configured issuer") }) } }