From 0002e5cc7f38b4a71c229e76189646537e29ae8f Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 29 Jul 2026 10:30:30 -0700 Subject: [PATCH] s3api: load document-style policies from the advanced IAM config (#10481) * s3api: load document-style policies from the advanced IAM config The advanced IAM file doubles as the S3 identity config when only -s3.iam.config is given. protojson drops its "document" field, so every policy landed with empty content and warned "skipping invalid policy" on each reload. Worse, if the same file also declares identities the empty content sticks in the policy map and fails the whole runtime policy sync into the IAM manager, so policies created later never reach it. * iam: skip an unparsable policy instead of failing the whole runtime sync One policy the engine cannot parse aborted SyncRuntimePolicies before it touched anything, so every other policy stayed unsynced and the engine kept serving whatever it last held. * s3api: reject a non-role RoleArn in AssumeRole as a bad request arn:aws:iam:::user/name can never resolve to a role, but the handler ran it through the trust-policy check and answered "not authorized to assume role", pointing the caller at a permission problem they do not have. * s3api: build the policy content before touching the entry Deleting "document" up front meant a marshal failure left the policy with neither field, so a later rewrite would emit it with no definition at all. * iam: pin the fail-closed handling of an unparsable policy Say in the comment that dropping it from the desired set deletes it from the engine on purpose, and cover it with a test. * s3api: widen the non-role RoleArn test to canonical ARN shapes The reported ARN omits the account id; a user ARN that carries one, and a non-principal ARN, must be rejected the same way. --- weed/iam/integration/iam_manager.go | 8 +- weed/s3api/auth_credentials.go | 2 +- .../s3api/auth_credentials_policy_document.go | 84 +++++++++++++++++++ .../auth_credentials_policy_document_test.go | 83 ++++++++++++++++++ .../auth_credentials_policy_sync_test.go | 65 ++++++++++++++ weed/s3api/s3api_sts.go | 8 ++ .../s3api_sts_assume_role_arn_shape_test.go | 60 +++++++++++++ 7 files changed, 308 insertions(+), 2 deletions(-) create mode 100644 weed/s3api/auth_credentials_policy_document.go create mode 100644 weed/s3api/auth_credentials_policy_document_test.go create mode 100644 weed/s3api/s3api_sts_assume_role_arn_shape_test.go diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index 228d53be7..6a04e09f9 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -466,7 +466,13 @@ func (m *IAMManager) SyncRuntimePolicies(ctx context.Context, policies []*iam_pb var document policy.PolicyDocument if err := json.Unmarshal([]byte(runtimePolicy.Content), &document); err != nil { - return fmt.Errorf("failed to parse runtime policy %q: %w", runtimePolicy.Name, err) + // Drop just this one: aborting here would leave every other policy + // unsynced. Leaving it out of desiredPolicies also deletes it from + // the engine below, which is the point — a policy whose stored + // definition no longer parses must stop granting access rather than + // keep enforcing a document the operator can no longer see. + glog.Warningf("skipping unparsable runtime policy %q: %v", runtimePolicy.Name, err) + continue } desiredPolicies[runtimePolicy.Name] = &document diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 1f98f11ee..0d051cfbe 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -580,7 +580,7 @@ func (iam *IdentityAccessManagement) LoadS3ApiConfigurationFromBytes(content []b func (iam *IdentityAccessManagement) loadS3ApiConfigurationFromBytes(content []byte, fromStaticFile bool) (*iam_pb.S3ApiConfiguration, error) { s3ApiConfiguration := &iam_pb.S3ApiConfiguration{} - if err := filer.ParseS3ConfigurationFromBytes(content, s3ApiConfiguration); err != nil { + if err := filer.ParseS3ConfigurationFromBytes(normalizeAdvancedIAMPolicies(content), s3ApiConfiguration); err != nil { glog.Warningf("unmarshal error: %v", err) return nil, fmt.Errorf("unmarshal error: %w", err) } diff --git a/weed/s3api/auth_credentials_policy_document.go b/weed/s3api/auth_credentials_policy_document.go new file mode 100644 index 000000000..dfa0ace29 --- /dev/null +++ b/weed/s3api/auth_credentials_policy_document.go @@ -0,0 +1,84 @@ +package s3api + +import ( + "encoding/json" + "strings" +) + +// normalizeAdvancedIAMPolicies rewrites policies written in the advanced IAM +// form ({"name": ..., "document": {...}}) into the S3 config form +// ({"name": ..., "content": "{...}"}). +// +// The advanced IAM file given by -s3.iam.config is also parsed as the S3 +// identity config when no -s3.config is given, and protojson drops the unknown +// "document" field. That leaves a policy with empty content, which fails every +// later parse and takes the whole runtime policy sync down with it. +func normalizeAdvancedIAMPolicies(configContent []byte) []byte { + var root map[string]json.RawMessage + if err := json.Unmarshal(configContent, &root); err != nil { + return configContent + } + rawPolicies, found := root["policies"] + if !found { + return configContent + } + var policies []map[string]json.RawMessage + if err := json.Unmarshal(rawPolicies, &policies); err != nil { + return configContent + } + + rewritten := false + for _, policy := range policies { + document, hasDocument := policy["document"] + if !hasDocument || hasPolicyContent(policy) { + continue + } + // A document already written as a JSON string is the content verbatim; + // an inline object becomes the JSON encoding of its own bytes. Nothing + // is mutated until the content is in hand, so a failure leaves the + // policy as it was rather than stripping its only definition. + content := document + if !isJSONString(document) { + encoded, err := json.Marshal(string(document)) + if err != nil { + continue + } + content = encoded + } + delete(policy, "document") + policy["content"] = content + rewritten = true + } + if !rewritten { + return configContent + } + + encodedPolicies, err := json.Marshal(policies) + if err != nil { + return configContent + } + root["policies"] = encodedPolicies + normalized, err := json.Marshal(root) + if err != nil { + return configContent + } + return normalized +} + +func hasPolicyContent(policy map[string]json.RawMessage) bool { + raw, found := policy["content"] + if !found { + return false + } + var content string + if err := json.Unmarshal(raw, &content); err != nil { + // Not a string: leave whatever it is for the proto parser to reject. + return true + } + return strings.TrimSpace(content) != "" +} + +func isJSONString(raw json.RawMessage) bool { + var s string + return json.Unmarshal(raw, &s) == nil +} diff --git a/weed/s3api/auth_credentials_policy_document_test.go b/weed/s3api/auth_credentials_policy_document_test.go new file mode 100644 index 000000000..c6b061b17 --- /dev/null +++ b/weed/s3api/auth_credentials_policy_document_test.go @@ -0,0 +1,83 @@ +package s3api + +import ( + "encoding/json" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/filer" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/stretchr/testify/require" +) + +// The advanced IAM file doubles as the S3 identity config when only +// -s3.iam.config is given, so its "document" policies must survive the parse. +func TestLoadAdvancedIAMConfigPolicyDocument(t *testing.T) { + config := []byte(`{ + "sts": {"issuer": "seaweedfs-sts"}, + "policies": [ + { + "name": "ClientPolicy", + "document": { + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": ["s3:*"], "Resource": ["*"]}] + } + } + ] +}`) + + parsed := &iam_pb.S3ApiConfiguration{} + require.NoError(t, filer.ParseS3ConfigurationFromBytes(normalizeAdvancedIAMPolicies(config), parsed)) + require.Len(t, parsed.Policies, 1) + require.Equal(t, "ClientPolicy", parsed.Policies[0].Name) + + var document map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(parsed.Policies[0].Content), &document)) + require.Equal(t, "2012-10-17", document["Version"]) +} + +func TestNormalizeAdvancedIAMPolicies(t *testing.T) { + for _, tc := range []struct { + name string + config string + content string + }{ + { + name: "document object", + config: `{"policies":[{"name":"p","document":{"Version":"2012-10-17"}}]}`, + content: `{"Version":"2012-10-17"}`, + }, + { + name: "document already encoded as a string", + config: `{"policies":[{"name":"p","document":"{\"Version\":\"2012-10-17\"}"}]}`, + content: `{"Version":"2012-10-17"}`, + }, + { + name: "content wins over document", + config: `{"policies":[{"name":"p","content":"{\"Version\":\"keep\"}","document":{"Version":"drop"}}]}`, + content: `{"Version":"keep"}`, + }, + { + name: "content only", + config: `{"policies":[{"name":"p","content":"{\"Version\":\"keep\"}"}]}`, + content: `{"Version":"keep"}`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + parsed := &iam_pb.S3ApiConfiguration{} + require.NoError(t, filer.ParseS3ConfigurationFromBytes(normalizeAdvancedIAMPolicies([]byte(tc.config)), parsed)) + require.Len(t, parsed.Policies, 1) + require.JSONEq(t, tc.content, parsed.Policies[0].Content) + }) + } +} + +// Anything that is not a policy list is handed to the proto parser untouched. +func TestNormalizeAdvancedIAMPoliciesLeavesOtherConfigsAlone(t *testing.T) { + for _, config := range []string{ + `not json`, + `{"identities":[{"name":"admin"}]}`, + `{"policies":{"p":{"document":{}}}}`, + } { + require.Equal(t, config, string(normalizeAdvancedIAMPolicies([]byte(config)))) + } +} diff --git a/weed/s3api/auth_credentials_policy_sync_test.go b/weed/s3api/auth_credentials_policy_sync_test.go index a5bdeeca4..ea7935e18 100644 --- a/weed/s3api/auth_credentials_policy_sync_test.go +++ b/weed/s3api/auth_credentials_policy_sync_test.go @@ -156,3 +156,68 @@ func TestResyncIAMManager_ReflectsCurrentPolicies(t *testing.T) { require.False(t, allows("alpha"), "resync should drop a policy no longer in the map") require.True(t, allows("beta"), "resync should add a policy newly in the map") } + +func TestResync_SkipsUnparsablePolicy(t *testing.T) { + mgr := newTestIAMManager(t) + iam := &IdentityAccessManagement{} + iam.SetIAMIntegration(NewS3IAMIntegration(mgr, "")) + + doc, _ := json.Marshal(map[string]interface{}{ + "Version": "2012-10-17", + "Statement": []map[string]interface{}{ + {"Effect": "Allow", "Action": "s3:*", "Resource": "arn:aws:s3:::good/*"}, + }, + }) + + iam.m.Lock() + iam.policies = map[string]*iam_pb.Policy{ + "broken": {Name: "broken"}, + "good": {Name: "good", Content: string(doc)}, + } + iam.m.Unlock() + iam.resyncIAMManagerPolicies() + + allowed, err := mgr.IsActionAllowed(context.Background(), &integration.ActionRequest{ + Principal: "arn:aws:iam::111122223333:user/test", + Action: "s3:PutObject", + Resource: "arn:aws:s3:::good/file.txt", + PolicyNames: []string{"good"}, + }) + require.NoError(t, err) + require.True(t, allowed, "an unparsable policy must not block the rest of the sync") +} + +// A policy whose stored definition stops parsing must stop granting access: +// enforcing a document the operator can no longer see is worse than denying. +func TestResync_UnparsablePolicyStopsGranting(t *testing.T) { + mgr := newTestIAMManager(t) + iam := &IdentityAccessManagement{} + iam.SetIAMIntegration(NewS3IAMIntegration(mgr, "")) + + doc, _ := json.Marshal(map[string]interface{}{ + "Version": "2012-10-17", + "Statement": []map[string]interface{}{ + {"Effect": "Allow", "Action": "s3:*", "Resource": "arn:aws:s3:::good/*"}, + }, + }) + allows := func() bool { + allowed, err := mgr.IsActionAllowed(context.Background(), &integration.ActionRequest{ + Principal: "arn:aws:iam::111122223333:user/test", + Action: "s3:PutObject", + Resource: "arn:aws:s3:::good/file.txt", + PolicyNames: []string{"good"}, + }) + require.NoError(t, err) + return allowed + } + + require.NoError(t, iam.PutPolicy("good", string(doc))) + require.True(t, allows()) + + iam.m.Lock() + iam.policies["good"] = &iam_pb.Policy{Name: "good", Content: "{not json"} + iam.m.Unlock() + iam.resyncIAMManagerPolicies() + + require.False(t, allows(), "an unparsable policy must not keep granting its old permissions") +} diff --git a/weed/s3api/s3api_sts.go b/weed/s3api/s3api_sts.go index e94b02bcc..5e217eede 100644 --- a/weed/s3api/s3api_sts.go +++ b/weed/s3api/s3api_sts.go @@ -393,6 +393,14 @@ func (h *STSHandlers) handleAssumeRole(w http.ResponseWriter, r *http.Request) { // is required. An explicit identity-side deny still wins (deny-always-wins). // Without a RoleArn the caller assumes a session for itself. if roleArn != "" { + // An ARN that names something other than a role can never resolve to one, + // and reporting that as "not authorized" sends the caller looking for a + // permission problem they do not have. + if utils.ExtractRoleNameFromArn(roleArn) == "" { + h.writeSTSErrorResponse(w, r, STSErrInvalidParameterValue, + fmt.Errorf("RoleArn %q is not an IAM role ARN, expected arn:aws:iam:::role/", roleArn)) + return + } callerArn := h.callerPrincipalArn(identity) if err := h.iam.ValidateTrustPolicyForPrincipal(r.Context(), roleArn, callerArn); err != nil { glog.V(2).Infof("AssumeRole: %s not authorized to assume %s: %v", identity.Name, roleArn, err) diff --git a/weed/s3api/s3api_sts_assume_role_arn_shape_test.go b/weed/s3api/s3api_sts_assume_role_arn_shape_test.go new file mode 100644 index 000000000..6b85a042d --- /dev/null +++ b/weed/s3api/s3api_sts_assume_role_arn_shape_test.go @@ -0,0 +1,60 @@ +package s3api + +import ( + "encoding/xml" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// An ARN that names a user, not a role, is a bad request: answering +// "not authorized to assume role" sends the caller hunting for permissions. +func TestAssumeRole_RejectsNonRoleArn(t *testing.T) { + manager := newTestSTSIntegrationManager(t) + + const accessKey, secretKey = "adminkey", "adminsecret" + iam := &IdentityAccessManagement{iamIntegration: NewS3IAMIntegration(manager, "")} + require.NoError(t, iam.loadS3ApiConfiguration(&iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{ + Name: "admin", + Credentials: []*iam_pb.Credential{{AccessKey: accessKey, SecretKey: secretKey}}, + Actions: []string{"Admin"}, + }}, + })) + stsHandlers := NewSTSHandlers(manager.GetSTSService(), iam) + + for _, roleArn := range []string{ + "arn:aws:iam:::user/test-user", // no account id, as reported + "arn:aws:iam::123456789012:user/test-user", // canonical user ARN + "arn:aws:iam::123456789012:group/test-team", // not a principal at all + "test-user", // not an ARN + } { + t.Run(roleArn, func(t *testing.T) { + body := url.Values{ + "Action": {"AssumeRole"}, + "Version": {"2011-06-15"}, + "RoleArn": {roleArn}, + "RoleSessionName": {"dev-session"}, + }.Encode() + req, err := newTestRequest(http.MethodPost, "http://sts.seaweedfs.test/", int64(len(body)), strings.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + require.NoError(t, signRequestV4(req, accessKey, secretKey)) + + rec := httptest.NewRecorder() + stsHandlers.handleAssumeRole(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Code, rec.Body.String()) + var resp STSErrorResponse + require.NoError(t, xml.Unmarshal(rec.Body.Bytes(), &resp)) + assert.Equal(t, string(STSErrInvalidParameterValue), resp.Error.Code) + assert.Contains(t, resp.Error.Message, "is not an IAM role ARN") + }) + } +}