From 4fa06aefc69b467fb2184e94fa4b2215fb38540f Mon Sep 17 00:00:00 2001 From: Anis Eleuch Date: Mon, 12 Feb 2024 17:36:16 +0100 Subject: [PATCH] Convert service account add/update expiration to cond values (#19024) In order to force some users allowed to create or update a service account to provide an expiration satifying the user policy conditions. --- cmd/admin-handlers-users.go | 60 +++++++++++++--------- cmd/admin-handlers-users_test.go | 88 ++++++++++++++++++++++++++++++++ go.mod | 2 +- go.sum | 2 + 4 files changed, 128 insertions(+), 24 deletions(-) diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 228cf5b5d..593a8d88e 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -27,6 +27,7 @@ import ( "net/http" "os" "sort" + "strconv" "time" "github.com/klauspost/compress/zip" @@ -774,28 +775,6 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re return } - // Permission checks: - // - // 1. Any type of account (i.e. access keys (previously/still called service - // accounts), STS accounts, internal IDP accounts, etc) with the - // policy.UpdateServiceAccountAdminAction permission can update any service - // account. - // - // 2. We would like to let a user update their own access keys, however it - // is currently blocked pending a re-design. Users are still able to delete - // and re-create them. - if !globalIAMSys.IsAllowed(policy.Args{ - AccountName: cred.AccessKey, - Groups: cred.Groups, - Action: policy.UpdateServiceAccountAdminAction, - ConditionValues: getConditionValues(r, "", cred), - IsOwner: owner, - Claims: cred.Claims, - }) { - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL) - return - } - password := cred.SecretKey reqBytes, err := madmin.DecryptData(password, io.LimitReader(r.Body, r.ContentLength)) if err != nil { @@ -816,6 +795,31 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re return } + condValues := getConditionValues(r, "", cred) + addExpirationToCondValues(updateReq.NewExpiration, condValues) + + // Permission checks: + // + // 1. Any type of account (i.e. access keys (previously/still called service + // accounts), STS accounts, internal IDP accounts, etc) with the + // policy.UpdateServiceAccountAdminAction permission can update any service + // account. + // + // 2. We would like to let a user update their own access keys, however it + // is currently blocked pending a re-design. Users are still able to delete + // and re-create them. + if !globalIAMSys.IsAllowed(policy.Args{ + AccountName: cred.AccessKey, + Groups: cred.Groups, + Action: policy.UpdateServiceAccountAdminAction, + ConditionValues: condValues, + IsOwner: owner, + Claims: cred.Claims, + }) { + writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL) + return + } + var sp *policy.Policy if len(updateReq.NewPolicy) > 0 { sp, err = policy.ParseConfig(bytes.NewReader(updateReq.NewPolicy)) @@ -2417,6 +2421,13 @@ func (a adminAPIHandlers) ImportIAM(w http.ResponseWriter, r *http.Request) { } } +func addExpirationToCondValues(exp *time.Time, condValues map[string][]string) { + if exp == nil { + return + } + condValues["DurationSeconds"] = []string{strconv.FormatInt(int64(exp.Sub(time.Now()).Seconds()), 10)} +} + func commonAddServiceAccount(r *http.Request) (context.Context, auth.Credentials, newServiceAccountOpts, madmin.AddServiceAccountReq, string, APIError) { ctx := r.Context() @@ -2472,13 +2483,16 @@ func commonAddServiceAccount(r *http.Request) (context.Context, auth.Credentials claims: make(map[string]interface{}), } + condValues := getConditionValues(r, "", cred) + addExpirationToCondValues(createReq.Expiration, condValues) + // Check if action is allowed if creating access key for another user // Check if action is explicitly denied if for self if !globalIAMSys.IsAllowed(policy.Args{ AccountName: cred.AccessKey, Groups: cred.Groups, Action: policy.CreateServiceAccountAdminAction, - ConditionValues: getConditionValues(r, "", cred), + ConditionValues: condValues, IsOwner: owner, Claims: cred.Claims, DenyOnly: (targetUser == cred.AccessKey || targetUser == cred.ParentUser), diff --git a/cmd/admin-handlers-users_test.go b/cmd/admin-handlers-users_test.go index ba4bff2dd..f91f63fca 100644 --- a/cmd/admin-handlers-users_test.go +++ b/cmd/admin-handlers-users_test.go @@ -208,6 +208,7 @@ func TestIAMInternalIDPServerSuite(t *testing.T) { suite.TestServiceAccountOpsByAdmin(c) suite.TestServiceAccountPrivilegeEscalationBug(c) suite.TestServiceAccountOpsByUser(c) + suite.TestServiceAccountDurationSecondsCondition(c) suite.TestAddServiceAccountPerms(c) suite.TearDownSuite(c) }, @@ -904,6 +905,93 @@ func (s *TestSuiteIAM) TestServiceAccountOpsByUser(c *check) { c.mustNotCreateSvcAccount(ctx, globalActiveCred.AccessKey, userAdmClient) } +func (s *TestSuiteIAM) TestServiceAccountDurationSecondsCondition(c *check) { + ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) + defer cancel() + + bucket := getRandomBucketName() + err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{}) + if err != nil { + c.Fatalf("bucket creat error: %v", err) + } + + // Create policy, user and associate policy + policy := "mypolicy" + policyBytes := []byte(fmt.Sprintf(`{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Deny", + "Action": [ + "admin:CreateServiceAccount", + "admin:UpdateServiceAccount" + ], + "Condition": {"NumericGreaterThan": {"svc:DurationSeconds": "3600"}} + }, + { + "Effect": "Allow", + "Action": [ + "s3:PutObject", + "s3:GetObject", + "s3:ListBucket" + ], + "Resource": [ + "arn:aws:s3:::%s/*" + ] + } + ] +}`, bucket)) + err = s.adm.AddCannedPolicy(ctx, policy, policyBytes) + if err != nil { + c.Fatalf("policy add error: %v", err) + } + + accessKey, secretKey := mustGenerateCredentials(c) + err = s.adm.SetUser(ctx, accessKey, secretKey, madmin.AccountEnabled) + if err != nil { + c.Fatalf("Unable to set user: %v", err) + } + + err = s.adm.SetPolicy(ctx, policy, accessKey, false) + if err != nil { + c.Fatalf("Unable to set policy: %v", err) + } + + // Create an madmin client with user creds + userAdmClient, err := madmin.NewWithOptions(s.endpoint, &madmin.Options{ + Creds: credentials.NewStaticV4(accessKey, secretKey, ""), + Secure: s.secure, + }) + if err != nil { + c.Fatalf("Err creating user admin client: %v", err) + } + userAdmClient.SetCustomTransport(s.TestSuiteCommon.client.Transport) + + distantExpiration := time.Now().Add(30 * time.Minute) + cr, err := userAdmClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ + TargetUser: accessKey, + AccessKey: "svc-accesskey", + SecretKey: "svc-secretkey", + Expiration: &distantExpiration, + }) + if err != nil { + c.Fatalf("Unable to create svc acc: %v", err) + } + + c.assertSvcAccS3Access(ctx, s, cr, bucket) + + closeExpiration := time.Now().Add(2 * time.Hour) + _, err = userAdmClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ + TargetUser: accessKey, + AccessKey: "svc-accesskey", + SecretKey: "svc-secretkey", + Expiration: &closeExpiration, + }) + if err == nil { + c.Fatalf("Creating a svc acc with distant expiration should fail") + } +} + func (s *TestSuiteIAM) TestServiceAccountOpsByAdmin(c *check) { ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) defer cancel() diff --git a/go.mod b/go.mod index 73a801661..4477ccdfe 100644 --- a/go.mod +++ b/go.mod @@ -54,7 +54,7 @@ require ( github.com/minio/madmin-go/v3 v3.0.45 github.com/minio/minio-go/v7 v7.0.66 github.com/minio/mux v1.9.0 - github.com/minio/pkg/v2 v2.0.9-0.20240130164631-ac3f1be23e94 + github.com/minio/pkg/v2 v2.0.9-0.20240209124402-7990a27fd79d github.com/minio/selfupdate v0.6.0 github.com/minio/sha256-simd v1.0.1 github.com/minio/simdjson-go v0.4.5 diff --git a/go.sum b/go.sum index adc910634..b3fe1e283 100644 --- a/go.sum +++ b/go.sum @@ -456,6 +456,8 @@ github.com/minio/mux v1.9.0 h1:dWafQFyEfGhJvK6AwLOt83bIG5bxKxKJnKMCi0XAaoA= github.com/minio/mux v1.9.0/go.mod h1:1pAare17ZRL5GpmNL+9YmqHoWnLmMZF9C/ioUCfy0BQ= github.com/minio/pkg/v2 v2.0.9-0.20240130164631-ac3f1be23e94 h1:nBZkkmq9HPfs2ZUDcRqPahe2js1mvMUR/7cBfW17Jik= github.com/minio/pkg/v2 v2.0.9-0.20240130164631-ac3f1be23e94/go.mod h1:yayUTo82b0RK+e97hGb1naC787mOtUEyDs3SIcwSyHI= +github.com/minio/pkg/v2 v2.0.9-0.20240209124402-7990a27fd79d h1:xGtyFgqwGy7Lc/i5udOKKeqsyRpQPlKQY2Pf4RiUDtk= +github.com/minio/pkg/v2 v2.0.9-0.20240209124402-7990a27fd79d/go.mod h1:yayUTo82b0RK+e97hGb1naC787mOtUEyDs3SIcwSyHI= github.com/minio/selfupdate v0.6.0 h1:i76PgT0K5xO9+hjzKcacQtO7+MjJ4JKA8Ak8XQ9DDwU= github.com/minio/selfupdate v0.6.0/go.mod h1:bO02GTIPCMQFTEvE5h4DjYB58bCoZ35XLeBf0buTDdM= github.com/minio/sha256-simd v0.1.1/go.mod h1:B5e1o+1/KgNmWrSQK08Y6Z1Vb5pwIktudl0J58iy0KM=