diff --git a/ChangeLog b/ChangeLog index 640c498feb..83ffa488af 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2069. [func] fdupont + Added a new sanity checker named "extended-info-checks" + which checks and eventually upgrades lease extended + info which store into lease user context in DHCPv4 + the dhcp-agent-options content and in DHCPv6 the + relay-msg fields and options. + (Gitlab #2595) + 2068. [func] djt Kea's official APK, Deb, and RPM packages have been restructured and made to follow a consistent packaging standard. Some of the diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index 6b3a76db7d..3e0a4e3739 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -3938,8 +3938,8 @@ would not want all the leases associated with it to disappear from the lease database. Kea has a mechanism to implement sanity checks for situations like this. -Kea supports a configuration scope called ``sanity-checks``. It -currently allows only a single parameter, called ``lease-checks``, which +Kea supports a configuration scope called ``sanity-checks``. +A parameter, called ``lease-checks``, governs the verification carried out when a new lease is loaded from a lease file. This mechanism permits Kea to attempt to correct inconsistent data. @@ -4046,6 +4046,32 @@ Or with remote and relay suboptions: purposes. As long as no other purpose also writes an "ISC" element to ``user-context`` there should not be a conflict. +Extended lease information is also subject to configurable sanity checking. +The parameter in the ``sanity-checks`` scope is named ``extended-info-checks`` +and supports these levels: + +- ``none`` - do no check nor upgrade. This level should be used on when + extended info is not used at all or when no badly formatted extended + info, including using the old format, is expected. + +- ``fix`` - fix some common inconsistencies and upgrade extended info + using the old format to the new one. It is the default level and is + convenient when Lease Query hook library is not loaded. + +- ``strict`` - fix all inconsistencies which have an impact on the (Bulk) + Lease Query hook library. + +- ``pedantic`` - enforce full conformance to the format produced by the + Kea code, for instance no extra entries are allowed at the exception + of ``comment``. + +.. note:: + + Currently this feature is currently implemented for the memfile + backend. The sanity check applies to the lease database in memory, + not to the lease file, i.e. inconsistent leases will stay in the lease + file. + .. _dhcp4-multi-threading-settings: Multi-Threading Settings diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index 504d988061..f95ab76cb1 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -3386,8 +3386,8 @@ would not want all the leases associated with it to disappear from the lease database. Kea has a mechanism to implement sanity checks for situations like this. -Kea supports a configuration scope called ``sanity-checks``. It -currently allows only a single parameter, called ``lease-checks``, which +Kea supports a configuration scope called ``sanity-checks``. +A parameter, called ``lease-checks``, governs the verification carried out when a new lease is loaded from a lease file. This mechanism permits Kea to attempt to correct inconsistent data. @@ -3513,6 +3513,32 @@ pretty-printed for clarity): container serving multiple purposes. As long as no other purpose also writes an "ISC" element to ``user-context`` there should not be a conflict. +Extended lease information is also subject to configurable sanity checking. +The parameter in the ``sanity-checks`` scope is named ``extended-info-checks`` +and supports these levels: + +- ``none`` - do no check nor upgrade. This level should be used on when + extended info is not used at all or when no badly formatted extended + info, including using the old format, is expected. + +- ``fix`` - fix some common inconsistencies and upgrade extended info + using the old format to the new one. It is the default level and is + convenient when Lease Query hook library is not loaded. + +- ``strict`` - fix all inconsistencies which have an impact on the (Bulk) + Lease Query hook library. + +- ``pedantic`` - enforce full conformance to the format produced by the + Kea code, for instance no extra entries are allowed at the exception + of ``comment``. + +.. note:: + + Currently this feature is currently implemented for the memfile + backend. The sanity check applies to the lease database in memory, + not to the lease file, i.e. inconsistent leases will stay in the lease + file. + .. _dhcp6-multi-threading-settings: Multi-Threading Settings diff --git a/src/lib/dhcpsrv/lease_mgr.cc b/src/lib/dhcpsrv/lease_mgr.cc index c852f34e6d..4be2377093 100644 --- a/src/lib/dhcpsrv/lease_mgr.cc +++ b/src/lib/dhcpsrv/lease_mgr.cc @@ -903,7 +903,7 @@ LeaseMgr::upgradeLease6ExtendedInfo(const Lease6Ptr& lease, } verifying = (upgraded ? "relays" : "relay-info"); - for (auto elem : relay_info->mapValue()) { + for (auto elem : relay->mapValue()) { if ((elem.first != "hop") && (elem.first != "link") && (elem.first != "peer") && diff --git a/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc b/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc index b926bfee59..7185b931ae 100644 --- a/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc +++ b/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc @@ -1496,3 +1496,119 @@ TEST_F(ExtendedInfoChecksTest, validRelayId6strict) { " \"link\": \"2001::2\" } ] } }", CfgConsistency::EXTENDED_INFO_CHECK_STRICT); } + +// Pedantic requires a peer entry. +TEST_F(ExtendedInfoChecksTest, noPeerpedantic) { + string description = "no peer, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\" } ] } }", + "", CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in peer [relay#0] a problem was found:" + " no peer)" }); +} + +// peer entry with bad type is dropped by pedantic sanity check level. +TEST_F(ExtendedInfoChecksTest, badTypePeer) { + string description = "peer is not a string, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": 1 } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in peer [relay#0] a problem was found:" + " peer is not a string)" }); +} + +// peer entry which is not an address is dropped by pedantic sanity check level. +TEST_F(ExtendedInfoChecksTest, notAddressPeer) { + string description = "peer is not an address, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"foo\" } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in peer [relay#0] a problem was found:" + " Failed to convert string to address 'foo':" + " Invalid argument)" }); +} + +// peer entry which is an IPv4 (vs IPv6) address is dropped by pedantic sanity +// check level. +TEST_F(ExtendedInfoChecksTest, notV6Peer) { + string description = "peer is v4, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"192.128.1.1\" } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in peer [relay#0] a problem was found:" + " peer is not an IPv6 address)" }); +} + +// Pedantic requires a hop entry. +TEST_F(ExtendedInfoChecksTest, noHop) { + string description = "no hop, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"2001::3\" } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in hop [relay#0] a problem was found:" + " no hop)" }); +} + +// hop entry with bad type is dropped by pedantic sanity check level. +TEST_F(ExtendedInfoChecksTest, badTypeHop) { + string description = "hop is not an integer pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"2001::3\", \"hop\": false } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in hop [relay#0] a problem was found:" + " hop is not an integer)" }); +} + +// Valid relay. +TEST_F(ExtendedInfoChecksTest, valid6Pedantic) { + string description = "valid, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"2001::3\", \"hop\": 10 } ] } }", + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"2001::3\", \"hop\": 10 } ] } }", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC); +} + +// Junk entries are dropped at the pedantic level. +TEST_F(ExtendedInfoChecksTest, junk6pedantic) { + string description = "junk entry, pedantic"; + check6(description, + "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\"," + " \"peer\": \"2001::3\", \"hop\": 10, \"foo\": 1 } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in relay-info [relay#0] a problem was found:" + " spurious 'foo' entry)" }); +} + +// Same with relays post upgrade checks. +TEST_F(ExtendedInfoChecksTest, junkRelayspedantic) { + string description = "junk entry, pedantic"; + check6(description, + "{ \"ISC\": { \"relays\": [ { \"link\": \"2001::2\"," + " \"peer\": \"2001::3\", \"hop\": 10, \"foo\": 1 } ] } }", "", + CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC, + { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL" + " extended info for lease 2001::1 failed checks" + " (in relays [relay#0] a problem was found:" + " spurious 'foo' entry)" }); +}