Repository: metal3-io/baremetal-operator
Pull request #881 (2021-05-17 14:21:29)

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.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/881#" title="Author self-approved">dhellmann</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dtantsur: /test-integration

LGTM, but let's mention this behavior at least somewhere in the docs?
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/881#pullrequestreview-661065158" title="Approved">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/881#" title="Author self-approved">dhellmann</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 1cfeb741..6086095c 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/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()
diff --git a/docs/api.md b/docs/api.md
index ba1bef00..72de94cd 100644
--- a/docs/api.md
+++ b/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.


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 277b1e9c..91006bb8 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/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)
+		})
+	}
+}