diff --git a/weed/admin/dash/user_management.go b/weed/admin/dash/user_management.go index 009f5cfca..d1b599acc 100644 --- a/weed/admin/dash/user_management.go +++ b/weed/admin/dash/user_management.go @@ -247,21 +247,29 @@ func (s *AdminServer) CreateAccessKey(username string, req *CreateAccessKeyReque req = &CreateAccessKeyRequest{} } - // Validate provided keys - if req.AccessKey != "" && (len(req.AccessKey) < 4 || len(req.AccessKey) > 128) { - return nil, fmt.Errorf("access key must be between 4 and 128 characters: %w", ErrInvalidInput) + // Validate provided keys (shared with the IAM API and embedded IAM paths). + if req.AccessKey != "" { + if err := iam.ValidateCallerSuppliedAccessKeyId(req.AccessKey); err != nil { + return nil, fmt.Errorf("%s: %w", err.Error(), ErrInvalidInput) + } } - if req.SecretKey != "" && (len(req.SecretKey) < 8 || len(req.SecretKey) > 128) { - return nil, fmt.Errorf("secret key must be between 8 and 128 characters: %w", ErrInvalidInput) + if req.SecretKey != "" { + if err := iam.ValidateCallerSuppliedSecretAccessKey(req.SecretKey); err != nil { + return nil, fmt.Errorf("%s: %w", err.Error(), ErrInvalidInput) + } + } + // Enforce the both-or-none rule to match the IAM API and embedded IAM + // paths — silently generating the missing half lets a caller end up + // with a credential they did not fully choose. + if (req.AccessKey != "") != (req.SecretKey != "") { + return nil, fmt.Errorf("access key and secret key must be supplied together: %w", ErrInvalidInput) } // Use provided keys or generate new ones accessKey := req.AccessKey + secretKey := req.SecretKey if accessKey == "" { accessKey = generateAccessKey() - } - secretKey := req.SecretKey - if secretKey == "" { secretKey = generateSecretKey() } diff --git a/weed/iam/redact.go b/weed/iam/redact.go new file mode 100644 index 000000000..7a27801c8 --- /dev/null +++ b/weed/iam/redact.go @@ -0,0 +1,32 @@ +package iam + +import "net/url" + +// sensitiveFormKeys is the set of IAM request form parameters whose values +// must never be written to logs. Matching is case-sensitive and uses the +// exact AWS IAM parameter name. Extend this when adding IAM actions that +// accept credentials, passwords, session tokens, or private keys. +var sensitiveFormKeys = map[string]struct{}{ + "SecretAccessKey": {}, + "Password": {}, + "NewPassword": {}, + "OldPassword": {}, + "PrivateKey": {}, + "SessionToken": {}, +} + +// RedactSensitiveFormValues returns a shallow copy of values with every +// sensitive key (see sensitiveFormKeys) replaced by "[REDACTED]". Intended +// for debug-level logging of IAM request forms so secrets do not leak into +// log sinks. +func RedactSensitiveFormValues(values url.Values) url.Values { + safe := make(url.Values, len(values)) + for k, v := range values { + if _, sensitive := sensitiveFormKeys[k]; sensitive { + safe[k] = []string{"[REDACTED]"} + } else { + safe[k] = v + } + } + return safe +} diff --git a/weed/iam/redact_test.go b/weed/iam/redact_test.go new file mode 100644 index 000000000..3f5bd7467 --- /dev/null +++ b/weed/iam/redact_test.go @@ -0,0 +1,33 @@ +package iam + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRedactSensitiveFormValues(t *testing.T) { + in := url.Values{ + "Action": []string{"CreateAccessKey"}, + "UserName": []string{"alice"}, + "AccessKeyId": []string{"AKIAALICE1"}, + "SecretAccessKey": []string{"very-secret"}, + "Password": []string{"hunter2"}, + "NewPassword": []string{"new-hunter2"}, + "OldPassword": []string{"old-hunter2"}, + "PrivateKey": []string{"-----BEGIN-----"}, + "SessionToken": []string{"tok"}, + } + out := RedactSensitiveFormValues(in) + + assert.Equal(t, []string{"CreateAccessKey"}, out["Action"]) + assert.Equal(t, []string{"alice"}, out["UserName"]) + assert.Equal(t, []string{"AKIAALICE1"}, out["AccessKeyId"]) + for _, k := range []string{"SecretAccessKey", "Password", "NewPassword", "OldPassword", "PrivateKey", "SessionToken"} { + assert.Equal(t, []string{"[REDACTED]"}, out[k], "key %q should be redacted", k) + } + + // Input is not mutated. + assert.Equal(t, []string{"very-secret"}, in["SecretAccessKey"]) +} diff --git a/weed/iam/validation.go b/weed/iam/validation.go new file mode 100644 index 000000000..699d90ae9 --- /dev/null +++ b/weed/iam/validation.go @@ -0,0 +1,68 @@ +package iam + +import ( + "fmt" + + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" +) + +// ValidateCallerSuppliedAccessKeyId checks that a caller-supplied AccessKeyId +// is 4 to 128 ASCII alphanumeric characters. Returns nil if valid. +// +// The alphanumeric restriction avoids characters that would break SigV4 +// canonicalization (e.g. '/' and '=' appear as delimiters in Credential +// headers), so this is a stricter superset of the rule AWS enforces. +func ValidateCallerSuppliedAccessKeyId(accessKeyId string) error { + if len(accessKeyId) < 4 || len(accessKeyId) > 128 { + return fmt.Errorf("AccessKeyId must be 4 to 128 alphanumeric characters") + } + for _, r := range accessKeyId { + if !((r >= 'A' && r <= 'Z') || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9')) { + return fmt.Errorf("AccessKeyId must be 4 to 128 alphanumeric characters") + } + } + return nil +} + +// ValidateCallerSuppliedSecretAccessKey checks that a caller-supplied +// SecretAccessKey is 8 to 128 characters. Returns nil if valid. +func ValidateCallerSuppliedSecretAccessKey(secretAccessKey string) error { + if len(secretAccessKey) < 8 || len(secretAccessKey) > 128 { + return fmt.Errorf("SecretAccessKey must be between 8 and 128 characters") + } + return nil +} + +// AccessKeyOwner identifies which entity in an S3ApiConfiguration already owns +// a given AccessKeyId. Returned by FindAccessKeyOwner for collision checks on +// caller-supplied credentials. +type AccessKeyOwner struct { + // Type is "user" or "service account". + Type string + // Name is the identity's Name (for users) or the service account's Id. + Name string +} + +// FindAccessKeyOwner scans s3cfg for an identity or service account whose +// credentials already contain accessKeyId. Returns nil if the key is free. +// +// Callers should log Name only at debug level — error responses returned to +// the caller should not include owner identity to avoid information leaks. +func FindAccessKeyOwner(s3cfg *iam_pb.S3ApiConfiguration, accessKeyId string) *AccessKeyOwner { + if s3cfg == nil || accessKeyId == "" { + return nil + } + for _, ident := range s3cfg.Identities { + for _, cred := range ident.Credentials { + if cred.AccessKey == accessKeyId { + return &AccessKeyOwner{Type: "user", Name: ident.Name} + } + } + } + for _, sa := range s3cfg.ServiceAccounts { + if sa.Credential != nil && sa.Credential.AccessKey == accessKeyId { + return &AccessKeyOwner{Type: "service account", Name: sa.Id} + } + } + return nil +} diff --git a/weed/iam/validation_test.go b/weed/iam/validation_test.go new file mode 100644 index 000000000..f5244c016 --- /dev/null +++ b/weed/iam/validation_test.go @@ -0,0 +1,104 @@ +package iam + +import ( + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/stretchr/testify/assert" +) + +func TestValidateCallerSuppliedAccessKeyId(t *testing.T) { + cases := []struct { + name string + input string + wantErr bool + }{ + {"empty", "", true}, + {"three chars", "abc", true}, + {"four chars ok", "abcd", false}, + {"mixed case alnum", "MyAppKey123", false}, + {"128 chars ok", strings.Repeat("a", 128), false}, + {"129 chars too long", strings.Repeat("a", 129), true}, + {"slash rejected", "foo/bar", true}, + {"equals rejected", "foo=bar", true}, + {"dash rejected", "foo-bar", true}, + {"underscore rejected", "foo_bar", true}, + {"unicode rejected", "fooö123", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCallerSuppliedAccessKeyId(tc.input) + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestValidateCallerSuppliedSecretAccessKey(t *testing.T) { + cases := []struct { + name string + input string + wantErr bool + }{ + {"empty", "", true}, + {"seven chars", "abcdefg", true}, + {"eight chars ok", "abcdefgh", false}, + {"128 chars ok", strings.Repeat("a", 128), false}, + {"129 chars too long", strings.Repeat("a", 129), true}, + {"non-alnum allowed in secret", "sec/ret=1", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCallerSuppliedSecretAccessKey(tc.input) + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestFindAccessKeyOwner(t *testing.T) { + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + { + Name: "alice", + Credentials: []*iam_pb.Credential{ + {AccessKey: "AKIAALICE1", SecretKey: "s"}, + }, + }, + {Name: "bob"}, + }, + ServiceAccounts: []*iam_pb.ServiceAccount{ + {Id: "svc-1", Credential: &iam_pb.Credential{AccessKey: "SVCKEY1"}}, + {Id: "svc-2"}, + }, + } + + t.Run("matches user", func(t *testing.T) { + owner := FindAccessKeyOwner(s3cfg, "AKIAALICE1") + assert.NotNil(t, owner) + assert.Equal(t, "user", owner.Type) + assert.Equal(t, "alice", owner.Name) + }) + t.Run("matches service account", func(t *testing.T) { + owner := FindAccessKeyOwner(s3cfg, "SVCKEY1") + assert.NotNil(t, owner) + assert.Equal(t, "service account", owner.Type) + assert.Equal(t, "svc-1", owner.Name) + }) + t.Run("no match", func(t *testing.T) { + assert.Nil(t, FindAccessKeyOwner(s3cfg, "NOTTAKEN")) + }) + t.Run("empty key", func(t *testing.T) { + assert.Nil(t, FindAccessKeyOwner(s3cfg, "")) + }) + t.Run("nil config", func(t *testing.T) { + assert.Nil(t, FindAccessKeyOwner(nil, "anything")) + }) +} diff --git a/weed/iamapi/iamapi_management_handlers.go b/weed/iamapi/iamapi_management_handlers.go index 8411ebb5c..cee4cc048 100644 --- a/weed/iamapi/iamapi_management_handlers.go +++ b/weed/iamapi/iamapi_management_handlers.go @@ -929,13 +929,38 @@ func (iama *IamApiServer) CreateAccessKey(s3cfg *iam_pb.S3ApiConfiguration, valu userName := values.Get("UserName") status := iam.StatusTypeActive - accessKeyId, err := StringWithCharset(21, charsetUpper) - if err != nil { - return resp, &IamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate access key: %w", err)} + accessKeyId := values.Get("AccessKeyId") + secretAccessKey := values.Get("SecretAccessKey") + if accessKeyId != "" { + if err := iamlib.ValidateCallerSuppliedAccessKeyId(accessKeyId); err != nil { + return resp, &IamError{Code: iam.ErrCodeInvalidInputException, Error: err} + } } - secretAccessKey, err := StringWithCharset(42, charset) - if err != nil { - return resp, &IamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate secret key: %w", err)} + if secretAccessKey != "" { + if err := iamlib.ValidateCallerSuppliedSecretAccessKey(secretAccessKey); err != nil { + return resp, &IamError{Code: iam.ErrCodeInvalidInputException, Error: err} + } + } + if (accessKeyId != "") != (secretAccessKey != "") { + return resp, &IamError{Code: iam.ErrCodeInvalidInputException, Error: fmt.Errorf("AccessKeyId and SecretAccessKey must be supplied together")} + } + if owner := iamlib.FindAccessKeyOwner(s3cfg, accessKeyId); owner != nil { + glog.V(4).Infof("CreateAccessKey: supplied AccessKeyId already in use by %s %s", owner.Type, owner.Name) + return resp, &IamError{Code: iam.ErrCodeEntityAlreadyExistsException, Error: fmt.Errorf("AccessKeyId is already in use")} + } + if accessKeyId == "" { + var err error + accessKeyId, err = StringWithCharset(21, charsetUpper) + if err != nil { + return resp, &IamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate access key: %w", err)} + } + } + if secretAccessKey == "" { + var err error + secretAccessKey, err = StringWithCharset(42, charset) + if err != nil { + return resp, &IamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate secret key: %w", err)} + } } resp.CreateAccessKeyResult.AccessKey.AccessKeyId = &accessKeyId @@ -1085,7 +1110,7 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { return } - glog.V(4).Infof("DoActions: %+v", values) + glog.V(4).Infof("DoActions: %+v", iamlib.RedactSensitiveFormValues(values)) var response iamlib.RequestIDSetter changed := true switch r.Form.Get("Action") { diff --git a/weed/iamapi/iamapi_management_handlers_test.go b/weed/iamapi/iamapi_management_handlers_test.go index 478bb41c5..9bffd4f85 100644 --- a/weed/iamapi/iamapi_management_handlers_test.go +++ b/weed/iamapi/iamapi_management_handlers_test.go @@ -3,6 +3,7 @@ package iamapi import ( "encoding/json" "net/url" + "strings" "testing" "github.com/aws/aws-sdk-go/service/iam" @@ -629,3 +630,266 @@ func TestListAttachedUserPolicies(t *testing.T) { assert.NotNil(t, iamErr) assert.Equal(t, iam.ErrCodeNoSuchEntityException, iamErr.Code) } + +func TestCreateAccessKeyWithCallerSuppliedKeys(t *testing.T) { + iama := newTestIamApiServer(Policies{}) + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + } + values := url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"myappkey"}, + "SecretAccessKey": []string{"mysecret1234"}, + } + resp, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.Nil(t, iamErr) + assert.Equal(t, "myappkey", *resp.CreateAccessKeyResult.AccessKey.AccessKeyId) + assert.Equal(t, "mysecret1234", *resp.CreateAccessKeyResult.AccessKey.SecretAccessKey) + assert.Equal(t, "alice", *resp.CreateAccessKeyResult.AccessKey.UserName) + assert.Equal(t, "myappkey", s3cfg.Identities[0].Credentials[0].AccessKey) + assert.Equal(t, "mysecret1234", s3cfg.Identities[0].Credentials[0].SecretKey) +} + +func TestCreateAccessKeyRandomGeneration(t *testing.T) { + iama := newTestIamApiServer(Policies{}) + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + } + values := url.Values{ + "UserName": []string{"alice"}, + } + resp, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.Nil(t, iamErr) + assert.NotEmpty(t, *resp.CreateAccessKeyResult.AccessKey.AccessKeyId) + assert.NotEmpty(t, *resp.CreateAccessKeyResult.AccessKey.SecretAccessKey) + assert.Len(t, s3cfg.Identities[0].Credentials, 1) +} + +func TestCreateAccessKeyRejectsWeakKeys(t *testing.T) { + iama := newTestIamApiServer(Policies{}) + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + } + // Too short + values := url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"ab"}, + "SecretAccessKey": []string{"validsecret1"}, + } + _, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + + // Short secret + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"validkey"}, + "SecretAccessKey": []string{"short"}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + + // SigV4 delimiters + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"foo/bar=baz"}, + "SecretAccessKey": []string{"validsecret1"}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) +} + +func TestCreateAccessKeyRejectsCollision(t *testing.T) { + iama := newTestIamApiServer(Policies{}) + // Use a distinctive owner name ("ownerAlpha") that shares no substring + // with the expected error message so the leak assertion is meaningful. + const ownerName = "ownerAlpha" + + t.Run("identity credential", func(t *testing.T) { + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + { + Name: ownerName, + Credentials: []*iam_pb.Credential{ + {AccessKey: "takenkey", SecretKey: "existingsecret"}, + }, + }, + {Name: "newuser"}, + }, + } + values := url.Values{ + "UserName": []string{"newuser"}, + "AccessKeyId": []string{"takenkey"}, + "SecretAccessKey": []string{"newsecret123"}, + } + _, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeEntityAlreadyExistsException, iamErr.Code) + assert.NotContains(t, iamErr.Error.Error(), ownerName, "should not leak owner name") + assert.Len(t, s3cfg.Identities[1].Credentials, 0) + }) + + t.Run("service account credential", func(t *testing.T) { + const saId = "svcAlpha" + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + {Name: "newuser"}, + }, + ServiceAccounts: []*iam_pb.ServiceAccount{ + { + Id: saId, + Credential: &iam_pb.Credential{AccessKey: "takenkey", SecretKey: "existingsecret"}, + }, + }, + } + values := url.Values{ + "UserName": []string{"newuser"}, + "AccessKeyId": []string{"takenkey"}, + "SecretAccessKey": []string{"newsecret123"}, + } + _, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeEntityAlreadyExistsException, iamErr.Code) + assert.NotContains(t, iamErr.Error.Error(), saId, "should not leak owner id") + // The service account's existing credential must be untouched, and + // no new credential should be attached to the identity. + assert.Equal(t, "takenkey", s3cfg.ServiceAccounts[0].Credential.AccessKey) + assert.Equal(t, "existingsecret", s3cfg.ServiceAccounts[0].Credential.SecretKey) + assert.Len(t, s3cfg.Identities[0].Credentials, 0) + }) +} + +func TestCreateAccessKeyBoundary(t *testing.T) { + iama := newTestIamApiServer(Policies{}) + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + } + // Exactly 4 chars - should pass + values := url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"abcd"}, + "SecretAccessKey": []string{"secretkey123"}, + } + resp, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.Nil(t, iamErr) + assert.Equal(t, "abcd", *resp.CreateAccessKeyResult.AccessKey.AccessKeyId) + + // Exactly 3 chars - should fail + s3cfg.Identities[0].Credentials = nil + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"abc"}, + "SecretAccessKey": []string{"secretkey123"}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + + // Exactly 128 chars - should pass + s3cfg.Identities[0].Credentials = nil + ak128 := strings.Repeat("a", 128) + sk128 := strings.Repeat("s", 128) + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{ak128}, + "SecretAccessKey": []string{sk128}, + } + resp, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.Nil(t, iamErr) + assert.Equal(t, ak128, *resp.CreateAccessKeyResult.AccessKey.AccessKeyId) + assert.Equal(t, sk128, *resp.CreateAccessKeyResult.AccessKey.SecretAccessKey) + + // 129 chars AccessKeyId - should fail + s3cfg.Identities[0].Credentials = nil + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{strings.Repeat("a", 129)}, + "SecretAccessKey": []string{sk128}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + + // 7-char SecretAccessKey - should fail + s3cfg.Identities[0].Credentials = nil + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"validkey"}, + "SecretAccessKey": []string{"1234567"}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + + // Exactly 8-char SecretAccessKey - should pass (lower boundary) + s3cfg.Identities[0].Credentials = nil + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"validkey"}, + "SecretAccessKey": []string{"12345678"}, + } + resp, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.Nil(t, iamErr) + assert.Equal(t, "12345678", *resp.CreateAccessKeyResult.AccessKey.SecretAccessKey) + + // 129-char SecretAccessKey - should fail + s3cfg.Identities[0].Credentials = nil + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"validkey"}, + "SecretAccessKey": []string{strings.Repeat("s", 129)}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) +} + +func TestCreateAccessKeyRejectsPartialSupply(t *testing.T) { + iama := newTestIamApiServer(Policies{}) + s3cfg := &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + } + // AccessKeyId supplied, SecretAccessKey omitted + values := url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"myappkey"}, + } + _, iamErr := iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + assert.Len(t, s3cfg.Identities[0].Credentials, 0) + + // SecretAccessKey supplied, AccessKeyId omitted + values = url.Values{ + "UserName": []string{"alice"}, + "SecretAccessKey": []string{"secretkey123"}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) + assert.Len(t, s3cfg.Identities[0].Credentials, 0) + + // Partial supply wins over collision: only AccessKeyId supplied, and + // it matches an existing credential. We must see InvalidInput, not + // EntityAlreadyExists — the both-or-none rule is more fundamental. + s3cfg = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + { + Name: "ownerAlpha", + Credentials: []*iam_pb.Credential{ + {AccessKey: "takenkey", SecretKey: "s"}, + }, + }, + {Name: "alice"}, + }, + } + values = url.Values{ + "UserName": []string{"alice"}, + "AccessKeyId": []string{"takenkey"}, + } + _, iamErr = iama.CreateAccessKey(s3cfg, values) + assert.NotNil(t, iamErr) + assert.Equal(t, iam.ErrCodeInvalidInputException, iamErr.Code) +} diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index 47f772520..1e581d545 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -410,32 +410,61 @@ func (e *EmbeddedIamApi) CreateAccessKey(s3cfg *iam_pb.S3ApiConfiguration, value userName := values.Get("UserName") status := iam.StatusTypeActive - // Generate AWS-standard access key: AKIA prefix + 16 random uppercase chars = 20 total - randomPart, err := iamStringWithCharset(AccessKeyLength-len(UserAccessKeyPrefix), iamCharsetUpper) - if err != nil { - return resp, &iamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate access key: %w", err)} + accessKeyId := values.Get("AccessKeyId") + secretAccessKey := values.Get("SecretAccessKey") + if accessKeyId != "" { + if err := iamlib.ValidateCallerSuppliedAccessKeyId(accessKeyId); err != nil { + return resp, &iamError{Code: iam.ErrCodeInvalidInputException, Error: err} + } } - accessKeyId := UserAccessKeyPrefix + randomPart - - secretAccessKey, err := iamStringWithCharset(SecretKeyLength, iamCharset) - if err != nil { - return resp, &iamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate secret key: %w", err)} + if secretAccessKey != "" { + if err := iamlib.ValidateCallerSuppliedSecretAccessKey(secretAccessKey); err != nil { + return resp, &iamError{Code: iam.ErrCodeInvalidInputException, Error: err} + } + } + if (accessKeyId != "") != (secretAccessKey != "") { + return resp, &iamError{Code: iam.ErrCodeInvalidInputException, Error: fmt.Errorf("AccessKeyId and SecretAccessKey must be supplied together")} } + // Find the target user before touching the RNG or scanning for collisions, + // so a missing user fails fast without consuming entropy. + var target *iam_pb.Identity + for _, ident := range s3cfg.Identities { + if userName == ident.Name { + target = ident + break + } + } + if target == nil { + return resp, &iamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(iamUserDoesNotExist, userName)} + } + + if owner := iamlib.FindAccessKeyOwner(s3cfg, accessKeyId); owner != nil { + glog.V(4).Infof("CreateAccessKey: supplied AccessKeyId already in use by %s %s", owner.Type, owner.Name) + return resp, &iamError{Code: iam.ErrCodeEntityAlreadyExistsException, Error: fmt.Errorf("AccessKeyId is already in use")} + } + if accessKeyId == "" { + randomPart, err := iamStringWithCharset(AccessKeyLength-len(UserAccessKeyPrefix), iamCharsetUpper) + if err != nil { + return resp, &iamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate access key: %w", err)} + } + accessKeyId = UserAccessKeyPrefix + randomPart + } + if secretAccessKey == "" { + var err error + secretAccessKey, err = iamStringWithCharset(SecretKeyLength, iamCharset) + if err != nil { + return resp, &iamError{Code: iam.ErrCodeServiceFailureException, Error: fmt.Errorf("failed to generate secret key: %w", err)} + } + } resp.CreateAccessKeyResult.AccessKey.AccessKeyId = &accessKeyId resp.CreateAccessKeyResult.AccessKey.SecretAccessKey = &secretAccessKey resp.CreateAccessKeyResult.AccessKey.UserName = &userName resp.CreateAccessKeyResult.AccessKey.Status = &status - for _, ident := range s3cfg.Identities { - if userName == ident.Name { - ident.Credentials = append(ident.Credentials, - &iam_pb.Credential{AccessKey: accessKeyId, SecretKey: secretAccessKey, Status: iamAccessKeyStatusActive}) - return resp, nil - } - } - // User not found - return error instead of implicitly creating the user - return resp, &iamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(iamUserDoesNotExist, userName)} + target.Credentials = append(target.Credentials, + &iam_pb.Credential{AccessKey: accessKeyId, SecretKey: secretAccessKey, Status: iamAccessKeyStatusActive}) + return resp, nil } // DeleteAccessKey deletes an access key for a user. @@ -2102,7 +2131,7 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s return nil, &iamError{Code: s3err.GetAPIError(s3err.ErrInternalError).Code, Error: fmt.Errorf("failed to get s3 api configuration: %v", err)} } - glog.V(4).Infof("IAM ExecuteAction: %+v", values) + glog.V(4).Infof("IAM ExecuteAction: %+v", iamlib.RedactSensitiveFormValues(values)) var response iamlib.RequestIDSetter changed := true switch values.Get("Action") { diff --git a/weed/s3api/s3api_embedded_iam_test.go b/weed/s3api/s3api_embedded_iam_test.go index 60f38b8fa..5635dc464 100644 --- a/weed/s3api/s3api_embedded_iam_test.go +++ b/weed/s3api/s3api_embedded_iam_test.go @@ -911,6 +911,346 @@ func TestEmbeddedIamCreateAccessKey(t *testing.T) { assert.Len(t, api.mockConfig.Identities[0].Credentials, 1) } +// TestEmbeddedIamCreateAccessKeyRejectsMissingUser verifies CreateAccessKey +// returns NoSuchEntity for an unknown user without mutating the config. +func TestEmbeddedIamCreateAccessKeyRejectsMissingUser(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "ExistingUser"}}, + } + + form := url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "GhostUser") + + req, _ := http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(api.DoActions) + apiRouter.ServeHTTP(rr, req) + + assert.NotEqual(t, http.StatusOK, rr.Code) + // No new identity and no credential appended to the existing one. + assert.Len(t, api.mockConfig.Identities, 1) + assert.Len(t, api.mockConfig.Identities[0].Credentials, 0) +} + +// TestEmbeddedIamCreateAccessKeyWithCallerSuppliedKeys tests creating an access key with caller-supplied credentials +func TestEmbeddedIamCreateAccessKeyWithCallerSuppliedKeys(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + {Name: "TestUser"}, + }, + } + + form := url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "TestUser") + form.Set("AccessKeyId", "myapp") + form.Set("SecretAccessKey", "mysecret123") + + req, _ := http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(api.DoActions) + apiRouter.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code) + + // Verify caller-supplied keys were used, not random ones + var out iamCreateAccessKeyResponse + err := xml.Unmarshal(rr.Body.Bytes(), &out) + require.NoError(t, err, "failed to unmarshal CreateAccessKey response") + require.NotNil(t, out.CreateAccessKeyResult.AccessKey.AccessKeyId) + require.NotNil(t, out.CreateAccessKeyResult.AccessKey.SecretAccessKey) + require.NotNil(t, out.CreateAccessKeyResult.AccessKey.UserName) + assert.Equal(t, "myapp", *out.CreateAccessKeyResult.AccessKey.AccessKeyId) + assert.Equal(t, "mysecret123", *out.CreateAccessKeyResult.AccessKey.SecretAccessKey) + assert.Equal(t, "TestUser", *out.CreateAccessKeyResult.AccessKey.UserName) + + // Verify credentials were persisted with caller-supplied keys + require.Len(t, api.mockConfig.Identities, 1) + require.Len(t, api.mockConfig.Identities[0].Credentials, 1) + assert.Equal(t, "myapp", api.mockConfig.Identities[0].Credentials[0].AccessKey) + assert.Equal(t, "mysecret123", api.mockConfig.Identities[0].Credentials[0].SecretKey) +} + +// TestEmbeddedIamCreateAccessKeyRejectsWeakKeys tests that weak caller-supplied keys are rejected +func TestEmbeddedIamCreateAccessKeyRejectsWeakKeys(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + {Name: "TestUser"}, + }, + } + + // AccessKeyId too short + form := url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "TestUser") + form.Set("AccessKeyId", "ab") + form.Set("SecretAccessKey", "validsecret123") + + req, _ := http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(api.DoActions) + apiRouter.ServeHTTP(rr, req) + + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "AccessKeyId must be 4 to 128 alphanumeric characters") + + // SecretAccessKey too short + form.Set("AccessKeyId", "validkey") + form.Set("SecretAccessKey", "short") + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "SecretAccessKey must be between 8 and 128 characters") + // AccessKeyId with SigV4 delimiters + form.Set("AccessKeyId", "foo/bar=baz") + form.Set("SecretAccessKey", "validsecret123") + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "AccessKeyId must be 4 to 128 alphanumeric characters") +} + +// TestEmbeddedIamCreateAccessKeyRejectsCollision tests that duplicate access keys are rejected +func TestEmbeddedIamCreateAccessKeyRejectsCollision(t *testing.T) { + api := NewEmbeddedIamApiForTest() + // Use a distinctive owner name ("ownerAlpha") so the leak assertion + // cannot accidentally match a word embedded in the error body. + const ownerName = "ownerAlpha" + api.mockConfig = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + { + Name: ownerName, + Credentials: []*iam_pb.Credential{ + {AccessKey: "takenkey", SecretKey: "existingsecret"}, + }, + }, + {Name: "NewUser"}, + }, + } + + form := url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "NewUser") + form.Set("AccessKeyId", "takenkey") + form.Set("SecretAccessKey", "newsecret123") + + req, _ := http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(api.DoActions) + apiRouter.ServeHTTP(rr, req) + + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "already in use") + assert.NotContains(t, rr.Body.String(), ownerName, "should not leak owner name") + + // Verify no credentials were added to NewUser + assert.Len(t, api.mockConfig.Identities[1].Credentials, 0) +} + +// TestEmbeddedIamCreateAccessKeyRejectsPartialSupply tests that supplying only +// one of AccessKeyId / SecretAccessKey is rejected. +func TestEmbeddedIamCreateAccessKeyRejectsPartialSupply(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + {Name: "TestUser"}, + }, + } + + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(api.DoActions) + + // AccessKeyId supplied, SecretAccessKey omitted + form := url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "TestUser") + form.Set("AccessKeyId", "myappkey") + + req, _ := http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "AccessKeyId and SecretAccessKey must be supplied together") + assert.Len(t, api.mockConfig.Identities[0].Credentials, 0) + + // SecretAccessKey supplied, AccessKeyId omitted + form = url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "TestUser") + form.Set("SecretAccessKey", "validsecret1") + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "AccessKeyId and SecretAccessKey must be supplied together") + assert.Len(t, api.mockConfig.Identities[0].Credentials, 0) +} + +// TestEmbeddedIamCreateAccessKeyBoundary tests key length boundaries +func TestEmbeddedIamCreateAccessKeyBoundary(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{ + {Name: "TestUser"}, + }, + } + + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(api.DoActions) + + // Exactly 4 chars — should pass + form := url.Values{} + form.Set("Action", "CreateAccessKey") + form.Set("UserName", "TestUser") + form.Set("AccessKeyId", "abcd") + form.Set("SecretAccessKey", "validsecret1") + + req, _ := http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.Equal(t, http.StatusOK, rr.Code) + + // Exactly 3 chars — should fail + api.mockConfig.Identities[0].Credentials = nil + form.Set("AccessKeyId", "abc") + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "alphanumeric") + + // Exactly 128 chars — should pass + api.mockConfig.Identities[0].Credentials = nil + ak128 := strings.Repeat("a", 128) + sk128 := strings.Repeat("s", 128) + form.Set("AccessKeyId", ak128) + form.Set("SecretAccessKey", sk128) + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.Equal(t, http.StatusOK, rr.Code) + + // 129 chars AccessKeyId — should fail + api.mockConfig.Identities[0].Credentials = nil + form.Set("AccessKeyId", strings.Repeat("a", 129)) + form.Set("SecretAccessKey", sk128) + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "alphanumeric") + + // 7-char SecretAccessKey — should fail + api.mockConfig.Identities[0].Credentials = nil + form.Set("AccessKeyId", "validkey") + form.Set("SecretAccessKey", "1234567") + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "SecretAccessKey must be between 8 and 128 characters") + + // Exactly 8-char SecretAccessKey — should pass (lower boundary) + api.mockConfig.Identities[0].Credentials = nil + form.Set("AccessKeyId", "validkey") + form.Set("SecretAccessKey", "12345678") + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.Equal(t, http.StatusOK, rr.Code) + + // 129-char SecretAccessKey — should fail + api.mockConfig.Identities[0].Credentials = nil + form.Set("AccessKeyId", "validkey") + form.Set("SecretAccessKey", strings.Repeat("s", 129)) + + req, _ = http.NewRequest("POST", "/", nil) + req.PostForm = form + req.Form = form + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr = httptest.NewRecorder() + apiRouter.ServeHTTP(rr, req) + assert.NotEqual(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "SecretAccessKey must be between 8 and 128 characters") +} + // TestEmbeddedIamDeleteAccessKey tests deleting an access key via direct form post func TestEmbeddedIamDeleteAccessKey(t *testing.T) { api := NewEmbeddedIamApiForTest()