strip whitespace after decoding secret values

We have had several users report problems with credential management
that turned out to be caused by extra whitespace in the encoded values
placed into the secrets they created. Most often this is caused by
using command line tools to encode the values. For example,

$ echo -n root | base64
cm9vdA==
$ echo root | base64
cm9vdAo=

This change assumes that extraneous whitespace in the encoded values
is not part of the real username or password, and strips it before
trying to use the credentials. The logic to extract the credentials
from the secret is moved into its own function so that new unit tests
can be added.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Author: dhellmann

controllers/metal3.io/baremetalhost_controller.go

@@ -1204,6 +1204,20 @@ func (r *BareMetalHostReconciler) getBMCSecretAndSetOwner(request ctrl.Request,
 	return bmcCredsSecret, nil
 }
 
+func credentialsFromSecret(bmcCredsSecret *corev1.Secret) *bmc.Credentials {
+	// We trim surrounding whitespace because those characters are
+	// unlikely to be part of the username or password and it is
+	// common for users to encode the values with a command like
+	//
+	//     echo "my-password" | base64
+	//
+	// which introduces a trailing newline.
+	return &bmc.Credentials{
+		Username: strings.TrimSpace(string(bmcCredsSecret.Data["username"])),
+		Password: strings.TrimSpace(string(bmcCredsSecret.Data["password"])),
+	}
+}
+
 // Make sure the credentials for the management controller look
 // right and manufacture bmc.Credentials.  This does not actually try
 // to use the credentials.
@@ -1221,10 +1235,7 @@ func (r *BareMetalHostReconciler) buildAndValidateBMCCredentials(request ctrl.Re
 		return nil, nil, &EmptyBMCAddressError{message: "Missing BMC connection detail 'Address'"}
 	}
 
-	bmcCreds = &bmc.Credentials{
-		Username: string(bmcCredsSecret.Data["username"]),
-		Password: string(bmcCredsSecret.Data["password"]),
-	}
+	bmcCreds = credentialsFromSecret(bmcCredsSecret)
 
 	// Verify that the secret contains the expected info.
 	err = bmcCreds.Validate()

controllers/metal3.io/baremetalhost_controller_test.go

@@ -23,6 +23,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
 	"github.com/metal3-io/baremetal-operator/pkg/utils"
 )
@@ -2000,3 +2001,63 @@ func TestInvalidBMHCanBeDeleted(t *testing.T) {
 		return host.Status.Provisioning.State == metal3v1alpha1.StateDeleting && len(host.Finalizers) == 0
 	})
 }
+
+func TestCredentialsFromSecret(t *testing.T) {
+	cases := []struct {
+		name     string
+		input    corev1.Secret
+		expected bmc.Credentials
+	}{
+		{
+			name:     "empty",
+			input:    corev1.Secret{},
+			expected: bmc.Credentials{},
+		},
+		{
+			name: "clean",
+			input: corev1.Secret{
+				Data: map[string][]byte{
+					"username": []byte("username"),
+					"password": []byte("password"),
+				},
+			},
+			expected: bmc.Credentials{
+				Username: "username",
+				Password: "password",
+			},
+		},
+		{
+			name: "newline",
+			input: corev1.Secret{
+				Data: map[string][]byte{
+					"username": []byte("username\n"),
+					"password": []byte("password\n"),
+				},
+			},
+			expected: bmc.Credentials{
+				Username: "username",
+				Password: "password",
+			},
+		},
+		{
+			name: "non-newline",
+			input: corev1.Secret{
+				Data: map[string][]byte{
+					"username": []byte(" username\t"),
+					"password": []byte(" password\t"),
+				},
+			},
+			expected: bmc.Credentials{
+				Username: "username",
+				Password: "password",
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			actual := credentialsFromSecret(&c.input)
+			assert.Equal(t, c.expected, *actual)
+		})
+	}
+}

docs/api.md

@@ -475,6 +475,10 @@ data:
   password: cGFzc3dvcmQ=
 ```
 
+NOTE: After decoding the secret content, whitespace is stripped from
+the beginning and end before the username and password values are
+used.
+
 ## Triggering Provisioning
 
 Several conditions must be met in order to initiate provisioning.