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

Issue:
Remove namespace field from secrets used by BMH API
**What steps did you take and what happened:**
The BareMetalHost CRD allows the userData, metaData, and networkData for
the provisioned host to be specified as links to k8s Secrets. There are
fields for both the Name and Namespace of the Secret, meaning that the
baremetal-operator will read a Secret from any namespace. A user with
access to create or edit a BareMetalHost can thus exfiltrate a Secret
from another namespace by using it as e.g. the userData for provisioning
some host (note that this need not be a real host, it could be a VM
somewhere).

**What did you expect to happen:**
We should fail with an error when a namespace is specified in the Secret
reference and it does not match the namespace of the BMH itself. We
should reject attempts to set such a configuration in the validating
webhook. And we should remove the namespace field in the next revision
of the API.

**Anything else you would like to add:**
https://github.com/metal3-io/baremetal-operator/security/advisories/GHSA-pqfh-xh7w-7h3p is limiting the secrets to same namespace now so API cleanup and validating webhook are just improvements/cleanup to the issue.

**Environment:**

- Baremetal Operator version: 0.8.0
- Environment (metal3-dev-env or other): any

@zaneb @dtantsur 


Pull request:
:bug: Add validation against cross-namespace secret references
Add to BMH creation a validation, which checks if any secret references is from namespaces other than BMH's. This is to prevent the secret leakage when a user references secrets from other ns just to get the secrets out.

Fixes #1965 


Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index f7c7c68d..01e950ff 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -1115,32 +1115,34 @@ func (host *BareMetalHost) OperationMetricForState(operation ProvisioningState)
 // GetChecksum method returns the checksum of an image.
 func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
 	if image == nil {
-		return "", "", false
+		return
 	}
 
 	if image.DiskFormat != nil && *image.DiskFormat == "live-iso" {
 		// Checksum is not required for live-iso
 		ok = true
-		return "", "", ok
+		return
 	}
 
 	if image.Checksum == "" {
 		// Return empty if checksum is not provided
-		return "", "", false
+		return
 	}
 
 	switch image.ChecksumType {
+	case "":
+		checksumType = string(MD5)
 	case MD5, SHA256, SHA512:
 		checksumType = string(image.ChecksumType)
-	case "", AutoChecksum:
+	case AutoChecksum:
 		// No type, let Ironic detect
 	default:
-		return "", "", false
+		return
 	}
 
 	checksum = image.Checksum
 	ok = true
-	return checksum, checksumType, ok
+	return
 }
 
 // +kubebuilder:object:root=true
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index 35d85d07..13da49ec 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -12,6 +12,9 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
+	corev1 "k8s.io/api/core/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 var (
@@ -34,6 +37,8 @@ func (host *BareMetalHost) validateHost() []error {
 		}
 	}
 
+	errs = append(errs, host.validateCrossNamespaceSecretReferences()...)
+
 	if raidErrors := validateRAID(host.Spec.RAID); raidErrors != nil {
 		errs = append(errs, raidErrors...)
 	}
@@ -309,3 +314,39 @@ func validateRebootAnnotation(rebootAnnotation string) error {
 
 	return nil
 }
+
+// validateCrossNamespaceSecretReferences validates that a SecretReference does not refer to a Secret
+// in a different namespace than the host resource.
+func validateCrossNamespaceSecretReferences(hostNamespace, hostName, fieldName string, ref *corev1.SecretReference) error {
+	if ref != nil &&
+		ref.Namespace != "" &&
+		ref.Namespace != hostNamespace {
+		return k8serrors.NewForbidden(
+			schema.GroupResource{
+				Group:    "metal3.io",
+				Resource: "baremetalhosts",
+			},
+			hostName,
+			fmt.Errorf("%s: cross-namespace Secret references are not allowed", fieldName),
+		)
+	}
+	return nil
+}
+
+// validateCrossNamespaceSecretReferences checks all Secret references in the BareMetalHost spec
+// to ensure they do not reference Secrets from other namespaces. This includes userData,
+// networkData, and metaData Secret references.
+func (host *BareMetalHost) validateCrossNamespaceSecretReferences() []error {
+	secretRefs := map[*corev1.SecretReference]string{
+		host.Spec.UserData:    "userData",
+		host.Spec.NetworkData: "networkData",
+		host.Spec.MetaData:    "metaData",
+	}
+	errs := []error{}
+	for ref, fieldName := range secretRefs {
+		if err := validateCrossNamespaceSecretReferences(host.Namespace, host.Name, fieldName, ref); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	return errs
+}


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go
index 242d73ae..6c49ad23 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go
@@ -338,7 +338,7 @@ func TestGetImageChecksum(t *testing.T) {
 			ExpectedType: "md5",
 		},
 		{
-			Scenario: "checksum value specified and type not specified, default to auto type",
+			Scenario: "checksum value specified but not type",
 			Host: BareMetalHost{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "myhost",
@@ -351,7 +351,7 @@ func TestGetImageChecksum(t *testing.T) {
 				},
 			},
 			Expected:     true,
-			ExpectedType: "",
+			ExpectedType: "md5",
 		},
 		{
 			Scenario: "checksum value specified, auto type",
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 0222249c..2ee11ed8 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -678,6 +679,97 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "invalid value for the inspect.metal3.io annotation, allowed are \"disabled\" or \"\"",
 		},
+		{
+			name: "crossNamespaceUserData",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					UserData: &corev1.SecretReference{
+						Name:      "test-secret",
+						Namespace: "different-namespace", // Different from host's namespace
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "baremetalhosts.metal3.io \"test\" is forbidden: userData: cross-namespace Secret references are not allowed",
+		},
+		{
+			name: "crossNamespaceNetworkData",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					NetworkData: &corev1.SecretReference{
+						Name:      "test-secret",
+						Namespace: "different-namespace", // Different from host's namespace
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "baremetalhosts.metal3.io \"test\" is forbidden: networkData: cross-namespace Secret references are not allowed",
+		},
+		{
+			name: "crossNamespaceMetaData",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					MetaData: &corev1.SecretReference{
+						Name:      "test-secret",
+						Namespace: "different-namespace", // Different from host's namespace
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "baremetalhosts.metal3.io \"test\" is forbidden: metaData: cross-namespace Secret references are not allowed",
+		},
+		{
+			name: "multipleSecretsCrossNamespace",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					UserData: &corev1.SecretReference{
+						Name:      "test-secret1",
+						Namespace: "different-namespace1",
+					},
+					NetworkData: &corev1.SecretReference{
+						Name:      "test-secret2",
+						Namespace: "different-namespace2",
+					},
+					MetaData: &corev1.SecretReference{
+						Name:      "test-secret3",
+						Namespace: "different-namespace3",
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "baremetalhosts.metal3.io \"test\" is forbidden: userData: cross-namespace Secret references are not allowed", // Should catch at least one error
+		},
+		{
+			name: "sameNamespaceSecrets",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om, // namespace is "test-namespace"
+				Spec: BareMetalHostSpec{
+					UserData: &corev1.SecretReference{
+						Name:      "test-secret1",
+						Namespace: "test-namespace", // Same as host's namespace
+					},
+					NetworkData: &corev1.SecretReference{
+						Name:      "test-secret2",
+						Namespace: "test-namespace", // Same as host's namespace
+					},
+					MetaData: &corev1.SecretReference{
+						Name:      "test-secret3",
+						Namespace: "test-namespace", // Same as host's namespace
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "", // Should be valid
+		},
 	}
 
 	for _, tt := range tests {