Pull request: dhcpd: imp normalization, validation

Updates AdguardTeam#3056.

Squashed commit of the following:

commit 875954f
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed May 5 19:54:24 2021 +0300

    all: imp err msgs

commit c6ea471
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed May 5 17:55:12 2021 +0300

    dhcpd: imp normalization, validation

CHANGELOG.md

@@ -17,6 +17,14 @@ and this project adheres to
 ## [v0.106.2] - 2021-05-17 (APPROX.)
 -->
 
+### Fixed
+
+- Uniqueness validation for dynamic DHCP leases ([#3056]).
+
+[#3056]: https://github.com/AdguardTeam/AdGuardHome/issues/3056
+
+
+
 ## [v0.106.1] - 2021-04-30
 
 ### Fixed

internal/aghnet/addr.go

@@ -48,32 +48,32 @@ const maxDomainLabelLen = 63
 // See https://stackoverflow.com/a/32294443/1892060.
 const maxDomainNameLen = 253
 
-const invalidCharMsg = "invalid char %q at index %d in %q"
-
 // ValidateDomainNameLabel returns an error if label is not a valid label of
 // a domain name.
 func ValidateDomainNameLabel(label string) (err error) {
+	defer agherr.Annotate("validating label %q: %w", &err, label)
+
 	l := len(label)
 	if l > maxDomainLabelLen {
-		return fmt.Errorf("%q is too long, max: %d", label, maxDomainLabelLen)
+		return fmt.Errorf("label is too long, max: %d", maxDomainLabelLen)
 	} else if l == 0 {
 		return agherr.Error("label is empty")
 	}
 
 	if r := label[0]; !IsValidHostOuterRune(rune(r)) {
-		return fmt.Errorf(invalidCharMsg, r, 0, label)
+		return fmt.Errorf("invalid char %q at index %d", r, 0)
 	} else if l == 1 {
 		return nil
 	}
 
 	for i, r := range label[1 : l-1] {
 		if !isValidHostRune(r) {
-			return fmt.Errorf(invalidCharMsg, r, i+1, label)
+			return fmt.Errorf("invalid char %q at index %d", r, i+1)
 		}
 	}
 
 	if r := label[l-1]; !IsValidHostOuterRune(rune(r)) {
-		return fmt.Errorf(invalidCharMsg, r, l-1, label)
+		return fmt.Errorf("invalid char %q at index %d", r, l-1)
 	}
 
 	return nil
@@ -87,6 +87,8 @@ func ValidateDomainNameLabel(label string) (err error) {
 // TODO(a.garipov): After making sure that this works correctly, port this into
 // module golibs.
 func ValidateDomainName(name string) (err error) {
+	defer agherr.Annotate("validating domain name %q: %w", &err, name)
+
 	name, err = idna.ToASCII(name)
 	if err != nil {
 		return err
@@ -96,7 +98,7 @@ func ValidateDomainName(name string) (err error) {
 	if l == 0 {
 		return agherr.Error("domain name is empty")
 	} else if l > maxDomainNameLen {
-		return fmt.Errorf("%q is too long, max: %d", name, maxDomainNameLen)
+		return fmt.Errorf("too long, max: %d", maxDomainNameLen)
 	}
 
 	labels := strings.Split(name, ".")

internal/aghnet/addr_test.go

@@ -95,40 +95,46 @@ func TestValidateDomainName(t *testing.T) {
 	}, {
 		name:       "empty",
 		in:         "",
-		wantErrMsg: `domain name is empty`,
+		wantErrMsg: `validating domain name "": domain name is empty`,
 	}, {
 		name: "bad_symbol",
 		in:   "!!!",
-		wantErrMsg: `invalid domain name label at index 0: ` +
-			`invalid char '!' at index 0 in "!!!"`,
+		wantErrMsg: `validating domain name "!!!": invalid domain name label at index 0: ` +
+			`validating label "!!!": invalid char '!' at index 0`,
 	}, {
 		name:       "bad_length",
 		in:         longDomainName,
-		wantErrMsg: `"` + longDomainName + `" is too long, max: 253`,
+		wantErrMsg: `validating domain name "` + longDomainName + `": too long, max: 253`,
 	}, {
 		name: "bad_label_length",
 		in:   longLabelDomainName,
-		wantErrMsg: `invalid domain name label at index 0: "` + longLabel +
-			`" is too long, max: 63`,
+		wantErrMsg: `validating domain name "` + longLabelDomainName + `": ` +
+			`invalid domain name label at index 0: validating label "` + longLabel +
+			`": label is too long, max: 63`,
 	}, {
-		name:       "bad_label_empty",
-		in:         "example..com",
-		wantErrMsg: `invalid domain name label at index 1: label is empty`,
+		name: "bad_label_empty",
+		in:   "example..com",
+		wantErrMsg: `validating domain name "example..com": ` +
+			`invalid domain name label at index 1: ` +
+			`validating label "": label is empty`,
 	}, {
 		name: "bad_label_first_symbol",
 		in:   "example.-aa.com",
-		wantErrMsg: `invalid domain name label at index 1:` +
-			` invalid char '-' at index 0 in "-aa"`,
+		wantErrMsg: `validating domain name "example.-aa.com": ` +
+			`invalid domain name label at index 1: ` +
+			`validating label "-aa": invalid char '-' at index 0`,
 	}, {
 		name: "bad_label_last_symbol",
 		in:   "example-.aa.com",
-		wantErrMsg: `invalid domain name label at index 0:` +
-			` invalid char '-' at index 7 in "example-"`,
+		wantErrMsg: `validating domain name "example-.aa.com": ` +
+			`invalid domain name label at index 0: ` +
+			`validating label "example-": invalid char '-' at index 7`,
 	}, {
 		name: "bad_label_symbol",
 		in:   "example.a!!!.com",
-		wantErrMsg: `invalid domain name label at index 1:` +
-			` invalid char '!' at index 1 in "a!!!"`,
+		wantErrMsg: `validating domain name "example.a!!!.com": ` +
+			`invalid domain name label at index 1: ` +
+			`validating label "a!!!": invalid char '!' at index 1`,
 	}}
 
 	for _, tc := range testCases {

internal/dhcpd/dhcpd.go

@@ -27,13 +27,13 @@ var webHandlersRegistered = false
 
 // Lease contains the necessary information about a DHCP lease
 type Lease struct {
+	// Expiry is the expiration time of the lease.  The unix timestamp value
+	// of 1 means that this is a static lease.
+	Expiry time.Time `json:"expires"`
+
+	Hostname string           `json:"hostname"`
 	HWAddr   net.HardwareAddr `json:"mac"`
 	IP       net.IP           `json:"ip"`
-	Hostname string           `json:"hostname"`
-
-	// Lease expiration time
-	// 1: static lease
-	Expiry time.Time `json:"expires"`
 }
 
 // IsStatic returns true if the lease is static.

internal/dhcpd/dhcpd_test.go

@@ -37,30 +37,34 @@ func TestDB(t *testing.T) {
 		SubnetMask: net.IP{255, 255, 255, 0},
 		notify:     testNotify,
 	})
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	s.srv6, err = v6Create(V6ServerConf{})
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	leases := []Lease{{
-		IP:     net.IP{192, 168, 10, 100},
-		HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
-		Expiry: time.Now().Add(time.Hour),
+		Expiry:   time.Now().Add(time.Hour),
+		Hostname: "static-1.local",
+		HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
+		IP:       net.IP{192, 168, 10, 100},
 	}, {
-		IP:     net.IP{192, 168, 10, 101},
-		HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xBB},
+		Hostname: "static-2.local",
+		HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xBB},
+		IP:       net.IP{192, 168, 10, 101},
 	}}
 
 	srv4, ok := s.srv4.(*v4Server)
 	require.True(t, ok)
 
 	err = srv4.addLease(&leases[0])
-	require.Nil(t, err)
-	require.Nil(t, s.srv4.AddStaticLease(leases[1]))
+	require.NoError(t, err)
+
+	err = s.srv4.AddStaticLease(leases[1])
+	require.NoError(t, err)
 
 	s.dbStore()
 	t.Cleanup(func() {
-		assert.Nil(t, os.Remove(dbFilename))
+		assert.NoError(t, os.Remove(dbFilename))
 	})
 	s.srv4.ResetLeases(nil)
 	s.dbLoad()