Pull request: 4698 Gateway IP in DHCP Lease

Closes #4698.

Squashed commit of the following:

commit 6be0caee58926f8cea1e10650fbde0c8d97d0dac
Author: Ildar Kamalov <ik@adguard.com>
Date:   Fri Jul 8 13:41:50 2022 +0300

    update translation

commit e0370656d05e8463d73ea73568cae81187c6b2e3
Author: Ildar Kamalov <ik@adguard.com>
Date:   Fri Jul 8 13:40:54 2022 +0300

    client: validate static lease ip

commit 7f4d00f9f3a54dc93ce5d5c45e9c21745f6e39d1
Merge: 2ee79626 77e5e27d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 8 13:20:15 2022 +0300

    Merge branch 'master' into 4698-lease-with-gateway

commit 2ee79626a1b0c7b113dbd22ba4ef6e85ea9913ec
Merge: 471b96b8 3505ce87
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 7 19:34:33 2022 +0300

    Merge branch 'master' into 4698-lease-with-gateway

commit 471b96b81da8920c1e71b7110050154f912677d2
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 7 16:07:23 2022 +0300

    dhcpd: imp docs

commit 67dd6c76f7d2df4712a57281e0f40f2ee1a1efa2
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 7 15:48:47 2022 +0300

    dhcpd: restrict gateway ip for lease

CHANGELOG.md

@@ -32,6 +32,8 @@ and this project adheres to
 
 ### Fixed
 
+- DHCP lease validation incorrectly letting users assign the IP address of the
+  gateway as the address of the lease ([#4698]).
 - Updater no longer expects a hardcoded name for  `AdGuardHome` executable
   ([#4219]).
 - Inconsistent names of runtime clients from hosts files ([#4683]).
@@ -44,6 +46,7 @@ and this project adheres to
 [#4219]: https://github.com/AdguardTeam/AdGuardHome/issues/4219
 [#4677]: https://github.com/AdguardTeam/AdGuardHome/issues/4677
 [#4683]: https://github.com/AdguardTeam/AdGuardHome/issues/4683
+[#4698]: https://github.com/AdguardTeam/AdGuardHome/issues/4698
 [#4699]: https://github.com/AdguardTeam/AdGuardHome/issues/4699
 
 [ddr-draft]: https://datatracker.ietf.org/doc/html/draft-ietf-add-ddr-08

client/src/__locales/en.json

@@ -47,6 +47,7 @@
     "form_error_server_name": "Invalid server name",
     "form_error_subnet": "Subnet \"{{cidr}}\" does not contain the IP address \"{{ip}}\"",
     "form_error_positive": "Must be greater than 0",
+    "form_error_gateway_ip": "Lease can't have the IP address of the gateway",
     "out_of_range_error": "Must be out of range \"{{start}}\"-\"{{end}}\"",
     "lower_range_start_error": "Must be lower than range start",
     "greater_range_start_error": "Must be greater than range start",

client/src/components/Settings/Dhcp/StaticLeases/Form.js

@@ -10,6 +10,7 @@ import {
     validateMac,
     validateRequiredValue,
     validateIpv4InCidr,
+    validateIpGateway,
 } from '../../../../helpers/validators';
 import { FORM_NAME } from '../../../../helpers/constants';
 import { toggleLeaseModal } from '../../../../actions';
@@ -57,6 +58,7 @@ const Form = ({
                             validateRequiredValue,
                             validateIpv4,
                             validateIpv4InCidr,
+                            validateIpGateway,
                         ]}
                     />
                 </div>
@@ -101,6 +103,7 @@ Form.propTypes = {
         ip: PropTypes.string.isRequired,
         hostname: PropTypes.string.isRequired,
         cidr: PropTypes.string.isRequired,
+        gatewayIp: PropTypes.string,
     }),
     pristine: PropTypes.bool.isRequired,
     handleSubmit: PropTypes.func.isRequired,

client/src/components/Settings/Dhcp/StaticLeases/Modal.js

@@ -13,6 +13,7 @@ const Modal = ({
     cidr,
     rangeStart,
     rangeEnd,
+    gatewayIp,
 }) => {
     const dispatch = useDispatch();
 
@@ -42,6 +43,7 @@ const Modal = ({
                         cidr,
                         rangeStart,
                         rangeEnd,
+                        gatewayIp,
                     }}
                     onSubmit={handleSubmit}
                     processingAdding={processingAdding}
@@ -61,6 +63,7 @@ Modal.propTypes = {
     cidr: PropTypes.string.isRequired,
     rangeStart: PropTypes.string,
     rangeEnd: PropTypes.string,
+    gatewayIp: PropTypes.string,
 };
 
 export default withTranslation()(Modal);

client/src/components/Settings/Dhcp/StaticLeases/index.js

@@ -24,6 +24,7 @@ const StaticLeases = ({
     cidr,
     rangeStart,
     rangeEnd,
+    gatewayIp,
 }) => {
     const [t] = useTranslation();
     const dispatch = useDispatch();
@@ -106,6 +107,7 @@ const StaticLeases = ({
                 cidr={cidr}
                 rangeStart={rangeStart}
                 rangeEnd={rangeEnd}
+                gatewayIp={gatewayIp}
             />
         </>
     );
@@ -119,6 +121,7 @@ StaticLeases.propTypes = {
     cidr: PropTypes.string.isRequired,
     rangeStart: PropTypes.string,
     rangeEnd: PropTypes.string,
+    gatewayIp: PropTypes.string,
 };
 
 cellWrap.propTypes = {

client/src/components/Settings/Dhcp/index.js

@@ -278,6 +278,7 @@ const Dhcp = () => {
                             cidr={cidr}
                             rangeStart={dhcp?.values?.v4?.range_start}
                             rangeEnd={dhcp?.values?.v4?.range_end}
+                            gatewayIp={dhcp?.values?.v4?.gateway_ip}
                         />
                         <div className="btn-list mt-2">
                             <button

client/src/helpers/validators.js

@@ -339,3 +339,14 @@ export const validatePasswordLength = (value) => {
     }
     return undefined;
 };
+
+/**
+ * @param value {string}
+ * @returns {Function}
+ */
+export const validateIpGateway = (value, allValues) => {
+    if (value === allValues.gatewayIp) {
+        return i18next.t('form_error_gateway_ip');
+    }
+    return undefined;
+};

internal/dhcpd/http.go

@@ -498,7 +498,6 @@ func (s *Server) handleDHCPAddStaticLease(w http.ResponseWriter, r *http.Request
 	}
 
 	ip4 := l.IP.To4()
-
 	if ip4 == nil {
 		l.IP = l.IP.To16()
 

internal/dhcpd/v4.go

@@ -333,12 +333,16 @@ func (s *v4Server) rmLease(lease *Lease) (err error) {
 	return errors.Error("lease not found")
 }
 
-// AddStaticLease adds a static lease.  It is safe for concurrent use.
+// AddStaticLease implements the DHCPServer interface for *v4Server.  It is safe
+// for concurrent use.
 func (s *v4Server) AddStaticLease(l *Lease) (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv4: adding static lease: %w") }()
 
-	if ip4 := l.IP.To4(); ip4 == nil {
+	ip := l.IP.To4()
+	if ip == nil {
 		return fmt.Errorf("invalid ip %q, only ipv4 is supported", l.IP)
+	} else if gwIP := s.conf.GatewayIP; gwIP.Equal(ip) {
+		return fmt.Errorf("can't assign the gateway IP %s to the lease", gwIP)
 	}
 
 	l.Expiry = time.Unix(leaseExpireStatic, 0)
@@ -377,7 +381,7 @@ func (s *v4Server) AddStaticLease(l *Lease) (err error) {
 		if err != nil {
 			err = fmt.Errorf(
 				"removing dynamic leases for %s (%s): %w",
-				l.IP,
+				ip,
 				l.HWAddr,
 				err,
 			)
@@ -387,7 +391,7 @@ func (s *v4Server) AddStaticLease(l *Lease) (err error) {
 
 		err = s.addLease(l)
 		if err != nil {
-			err = fmt.Errorf("adding static lease for %s (%s): %w", l.IP, l.HWAddr, err)
+			err = fmt.Errorf("adding static lease for %s (%s): %w", ip, l.HWAddr, err)
 
 			return
 		}

internal/dhcpd/v4_test.go

@@ -4,6 +4,7 @@
 package dhcpd
 
 import (
+	"fmt"
 	"net"
 	"strings"
 	"testing"
@@ -16,6 +17,13 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+var (
+	DefaultRangeStart = net.IP{192, 168, 10, 100}
+	DefaultRangeEnd   = net.IP{192, 168, 10, 200}
+	DefaultGatewayIP  = net.IP{192, 168, 10, 1}
+	DefaultSubnetMask = net.IP{255, 255, 255, 0}
+)
+
 func notify4(flags uint32) {
 }
 
@@ -24,10 +32,10 @@ func notify4(flags uint32) {
 func defaultV4ServerConf() (conf V4ServerConf) {
 	return V4ServerConf{
 		Enabled:    true,
-		RangeStart: net.IP{192, 168, 10, 100},
+		RangeStart: DefaultRangeStart,
-		RangeEnd:   net.IP{192, 168, 10, 200},
+		RangeEnd:   DefaultRangeEnd,
-		GatewayIP:  net.IP{192, 168, 10, 1},
+		GatewayIP:  DefaultGatewayIP,
-		SubnetMask: net.IP{255, 255, 255, 0},
+		SubnetMask: DefaultSubnetMask,
 		notify:     notify4,
 	}
 }
@@ -44,44 +52,86 @@ func defaultSrv(t *testing.T) (s DHCPServer) {
 	return s
 }
 
-func TestV4_AddRemove_static(t *testing.T) {
+func TestV4Server_AddRemove_static(t *testing.T) {
 	s := defaultSrv(t)
 
 	ls := s.GetLeases(LeasesStatic)
-	assert.Empty(t, ls)
+	require.Empty(t, ls)
 
-	// Add static lease.
+	testCases := []struct {
-	l := &Lease{
+		lease      *Lease
-		Hostname: "static-1.local",
+		name       string
-		HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
+		wantErrMsg string
-		IP:       net.IP{192, 168, 10, 150},
+	}{{
+		lease: &Lease{
+			Hostname: "success.local",
+			HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
+			IP:       net.IP{192, 168, 10, 150},
+		},
+		name:       "success",
+		wantErrMsg: "",
+	}, {
+		lease: &Lease{
+			Hostname: "probably-router.local",
+			HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
+			IP:       DefaultGatewayIP,
+		},
+		name: "with_gateway_ip",
+		wantErrMsg: "dhcpv4: adding static lease: " +
+			"can't assign the gateway IP 192.168.10.1 to the lease",
+	}, {
+		lease: &Lease{
+			Hostname: "ip6.local",
+			HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
+			IP:       net.ParseIP("ffff::1"),
+		},
+		name: "ipv6",
+		wantErrMsg: `dhcpv4: adding static lease: ` +
+			`invalid ip "ffff::1", only ipv4 is supported`,
+	}, {
+		lease: &Lease{
+			Hostname: "bad-mac.local",
+			HWAddr:   net.HardwareAddr{0xAA, 0xAA},
+			IP:       net.IP{192, 168, 10, 150},
+		},
+		name: "bad_mac",
+		wantErrMsg: `dhcpv4: adding static lease: bad mac address "aa:aa": ` +
+			`bad mac address length 2, allowed: [6 8 20]`,
+	}, {
+		lease: &Lease{
+			Hostname: "bad-lbl-.local",
+			HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
+			IP:       net.IP{192, 168, 10, 150},
+		},
+		name: "bad_hostname",
+		wantErrMsg: `dhcpv4: adding static lease: validating hostname: ` +
+			`bad domain name "bad-lbl-.local": ` +
+			`bad domain name label "bad-lbl-": bad domain name label rune '-'`,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := s.AddStaticLease(tc.lease)
+			testutil.AssertErrorMsg(t, tc.wantErrMsg, err)
+			if tc.wantErrMsg != "" {
+				return
+			}
+
+			err = s.RemoveStaticLease(&Lease{
+				IP:     tc.lease.IP,
+				HWAddr: tc.lease.HWAddr,
+			})
+			diffErrMsg := fmt.Sprintf("dhcpv4: lease for ip %s is different: %+v", tc.lease.IP, tc.lease)
+			testutil.AssertErrorMsg(t, diffErrMsg, err)
+
+			// Remove static lease.
+			err = s.RemoveStaticLease(tc.lease)
+			require.NoError(t, err)
+		})
+
+		ls = s.GetLeases(LeasesStatic)
+		require.Emptyf(t, ls, "after %s", tc.name)
 	}
-
-	err := s.AddStaticLease(l)
-	require.NoError(t, err)
-
-	err = s.AddStaticLease(l)
-	assert.Error(t, err)
-
-	ls = s.GetLeases(LeasesStatic)
-	require.Len(t, ls, 1)
-
-	assert.True(t, l.IP.Equal(ls[0].IP))
-	assert.Equal(t, l.HWAddr, ls[0].HWAddr)
-	assert.True(t, ls[0].IsStatic())
-
-	// Try to remove static lease.
-	err = s.RemoveStaticLease(&Lease{
-		IP:     net.IP{192, 168, 10, 110},
-		HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
-	})
-	assert.Error(t, err)
-
-	// Remove static lease.
-	err = s.RemoveStaticLease(l)
-	require.NoError(t, err)
-	ls = s.GetLeases(LeasesStatic)
-	assert.Empty(t, ls)
 }
 
 func TestV4_AddReplace(t *testing.T) {