Merge pull request #54 from DClabaut/51-fix-create-user-before-policy

fix: Infinite loop if user has been created without ListBucket permission
InseeFrLab · Feb 5, 2025 · a683dec · a683dec
2 parents 71574e0 + a4ea3c9
commit a683dec
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 24 deletions.
diff --git a/README.md b/README.md
@@ -205,10 +205,22 @@ spec:
   # Content of the policy, as a multiline string 
   # This should be IAM compliant JSON - follow the guidelines of the actual
   # S3 provider you're using, as sometimes only a subset is available.
+  # The first Statement (Allow ListBucket) should be applied to every user,
+  # as s3-operator uses this call to verify that credentials are valid when
+  # reconciling an existing user.
   policyContent: >-
     {
       "Version": "2012-10-17",
       "Statement": [
+      {
+        "Effect": "Allow",
+        "Action": [
+          "s3:ListBucket"
+        ],
+        "Resource": [
+          "arn:aws:s3:::*"
+        ]
+      },
       {
         "Effect": "Allow",
         "Action": [

diff --git a/controllers/user_controller.go b/controllers/user_controller.go
@@ -154,30 +154,6 @@ func (r *S3UserReconciler) handleS3ExistingUser(ctx context.Context, userResourc
 		return r.handleS3NewUser(ctx, userResource)
 	}
 
-	// If a matching secret is found, then we check if it is still valid, as in : do the credentials it
-	// contains still allow authenticating the S3User on the backend ? If not, the user is deleted and recreated.
-	// credentialsValid, err := r.S3Client.CheckUserCredentialsValid(userResource.Name, userResource.Spec.AccessKey, string(userOwnedSecret.Data["secretKey"]))
-	credentialsValid, err := r.S3Client.CheckUserCredentialsValid(userResource.Name, string(userOwnedSecret.Data["accessKey"]), string(userOwnedSecret.Data["secretKey"]))
-	if err != nil {
-		logger.Error(err, "An error occurred when checking if user credentials were valid", "user", userResource.Name)
-		return r.setS3UserStatusConditionAndUpdate(ctx, userResource, "OperatorFailed", metav1.ConditionFalse, "S3UserCredentialsCheckFailed",
-			fmt.Sprintf("Checking the S3User %s's credentials on S3 server has failed", userResource.Name), err)
-	}
-
-	if !credentialsValid {
-		logger.Info("The secret containing the credentials will be deleted, and the user will be deleted from the S3 backend, then recreated (through another reconcile)")
-		r.deleteSecret(ctx, &userOwnedSecret)
-		err = r.S3Client.DeleteUser(userResource.Spec.AccessKey)
-		if err != nil {
-			logger.Error(err, "Could not delete user on S3 server", "user", userResource.Name)
-			return r.setS3UserStatusConditionAndUpdate(ctx, userResource, "OperatorFailed", metav1.ConditionFalse, "S3UserDeletionFailed",
-				fmt.Sprintf("Deletion of S3user %s on S3 server has failed", userResource.Name), err)
-		}
-
-		return r.handleS3NewUser(ctx, userResource)
-
-	}
-
 	// --- End Secret management section
 
 	logger.Info("Checking user policies")
@@ -224,6 +200,30 @@ func (r *S3UserReconciler) handleS3ExistingUser(ctx context.Context, userResourc
 		}
 	}
 
+        // If a matching secret is found, then we check if it is still valid, as in : do the credentials it
+        // contains still allow authenticating the S3User on the backend ? If not, the user is deleted and recreated.
+        // credentialsValid, err := r.S3Client.CheckUserCredentialsValid(userResource.Name, userResource.Spec.AccessKey, string(userOwnedSecret.Data["secretKey"]))
+        credentialsValid, err := r.S3Client.CheckUserCredentialsValid(userResource.Name, string(userOwnedSecret.Data["accessKey"]), string(userOwnedSecret.Data["secretKey"]))
+        if err != nil {
+                logger.Error(err, "An error occurred when checking if user credentials were valid", "user", userResource.Name)
+                return r.setS3UserStatusConditionAndUpdate(ctx, userResource, "OperatorFailed", metav1.ConditionFalse, "S3UserCredentialsCheckFailed",
+                        fmt.Sprintf("Checking the S3User %s's credentials on S3 server has failed", userResource.Name), err)
+        }
+
+        if !credentialsValid {
+                logger.Info("The secret containing the credentials will be deleted, and the user will be deleted from the S3 backend, then recreated (through another reconcile)")
+                r.deleteSecret(ctx, &userOwnedSecret)
+                err = r.S3Client.DeleteUser(userResource.Spec.AccessKey)
+                if err != nil {
+                        logger.Error(err, "Could not delete user on S3 server", "user", userResource.Name)
+                        return r.setS3UserStatusConditionAndUpdate(ctx, userResource, "OperatorFailed", metav1.ConditionFalse, "S3UserDeletionFailed",
+                                fmt.Sprintf("Deletion of S3user %s on S3 server has failed", userResource.Name), err)
+                }
+
+                return r.handleS3NewUser(ctx, userResource)
+
+        }
+
 	logger.Info("User was reconciled without error")
 
 	// Re-fetch the S3User to ensure we have the latest state after updating the secret