file: Rework leases file parsing to allow trailing comments

Also warn on duplicate IP/MAC addresses and use netip for a better API

Signed-off-by: Rick Beton <rick.beton@gmail.com>
Signed-off-by: Anatole Denis <natolumin@unverle.fr>

plugins/file/plugin.go

@@ -4,38 +4,59 @@
 
 // Package file enables static mapping of MAC <--> IP addresses.
 // The mapping is stored in a text file, where each mapping is described by one line containing
-// two fields separated by spaces: MAC address, and IP address. For example:
+// two fields separated by whitespace: MAC address and IP address. For example:
 //
-//  $ cat file_leases.txt
-//  00:11:22:33:44:55 10.0.0.1
-//  01:23:45:67:89:01 10.0.10.10
+//	$ cat leases_v4.txt
+//	# IPv4 fixed addresses
+//	00:11:22:33:44:55 10.0.0.1
+//	a1:b2:c3:d4:e5:f6 10.0.10.10  # lowercase is permitted
+//
+//	$ cat leases_v6.txt
+//	# IPv6 fixed addresses
+//	00:11:22:33:44:55 2001:db8::10:1
+//	A1:B2:C3:D4:E5:F6 2001:db8::10:2
+//
+// Any text following '#' is a comment that is ignored.
+//
+// MAC addresses can be upper or lower case. IPv6 addresses should use lowercase, as per RFC-5952.
+//
+// Each MAC or IP address should normally be unique within the file. Warnings will be logged for
+// any duplicates.
 //
 // To specify the plugin configuration in the server6/server4 sections of the config file, just
 // pass the leases file name as plugin argument, e.g.:
 //
-//  $ cat config.yml
+//	$ cat config.yml
 //
-//  server6:
-//     ...
-//     plugins:
-//       - file: "file_leases.txt" [autorefresh]
-//     ...
+//	server6:
+//	   ...
+//	   plugins:
+//	     - file: "file_leases.txt" [autorefresh]
+//	   ...
 //
 // If the file path is not absolute, it is relative to the cwd where coredhcp is run.
 //
-// Optionally, when the 'autorefresh' argument is given, the plugin will try to refresh
-// the lease mapping during runtime whenever the lease file is updated.
+// The optional keyword 'autorefresh' can be used as shown, or it can be omitted. When
+// present, the plugin will try to refresh the lease mapping during runtime whenever
+// the lease file is updated.
+//
+// For DHCPv4 `server4`, note that the file plugin must come after any general plugins
+// needed, e.g. dns or router. The order is unimportant for DHCPv6, but will affect the
+// order of options in the DHCPv6 response.
 package file
 
 import (
-	"bytes"
+	"bufio"
 	"errors"
 	"fmt"
 	"net"
+	"net/netip"
 	"os"
+	"sort"
 	"strings"
 	"sync"
 	"time"
+	"unicode"
 
 	"github.com/coredhcp/coredhcp/handler"
 	"github.com/coredhcp/coredhcp/logger"
@@ -61,86 +82,94 @@ var Plugin = plugins.Plugin{
 var recLock sync.RWMutex
 
 // StaticRecords holds a MAC -> IP address mapping
-var StaticRecords map[string]net.IP
-
-// DHCPv6Records and DHCPv4Records are mappings between MAC addresses in
-// form of a string, to network configurations.
-var (
-	DHCPv6Records map[string]net.IP
-	DHCPv4Records map[string]net.IP
-)
+var StaticRecords map[string]netip.Addr
 
 // LoadDHCPv4Records loads the DHCPv4Records global map with records stored on
 // the specified file. The records have to be one per line, a mac address and an
 // IPv4 address.
-func LoadDHCPv4Records(filename string) (map[string]net.IP, error) {
-	log.Infof("reading leases from %s", filename)
-	data, err := os.ReadFile(filename)
-	if err != nil {
-		return nil, err
-	}
-	records := make(map[string]net.IP)
-	for _, lineBytes := range bytes.Split(data, []byte{'\n'}) {
-		line := string(lineBytes)
-		if len(line) == 0 {
-			continue
-		}
-		if strings.HasPrefix(line, "#") {
-			continue
-		}
-		tokens := strings.Fields(line)
-		if len(tokens) != 2 {
-			return nil, fmt.Errorf("malformed line, want 2 fields, got %d: %s", len(tokens), line)
-		}
-		hwaddr, err := net.ParseMAC(tokens[0])
-		if err != nil {
-			return nil, fmt.Errorf("malformed hardware address: %s", tokens[0])
-		}
-		ipaddr := net.ParseIP(tokens[1])
-		if ipaddr.To4() == nil {
-			return nil, fmt.Errorf("expected an IPv4 address, got: %v", ipaddr)
-		}
-		records[hwaddr.String()] = ipaddr
-	}
-
-	return records, nil
+func LoadDHCPv4Records(filename string) (map[string]netip.Addr, error) {
+	return loadDHCPRecords(filename, 4, netip.Addr.Is4)
 }
 
 // LoadDHCPv6Records loads the DHCPv6Records global map with records stored on
 // the specified file. The records have to be one per line, a mac address and an
 // IPv6 address.
-func LoadDHCPv6Records(filename string) (map[string]net.IP, error) {
-	log.Infof("reading leases from %s", filename)
-	data, err := os.ReadFile(filename)
+func LoadDHCPv6Records(filename string) (map[string]netip.Addr, error) {
+	return loadDHCPRecords(filename, 6, netip.Addr.Is6)
+}
+
+// loadDHCPRecords loads the MAC<->IP mappings with records stored on
+// the specified file. The records have to be one per line, a mac address and an
+// IP address.
+func loadDHCPRecords(filename string, protVer int, check func(netip.Addr) bool) (map[string]netip.Addr, error) {
+	log.Infof("reading IPv%d leases from %s", protVer, filename)
+	addresses := make(map[string]int)
+	f, err := os.Open(filename)
 	if err != nil {
 		return nil, err
 	}
-	records := make(map[string]net.IP)
-	for _, lineBytes := range bytes.Split(data, []byte{'\n'}) {
-		line := string(lineBytes)
-		if len(line) == 0 {
-			continue
+	defer f.Close() //nolint:errcheck // read-only open()
+
+	records := make(map[string]netip.Addr)
+	scanner := bufio.NewScanner(f)
+	lineNo := 0
+	for scanner.Scan() {
+		line := strings.TrimSpace(scanner.Text())
+		lineNo++
+		if comment := strings.IndexRune(line, '#'); comment >= 0 {
+			line = strings.TrimRightFunc(line[:comment], unicode.IsSpace)
 		}
-		if strings.HasPrefix(line, "#") {
+		if len(line) == 0 {
 			continue
 		}
+
 		tokens := strings.Fields(line)
 		if len(tokens) != 2 {
-			return nil, fmt.Errorf("malformed line, want 2 fields, got %d: %s", len(tokens), line)
+			return nil, fmt.Errorf("%s:%d malformed line, want 2 fields, got %d: %s", filename, lineNo, len(tokens), line)
 		}
 		hwaddr, err := net.ParseMAC(tokens[0])
 		if err != nil {
-			return nil, fmt.Errorf("malformed hardware address: %s", tokens[0])
+			return nil, fmt.Errorf("%s:%d malformed hardware address: %s", filename, lineNo, tokens[0])
 		}
-		ipaddr := net.ParseIP(tokens[1])
-		if ipaddr.To16() == nil || ipaddr.To4() != nil {
-			return nil, fmt.Errorf("expected an IPv6 address, got: %v", ipaddr)
+		ipaddr, err := netip.ParseAddr(tokens[1])
+		if err != nil {
+			return nil, fmt.Errorf("%s:%d expected an IPv%d address, got: %s", filename, lineNo, protVer, tokens[1])
+		}
+		if !check(ipaddr) {
+			return nil, fmt.Errorf("%s:%d expected an IPv%d address, got: %s", filename, lineNo, protVer, ipaddr)
 		}
+
+		// note that net.HardwareAddr.String() uses lowercase hexadecimal
+		// so there's no need to convert to lowercase
 		records[hwaddr.String()] = ipaddr
+		addresses[strings.ToLower(tokens[0])]++
+		addresses[strings.ToLower(tokens[1])]++
 	}
+
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+
+	duplicatesWarning(addresses)
+
 	return records, nil
 }
 
+func duplicatesWarning(ipAddresses map[string]int) {
+	var duplicates []string
+	for ipAddress, count := range ipAddresses {
+		if count > 1 {
+			duplicates = append(duplicates, fmt.Sprintf("Address %s is in %d records", ipAddress, count))
+		}
+	}
+
+	sort.Strings(duplicates)
+
+	for _, warning := range duplicates {
+		log.Warning(warning)
+	}
+}
+
 // Handler6 handles DHCPv6 packets for the file plugin
 func Handler6(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool) {
 	m, err := req.GetInnerMessage()
@@ -175,7 +204,7 @@ func Handler6(req, resp dhcpv6.DHCPv6) (dhcpv6.DHCPv6, bool) {
 		IaId: m.Options.OneIANA().IaId,
 		Options: dhcpv6.IdentityOptions{Options: []dhcpv6.Option{
 			&dhcpv6.OptIAAddress{
-				IPv6Addr:          ipaddr,
+				IPv6Addr:          ipaddr.AsSlice(),
 				PreferredLifetime: 3600 * time.Second,
 				ValidLifetime:     3600 * time.Second,
 			},
@@ -194,8 +223,8 @@ func Handler4(req, resp *dhcpv4.DHCPv4) (*dhcpv4.DHCPv4, bool) {
 		log.Warningf("MAC address %s is unknown", req.ClientHWAddr.String())
 		return resp, false
 	}
-	resp.YourIPAddr = ipaddr
 	log.Debugf("found IP address %s for MAC %s", ipaddr, req.ClientHWAddr.String())
+	resp.YourIPAddr = ipaddr.AsSlice()
 	return resp, true
 }
 
@@ -260,7 +289,7 @@ func setupFile(v6 bool, args ...string) (handler.Handler6, handler.Handler4, err
 
 func loadFromFile(v6 bool, filename string) error {
 	var err error
-	var records map[string]net.IP
+	var records map[string]netip.Addr
 	var protver int
 	if v6 {
 		protver = 6

plugins/file/plugin_test.go

@@ -6,6 +6,7 @@ package file
 
 import (
 	"net"
+	"net/netip"
 	"os"
 	"testing"
 	"time"
@@ -25,12 +26,11 @@ func TestLoadDHCPv4Records(t *testing.T) {
 			require.NoError(t, os.Remove(tmp.Name()))
 		}()
 
-		// fill temp file with valid lease lines and some comments
-		_, err = tmp.WriteString("00:11:22:33:44:55 192.0.2.100\n")
-		require.NoError(t, err)
-		_, err = tmp.WriteString("11:22:33:44:55:66 192.0.2.101\n")
-		require.NoError(t, err)
-		_, err = tmp.WriteString("# this is a comment\n")
+		// fill temp file with valid lease lines (mixed case) and some comments
+		_, err = tmp.WriteString(`00:11:22:33:44:aa 192.0.2.100
+ 11:BB:33:DD:55:FF 	 192.0.2.101  # arbitrary spaces and trailing comment
+ # this is a simple comment
+`)
 		require.NoError(t, err)
 		require.NoError(t, tmp.Close())
 
@@ -40,11 +40,11 @@ func TestLoadDHCPv4Records(t *testing.T) {
 		}
 
 		if assert.Equal(t, 2, len(records)) {
-			if assert.Contains(t, records, "00:11:22:33:44:55") {
-				assert.Equal(t, net.ParseIP("192.0.2.100"), records["00:11:22:33:44:55"])
+			if assert.Contains(t, records, "00:11:22:33:44:aa") {
+				assert.Equal(t, netip.MustParseAddr("192.0.2.100"), records["00:11:22:33:44:aa"])
 			}
-			if assert.Contains(t, records, "11:22:33:44:55:66") {
-				assert.Equal(t, net.ParseIP("192.0.2.101"), records["11:22:33:44:55:66"])
+			if assert.Contains(t, records, "11:bb:33:dd:55:ff") {
+				assert.Equal(t, netip.MustParseAddr("192.0.2.101"), records["11:bb:33:dd:55:ff"])
 			}
 		}
 	})
@@ -100,6 +100,45 @@ func TestLoadDHCPv4Records(t *testing.T) {
 		assert.Error(t, err)
 	})
 
+	t.Run("duplicate MAC address are allowed", func(t *testing.T) {
+		// setup temp leases file
+		tmp, err := os.CreateTemp("", "test_plugin_file")
+		require.NoError(t, err)
+		defer func() {
+			require.NoError(t, os.Remove(tmp.Name()))
+		}()
+
+		// add lines with duplicate MAC addresses to check for no error
+		_, err = tmp.WriteString(`aa:11:11:11:11:11 1.2.3.4
+AA:11:11:11:11:11 5.6.7.8
+`)
+		require.NoError(t, err)
+		require.NoError(t, tmp.Close())
+
+		_, err = LoadDHCPv4Records(tmp.Name())
+		assert.NoError(t, err)
+	})
+
+	t.Run("duplicate IP address are allowed", func(t *testing.T) {
+		// setup temp leases file
+		tmp, err := os.CreateTemp("", "test_plugin_file")
+		require.NoError(t, err)
+		defer func() {
+			require.NoError(t, os.Remove(tmp.Name()))
+		}()
+
+		// add line with duplicate IPv4 addresses to check for no error
+		_, err = tmp.WriteString(`11:11:11:11:11:11 1.2.3.4
+22:22:22:22:22:22 1.2.3.4
+33:33:33:33:33:33 1.2.3.4
+`)
+		require.NoError(t, err)
+		require.NoError(t, tmp.Close())
+
+		_, err = LoadDHCPv4Records(tmp.Name())
+		assert.NoError(t, err)
+	})
+
 	t.Run("lease with IPv6 address should raise error", func(t *testing.T) {
 		// setup temp leases file
 		tmp, err := os.CreateTemp("", "test_plugin_file")
@@ -128,11 +167,10 @@ func TestLoadDHCPv6Records(t *testing.T) {
 		}()
 
 		// fill temp file with valid lease lines and some comments
-		_, err = tmp.WriteString("00:11:22:33:44:55 2001:db8::10:1\n")
-		require.NoError(t, err)
-		_, err = tmp.WriteString("11:22:33:44:55:66 2001:db8::10:2\n")
-		require.NoError(t, err)
-		_, err = tmp.WriteString("# this is a comment\n")
+		_, err = tmp.WriteString(`00:11:22:33:44:aa 2001:db8::10:1
+ 11:BB:33:DD:55:FF 	 2001:db8::10:2  # arbitrary spaces and trailing comment
+ # this is a simple comment
+`)
 		require.NoError(t, err)
 		require.NoError(t, tmp.Close())
 
@@ -142,11 +180,11 @@ func TestLoadDHCPv6Records(t *testing.T) {
 		}
 
 		if assert.Equal(t, 2, len(records)) {
-			if assert.Contains(t, records, "00:11:22:33:44:55") {
-				assert.Equal(t, net.ParseIP("2001:db8::10:1"), records["00:11:22:33:44:55"])
+			if assert.Contains(t, records, "00:11:22:33:44:aa") {
+				assert.Equal(t, netip.MustParseAddr("2001:db8::10:1"), records["00:11:22:33:44:aa"])
 			}
-			if assert.Contains(t, records, "11:22:33:44:55:66") {
-				assert.Equal(t, net.ParseIP("2001:db8::10:2"), records["11:22:33:44:55:66"])
+			if assert.Contains(t, records, "11:bb:33:dd:55:ff") {
+				assert.Equal(t, netip.MustParseAddr("2001:db8::10:2"), records["11:bb:33:dd:55:ff"])
 			}
 		}
 	})
@@ -202,6 +240,45 @@ func TestLoadDHCPv6Records(t *testing.T) {
 		assert.Error(t, err)
 	})
 
+	t.Run("duplicate MAC address are allowed", func(t *testing.T) {
+		// setup temp leases file
+		tmp, err := os.CreateTemp("", "test_plugin_file")
+		require.NoError(t, err)
+		defer func() {
+			require.NoError(t, os.Remove(tmp.Name()))
+		}()
+
+		// add lines with duplicate MAC addresses to trigger an error
+		_, err = tmp.WriteString(`aa:11:11:11:11:11 2001:db8::10:1
+AA:11:11:11:11:11 2001:db8::10:2
+`)
+		require.NoError(t, err)
+		require.NoError(t, tmp.Close())
+
+		_, err = LoadDHCPv6Records(tmp.Name())
+		assert.NoError(t, err)
+	})
+
+	t.Run("duplicate IP address are allowed", func(t *testing.T) {
+		// setup temp leases file
+		tmp, err := os.CreateTemp("", "test_plugin_file")
+		require.NoError(t, err)
+		defer func() {
+			require.NoError(t, os.Remove(tmp.Name()))
+		}()
+
+		// add lines with duplicate IPv6 addresses to trigger an error
+		_, err = tmp.WriteString(`11:11:11:11:11:11 2001:db8::10:1
+22:22:22:22:22:22 2001:db8::10:1
+33:33:33:33:33:33 2001:db8::10:1
+`)
+		require.NoError(t, err)
+		require.NoError(t, tmp.Close())
+
+		_, err = LoadDHCPv6Records(tmp.Name())
+		assert.NoError(t, err)
+	})
+
 	t.Run("lease with IPv4 address should raise error", func(t *testing.T) {
 		// setup temp leases file
 		tmp, err := os.CreateTemp("", "test_plugin_file")
@@ -223,7 +300,7 @@ func TestLoadDHCPv6Records(t *testing.T) {
 func TestHandler4(t *testing.T) {
 	t.Run("unknown MAC", func(t *testing.T) {
 		// prepare DHCPv4 request
-		mac := "00:11:22:33:44:55"
+		mac := "aa:11:22:33:44:55"
 		claddr, _ := net.ParseMAC(mac)
 		req := &dhcpv4.DHCPv4{
 			ClientHWAddr: claddr,
@@ -241,7 +318,7 @@ func TestHandler4(t *testing.T) {
 
 	t.Run("known MAC", func(t *testing.T) {
 		// prepare DHCPv4 request
-		mac := "00:11:22:33:44:55"
+		mac := "aa:11:22:33:44:55"
 		claddr, _ := net.ParseMAC(mac)
 		req := &dhcpv4.DHCPv4{
 			ClientHWAddr: claddr,
@@ -250,8 +327,8 @@ func TestHandler4(t *testing.T) {
 		assert.Nil(t, resp.ClientIPAddr)
 
 		// add lease for the MAC in the lease map
-		clIPAddr := net.ParseIP("192.0.2.100")
-		StaticRecords = map[string]net.IP{
+		clIPAddr := netip.MustParseAddr("192.0.2.100")
+		StaticRecords = map[string]netip.Addr{
 			mac: clIPAddr,
 		}
 
@@ -260,17 +337,17 @@ func TestHandler4(t *testing.T) {
 		result, stop := Handler4(req, resp)
 		assert.Same(t, result, resp)
 		assert.True(t, stop)
-		assert.Equal(t, clIPAddr, result.YourIPAddr)
+		assert.Equal(t, net.IP(clIPAddr.AsSlice()), result.YourIPAddr)
 
 		// cleanup
-		StaticRecords = make(map[string]net.IP)
+		StaticRecords = make(map[string]netip.Addr)
 	})
 }
 
 func TestHandler6(t *testing.T) {
 	t.Run("unknown MAC", func(t *testing.T) {
 		// prepare DHCPv6 request
-		mac := "11:22:33:44:55:66"
+		mac := "aa:11:22:33:44:55"
 		claddr, _ := net.ParseMAC(mac)
 		req, err := dhcpv6.NewSolicit(claddr)
 		require.NoError(t, err)
@@ -287,7 +364,7 @@ func TestHandler6(t *testing.T) {
 
 	t.Run("known MAC", func(t *testing.T) {
 		// prepare DHCPv6 request
-		mac := "11:22:33:44:55:66"
+		mac := "aa:11:22:33:44:55"
 		claddr, _ := net.ParseMAC(mac)
 		req, err := dhcpv6.NewSolicit(claddr)
 		require.NoError(t, err)
@@ -296,8 +373,8 @@ func TestHandler6(t *testing.T) {
 		assert.Equal(t, 0, len(resp.GetOption(dhcpv6.OptionIANA)))
 
 		// add lease for the MAC in the lease map
-		clIPAddr := net.ParseIP("2001:db8::10:1")
-		StaticRecords = map[string]net.IP{
+		clIPAddr := netip.MustParseAddr("2001:db8::10:1")
+		StaticRecords = map[string]netip.Addr{
 			mac: clIPAddr,
 		}
 
@@ -311,7 +388,7 @@ func TestHandler6(t *testing.T) {
 		}
 
 		// cleanup
-		StaticRecords = make(map[string]net.IP)
+		StaticRecords = make(map[string]netip.Addr)
 	})
 }
 
@@ -340,7 +417,7 @@ func TestSetupFile(t *testing.T) {
 	}()
 
 	t.Run("typical case", func(t *testing.T) {
-		_, err = tmp.WriteString("00:11:22:33:44:55 2001:db8::10:1\n")
+		_, err = tmp.WriteString("aa:11:22:33:44:55 2001:db8::10:1\n")
 		require.NoError(t, err)
 		_, err = tmp.WriteString("11:22:33:44:55:66 2001:db8::10:2\n")
 		require.NoError(t, err)
@@ -351,6 +428,8 @@ func TestSetupFile(t *testing.T) {
 		_, _, err = setupFile(true, tmp.Name())
 		if assert.NoError(t, err) {
 			assert.Equal(t, 2, len(StaticRecords))
+			assert.Equal(t, StaticRecords["aa:11:22:33:44:55"], netip.MustParseAddr("2001:db8::10:1"))
+			assert.Equal(t, StaticRecords["11:22:33:44:55:66"], netip.MustParseAddr("2001:db8::10:2"))
 		}
 	})
 
@@ -361,8 +440,9 @@ func TestSetupFile(t *testing.T) {
 		}
 		// we add more leases to the file
 		// this should trigger an event to refresh the leases database
-		// without calling setupFile again
-		_, err = tmp.WriteString("22:33:44:55:66:77 2001:db8::10:3\n")
+		// without calling setupFile again.
+		// Note that the IPv6 address is uppercase (allowed but not best practice)
+		_, err = tmp.WriteString("22:33:44:55:66:77 2001:DB8::10:3\n")
 		require.NoError(t, err)
 		// since the event is processed asynchronously, give it a little time
 		time.Sleep(time.Millisecond * 100)
@@ -372,5 +452,6 @@ func TestSetupFile(t *testing.T) {
 		defer recLock.RUnlock()
 
 		assert.Equal(t, 3, len(StaticRecords))
+		assert.Equal(t, StaticRecords["22:33:44:55:66:77"], netip.MustParseAddr("2001:db8::10:3"))
 	})
 }