diff --git a/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml b/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml index 6a551a6c9..045b95c2e 100644 --- a/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml +++ b/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml @@ -176,6 +176,9 @@ spec: {{- if $volume.dataCenter }} -dataCenter={{ $volume.dataCenter }} \ {{- end }} + {{- if $volume.id }} + -id={{ $volume.id }} \ + {{- end }} -ip.bind={{ $volume.ipBind }} \ -readMode={{ $volume.readMode }} \ {{- if $volume.whiteList }} diff --git a/k8s/charts/seaweedfs/values.yaml b/k8s/charts/seaweedfs/values.yaml index dd14f1ca0..c4fd3a841 100644 --- a/k8s/charts/seaweedfs/values.yaml +++ b/k8s/charts/seaweedfs/values.yaml @@ -401,6 +401,10 @@ volume: # Volume server's rack name rack: null + # Stable identifier for the volume server, independent of IP address + # Useful for Kubernetes environments with hostPath volumes to maintain stable identity + id: null + # Volume server's data center name dataCenter: null diff --git a/weed/iam/helpers.go b/weed/iam/helpers.go index e05219149..ef94940af 100644 --- a/weed/iam/helpers.go +++ b/weed/iam/helpers.go @@ -68,28 +68,92 @@ func StringSlicesEqual(a, b []string) bool { return true } +// fineGrainedActionMap maps S3 IAM action names to internal S3 action constants. +// Supports both prefixed (e.g., "s3:DeleteObject") and unprefixed (e.g., "DeleteObject") formats. +// Populated in init() to avoid duplication of prefixed/unprefixed variants. +var fineGrainedActionMap = map[string]string{ + // Coarse-grained actions (populated statically) + StatementActionAdmin: s3_constants.ACTION_ADMIN, + StatementActionWrite: s3_constants.ACTION_WRITE, + StatementActionWriteAcp: s3_constants.ACTION_WRITE_ACP, + StatementActionRead: s3_constants.ACTION_READ, + StatementActionReadAcp: s3_constants.ACTION_READ_ACP, + StatementActionList: s3_constants.ACTION_LIST, + StatementActionTagging: s3_constants.ACTION_TAGGING, + StatementActionDelete: s3_constants.ACTION_DELETE_BUCKET, +} + +// baseS3ActionMap defines the base S3 actions that will be populated with both +// prefixed (s3:Action) and unprefixed (Action) variants in init(). +var baseS3ActionMap = map[string]string{ + // Object operations + "DeleteObject": s3_constants.ACTION_WRITE, + "PutObject": s3_constants.ACTION_WRITE, + "GetObject": s3_constants.ACTION_READ, + "DeleteObjectVersion": s3_constants.ACTION_WRITE, + "GetObjectVersion": s3_constants.ACTION_READ, + // Tagging operations + "GetObjectTagging": s3_constants.ACTION_TAGGING, + "GetObjectVersionTagging": s3_constants.ACTION_TAGGING, + "PutObjectTagging": s3_constants.ACTION_TAGGING, + "DeleteObjectTagging": s3_constants.ACTION_TAGGING, + "GetBucketTagging": s3_constants.ACTION_TAGGING, + "PutBucketTagging": s3_constants.ACTION_TAGGING, + "DeleteBucketTagging": s3_constants.ACTION_TAGGING, + // ACL operations + "PutObjectAcl": s3_constants.ACTION_WRITE_ACP, + "GetObjectAcl": s3_constants.ACTION_READ_ACP, + "GetObjectVersionAcl": s3_constants.ACTION_READ_ACP, + "PutBucketAcl": s3_constants.ACTION_WRITE_ACP, + "GetBucketAcl": s3_constants.ACTION_READ_ACP, + // Bucket operations + "DeleteBucket": s3_constants.ACTION_DELETE_BUCKET, + "DeleteBucketPolicy": s3_constants.ACTION_ADMIN, + "ListBucket": s3_constants.ACTION_LIST, + "ListBucketVersions": s3_constants.ACTION_LIST, + "ListAllMyBuckets": s3_constants.ACTION_LIST, + "GetBucketLocation": s3_constants.ACTION_READ, + "GetBucketVersioning": s3_constants.ACTION_READ, + "PutBucketVersioning": s3_constants.ACTION_WRITE, + "GetBucketCors": s3_constants.ACTION_READ, + "PutBucketCors": s3_constants.ACTION_WRITE, + "DeleteBucketCors": s3_constants.ACTION_WRITE, + "GetBucketNotification": s3_constants.ACTION_READ, + "PutBucketNotification": s3_constants.ACTION_WRITE, + "GetBucketObjectLockConfiguration": s3_constants.ACTION_READ, + "PutBucketObjectLockConfiguration": s3_constants.ACTION_WRITE, + // Multipart upload operations + "CreateMultipartUpload": s3_constants.ACTION_WRITE, + "UploadPart": s3_constants.ACTION_WRITE, + "CompleteMultipartUpload": s3_constants.ACTION_WRITE, + "AbortMultipartUpload": s3_constants.ACTION_WRITE, + "ListMultipartUploads": s3_constants.ACTION_WRITE, + "ListParts": s3_constants.ACTION_WRITE, + // Retention and legal hold operations + "GetObjectRetention": s3_constants.ACTION_READ, + "PutObjectRetention": s3_constants.ACTION_WRITE, + "GetObjectLegalHold": s3_constants.ACTION_READ, + "PutObjectLegalHold": s3_constants.ACTION_WRITE, + "BypassGovernanceRetention": s3_constants.ACTION_WRITE, +} + +func init() { + // Populate both prefixed and unprefixed variants for all base S3 actions. + // This avoids duplication and makes it easy to add new actions in one place. + for action, constant := range baseS3ActionMap { + fineGrainedActionMap[action] = constant // unprefixed: "DeleteObject" + fineGrainedActionMap["s3:"+action] = constant // prefixed: "s3:DeleteObject" + } +} + // MapToStatementAction converts a policy statement action to an S3 action constant. +// It handles both coarse-grained statement actions (e.g., "Put*", "Get*") and +// fine-grained S3 actions (e.g., "s3:DeleteObject", "s3:PutObject") via exact lookup. func MapToStatementAction(action string) string { - switch action { - case StatementActionAdmin: - return s3_constants.ACTION_ADMIN - case StatementActionWrite: - return s3_constants.ACTION_WRITE - case StatementActionWriteAcp: - return s3_constants.ACTION_WRITE_ACP - case StatementActionRead: - return s3_constants.ACTION_READ - case StatementActionReadAcp: - return s3_constants.ACTION_READ_ACP - case StatementActionList: - return s3_constants.ACTION_LIST - case StatementActionTagging: - return s3_constants.ACTION_TAGGING - case StatementActionDelete: - return s3_constants.ACTION_DELETE_BUCKET - default: - return "" + if val, ok := fineGrainedActionMap[action]; ok { + return val } + return "" } // MapToIdentitiesAction converts an S3 action constant to a policy statement action. @@ -123,5 +187,3 @@ func MaskAccessKey(accessKeyId string) string { } return accessKeyId } - - diff --git a/weed/iam/helpers_test.go b/weed/iam/helpers_test.go index 6b6744475..f2da389c3 100644 --- a/weed/iam/helpers_test.go +++ b/weed/iam/helpers_test.go @@ -88,12 +88,25 @@ func TestMapToStatementAction(t *testing.T) { {StatementActionRead, s3_constants.ACTION_READ}, {StatementActionList, s3_constants.ACTION_LIST}, {StatementActionDelete, s3_constants.ACTION_DELETE_BUCKET}, + // Test fine-grained S3 action mappings (Issue #7864) + {"DeleteObject", s3_constants.ACTION_WRITE}, + {"s3:DeleteObject", s3_constants.ACTION_WRITE}, + {"PutObject", s3_constants.ACTION_WRITE}, + {"s3:PutObject", s3_constants.ACTION_WRITE}, + {"GetObject", s3_constants.ACTION_READ}, + {"s3:GetObject", s3_constants.ACTION_READ}, + {"ListBucket", s3_constants.ACTION_LIST}, + {"s3:ListBucket", s3_constants.ACTION_LIST}, + {"PutObjectAcl", s3_constants.ACTION_WRITE_ACP}, + {"s3:PutObjectAcl", s3_constants.ACTION_WRITE_ACP}, + {"GetObjectAcl", s3_constants.ACTION_READ_ACP}, + {"s3:GetObjectAcl", s3_constants.ACTION_READ_ACP}, {"unknown", ""}, } for _, test := range tests { result := MapToStatementAction(test.input) - assert.Equal(t, test.expected, result) + assert.Equal(t, test.expected, result, "Failed for input: %s", test.input) } } @@ -132,5 +145,3 @@ func TestMaskAccessKey(t *testing.T) { assert.Equal(t, test.expected, result) } } - - diff --git a/weed/s3api/policy_engine/engine_test.go b/weed/s3api/policy_engine/engine_test.go index 4de537ac1..f8297b56b 100644 --- a/weed/s3api/policy_engine/engine_test.go +++ b/weed/s3api/policy_engine/engine_test.go @@ -232,7 +232,7 @@ func TestConvertIdentityToPolicy(t *testing.T) { "Admin:bucket2", } - policy, err := ConvertIdentityToPolicy(identityActions, "bucket1") + policy, err := ConvertIdentityToPolicy(identityActions) if err != nil { t.Fatalf("Failed to convert identity to policy: %v", err) } @@ -252,13 +252,17 @@ func TestConvertIdentityToPolicy(t *testing.T) { } actions := normalizeToStringSlice(stmt.Action) - if len(actions) != 3 { - t.Errorf("Expected 3 read actions, got %d", len(actions)) + // Read action now includes: GetObject, GetObjectVersion, ListBucket, ListBucketVersions, + // GetObjectAcl, GetObjectVersionAcl, GetObjectTagging, GetObjectVersionTagging, + // GetBucketLocation, GetBucketVersioning, GetBucketAcl, GetBucketCors, GetBucketTagging, GetBucketNotification + if len(actions) != 14 { + t.Errorf("Expected 14 read actions, got %d: %v", len(actions), actions) } resources := normalizeToStringSlice(stmt.Resource) + // Read action now includes both bucket ARN (for ListBucket*) and object ARN (for GetObject*) if len(resources) != 2 { - t.Errorf("Expected 2 resources, got %d", len(resources)) + t.Errorf("Expected 2 resources (bucket and bucket/*), got %d: %v", len(resources), resources) } } @@ -797,8 +801,8 @@ func TestExistingObjectTagCondition(t *testing.T) { t.Run(tt.name, func(t *testing.T) { args := &PolicyEvaluationArgs{ Action: "s3:GetObject", - Resource: "arn:aws:s3:::test-bucket/test-object", - Principal: "*", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", ObjectEntry: tagsToEntry(tt.objectTags), } @@ -874,8 +878,8 @@ func TestExistingObjectTagConditionMultipleTags(t *testing.T) { t.Run(tt.name, func(t *testing.T) { args := &PolicyEvaluationArgs{ Action: "s3:GetObject", - Resource: "arn:aws:s3:::test-bucket/test-object", - Principal: "*", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", ObjectEntry: tagsToEntry(tt.objectTags), } @@ -946,8 +950,8 @@ func TestExistingObjectTagDenyPolicy(t *testing.T) { t.Run(tt.name, func(t *testing.T) { args := &PolicyEvaluationArgs{ Action: "s3:GetObject", - Resource: "arn:aws:s3:::test-bucket/test-object", - Principal: "*", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", ObjectEntry: tagsToEntry(tt.objectTags), } diff --git a/weed/s3api/policy_engine/examples.go b/weed/s3api/policy_engine/examples.go index 6f14127f3..c4cea6512 100644 --- a/weed/s3api/policy_engine/examples.go +++ b/weed/s3api/policy_engine/examples.go @@ -391,7 +391,7 @@ func ExampleLegacyIntegration() { } // Convert to policy - policy, err := ConvertIdentityToPolicy(legacyActions, "bucket1") + policy, err := ConvertIdentityToPolicy(legacyActions) if err != nil { fmt.Printf("Error converting identity to policy: %v\n", err) return diff --git a/weed/s3api/policy_engine/integration.go b/weed/s3api/policy_engine/integration.go index 17bcec112..8c9495088 100644 --- a/weed/s3api/policy_engine/integration.go +++ b/weed/s3api/policy_engine/integration.go @@ -123,12 +123,70 @@ func (p *PolicyBackedIAM) evaluateUsingPolicyConversion(action, bucketName, obje return false } +// extractBucketAndPrefix extracts bucket name and prefix from a resource pattern. +// Examples: +// +// "bucket" -> bucket="bucket", prefix="" +// "bucket/*" -> bucket="bucket", prefix="" +// "bucket/prefix/*" -> bucket="bucket", prefix="prefix" +// "bucket/a/b/c/*" -> bucket="bucket", prefix="a/b/c" +func extractBucketAndPrefix(pattern string) (string, string) { + // Validate input + pattern = strings.TrimSpace(pattern) + if pattern == "" || pattern == "/" { + return "", "" + } + + // Remove trailing /* if present + pattern = strings.TrimSuffix(pattern, "/*") + + // Remove a single trailing slash to avoid empty path segments + if strings.HasSuffix(pattern, "/") { + pattern = pattern[:len(pattern)-1] + } + if pattern == "" { + return "", "" + } + + // Split on the first / + parts := strings.SplitN(pattern, "/", 2) + bucket := strings.TrimSpace(parts[0]) + if bucket == "" { + return "", "" + } + + if len(parts) == 1 { + // No slash, entire pattern is bucket + return bucket, "" + } + // Has slash, first part is bucket, rest is prefix + prefix := strings.Trim(parts[1], "/") + return bucket, prefix +} + +// buildObjectResourceArn generates ARNs for object-level access. +// It properly handles both bucket-level (all objects) and prefix-level access. +// Returns empty slice if bucket is invalid to prevent generating malformed ARNs. +func buildObjectResourceArn(resourcePattern string) []string { + bucket, prefix := extractBucketAndPrefix(resourcePattern) + // If bucket is empty, the pattern is invalid; avoid generating malformed ARNs + if bucket == "" { + return []string{} + } + if prefix != "" { + // Prefix-based access: restrict to objects under this prefix + return []string{fmt.Sprintf("arn:aws:s3:::%s/%s/*", bucket, prefix)} + } + // Bucket-level access: all objects in bucket + return []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} +} + // ConvertIdentityToPolicy converts a legacy identity action to an AWS policy -func ConvertIdentityToPolicy(identityActions []string, bucketName string) (*PolicyDocument, error) { +func ConvertIdentityToPolicy(identityActions []string) (*PolicyDocument, error) { statements := make([]PolicyStatement, 0) for _, action := range identityActions { - stmt, err := convertSingleAction(action, bucketName) + stmt, err := convertSingleAction(action) if err != nil { glog.Warningf("Failed to convert action %s: %v", action, err) continue @@ -148,8 +206,9 @@ func ConvertIdentityToPolicy(identityActions []string, bucketName string) (*Poli }, nil } -// convertSingleAction converts a single legacy action to a policy statement -func convertSingleAction(action, bucketName string) (*PolicyStatement, error) { +// convertSingleAction converts a single legacy action to a policy statement. +// action format: "ActionType:ResourcePattern" (e.g., "Write:bucket/prefix/*") +func convertSingleAction(action string) (*PolicyStatement, error) { parts := strings.Split(action, ":") if len(parts) != 2 { return nil, fmt.Errorf("invalid action format: %s", action) @@ -163,111 +222,158 @@ func convertSingleAction(action, bucketName string) (*PolicyStatement, error) { switch actionType { case "Read": - s3Actions = []string{"s3:GetObject", "s3:GetObjectVersion", "s3:ListBucket"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level read access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{ - fmt.Sprintf("arn:aws:s3:::%s", bucket), - fmt.Sprintf("arn:aws:s3:::%s/*", bucket), - } + // Read includes both object-level (GetObject, GetObjectAcl, GetObjectTagging, GetObjectVersions) + // and bucket-level operations (ListBucket, GetBucketLocation, GetBucketVersioning, GetBucketCors, etc.) + s3Actions = []string{ + "s3:GetObject", + "s3:GetObjectVersion", + "s3:GetObjectAcl", + "s3:GetObjectVersionAcl", + "s3:GetObjectTagging", + "s3:GetObjectVersionTagging", + "s3:ListBucket", + "s3:ListBucketVersions", + "s3:GetBucketLocation", + "s3:GetBucketVersioning", + "s3:GetBucketAcl", + "s3:GetBucketCors", + "s3:GetBucketTagging", + "s3:GetBucketNotification", + } + bucket, _ := extractBucketAndPrefix(resourcePattern) + objectResources := buildObjectResourceArn(resourcePattern) + // Include both bucket ARN (for ListBucket* and Get*Bucket operations) and object ARNs (for GetObject* operations) + if bucket != "" { + resources = append([]string{fmt.Sprintf("arn:aws:s3:::%s", bucket)}, objectResources...) } else { - // Bucket-level read access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + resources = objectResources } case "Write": - s3Actions = []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level write access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} + // Write includes object-level writes (PutObject, DeleteObject, PutObjectAcl, DeleteObjectVersion, DeleteObjectTagging, PutObjectTagging) + // and bucket-level writes (PutBucketVersioning, PutBucketCors, DeleteBucketCors, PutBucketAcl, PutBucketTagging, DeleteBucketTagging, PutBucketNotification) + // and multipart upload operations (AbortMultipartUpload, ListMultipartUploads, ListParts). + // ListMultipartUploads and ListParts are included because they are part of the multipart upload workflow + // and require Write permissions to be meaningful (no point listing uploads if you can't abort/complete them). + s3Actions = []string{ + "s3:PutObject", + "s3:PutObjectAcl", + "s3:PutObjectTagging", + "s3:DeleteObject", + "s3:DeleteObjectVersion", + "s3:DeleteObjectTagging", + "s3:AbortMultipartUpload", + "s3:ListMultipartUploads", + "s3:ListParts", + "s3:PutBucketAcl", + "s3:PutBucketCors", + "s3:PutBucketTagging", + "s3:PutBucketNotification", + "s3:PutBucketVersioning", + "s3:DeleteBucketTagging", + "s3:DeleteBucketCors", + } + bucket, _ := extractBucketAndPrefix(resourcePattern) + objectResources := buildObjectResourceArn(resourcePattern) + // Include bucket ARN so bucket-level write operations (e.g., PutBucketVersioning, PutBucketCors) + // have the correct resource, while still allowing object-level writes. + if bucket != "" { + resources = append([]string{fmt.Sprintf("arn:aws:s3:::%s", bucket)}, objectResources...) } else { - // Bucket-level write access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + resources = objectResources } case "Admin": s3Actions = []string{"s3:*"} - resources = []string{ - fmt.Sprintf("arn:aws:s3:::%s", resourcePattern), - fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern), + bucket, prefix := extractBucketAndPrefix(resourcePattern) + if bucket == "" { + // Invalid pattern, return error + return nil, fmt.Errorf("Admin action requires a valid bucket name") } - - case "List": - s3Actions = []string{"s3:ListBucket", "s3:ListBucketVersions"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level list access - extract bucket from "bucket/prefix/*" pattern - patternWithoutWildcard := strings.TrimSuffix(resourcePattern, "/*") - parts := strings.SplitN(patternWithoutWildcard, "/", 2) - bucket := parts[0] + if prefix != "" { + // Subpath admin access: restrict to objects under this prefix + resources = []string{ + fmt.Sprintf("arn:aws:s3:::%s", bucket), + fmt.Sprintf("arn:aws:s3:::%s/%s/*", bucket, prefix), + } + } else { + // Bucket-level admin access: full bucket permissions resources = []string{ fmt.Sprintf("arn:aws:s3:::%s", bucket), fmt.Sprintf("arn:aws:s3:::%s/*", bucket), } + } + + case "List": + // List includes bucket listing operations and also ListAllMyBuckets + s3Actions = []string{"s3:ListBucket", "s3:ListBucketVersions", "s3:ListAllMyBuckets"} + // ListBucket actions only require bucket ARN, not object-level ARNs + bucket, _ := extractBucketAndPrefix(resourcePattern) + if bucket != "" { + resources = []string{fmt.Sprintf("arn:aws:s3:::%s", bucket)} } else { - // Bucket-level list access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + // Invalid pattern, return empty resources to fail validation + resources = []string{} } case "Tagging": - s3Actions = []string{"s3:GetObjectTagging", "s3:PutObjectTagging", "s3:DeleteObjectTagging"} - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} + // Tagging includes both object-level and bucket-level tagging operations + s3Actions = []string{ + "s3:GetObjectTagging", + "s3:PutObjectTagging", + "s3:DeleteObjectTagging", + "s3:GetBucketTagging", + "s3:PutBucketTagging", + "s3:DeleteBucketTagging", + } + bucket, _ := extractBucketAndPrefix(resourcePattern) + objectResources := buildObjectResourceArn(resourcePattern) + // Include bucket ARN so bucket-level tagging operations have the correct resource + if bucket != "" { + resources = append([]string{fmt.Sprintf("arn:aws:s3:::%s", bucket)}, objectResources...) + } else { + resources = objectResources + } case "BypassGovernanceRetention": s3Actions = []string{"s3:BypassGovernanceRetention"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level bypass governance access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - // Bucket-level bypass governance access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "GetObjectRetention": s3Actions = []string{"s3:GetObjectRetention"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "PutObjectRetention": s3Actions = []string{"s3:PutObjectRetention"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "GetObjectLegalHold": s3Actions = []string{"s3:GetObjectLegalHold"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "PutObjectLegalHold": s3Actions = []string{"s3:PutObjectLegalHold"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "GetBucketObjectLockConfiguration": s3Actions = []string{"s3:GetBucketObjectLockConfiguration"} - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + bucket, _ := extractBucketAndPrefix(resourcePattern) + if bucket != "" { + resources = []string{fmt.Sprintf("arn:aws:s3:::%s", bucket)} + } else { + // Invalid pattern, return empty resources to fail validation + resources = []string{} + } case "PutBucketObjectLockConfiguration": s3Actions = []string{"s3:PutBucketObjectLockConfiguration"} - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + bucket, _ := extractBucketAndPrefix(resourcePattern) + if bucket != "" { + resources = []string{fmt.Sprintf("arn:aws:s3:::%s", bucket)} + } else { + // Invalid pattern, return empty resources to fail validation + resources = []string{} + } default: return nil, fmt.Errorf("unknown action type: %s", actionType) @@ -416,27 +522,15 @@ func ConvertLegacyActions(legacyActions []string) ([]string, error) { return uniqueActions, nil } -// GetResourcesFromLegacyAction extracts resources from a legacy action +// GetResourcesFromLegacyAction extracts resources from a legacy action. +// It delegates to convertSingleAction to ensure consistent resource ARN generation +// across the codebase and avoid duplicating action-type-specific logic. func GetResourcesFromLegacyAction(legacyAction string) ([]string, error) { - parts := strings.Split(legacyAction, ":") - if len(parts) != 2 { - return nil, fmt.Errorf("invalid action format: %s", legacyAction) + stmt, err := convertSingleAction(legacyAction) + if err != nil { + return nil, err } - - resourcePattern := parts[1] - resources := make([]string, 0) - - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = append(resources, fmt.Sprintf("arn:aws:s3:::%s", bucket)) - resources = append(resources, fmt.Sprintf("arn:aws:s3:::%s/*", bucket)) - } else { - // Bucket-level access - resources = append(resources, fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)) - } - - return resources, nil + return stmt.Resource.Strings(), nil } // CreatePolicyFromLegacyIdentity creates a policy document from legacy identity actions @@ -447,6 +541,12 @@ func CreatePolicyFromLegacyIdentity(identityName string, actions []string) (*Pol resourceActions := make(map[string][]string) for _, action := range actions { + // Validate action format before processing + if err := ValidateActionMapping(action); err != nil { + glog.Warningf("Skipping invalid action %q for identity %q: %v", action, identityName, err) + continue + } + parts := strings.Split(action, ":") if len(parts) != 2 { continue @@ -464,23 +564,53 @@ func CreatePolicyFromLegacyIdentity(identityName string, actions []string) (*Pol // Create statements for each resource pattern for resourcePattern, actionTypes := range resourceActions { s3Actions := make([]string, 0) - + resourceSet := make(map[string]struct{}) + + // Collect S3 actions and aggregate resource ARNs from all action types. + // Different action types have different resource ARN requirements: + // - List: bucket-level ARNs only + // - Read/Write/Tagging: object-level ARNs + // - Admin: full bucket access + // We must merge all required ARNs for the combined policy statement. for _, actionType := range actionTypes { if actionType == "Admin" { s3Actions = []string{"s3:*"} + // Admin action determines the resources, so we can break after processing it. + res, err := GetResourcesFromLegacyAction(fmt.Sprintf("Admin:%s", resourcePattern)) + if err != nil { + glog.Warningf("Failed to get resources for Admin action on %s: %v", resourcePattern, err) + resourceSet = nil // Invalidate to skip this statement + break + } + for _, r := range res { + resourceSet[r] = struct{}{} + } break } if mapped, exists := GetActionMappings()[actionType]; exists { s3Actions = append(s3Actions, mapped...) + res, err := GetResourcesFromLegacyAction(fmt.Sprintf("%s:%s", actionType, resourcePattern)) + if err != nil { + glog.Warningf("Failed to get resources for %s action on %s: %v", actionType, resourcePattern, err) + resourceSet = nil // Invalidate to skip this statement + break + } + for _, r := range res { + resourceSet[r] = struct{}{} + } } } - resources, err := GetResourcesFromLegacyAction(fmt.Sprintf("dummy:%s", resourcePattern)) - if err != nil { + if resourceSet == nil || len(s3Actions) == 0 { continue } + resources := make([]string, 0, len(resourceSet)) + for r := range resourceSet { + resources = append(resources, r) + } + statement := PolicyStatement{ Sid: fmt.Sprintf("%s-%s", identityName, strings.ReplaceAll(resourcePattern, "/", "-")), Effect: PolicyEffectAllow, diff --git a/weed/s3api/policy_engine/integration_test.go b/weed/s3api/policy_engine/integration_test.go new file mode 100644 index 000000000..6e74e51cb --- /dev/null +++ b/weed/s3api/policy_engine/integration_test.go @@ -0,0 +1,373 @@ +package policy_engine + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestConvertSingleActionDeleteObject tests support for s3:DeleteObject action (Issue #7864) +func TestConvertSingleActionDeleteObject(t *testing.T) { + // Test that Write action includes DeleteObject S3 action + stmt, err := convertSingleAction("Write:bucket") + assert.NoError(t, err) + assert.NotNil(t, stmt) + + // Check that s3:DeleteObject is included in the actions + actions := stmt.Action.Strings() + assert.Contains(t, actions, "s3:DeleteObject", "Write action should include s3:DeleteObject") + assert.Contains(t, actions, "s3:PutObject", "Write action should include s3:PutObject") +} + +// TestConvertSingleActionSubpath tests subpath handling for legacy actions (Issue #7864) +func TestConvertSingleActionSubpath(t *testing.T) { + testCases := []struct { + name string + action string + expectedActions []string + expectedResources []string + description string + }{ + { + name: "Write_on_bucket", + action: "Write:mybucket", + expectedActions: []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", "s3:DeleteBucketTagging", "s3:DeleteBucketCors"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Write permission on bucket should include bucket and object ARNs", + }, + { + name: "Write_on_bucket_with_wildcard", + action: "Write:mybucket/*", + expectedActions: []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", "s3:DeleteBucketTagging", "s3:DeleteBucketCors"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Write permission with /* should include bucket and object ARNs", + }, + { + name: "Write_on_subpath", + action: "Write:mybucket/sub_path/*", + expectedActions: []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", "s3:DeleteBucketTagging", "s3:DeleteBucketCors"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/sub_path/*"}, + description: "Write permission on subpath should include bucket and subpath objects ARNs", + }, + { + name: "Read_on_subpath", + action: "Read:mybucket/documents/*", + expectedActions: []string{"s3:GetObject", "s3:GetObjectVersion", "s3:ListBucket", "s3:ListBucketVersions", "s3:GetObjectAcl", "s3:GetObjectVersionAcl", "s3:GetObjectTagging", "s3:GetObjectVersionTagging", "s3:GetBucketLocation", "s3:GetBucketVersioning", "s3:GetBucketAcl", "s3:GetBucketCors", "s3:GetBucketTagging", "s3:GetBucketNotification"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/documents/*"}, + description: "Read permission on subpath should include bucket ARN and subpath objects", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + stmt, err := convertSingleAction(tc.action) + assert.NoError(t, err, tc.description) + assert.NotNil(t, stmt) + + // Check actions + actions := stmt.Action.Strings() + for _, expectedAction := range tc.expectedActions { + assert.Contains(t, actions, expectedAction, + "Action %s should be included for %s", expectedAction, tc.action) + } + + // Check resources - verify all expected resources are present + resources := stmt.Resource.Strings() + assert.ElementsMatch(t, resources, tc.expectedResources, + "Resources should match exactly for %s. Got %v, expected %v", tc.action, resources, tc.expectedResources) + }) + } +} + +// TestConvertSingleActionSubpathDeleteAllowed tests that DeleteObject works on subpaths +func TestConvertSingleActionSubpathDeleteAllowed(t *testing.T) { + // This test specifically addresses Issue #7864 part 1: + // "when a user is granted permission to a subpath, eg s3.configure -user someuser + // -actions Write -buckets some_bucket/sub_path/* -apply + // the user will only be able to put, but not delete object under somebucket/sub_path" + + stmt, err := convertSingleAction("Write:some_bucket/sub_path/*") + assert.NoError(t, err) + + // The fix: s3:DeleteObject should be in the allowed actions + actions := stmt.Action.Strings() + assert.Contains(t, actions, "s3:DeleteObject", + "Write permission on subpath should allow deletion of objects in that path") + + // The resource should be restricted to the subpath + resources := stmt.Resource.Strings() + assert.Contains(t, resources, "arn:aws:s3:::some_bucket/sub_path/*", + "Delete permission should apply to objects under the subpath") +} + +// TestConvertSingleActionNestedPaths tests deeply nested paths +func TestConvertSingleActionNestedPaths(t *testing.T) { + testCases := []struct { + action string + expectedResources []string + }{ + { + action: "Write:bucket/a/b/c/*", + expectedResources: []string{"arn:aws:s3:::bucket", "arn:aws:s3:::bucket/a/b/c/*"}, + }, + { + action: "Read:bucket/data/documents/2024/*", + expectedResources: []string{"arn:aws:s3:::bucket", "arn:aws:s3:::bucket/data/documents/2024/*"}, + }, + } + + for _, tc := range testCases { + stmt, err := convertSingleAction(tc.action) + assert.NoError(t, err) + + resources := stmt.Resource.Strings() + assert.ElementsMatch(t, resources, tc.expectedResources) + } +} + +// TestGetResourcesFromLegacyAction tests that GetResourcesFromLegacyAction generates +// action-appropriate resources consistent with convertSingleAction +func TestGetResourcesFromLegacyAction(t *testing.T) { + testCases := []struct { + name string + action string + expectedResources []string + description string + }{ + // List actions - bucket-only (no object ARNs) + { + name: "List_on_bucket", + action: "List:mybucket", + expectedResources: []string{"arn:aws:s3:::mybucket"}, + description: "List action should only have bucket ARN", + }, + { + name: "List_on_bucket_with_wildcard", + action: "List:mybucket/*", + expectedResources: []string{"arn:aws:s3:::mybucket"}, + description: "List action should only have bucket ARN regardless of wildcard", + }, + // Read actions - bucket and object-level ARNs (includes List* and Get* operations) + { + name: "Read_on_bucket", + action: "Read:mybucket", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Read action should have both bucket and object ARNs", + }, + { + name: "Read_on_subpath", + action: "Read:mybucket/documents/*", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/documents/*"}, + description: "Read action on subpath should have bucket ARN and object ARN for subpath", + }, + // Write actions - bucket and object ARNs (includes bucket-level operations) + { + name: "Write_on_subpath", + action: "Write:mybucket/sub_path/*", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/sub_path/*"}, + description: "Write action should have bucket and object ARNs", + }, + // Admin actions - both bucket and object ARNs + { + name: "Admin_on_bucket", + action: "Admin:mybucket", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Admin action should have both bucket and object ARNs", + }, + { + name: "Admin_on_subpath", + action: "Admin:mybucket/admin/section/*", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/admin/section/*"}, + description: "Admin action on subpath should restrict to subpath, preventing privilege escalation", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + resources, err := GetResourcesFromLegacyAction(tc.action) + assert.NoError(t, err, tc.description) + assert.ElementsMatch(t, resources, tc.expectedResources, + "Resources should match expected. Got %v, expected %v", resources, tc.expectedResources) + + // Also verify consistency with convertSingleAction where applicable + stmt, err := convertSingleAction(tc.action) + assert.NoError(t, err) + + stmtResources := stmt.Resource.Strings() + assert.ElementsMatch(t, resources, stmtResources, + "GetResourcesFromLegacyAction should match convertSingleAction resources for %s", tc.action) + }) + } +} + +// TestExtractBucketAndPrefixEdgeCases validates edge case handling in extractBucketAndPrefix +func TestExtractBucketAndPrefixEdgeCases(t *testing.T) { + testCases := []struct { + name string + pattern string + expectedBucket string + expectedPrefix string + description string + }{ + { + name: "Empty string", + pattern: "", + expectedBucket: "", + expectedPrefix: "", + description: "Empty pattern should return empty strings", + }, + { + name: "Whitespace only", + pattern: " ", + expectedBucket: "", + expectedPrefix: "", + description: "Whitespace-only pattern should return empty strings", + }, + { + name: "Slash only", + pattern: "/", + expectedBucket: "", + expectedPrefix: "", + description: "Slash-only pattern should return empty strings", + }, + { + name: "Double slash prefix", + pattern: "bucket//prefix/*", + expectedBucket: "bucket", + expectedPrefix: "prefix", + description: "Double slash should be normalized (trailing slashes removed)", + }, + { + name: "Normal bucket", + pattern: "mybucket", + expectedBucket: "mybucket", + expectedPrefix: "", + description: "Bucket-only pattern should work correctly", + }, + { + name: "Bucket with prefix", + pattern: "mybucket/myprefix/*", + expectedBucket: "mybucket", + expectedPrefix: "myprefix", + description: "Bucket with prefix should be parsed correctly", + }, + { + name: "Nested prefix", + pattern: "mybucket/a/b/c/*", + expectedBucket: "mybucket", + expectedPrefix: "a/b/c", + description: "Nested prefix should be preserved", + }, + { + name: "Bucket with trailing slash", + pattern: "mybucket/", + expectedBucket: "mybucket", + expectedPrefix: "", + description: "Trailing slash on bucket should be normalized", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + bucket, prefix := extractBucketAndPrefix(tc.pattern) + assert.Equal(t, tc.expectedBucket, bucket, tc.description) + assert.Equal(t, tc.expectedPrefix, prefix, tc.description) + }) + } +} + +// TestCreatePolicyFromLegacyIdentityMultipleActions validates correct resource ARN aggregation +// when multiple action types target the same resource pattern +func TestCreatePolicyFromLegacyIdentityMultipleActions(t *testing.T) { + testCases := []struct { + name string + identityName string + actions []string + expectedStatements int + expectedActionsInStmt1 []string + expectedResourcesInStmt1 []string + description string + }{ + { + name: "List_and_Write_on_subpath", + identityName: "data-manager", + actions: []string{"List:mybucket/data/*", "Write:mybucket/data/*"}, + expectedStatements: 1, + expectedActionsInStmt1: []string{ + "s3:ListBucket", "s3:ListBucketVersions", "s3:ListAllMyBuckets", + "s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", + "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", + "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", + "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", + "s3:DeleteBucketTagging", "s3:DeleteBucketCors", + }, + expectedResourcesInStmt1: []string{ + "arn:aws:s3:::mybucket", // From List and Write actions + "arn:aws:s3:::mybucket/data/*", // From Write action + }, + description: "List + Write on same subpath should aggregate all actions and both bucket and object ARNs", + }, + { + name: "Read_and_Tagging_on_bucket", + identityName: "tag-reader", + actions: []string{"Read:mybucket", "Tagging:mybucket"}, + expectedStatements: 1, + expectedActionsInStmt1: []string{ + "s3:GetObject", "s3:GetObjectVersion", + "s3:ListBucket", "s3:ListBucketVersions", + "s3:GetObjectAcl", "s3:GetObjectVersionAcl", + "s3:GetObjectTagging", "s3:GetObjectVersionTagging", + "s3:PutObjectTagging", "s3:DeleteObjectTagging", + "s3:GetBucketLocation", "s3:GetBucketVersioning", + "s3:GetBucketAcl", "s3:GetBucketCors", "s3:GetBucketTagging", + "s3:GetBucketNotification", "s3:PutBucketTagging", "s3:DeleteBucketTagging", + }, + expectedResourcesInStmt1: []string{ + "arn:aws:s3:::mybucket", + "arn:aws:s3:::mybucket/*", + }, + description: "Read + Tagging on same bucket should aggregate all bucket and object-level actions and ARNs", + }, + { + name: "Admin_with_other_actions", + identityName: "admin-user", + actions: []string{"Admin:mybucket/admin/*", "Write:mybucket/admin/*"}, + expectedStatements: 1, + expectedActionsInStmt1: []string{"s3:*"}, + expectedResourcesInStmt1: []string{ + "arn:aws:s3:::mybucket", + "arn:aws:s3:::mybucket/admin/*", + }, + description: "Admin action should dominate and set s3:*, other actions still processed for resources", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + policy, err := CreatePolicyFromLegacyIdentity(tc.identityName, tc.actions) + assert.NoError(t, err, tc.description) + assert.NotNil(t, policy) + + // Check statement count + assert.Equal(t, tc.expectedStatements, len(policy.Statement), + "Expected %d statement(s), got %d", tc.expectedStatements, len(policy.Statement)) + + if tc.expectedStatements > 0 { + stmt := policy.Statement[0] + + // Check actions + actualActions := stmt.Action.Strings() + for _, expectedAction := range tc.expectedActionsInStmt1 { + assert.Contains(t, actualActions, expectedAction, + "Action %s should be included in statement", expectedAction) + } + + // Check resources - all expected resources should be present + actualResources := stmt.Resource.Strings() + assert.ElementsMatch(t, tc.expectedResourcesInStmt1, actualResources, + "Statement should aggregate all required resource ARNs. Got %v, expected %v", + actualResources, tc.expectedResourcesInStmt1) + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 9b1495128..054a26264 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -1028,6 +1028,9 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R w.WriteHeader(http.StatusOK) } + // Track time to first byte metric + TimeToFirstByte(r.Method, t0, r) + // Stream directly to response with counting wrapper tStreamExec := time.Now() glog.V(4).Infof("streamFromVolumeServers: starting streamFn, offset=%d, size=%d", offset, size) @@ -1236,8 +1239,13 @@ func (s3a *S3ApiServer) streamFromVolumeServersWithSSE(w http.ResponseWriter, r // Now write status code (headers are all set) if isRangeRequest { w.WriteHeader(http.StatusPartialContent) + } else { + w.WriteHeader(http.StatusOK) } + // Track time to first byte metric + TimeToFirstByte(r.Method, t0, r) + // Full Range Optimization: Use ViewFromChunks to only fetch/decrypt needed chunks tDecryptSetup := time.Now()