add explicit deny support for service accounts (#13657)

creating service accounts is implicitly enabled
for all users, this PR however adds support to
reject creating service accounts, with an explicit
"Deny" policy.

cmd/admin-handlers-users.go

@@ -564,6 +564,22 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque
 	// that targetUser is a real user (i.e. not derived
 	// credentials).
 	if isSvcAccForRequestor {
+		// Check if adding service account is explicitly denied.
+		//
+		// This allows turning off service accounts for request sender,
+		// if there is no deny statement this call is implicitly enabled.
+		if !globalIAMSys.IsAllowed(iampolicy.Args{
+			AccountName:     requestorUser,
+			Action:          iampolicy.CreateServiceAccountAdminAction,
+			ConditionValues: getConditionValues(r, "", cred.AccessKey, claims),
+			IsOwner:         owner,
+			Claims:          claims,
+			DenyOnly:        true,
+		}) {
+			writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL)
+			return
+		}
+
 		if requestorIsDerivedCredential {
 			if requestorParentUser == "" {
 				writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx,

cmd/admin-handlers-users_test.go

@@ -121,6 +121,18 @@ func (s *TestSuiteIAM) RestartIAMSuite(c *check) {
 	s.iamSetup(c)
 }
 
+func (s *TestSuiteIAM) getAdminClient(c *check, accessKey, secretKey, sessionToken string) *madmin.AdminClient {
+	madmClnt, err := madmin.NewWithOptions(s.endpoint, &madmin.Options{
+		Creds:  credentials.NewStaticV4(accessKey, secretKey, sessionToken),
+		Secure: s.secure,
+	})
+	if err != nil {
+		c.Fatalf("error creating user admin client: %s", err)
+	}
+	madmClnt.SetCustomTransport(s.TestSuiteCommon.client.Transport)
+	return madmClnt
+}
+
 func (s *TestSuiteIAM) getUserClient(c *check, accessKey, secretKey, sessionToken string) *minio.Client {
 	client, err := minio.New(s.endpoint, &minio.Options{
 		Creds:     credentials.NewStaticV4(accessKey, secretKey, sessionToken),
@@ -140,6 +152,7 @@ func runAllIAMTests(suite *TestSuiteIAM, c *check) {
 	suite.TestCannedPolicies(c)
 	suite.TestGroupAddRemove(c)
 	suite.TestServiceAccountOps(c)
+	suite.TestAddServiceAccountPerms(c)
 	suite.TearDownSuite(c)
 }
 
@@ -246,6 +259,118 @@ func (s *TestSuiteIAM) TestUserCreate(c *check) {
 	}
 }
 
+func (s *TestSuiteIAM) TestAddServiceAccountPerms(c *check) {
+	ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
+	defer cancel()
+
+	// 1. Create a policy
+	policy1 := "deny-svc"
+	policy2 := "allow-svc"
+	policyBytes := []byte(`{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Deny",
+   "Action": [
+    "admin:CreateServiceAccount"
+   ]
+  }
+ ]
+}`)
+
+	newPolicyBytes := []byte(`{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Allow",
+   "Action": [
+    "s3:ListBucket"
+   ],
+   "Resource": [
+    "arn:aws:s3:::testbucket/*"
+   ]
+  }
+ ]
+}`)
+
+	err := s.adm.AddCannedPolicy(ctx, policy1, policyBytes)
+	if err != nil {
+		c.Fatalf("policy add error: %v", err)
+	}
+
+	err = s.adm.AddCannedPolicy(ctx, policy2, newPolicyBytes)
+	if err != nil {
+		c.Fatalf("policy add error: %v", err)
+	}
+
+	// 2. Verify that policy json is validated by server
+	invalidPolicyBytes := policyBytes[:len(policyBytes)-1]
+	err = s.adm.AddCannedPolicy(ctx, policy1+"invalid", invalidPolicyBytes)
+	if err == nil {
+		c.Fatalf("invalid policy creation success")
+	}
+
+	// 3. Create a user, associate policy and verify access
+	accessKey, secretKey := mustGenerateCredentials(c)
+	err = s.adm.SetUser(ctx, accessKey, secretKey, madmin.AccountEnabled)
+	if err != nil {
+		c.Fatalf("Unable to set user: %v", err)
+	}
+	// 3.1 check that user does not have any access to the bucket
+	uClient := s.getUserClient(c, accessKey, secretKey, "")
+	c.mustNotListObjects(ctx, uClient, "testbucket")
+
+	// 3.2 associate policy to user
+	err = s.adm.SetPolicy(ctx, policy1, accessKey, false)
+	if err != nil {
+		c.Fatalf("Unable to set policy: %v", err)
+	}
+
+	admClnt := s.getAdminClient(c, accessKey, secretKey, "")
+
+	// 3.3 check user does not have explicit permissions to create service account.
+	c.mustNotCreateSvcAccount(ctx, accessKey, admClnt)
+
+	// 4. Verify the policy appears in listing
+	ps, err := s.adm.ListCannedPolicies(ctx)
+	if err != nil {
+		c.Fatalf("policy list err: %v", err)
+	}
+	_, ok := ps[policy1]
+	if !ok {
+		c.Fatalf("policy was missing!")
+	}
+
+	// 3.2 associate policy to user
+	err = s.adm.SetPolicy(ctx, policy2, accessKey, false)
+	if err != nil {
+		c.Fatalf("Unable to set policy: %v", err)
+	}
+
+	// 3.3 check user can create service account implicitly.
+	c.mustCreateSvcAccount(ctx, accessKey, admClnt)
+
+	_, ok = ps[policy2]
+	if !ok {
+		c.Fatalf("policy was missing!")
+	}
+
+	err = s.adm.RemoveUser(ctx, accessKey)
+	if err != nil {
+		c.Fatalf("user could not be deleted: %v", err)
+	}
+
+	err = s.adm.RemoveCannedPolicy(ctx, policy1)
+	if err != nil {
+		c.Fatalf("policy del err: %v", err)
+	}
+
+	err = s.adm.RemoveCannedPolicy(ctx, policy2)
+	if err != nil {
+		c.Fatalf("policy del err: %v", err)
+	}
+}
+
 func (s *TestSuiteIAM) TestPolicyCreate(c *check) {
 	ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
 	defer cancel()
@@ -750,6 +875,24 @@ func (s *TestSuiteIAM) TestServiceAccountOps(c *check) {
 	}
 }
 
+func (c *check) mustCreateSvcAccount(ctx context.Context, tgtUser string, admClnt *madmin.AdminClient) {
+	_, err := admClnt.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
+		TargetUser: tgtUser,
+	})
+	if err != nil {
+		c.Fatalf("user should be able to create service accounts %s", err)
+	}
+}
+
+func (c *check) mustNotCreateSvcAccount(ctx context.Context, tgtUser string, admClnt *madmin.AdminClient) {
+	_, err := admClnt.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
+		TargetUser: tgtUser,
+	})
+	if err == nil {
+		c.Fatalf("user was able to add service accounts unexpectedly!")
+	}
+}
+
 func (c *check) mustNotListObjects(ctx context.Context, client *minio.Client, bucket string) {
 	res := client.ListObjects(ctx, bucket, minio.ListObjectsOptions{})
 	v, ok := <-res