fix: stop misclassifying plain S3 credentials as base64 (#2245)

internal/webhook/everest/v1alpha1/databasecluster_defaulter_test.go

@@ -37,10 +37,13 @@
 	_ = everestv1alpha1.AddToScheme(scheme)
 
 	const (
-		ns            = "test-ns"
-		secretName    = "s3-creds"
-		testAccessKey = "ZmFrZUFjY2Vzc0tleQ==" // base64 for "fakeAccessKey"
-		testSecretKey = "ZmFrZVNlY3JldEtleQ==" //nolint:gosec // base64 for "fakeSecretKey"
+		ns         = "test-ns"
+		secretName = "s3-creds"
+		// 32-char plain-alphanumeric strings: previously misclassified as
+		// base64 by the old IsBase64Encoded heuristic (length % 4 == 0).
+		// Regression case for openeverest/openeverest#2245.
+		testAccessKey = "AKIAIOSFODNN7EXAMPLEAAAAAAAAAAAA"
+		testSecretKey = "wJalrXUtnFEMIKAAAAAAAAAAAAAAAAAA" //nolint:gosec
 	)
 
 	apiObjects := []runtime.Object{
@@ -96,14 +99,14 @@
 	err := defaulter.Default(t.Context(), db)
 	require.NoError(t, err)
 
-	// Check that the credentials are removed from the spec
+	// Credentials are removed from the spec.
 	assert.Empty(t, db.Spec.DataSource.DataImport.Source.S3.AccessKeyID)
 	assert.Empty(t, db.Spec.DataSource.DataImport.Source.S3.SecretAccessKey)
 
-	// Check that the secret was created and contains the expected data
+	// Credentials land in StringData verbatim (see openeverest/openeverest#2245).
 	secret := &corev1.Secret{}
 	err = client.Get(t.Context(), types.NamespacedName{Namespace: ns, Name: secretName}, secret)
 	require.NoError(t, err)
-	assert.Equal(t, testAccessKey, string(secret.Data[accessKeyIDSecretKey]))
-	assert.Equal(t, testSecretKey, string(secret.Data[secretAccessKeySecretKey]))
+	assert.Equal(t, testAccessKey, secret.StringData[accessKeyIDSecretKey])
+	assert.Equal(t, testSecretKey, secret.StringData[secretAccessKeySecretKey])
 }

internal/webhook/everest/v1alpha1/dataimportjob_defaulter.go

@@ -31,7 +31,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
 	everestv1alpha1 "github.com/percona/everest-operator/api/everest/v1alpha1"
-	"github.com/percona/everest-operator/utils"
 )
 
 // SetupDataImportJobWebhookWithManager sets up the mutation webhook for DataImportJob.
@@ -115,19 +114,14 @@ func mutateS3CredentialsSecret(
 	secret *corev1.Secret,
 	accessKeyID, secretAccessKey string,
 ) error {
-	switch {
-	case utils.IsBase64Encoded(accessKeyID) && utils.IsBase64Encoded(secretAccessKey):
-		secret.Data = map[string][]byte{
-			accessKeyIDSecretKey:     []byte(accessKeyID),
-			secretAccessKeySecretKey: []byte(secretAccessKey),
-		}
-	case !utils.IsBase64Encoded(accessKeyID) && !utils.IsBase64Encoded(secretAccessKey):
-		secret.StringData = map[string]string{
-			accessKeyIDSecretKey:     accessKeyID,
-			secretAccessKeySecretKey: secretAccessKey,
-		}
-	default:
-		return errors.New("both accessKeyID and secretAccessKey must be either base64 encoded or not")
+	// Always treat user-provided credentials as plain strings and write to
+	// StringData; the API server will encode them when persisting. The
+	// previous base64 heuristic produced false positives for plain-text
+	// AWS keys whose length happened to be a multiple of 4, corrupting the
+	// stored secret. See openeverest/openeverest#2245.
+	secret.StringData = map[string]string{
+		accessKeyIDSecretKey:     accessKeyID,
+		secretAccessKeySecretKey: secretAccessKey,
 	}
 	return nil
 }

internal/webhook/everest/v1alpha1/dataimportjob_defaulter_test.go

@@ -27,27 +27,8 @@
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	everestv1alpha1 "github.com/percona/everest-operator/api/everest/v1alpha1"
-	"github.com/percona/everest-operator/utils"
 )
 
-func TestIsBase64Encoded(t *testing.T) {
-	t.Parallel()
-	cases := []struct {
-		input    string
-		expected bool
-	}{
-		{"", false},
-		{"Zm9v", true},     // "foo" in base64
-		{"Zm9vYmFy", true}, // "foobar" in base64
-		{"notbase64", false},
-		{"Zm9vYmFyIQ==", true}, // "foobar!" in base64
-		{"Zm9vYmFyIQ", false},  // invalid base64 (wrong padding)
-	}
-	for _, c := range cases {
-		assert.Equal(t, c.expected, utils.IsBase64Encoded(c.input), "input: %q", c.input)
-	}
-}
-
 func TestDataImportJobDefaulter(t *testing.T) {
 	t.Parallel()
 	scheme := runtime.NewScheme()
@@ -57,8 +38,11 @@
 	const (
 		ns         = "test-ns"
 		secretName = "s3-creds"
-		accessKey  = "ZmFrZUFjY2Vzc0tleQ==" // base64 for "fakeAccessKey"
-		secretKey  = "ZmFrZVNlY3JldEtleQ==" //nolint:gosec // base64 for "fakeSecretKey"
+		// 32-char plain-alphanumeric strings: previously misclassified as
+		// base64 by the old IsBase64Encoded heuristic (length % 4 == 0).
+		// Regression case for openeverest/openeverest#2245.
+		accessKey = "AKIAIOSFODNN7EXAMPLEAAAAAAAAAAAA"
+		secretKey = "wJalrXUtnFEMIKAAAAAAAAAAAAAAAAAA" //nolint:gosec
 	)
 
 	dij := &everestv1alpha1.DataImportJob{
@@ -90,14 +74,13 @@
 	err := defaulter.Default(t.Context(), dij)
 	require.NoError(t, err)
 
-	// Check that the credentials are removed from the spec
+	// Credentials are removed from the spec.
 	assert.Empty(t, dij.Spec.DataImportJobTemplate.Source.S3.AccessKeyID)
 	assert.Empty(t, dij.Spec.DataImportJobTemplate.Source.S3.SecretAccessKey)
 
-	// Check that the secret was created and contains the expected data
 	secret := &corev1.Secret{}
 	err = client.Get(t.Context(), types.NamespacedName{Namespace: ns, Name: secretName}, secret)
 	require.NoError(t, err)
-	assert.Equal(t, accessKey, string(secret.Data[accessKeyIDSecretKey]))
-	assert.Equal(t, secretKey, string(secret.Data[secretAccessKeySecretKey]))
+	assert.Equal(t, accessKey, secret.StringData[accessKeyIDSecretKey])
+	assert.Equal(t, secretKey, secret.StringData[secretAccessKeySecretKey])
 }