Repository: metal3-io/baremetal-operator (go)

Issue:
BMC passwords can sometimes be logged
Since b8dba2c016befacf2ff65004a2ff73008d5ab63a we use the nodeUpdater with its built-in logging to update the ironic node when the credentials change. This inadvertantly meant the BMC credentials will be logged in this case.

This should be very rare in practice; BMC credentials don't often change. However, it should be fixed.

Pull request:
Avoid logging BMC password when creds change
Since b8dba2c016befacf2ff65004a2ff73008d5ab63a we use the nodeUpdater
with its built-in logging to update the ironic node when the credentials
change. This inadvertantly meant the BMC credentials will be logged in
this case (which is very rare; credentials don't often change).

To prevent this, sanitise new values as they are being logged. Ironic
does not return existing values for passwords, so there is no danger of
them being logged through that route. Currently password fields are only
redacted if they are set as part of a map; in practice this will be safe
for the foreseeable future as the bmc credentials are always set through
a map returned from the driver and not individually.

Fixes #980

Patch:
diff --git a/pkg/provisioner/ironic/update_opts.go b/pkg/provisioner/ironic/update_opts.go
index ace48f52..a0ff027c 100644
--- a/pkg/provisioner/ironic/update_opts.go
+++ b/pkg/provisioner/ironic/update_opts.go
@@ -3,6 +3,7 @@ package ironic
 import (
 	"fmt"
 	"reflect"
+	"strings"
 
 	"github.com/go-logr/logr"
 
@@ -72,6 +73,27 @@ func deref(v interface{}) interface{} {
 	return ptrVal.Elem().Interface()
 }
 
+func sanitisedValue(data interface{}) interface{} {
+	dataType := reflect.TypeOf(data)
+	if dataType.Kind() != reflect.Map ||
+		dataType.Key().Kind() != reflect.String {
+		return data
+	}
+
+	value := reflect.ValueOf(data)
+	safeValue := reflect.MakeMap(dataType)
+
+	for _, k := range value.MapKeys() {
+		safeDatumValue := value.MapIndex(k)
+		if strings.Contains(k.String(), "password") {
+			safeDatumValue = reflect.ValueOf("<redacted>")
+		}
+		safeValue.SetMapIndex(k, safeDatumValue)
+	}
+
+	return safeValue.Interface()
+}
+
 func getUpdateOperation(name string, currentData map[string]interface{}, desiredValue interface{}, path string, log logr.Logger) *nodes.UpdateOperation {
 	current, present := currentData[name]
 
@@ -81,10 +103,11 @@ func getUpdateOperation(name string, currentData map[string]interface{}, desired
 			if log != nil {
 				if present {
 					log.Info("updating option data",
-						"value", desiredValue, "old_value", current)
+						"value", sanitisedValue(desiredValue),
+						"old_value", current)
 				} else {
 					log.Info("adding option data",
-						"value", desiredValue)
+						"value", sanitisedValue(desiredValue))
 				}
 			}
 			return &nodes.UpdateOperation{


Test patch:
diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go
index 44044962..57a26009 100644
--- a/pkg/provisioner/ironic/updateopts_test.go
+++ b/pkg/provisioner/ironic/updateopts_test.go
@@ -1229,3 +1229,31 @@ func TestGetUpdateOptsForNodeSecureBoot(t *testing.T) {
 		})
 	}
 }
+
+func TestSanitisedValue(t *testing.T) {
+	unchanged := []interface{}{
+		"foo",
+		42,
+		true,
+		[]string{"bar", "baz"},
+		map[string]string{"foo": "bar"},
+		map[string][]string{"foo": {"bar", "baz"}},
+		map[string]interface{}{"foo": []string{"bar", "baz"}, "bar": 42},
+	}
+
+	for _, u := range unchanged {
+		assert.Exactly(t, u, sanitisedValue(u))
+	}
+
+	unsafe := map[string]interface{}{
+		"foo":           "bar",
+		"password":      "secret",
+		"ipmi_password": "secret",
+	}
+	safe := map[string]interface{}{
+		"foo":           "bar",
+		"password":      "<redacted>",
+		"ipmi_password": "<redacted>",
+	}
+	assert.Exactly(t, safe, sanitisedValue(unsafe))
+}