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") + }) + } +}