From 92ebf0d15ead3054afed185f739fafc2c4c59111 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Fri, 4 Oct 2024 17:13:57 +0300 Subject: [PATCH] More robust MAC address matching On darwin bootp uses non-standard MAC address format[1]: "8e:1:99:9c:54:b1" instead of "8e:01:99:9c:54:b1". We fixed this by trimming leading "0" in the string before looking up the IP address. There are several issues with the current code: - Fragile, will break if bootp changes the format (unlikely) - Fixing the wrong place, the drivers should not care about the MAC address format. - The tests were confusing, showing that we can match standard MAC addresses while the actual code could match only non-standard bootp addresses. - Logging wrong MAC address since we trimmed leading zeros before logging This change replace trimming leading zeros with parsing MAC address strings and comparing the bytes. The test includes now both standard and non-standard MAC addresses. [1] https://openradar.appspot.com/FB15382970 --- pkg/drivers/common.go | 43 +++++++++++++++++++++++++++------- pkg/drivers/common_test.go | 15 ++++++++++++ pkg/drivers/hyperkit/driver.go | 2 -- pkg/drivers/qemu/qemu.go | 5 +--- pkg/drivers/vfkit/vfkit.go | 2 -- 5 files changed, 50 insertions(+), 17 deletions(-) diff --git a/pkg/drivers/common.go b/pkg/drivers/common.go index d7e3d3c44911..f17590838f5f 100644 --- a/pkg/drivers/common.go +++ b/pkg/drivers/common.go @@ -18,11 +18,12 @@ package drivers import ( "bufio" + "bytes" "fmt" "io" + "net" "os" "path/filepath" - "regexp" "strings" "syscall" @@ -40,8 +41,6 @@ import ( // LeasesPath is the path to dhcpd leases const LeasesPath = "/var/db/dhcpd_leases" -var leadingZeroRegexp = regexp.MustCompile(`0([A-Fa-f0-9](:|$))`) - // This file is for common code shared among internal machine drivers // Code here should not be called from within minikube @@ -190,7 +189,7 @@ func fixMachinePermissions(path string) error { type DHCPEntry struct { Name string IPAddress string - HWAddress string + HWAddress net.HardwareAddr ID string Lease string } @@ -201,6 +200,13 @@ func GetIPAddressByMACAddress(mac string) (string, error) { } func getIPAddressFromFile(mac, path string) (string, error) { + // Due to https://openradar.appspot.com/FB15382970 we need to parse the MAC + // address and compare the bytes. + macAddress, err := parseMAC(mac) + if err != nil { + return "", err + } + log.Debugf("Searching for %s in %s ...", mac, path) file, err := os.Open(path) if err != nil { @@ -215,7 +221,10 @@ func getIPAddressFromFile(mac, path string) (string, error) { log.Debugf("Found %d entries in %s!", len(dhcpEntries), path) for _, dhcpEntry := range dhcpEntries { log.Debugf("dhcp entry: %+v", dhcpEntry) - if dhcpEntry.HWAddress == mac { + if dhcpEntry.HWAddress == nil { + continue + } + if bytes.Equal(dhcpEntry.HWAddress, macAddress) { log.Debugf("Found match: %s", mac) return dhcpEntry.IPAddress, nil } @@ -252,7 +261,13 @@ func parseDHCPdLeasesFile(file io.Reader) ([]DHCPEntry, error) { dhcpEntry.IPAddress = val case "hw_address": // The mac addresses have a '1,' at the start. - dhcpEntry.HWAddress = val[2:] + macAddress, err := parseMAC(val[2:]) + if err != nil { + log.Warnf("unable to parse hw_address in dhcp leases file: %q: %s", + val[2:], err) + continue + } + dhcpEntry.HWAddress = macAddress case "identifier": dhcpEntry.ID = val case "lease": @@ -264,7 +279,17 @@ func parseDHCPdLeasesFile(file io.Reader) ([]DHCPEntry, error) { return dhcpEntries, scanner.Err() } -// TrimMacAddress trimming "0" of the ten's digit -func TrimMacAddress(rawUUID string) string { - return leadingZeroRegexp.ReplaceAllString(rawUUID, "$1") +// parseMAC parse both standard fixeed size MAC address "%02x:..." and the +// variable size MAC address on drawin "%x:...". +func parseMAC(mac string) (net.HardwareAddr, error) { + hw := make(net.HardwareAddr, 6) + n, err := fmt.Sscanf(mac, "%x:%x:%x:%x:%x:%x", + &hw[0], &hw[1], &hw[2], &hw[3], &hw[4], &hw[5]) + if n != len(hw) { + return nil, fmt.Errorf("invalid MAC address: %q", mac) + } + if err != nil { + return nil, fmt.Errorf("unable to parse MAC address: %q: %s", mac, err) + } + return hw, nil } diff --git a/pkg/drivers/common_test.go b/pkg/drivers/common_test.go index 3a6d0569e557..ee35cdb2367a 100644 --- a/pkg/drivers/common_test.go +++ b/pkg/drivers/common_test.go @@ -54,6 +54,13 @@ var validLeases = []byte(`{ identifier=1,a2:b3:c4:d5:e6:f7 lease=0x597e1267 } +{ + name=bootp + ip_address=192.168.105.2 + hw_address=1,8e:1:99:9c:54:b1 + identifier=1,8e:1:99:9c:54:b1 + lease=0x597e1268 +} { name=bar ip_address=192.168.64.3 @@ -98,6 +105,14 @@ func Test_getIpAddressFromFile(t *testing.T) { "1.2.3.4", false, }, + { + // bootp use "%x" format instead of "%02x" + // https://openradar.appspot.com/FB15382970 + "valid-bootp", + args{"8e:01:99:9c:54:b1", dhcpFile}, + "192.168.105.2", + false, + }, { "duplicate", args{"a4:b5:c6:d7:e8:f9", dhcpFile}, diff --git a/pkg/drivers/hyperkit/driver.go b/pkg/drivers/hyperkit/driver.go index 8516655caf3b..2147f3a03729 100644 --- a/pkg/drivers/hyperkit/driver.go +++ b/pkg/drivers/hyperkit/driver.go @@ -261,8 +261,6 @@ func (d *Driver) Start() error { return errors.Wrap(err, "getting MAC address from UUID") } - // Need to strip 0's - mac = pkgdrivers.TrimMacAddress(mac) log.Debugf("Generated MAC %s", mac) log.Debugf("Starting with cmdline: %s", d.Cmdline) diff --git a/pkg/drivers/qemu/qemu.go b/pkg/drivers/qemu/qemu.go index 018974d07f6e..5ccc8659da71 100644 --- a/pkg/drivers/qemu/qemu.go +++ b/pkg/drivers/qemu/qemu.go @@ -505,10 +505,7 @@ func (d *Driver) Start() error { case "socket_vmnet": var err error getIP := func() error { - // QEMU requires MAC address with leading 0s - // But socket_vmnet writes the MAC address to the dhcp leases file with leading 0s stripped - mac := pkgdrivers.TrimMacAddress(d.MACAddress) - d.IPAddress, err = pkgdrivers.GetIPAddressByMACAddress(mac) + d.IPAddress, err = pkgdrivers.GetIPAddressByMACAddress(d.MACAddress) if err != nil { return errors.Wrap(err, "failed to get IP address") } diff --git a/pkg/drivers/vfkit/vfkit.go b/pkg/drivers/vfkit/vfkit.go index 7f3bfd36e9be..cee4727a722a 100644 --- a/pkg/drivers/vfkit/vfkit.go +++ b/pkg/drivers/vfkit/vfkit.go @@ -265,8 +265,6 @@ func (d *Driver) Start() error { return err } - // Need to strip 0's - mac = pkgdrivers.TrimMacAddress(mac) if err := d.setupIP(mac); err != nil { return err }