diff --git a/test/s3/iam/s3_iam_distributed_test.go b/test/s3/iam/s3_iam_distributed_test.go index acb1e61f4..545a56bcb 100644 --- a/test/s3/iam/s3_iam_distributed_test.go +++ b/test/s3/iam/s3_iam_distributed_test.go @@ -132,7 +132,7 @@ func TestS3IAMDistributedTests(t *testing.T) { var lastErr error for attempt := 0; attempt <= maxRetries; attempt++ { if attempt > 0 { - time.Sleep(retryDelay * time.Duration(attempt)) // Exponential backoff + time.Sleep(retryDelay * time.Duration(attempt)) // Linear backoff } lastErr = operation() diff --git a/test/s3/iam/s3_iam_framework.go b/test/s3/iam/s3_iam_framework.go index 622d9ddd8..ceacce23a 100644 --- a/test/s3/iam/s3_iam_framework.go +++ b/test/s3/iam/s3_iam_framework.go @@ -390,9 +390,8 @@ func (f *S3IAMTestFramework) generateSTSSessionToken(username, roleName string, return "", err } - // Note: In a real implementation, we would also create the session in the STS session store - // For now, we'll rely on the fact that if JWT validation passes, the session should be considered valid - // This is a limitation of our current testing approach + // The generated JWT is self-contained and includes all necessary session information. + // The stateless design of the STS service means no external session storage is required. return tokenString, nil } diff --git a/weed/iam/integration/role_store.go b/weed/iam/integration/role_store.go index 295115be4..3e52e2afc 100644 --- a/weed/iam/integration/role_store.go +++ b/weed/iam/integration/role_store.go @@ -317,11 +317,21 @@ func (f *FilerRoleStore) DeleteRole(ctx context.Context, filerAddress string, ro } glog.V(3).Infof("Deleting role %s", roleName) - _, err := client.DeleteEntry(ctx, request) + resp, err := client.DeleteEntry(ctx, request) if err != nil { + if strings.Contains(err.Error(), "not found") { + return nil // Idempotent: deletion of non-existent role is successful + } return fmt.Errorf("failed to delete role %s: %v", roleName, err) } + if resp.Error != "" { + if strings.Contains(resp.Error, "not found") { + return nil // Idempotent: deletion of non-existent role is successful + } + return fmt.Errorf("failed to delete role %s: %s", roleName, resp.Error) + } + return nil }) } diff --git a/weed/iam/sts/provider_factory.go b/weed/iam/sts/provider_factory.go index 3b394f7af..189affa33 100644 --- a/weed/iam/sts/provider_factory.go +++ b/weed/iam/sts/provider_factory.go @@ -133,23 +133,29 @@ func (f *ProviderFactory) convertToOIDCConfig(configMap map[string]interface{}) // Convert scopes array if scopesInterface, ok := configMap[ConfigFieldScopes]; ok { - if scopes, err := f.convertToStringSlice(scopesInterface); err == nil { - config.Scopes = scopes + scopes, err := f.convertToStringSlice(scopesInterface) + if err != nil { + return nil, fmt.Errorf("failed to convert scopes: %w", err) } + config.Scopes = scopes } // Convert claims mapping if claimsMapInterface, ok := configMap["claimsMapping"]; ok { - if claimsMap, err := f.convertToStringMap(claimsMapInterface); err == nil { - config.ClaimsMapping = claimsMap + claimsMap, err := f.convertToStringMap(claimsMapInterface) + if err != nil { + return nil, fmt.Errorf("failed to convert claimsMapping: %w", err) } + config.ClaimsMapping = claimsMap } // Convert role mapping if roleMappingInterface, ok := configMap["roleMapping"]; ok { - if roleMapping, err := f.convertToRoleMapping(roleMappingInterface); err == nil { - config.RoleMapping = roleMapping + roleMapping, err := f.convertToRoleMapping(roleMappingInterface) + if err != nil { + return nil, fmt.Errorf("failed to convert roleMapping: %w", err) } + config.RoleMapping = roleMapping } glog.V(3).Infof("Converted OIDC config: issuer=%s, clientId=%s, jwksUri=%s", diff --git a/weed/iam/sts/provider_factory_test.go b/weed/iam/sts/provider_factory_test.go index 61b9f44e6..726833c09 100644 --- a/weed/iam/sts/provider_factory_test.go +++ b/weed/iam/sts/provider_factory_test.go @@ -198,6 +198,64 @@ func TestProviderFactory_ConvertToStringSlice(t *testing.T) { }) } +func TestProviderFactory_ConfigConversionErrors(t *testing.T) { + factory := NewProviderFactory() + + t.Run("invalid scopes type", func(t *testing.T) { + config := &ProviderConfig{ + Name: "invalid-scopes", + Type: "oidc", + Enabled: true, + Config: map[string]interface{}{ + "issuer": "https://test-issuer.com", + "clientId": "test-client", + "scopes": "invalid-not-array", // Should be array + }, + } + + provider, err := factory.CreateProvider(config) + assert.Error(t, err) + assert.Nil(t, provider) + assert.Contains(t, err.Error(), "failed to convert scopes") + }) + + t.Run("invalid claimsMapping type", func(t *testing.T) { + config := &ProviderConfig{ + Name: "invalid-claims", + Type: "oidc", + Enabled: true, + Config: map[string]interface{}{ + "issuer": "https://test-issuer.com", + "clientId": "test-client", + "claimsMapping": "invalid-not-map", // Should be map + }, + } + + provider, err := factory.CreateProvider(config) + assert.Error(t, err) + assert.Nil(t, provider) + assert.Contains(t, err.Error(), "failed to convert claimsMapping") + }) + + t.Run("invalid roleMapping type", func(t *testing.T) { + config := &ProviderConfig{ + Name: "invalid-roles", + Type: "oidc", + Enabled: true, + Config: map[string]interface{}{ + "issuer": "https://test-issuer.com", + "clientId": "test-client", + "roleMapping": "invalid-not-map", // Should be map + }, + } + + provider, err := factory.CreateProvider(config) + assert.Error(t, err) + assert.Nil(t, provider) + assert.Contains(t, err.Error(), "failed to convert roleMapping") + }) +} + func TestProviderFactory_ConvertToStringMap(t *testing.T) { factory := NewProviderFactory() diff --git a/weed/iam/sts/sts_service.go b/weed/iam/sts/sts_service.go index 7b523b8d2..0bd8229b0 100644 --- a/weed/iam/sts/sts_service.go +++ b/weed/iam/sts/sts_service.go @@ -641,8 +641,13 @@ func (s *STSService) validateRoleAssumptionForWebIdentity(ctx context.Context, r return fmt.Errorf("invalid role ARN format: got %s, expected format: %s*", roleArn, expectedPrefix) } - // For testing, reject non-existent roles + // Extract role name and validate ARN format roleName := extractRoleNameFromArn(roleArn) + if roleName == "" { + return fmt.Errorf("invalid role ARN format: %s", roleArn) + } + + // For testing, reject non-existent roles if roleName == "NonExistentRole" { return fmt.Errorf("role does not exist: %s", roleName) } @@ -678,8 +683,13 @@ func (s *STSService) validateRoleAssumptionForCredentials(ctx context.Context, r return fmt.Errorf("invalid role ARN format: got %s, expected format: %s*", roleArn, expectedPrefix) } - // For testing, reject non-existent roles + // Extract role name and validate ARN format roleName := extractRoleNameFromArn(roleArn) + if roleName == "" { + return fmt.Errorf("invalid role ARN format: %s", roleArn) + } + + // For testing, reject non-existent roles if roleName == "NonExistentRole" { return fmt.Errorf("role does not exist: %s", roleName) } diff --git a/weed/iam/sts/token_utils.go b/weed/iam/sts/token_utils.go index 12cddfb1c..3fa610f3a 100644 --- a/weed/iam/sts/token_utils.go +++ b/weed/iam/sts/token_utils.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/hex" "fmt" + "strings" "time" "github.com/golang-jwt/jwt/v5" @@ -207,15 +208,20 @@ func GenerateSessionId() (string, error) { func GenerateAssumedRoleArn(roleArn, sessionName string) string { // Convert role ARN to assumed role user ARN // arn:seaweed:iam::role/RoleName -> arn:seaweed:sts::assumed-role/RoleName/SessionName - return fmt.Sprintf("arn:seaweed:sts::assumed-role/%s/%s", extractRoleNameFromArn(roleArn), sessionName) + roleName := extractRoleNameFromArn(roleArn) + if roleName == "" { + // This should not happen if validation is done properly upstream + return fmt.Sprintf("arn:seaweed:sts::assumed-role/INVALID-ARN/%s", sessionName) + } + return fmt.Sprintf("arn:seaweed:sts::assumed-role/%s/%s", roleName, sessionName) } // extractRoleNameFromArn extracts the role name from a role ARN func extractRoleNameFromArn(roleArn string) string { // Simple extraction for arn:seaweed:iam::role/RoleName prefix := "arn:seaweed:iam::role/" - if len(roleArn) > len(prefix) && roleArn[:len(prefix)] == prefix { + if strings.HasPrefix(roleArn, prefix) && len(roleArn) > len(prefix) { return roleArn[len(prefix):] } - return "UnknownRole" + return "" } diff --git a/weed/iam/utils/arn_utils.go b/weed/iam/utils/arn_utils.go index f47259b48..8345886a5 100644 --- a/weed/iam/utils/arn_utils.go +++ b/weed/iam/utils/arn_utils.go @@ -23,6 +23,7 @@ func ExtractRoleNameFromPrincipal(principal string) string { return principal[len(iamPrefix):] } - // For backwards compatibility, return original principal if no recognized format - return principal + // Return empty string to signal invalid ARN format + // This allows callers to handle the error explicitly instead of masking it + return "" } diff --git a/weed/s3api/s3_iam_simple_test.go b/weed/s3api/s3_iam_simple_test.go index ced26c398..1673b0819 100644 --- a/weed/s3api/s3_iam_simple_test.go +++ b/weed/s3api/s3_iam_simple_test.go @@ -451,7 +451,7 @@ func TestExtractRoleNameFromPrincipal(t *testing.T) { { name: "invalid format", principal: "invalid-principal", - expected: "invalid-principal", // Returns original on failure + expected: "", // Returns empty string to signal invalid format }, { name: "missing session name",