Merge pull request #4378 from stbenjam/multicast-mac

baremetal: validate that macs are EUI-48 and unicast

data/data/install.openshift.io_installconfigs.yaml

@@ -852,7 +852,7 @@ spec:
                     description: External bridge is used for external communication.
                     type: string
                   externalMACAddress:
-                    description: ExternalMACAddress is used to allow setting a static MAC address for the bootstrap host on the external network. If left blank, libvirt will generate one for you.
+                    description: ExternalMACAddress is used to allow setting a static unicast MAC address for the bootstrap host on the external network. Consider using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will generate one for you.
                     type: string
                   hosts:
                     description: Hosts is the information needed to create the objects in Ironic.
@@ -953,7 +953,7 @@ spec:
                     description: ClusterProvisioningIP is the IP on the dedicated provisioning network where the baremetal-operator pod runs provisioning services, and an http server to cache some downloaded content e.g RHCOS/IPA images
                     type: string
                   provisioningMACAddress:
-                    description: ProvisioningMACAddress is used to allow setting a static MAC address for the bootstrap host on the provisioning network. If left blank, libvirt will generate one for you.
+                    description: ProvisioningMACAddress is used to allow setting a static unicast MAC address for the bootstrap host on the provisioning network. Consider using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will generate one for you.
                     type: string
                   provisioningNetwork:
                     default: Managed

pkg/types/baremetal/platform.go

@@ -83,8 +83,9 @@ type Platform struct {
 	// +optional
 	ExternalBridge string `json:"externalBridge,omitempty"`
 
-	// ExternalMACAddress is used to allow setting a static MAC address for
-	// the bootstrap host on the external network. If left blank, libvirt will
+	// ExternalMACAddress is used to allow setting a static unicast MAC
+	// address for the bootstrap host on the external network. Consider
+	// using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will
 	// generate one for you.
 	// +optional
 	ExternalMACAddress string `json:"externalMACAddress,omitempty"`
@@ -99,8 +100,9 @@ type Platform struct {
 	// +optional
 	ProvisioningBridge string `json:"provisioningBridge,omitempty"`
 
-	// ProvisioningMACAddress is used to allow setting a static MAC address for
-	// the bootstrap host on the provisioning network. If left blank, libvirt will
+	// ProvisioningMACAddress is used to allow setting a static unicast MAC
+	// address for the bootstrap host on the provisioning network. Consider
+	// using the QEMU vendor prefix `52:54:00`. If left blank, libvirt will
 	// generate one for you.
 	// +optional
 	ProvisioningMACAddress string `json:"provisioningMACAddress,omitempty"`

pkg/types/baremetal/validation/platform_test.go

@@ -309,6 +309,13 @@ func TestValidateProvisioning(t *testing.T) {
 				ExternalMACAddress("CA:FE:CA:FE:CA:FE").
 				build(),
 		},
+		{
+			name: "invalid_multicast_mac",
+			platform: platform().
+				ExternalMACAddress("7D:CE:E3:29:35:6F").
+				build(),
+			expected: "expected unicast mac address, found multicast",
+		},
 		{
 			name: "invalid_bootstrapprovip_wrongCIDR",
 			platform: platform().

pkg/validate/validate.go

@@ -184,10 +184,43 @@ func IP(ip string) error {
 	return nil
 }
 
-// MAC validates that a value is a valid mac address
+// MAC validates that a value is a valid unicast EUI-48 MAC address
 func MAC(addr string) error {
-	_, err := net.ParseMAC(addr)
-	return err
+	hwAddr, err := net.ParseMAC(addr)
+	if err != nil {
+		return err
+	}
+
+	// net.ParseMAC checks for any valid mac, including 20-octet infiniband
+	// MAC's. Let's make sure we have an EUI-48 MAC, consisting of 6 octets
+	if len(hwAddr) != 6 {
+		return fmt.Errorf("invalid MAC address")
+	}
+
+	// We also need to check that the MAC is a valid unicast address. A multicast
+	// address is an address where the least significant bit of the most significant
+	// byte is 1.
+	//
+	//      Example 1: Multicast MAC
+	//      ------------------------
+	//      7D:CE:E3:29:35:6F
+	//       ^--> most significant byte
+	//
+	//      0x7D is 0b11111101
+	//                       ^--> this is a multicast MAC
+	//
+	//      Example 2: Unicast MAC
+	//      ----------------------
+	//      7A:CE:E3:29:35:6F
+	//       ^--> most significant byte
+	//
+	//      0x7A is 0b11111010
+	//                       ^--> this is a unicast MAC
+	if hwAddr[0]&1 == 1 {
+		return fmt.Errorf("expected unicast mac address, found multicast")
+	}
+
+	return nil
 }
 
 // UUID validates that a uuid is non-empty and a valid uuid.

pkg/validate/validate_test.go

@@ -563,3 +563,41 @@ func TestURI(t *testing.T) {
 		})
 	}
 }
+
+func TestMAC(t *testing.T) {
+	cases := []struct {
+		name     string
+		addr     string
+		expected string
+	}{
+		{
+			name: "valid_mac",
+			addr: "7A:CE:E3:29:35:6F",
+		},
+		{
+			name:     "invalid_multicast",
+			addr:     "7D:CE:E3:29:35:6F",
+			expected: "expected unicast mac address",
+		},
+		{
+			name:     "invalid_infiniband",
+			addr:     "00-00-00-00-fe-80-00-00-00-00-00-00-02-00-5e-10-00-00-00-01",
+			expected: "invalid MAC address",
+		},
+		{
+			name:     "invalid_mac",
+			addr:     "this is a bad mac",
+			expected: "invalid MAC address",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := MAC(tc.addr)
+			if tc.expected == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.Regexp(t, tc.expected, err)
+			}
+		})
+	}
+}