feat!: settle the open review questions; release 4.7.0

## feat!: settle the open review questions on roles, diffs, SSH and SNMP

- roles: a driver that sets ROLE itself replaces the role it inherits,
  so a Windows driver on OSDriver is "windows", not also "linux".
- os: OSDriver's account listing is get_os_users; NAPALM's get_users has
  another shape and a runtime stub, so hasattr could not tell.
- README: NAPALM's own getters must come from the driver or a mixin
  listed before the first role base; its stubs win the MRO otherwise.
- firewall/DHCP diffs: an empty key, a key given twice, or one more than
  one live entry holds raises ValueError instead of overwriting by guess;
  live entries without a key are never matched.
- ssh_server: only host key algorithms the host has a key for count as
  staying; when the keys cannot be read, the host key list is not
  touched. sshd_config and its copy keep their file mode (GNU and BSD
  stat), 644 only for a new file.
- SNMP: 64-bit ifXTable octets and ifHighSpeed, with the 32-bit counters
  as fallback for agents without ifXTable.
- pyproject: declare typing-extensions, which models.py imports.

BREAKING CHANGE: OSDriver.get_users is now get_os_users; diffs raise
ValueError on keys that cannot name one entry; plan_ssh_hardening
removes no host key algorithm without host_keys.

## fix: tell "could not read" and "does not install" apart in update and VIP readers

- package_updates: the dnf security reader records dnf's exit status and
  raises when it is missing or non-zero, as the apt reader does; a failed
  dnf no longer reads as "no security advisories".
- host_status: an enabled dnf-automatic.timer counts as auto updates only
  with apply_updates = yes (dnf5's automatic.conf wins over dnf4's);
  dnf-automatic-install.timer always does. Unattended-upgrades counts only
  while the program is installed, not from a conffile left behind.
- virtual_ips: a host with systemctl but without systemd as PID 1 reads as
  "did not look" (None), not as "has none". keepalived blocks whose brace
  is on the next line are read.

BREAKING CHANGE: parse_dnf_security expects DNF_SECURITY_COMMAND's
__DNF_RC marker and raises ValueError without it.

napalm_device_types/_matching.py

@@ -0,0 +1,47 @@
+# -*- coding: utf-8 -*-
+"""Keying live and desired entries for a diff, refusing what cannot be matched.
+
+A diff matches a desired entry to a live one by a key (a rule's description, a
+reservation's MAC, a subnet's CIDR). An empty key or one held twice cannot
+name a single entry, and guessing would overwrite one the caller never meant.
+"""
+
+from __future__ import annotations
+
+from typing import Callable, Dict, Iterable, List, Set, Tuple, TypeVar
+
+T = TypeVar("T")
+
+
+def index_live(entries: Iterable[T], key: Callable[[T], str]) -> Tuple[Dict[str, T], Set[str]]:
+    """Live entries by key, and the keys more than one of them holds.
+
+    Entries without a key are left out: they are not the caller's to match.
+    """
+    by_key: Dict[str, T] = {}
+    shared: Set[str] = set()
+    for entry in entries:
+        k = key(entry)
+        if not k:
+            continue
+        if k in by_key:
+            shared.add(k)
+        by_key[k] = entry
+    for k in shared:
+        del by_key[k]
+    return by_key, shared
+
+
+def check_desired(entries: List[T], key: Callable[[T], str], shared: Set[str], what: str) -> None:
+    """``ValueError`` for a desired entry with no key, a key given twice, or a
+    key that names more than one live entry."""
+    seen: Set[str] = set()
+    for entry in entries:
+        k = key(entry)
+        if not k:
+            raise ValueError(f"a desired entry has no {what}")
+        if k in seen:
+            raise ValueError(f"{what} {k!r} is given twice")
+        if k in shared:
+            raise ValueError(f"{what} {k!r} names more than one entry on the device")
+        seen.add(k)

napalm_device_types/_ucd_metrics.py

@@ -25,6 +25,12 @@ OID_IF_IN_OCT  = "1.3.6.1.2.1.2.2.1.10"
 OID_IF_OUT_OCT = "1.3.6.1.2.1.2.2.1.16"
 OID_IF_IN_ERR  = "1.3.6.1.2.1.2.2.1.14"
 OID_IF_OUT_ERR = "1.3.6.1.2.1.2.2.1.20"
+# ifXTable: 64-bit octets and Mbps. The 32-bit counters wrap every ~34 s at
+# 1 Gbit/s and ifSpeed stops at 4294 Mbps; an agent without ifXTable (SNMPv1)
+# falls back to them.
+OID_IF_HC_IN_OCT  = "1.3.6.1.2.1.31.1.1.1.6"
+OID_IF_HC_OUT_OCT = "1.3.6.1.2.1.31.1.1.1.10"
+OID_IF_HIGH_SPEED = "1.3.6.1.2.1.31.1.1.1.15"
 
 IF_SKIP_DEFAULT = re.compile(r'^(lo|sit\d|tun\d|docker|veth|br-|virbr)', re.IGNORECASE)
 
@@ -52,6 +58,9 @@ def build_if_metrics(
     *,
     if_skip: re.Pattern[str] | None = None,
     tx_err_is_drop: bool = False,
+    hc_in_oct: Optional[Dict[str, str]] = None,
+    hc_out_oct: Optional[Dict[str, str]] = None,
+    high_speed: Optional[Dict[str, str]] = None,
 ) -> None:
     if if_skip is None:
         if_skip = IF_SKIP_DEFAULT
@@ -61,8 +70,8 @@ def build_if_metrics(
             continue
         iface: Dict[str, Any] = {"name": name}
         try:
-            iface["rx_bytes"]  = int(in_oct.get(idx, 0)  or 0)
-            iface["tx_bytes"]  = int(out_oct.get(idx, 0) or 0)
+            iface["rx_bytes"]  = int((hc_in_oct or {}).get(idx) or in_oct.get(idx) or 0)
+            iface["tx_bytes"]  = int((hc_out_oct or {}).get(idx) or out_oct.get(idx) or 0)
             iface["rx_errors"] = int(in_err.get(idx, 0)  or 0)
             tx_val = int(out_err.get(idx, 0) or 0)
             if tx_err_is_drop:
@@ -70,8 +79,12 @@ def build_if_metrics(
                 iface["tx_drops"]  = tx_val
             else:
                 iface["tx_errors"] = tx_val
+            high = (high_speed or {}).get(idx)
             spd = speed.get(idx)
-            iface["speed_mbps"] = int(spd) // 1_000_000 if spd and int(spd) > 0 else 0
+            if high and int(high) > 0:
+                iface["speed_mbps"] = int(high)
+            else:
+                iface["speed_mbps"] = int(spd) // 1_000_000 if spd and int(spd) > 0 else 0
         except (ValueError, TypeError):
             pass
         interfaces[name] = iface
@@ -92,7 +105,9 @@ async def collect_ucd_metrics(
         mem_total_raw, mem_free_raw, mem_buf_raw, mem_cache_raw,
         swap_total_raw, swap_avail_raw,
         load1_raw, load5_raw, load15_raw,
-    ), (descr, speed, in_oct, out_oct, in_err, out_err) = await asyncio.gather(
+    ), (
+        descr, speed, in_oct, out_oct, in_err, out_err, hc_in_oct, hc_out_oct, high_speed,
+    ) = await asyncio.gather(
         asyncio.gather(
             snmp_get(OID_SYS_UPTIME),
             snmp_get(OID_CPU_IDLE),
@@ -113,6 +128,9 @@ async def collect_ucd_metrics(
             snmp_walk(OID_IF_OUT_OCT),
             snmp_walk(OID_IF_IN_ERR),
             snmp_walk(OID_IF_OUT_ERR),
+            snmp_walk(OID_IF_HC_IN_OCT),
+            snmp_walk(OID_IF_HC_OUT_OCT),
+            snmp_walk(OID_IF_HIGH_SPEED),
         ),
     )
 
@@ -161,5 +179,6 @@ async def collect_ucd_metrics(
                 pass
 
     build_if_metrics(metrics, descr, speed, in_oct, out_oct, in_err, out_err,
-                     if_skip=if_skip, tx_err_is_drop=tx_err_is_drop)
+                     if_skip=if_skip, tx_err_is_drop=tx_err_is_drop,
+                     hc_in_oct=hc_in_oct, hc_out_oct=hc_out_oct, high_speed=high_speed)
     return metrics

napalm_device_types/dhcp.py

@@ -26,6 +26,7 @@ from __future__ import annotations
 import re
 from typing import Any, Dict, Iterator, List, Optional, TYPE_CHECKING
 
+from napalm_device_types._matching import check_desired, index_live
 from napalm_device_types.models import (
     DhcpReservationDiffDict,
     DhcpReservationDict,
@@ -238,20 +239,26 @@ class DhcpServerMixin:
         those apart from ones simply no longer wanted. Callers wanting
         delete/cleanup semantics must implement that themselves, deliberately.
 
+        Live reservations without a MAC are never matched. A desired one
+        without a MAC, a MAC given twice, or one more than one live
+        reservation holds raise ``ValueError``.
+
         :param desired: The complete desired reservation set.
         :returns: ``{"add": [...], "update": [{"uuid", "reservation",
             "changed_fields"}, ...]}``.
+        :raises ValueError: When a desired reservation cannot name a single live one.
         """
-        live_by_mac: Dict[str, DhcpReservationDict] = {
-            normalize_mac(reservation.get("mac")): reservation
-            for reservation in self.get_dhcp_reservations()
-        }
+        def key(reservation: DhcpReservationDict) -> str:
+            return normalize_mac(reservation.get("mac"))
+
+        live_by_mac, shared = index_live(self.get_dhcp_reservations(), key)
+        check_desired(desired, key, shared, "MAC")
 
         add: List[DhcpReservationDict] = []
         update: List[DhcpReservationUpdateDict] = []
 
         for desired_reservation in desired:
-            live = live_by_mac.get(normalize_mac(desired_reservation.get("mac")))
+            live = live_by_mac.get(key(desired_reservation))
             if live is None:
                 add.append(desired_reservation)
                 continue
@@ -333,19 +340,24 @@ class DhcpServerMixin:
         subnet takes DHCP down for a whole VLAN, and this method cannot tell
         "no longer wanted" from "was never netOrk's to describe".
 
+        An empty or repeated subnet raises ``ValueError``, as for reservations.
+
         :param desired: The complete desired subnet set.
         :returns: ``{"add": [...], "update": [{"uuid", "subnet",
             "changed_fields"}, ...]}``.
+        :raises ValueError: When a desired subnet cannot name a single live one.
         """
-        live_by_cidr: Dict[str, DhcpSubnetDict] = {
-            normalize_cidr(live.get("subnet")): live for live in self.get_dhcp_subnets()
-        }
+        def key(subnet: DhcpSubnetDict) -> str:
+            return normalize_cidr(subnet.get("subnet"))
+
+        live_by_cidr, shared = index_live(self.get_dhcp_subnets(), key)
+        check_desired(desired, key, shared, "subnet")
 
         add: List[DhcpSubnetDict] = []
         update: List[DhcpSubnetUpdateDict] = []
 
         for desired_subnet in desired:
-            live = live_by_cidr.get(normalize_cidr(desired_subnet.get("subnet")))
+            live = live_by_cidr.get(key(desired_subnet))
             if live is None:
                 add.append(desired_subnet)
                 continue

napalm_device_types/firewall_rules.py

@@ -16,6 +16,7 @@ from __future__ import annotations
 
 from typing import Any, Dict, Iterator, List, Optional, TYPE_CHECKING
 
+from napalm_device_types._matching import check_desired, index_live
 from napalm_device_types.models import (
     FirewallRuleDict,
     FirewallRuleDiffDict,
@@ -102,19 +103,27 @@ class FirewallRuleMixin:
         from ones simply no longer wanted. Callers wanting delete/cleanup
         semantics must implement that themselves, deliberately.
 
+        Live rules without a description are never matched. A desired rule
+        without one, two desired rules with the same one, or one whose
+        description more than one live rule holds raise ``ValueError``: any
+        guess would overwrite a rule the caller did not mean.
+
         :param desired: The complete desired rule set.
         :returns: ``{"add": [...], "update": [{"uuid", "rule",
             "changed_fields"}, ...]}``.
+        :raises ValueError: When a desired rule cannot name a single live one.
         """
-        live_by_description: Dict[str, FirewallRuleDict] = {
-            rule["description"]: rule for rule in self.get_firewall_rules()
-        }
+        def key(rule: FirewallRuleDict) -> str:
+            return (rule.get("description") or "").strip()
+
+        live_by_description, shared = index_live(self.get_firewall_rules(), key)
+        check_desired(desired, key, shared, "description")
 
         add: List[FirewallRuleDict] = []
         update: List[FirewallRuleUpdateDict] = []
 
         for desired_rule in desired:
-            live_rule = live_by_description.get(desired_rule["description"])
+            live_rule = live_by_description.get(key(desired_rule))
             if live_rule is None:
                 add.append(desired_rule)
                 continue

napalm_device_types/host_status.py

@@ -23,8 +23,12 @@ It is ``None`` when none of these could be read, for example in a container
 without a ``/lib/modules`` of its own.
 
 **Auto updates** is apt's ``APT::Periodic::Unattended-Upgrade`` (set, not "0",
-and ``apt-daily-upgrade.timer`` not disabled) or an enabled dnf-automatic timer.
-It is ``None`` on a host with neither apt nor dnf-automatic. When they last ran
+``apt-daily-upgrade.timer`` not disabled, and unattended-upgrades installed: a
+package removed but not purged leaves its "1" behind) or an enabled
+``dnf-automatic-install.timer``. ``dnf-automatic.timer`` installs only with
+``apply_updates = yes`` in ``automatic.conf`` (dnf5's in ``dnf5-plugins/``
+wins); by default it only downloads. It is ``None`` on a host with neither apt
+nor dnf-automatic. When they last ran
 successfully is ``/var/lib/apt/periodic/upgrade-stamp``, which
 ``apt.systemd.daily`` touches only after unattended-upgrade succeeded.
 ``update-success-stamp`` would say nothing: every ``apt-get update`` touches it,
@@ -68,7 +72,11 @@ _BEGIN = "HSTAT_BEGIN"
 _END = "HSTAT_END"
 _REBOOT_FILE = "/var/run/reboot-required"
 _APT_TIMER = "apt-daily-upgrade.timer"
-_DNF_TIMERS = ("dnf-automatic.timer", "dnf-automatic-install.timer")
+_DNF_TIMER = "dnf-automatic.timer"
+_DNF_INSTALL_TIMER = "dnf-automatic-install.timer"
+_DNF_TIMERS = (_DNF_TIMER, _DNF_INSTALL_TIMER)
+#: dnf4's, then dnf5's: the last ``apply_updates`` printed is the one that holds.
+_DNF_AUTOMATIC_CONF = "/etc/dnf/automatic.conf /etc/dnf/dnf5-plugins/automatic.conf"
 _UPGRADE_STAMP = "/var/lib/apt/periodic/upgrade-stamp"
 _DPKG_JOURNAL = "/var/lib/dpkg/updates"
 #: Where an upgrade writes: the kernel's modules, its initrd, the package files.
@@ -100,6 +108,10 @@ HOST_STATUS_COMMAND = (
     "echo '[freebsd-running]'; freebsd-version -r; fi; "
     "if command -v apt-config >/dev/null 2>&1; then echo '[apt-config]'; "
     "apt-config dump 2>/dev/null | grep '^APT::Periodic::Unattended-Upgrade '; "
+    # Named through a variable: spelt out, the program name would match BUSY_CHECK
+    # in this very command line.
+    "uu=unattended-upgrade; { command -v \"$uu\" || [ -x \"/usr/bin/$uu\" ]; } >/dev/null 2>&1 "
+    "&& echo '[unattended-upgrade]'; "
     f"echo '[upgrade-stamp]'; stat -c %Y {_UPGRADE_STAMP} 2>/dev/null; fi; "
     "if command -v dpkg-query >/dev/null 2>&1; then "
     f"{BUSY_CHECK}"
@@ -109,10 +121,15 @@ HOST_STATUS_COMMAND = (
     f"echo '[filesystems]'; df -Pk {_FILESYSTEMS}; "
     f"echo '[timers]'; for u in {_APT_TIMER} {' '.join(_DNF_TIMERS)}; do "
     'printf \'%s %s\\n\' "$u" "$(systemctl is-enabled "$u" 2>/dev/null)"; done; '
+    "echo '[dnf-automatic]'; grep -h '^[[:space:]]*apply_updates[[:space:]]*=' "
+    f"{_DNF_AUTOMATIC_CONF} 2>/dev/null; "
     "printf '%s%s\\n' HSTAT_ END; } 2>/dev/null | cat"
 )
 
 _PERIODIC = re.compile(r'^APT::Periodic::Unattended-Upgrade\s+"([^"]*)"')
+_APPLY_UPDATES = re.compile(r"^apply_updates\s*=\s*(\S*)")
+#: What libdnf takes for a true boolean.
+_TRUE = frozenset({"1", "yes", "true", "on"})
 _OFF_STATES = frozenset({"disabled", "masked"})
 _INTERVAL = re.compile(r"(\d+)([smhd]?)")
 _UNIT_SECONDS = {"s": 1, "m": 60, "h": 3600, "d": 86400, "": 86400}
@@ -227,14 +244,25 @@ def _periodic(sections: Dict[str, List[str]]) -> Optional[str]:
     return match.group(1) if match else None
 
 
+def _dnf_applies(sections: Dict[str, List[str]]) -> bool:
+    """Whether dnf-automatic.timer installs what it downloads; dnf's default is no."""
+    lines = sections.get("dnf-automatic") or []
+    values = [m.group(1) for m in map(_APPLY_UPDATES.match, lines) if m]
+    return bool(values) and values[-1].lower() in _TRUE
+
+
 def _auto_updates(sections: Dict[str, List[str]]) -> Optional[bool]:
     timers = _timer_states(sections)
-    if any(timers.get(t) == "enabled" for t in _DNF_TIMERS):
+    if timers.get(_DNF_INSTALL_TIMER) == "enabled":
+        return True
+    downloads_only = timers.get(_DNF_TIMER) == "enabled"
+    if downloads_only and _dnf_applies(sections):
         return True
     if "apt-config" not in sections:
-        return None
+        return False if downloads_only else None
     switched_on = _periodic(sections) not in (None, "", "0")
-    return switched_on and timers.get(_APT_TIMER) not in _OFF_STATES
+    installed = "unattended-upgrade" in sections
+    return switched_on and installed and timers.get(_APT_TIMER) not in _OFF_STATES
 
 
 def _interval(sections: Dict[str, List[str]]) -> Optional[int]:

napalm_device_types/os.py

@@ -36,7 +36,8 @@ class OSDriver(UpdateMixin, ServiceControlMixin, PackageManagementMixin, HealthM
 
     #: Stable key netOrk exposes as ``device_class``. The order in which a
     #: driver lists its role bases is the ranking; see
-    #: :func:`napalm_device_types.roles.primary_role_of`.
+    #: :func:`napalm_device_types.roles.primary_role_of`. A driver for another
+    #: OS sets its own (``ROLE = "windows"``), which replaces this one.
     ROLE: str = "linux"
     TYPE_LABEL: str = "OS"
 
@@ -62,7 +63,7 @@ class OSDriver(UpdateMixin, ServiceControlMixin, PackageManagementMixin, HealthM
         # Users
         # ------------------------------------------------------------------
 
-        def get_users(self) -> List[UserDict]:
+        def get_os_users(self) -> List[UserDict]:
             """
             Returns a list of local OS user accounts.
 

napalm_device_types/package_updates.py

@@ -28,13 +28,18 @@ from napalm_device_types.terminal import strip_terminal_codes
 #: so it is apt's, not cat's.
 APT_UPGRADABLE_COMMAND = "{ LC_ALL=C apt list --upgradable 2>/dev/null; echo __APT_RC=$?; } | cat"
 
-#: Read-only. Lists the packages that a pending security advisory covers.
-DNF_SECURITY_COMMAND = "LC_ALL=C dnf updateinfo list --security --quiet 2>/dev/null"
+#: Read-only. Lists the packages that a pending security advisory covers. A
+#: failed dnf prints nothing, the same as no advisories, so its exit status is
+#: printed inside the group, as apt's is.
+DNF_SECURITY_COMMAND = (
+    "{ LC_ALL=C dnf updateinfo list --security --quiet 2>/dev/null; echo __DNF_RC=$?; } | cat"
+)
 
 # openssl/noble-updates,noble-security 3.0.13-0ubuntu3.6 amd64 [upgradable from: 3.0.13-0ubuntu3.5]
 _APT_LINE = re.compile(r"^(\S+)/(\S+)\s+(\S+)\s+\S+\s+\[upgradable from:\s+(\S+)\]")
 _SECURITY_SUITE = "-security"
 _APT_STATUS = re.compile(r"^__APT_RC=(\d+)\s*$", re.MULTILINE)
+_DNF_STATUS = re.compile(r"^__DNF_RC=(\d+)\s*$", re.MULTILINE)
 
 
 def _joined_lines(output: str) -> List[str]:
@@ -49,17 +54,17 @@ def _joined_lines(output: str) -> List[str]:
     return lines
 
 
-def _apt_listing(output: str) -> str:
-    """The listing without its exit status, or ``ValueError`` when apt failed or
-    the output was cut short -- "could not read" must never look like "nothing
-    pending"."""
+def _listing(output: str, status: re.Pattern[str], what: str) -> str:
+    """The listing without its exit status, or ``ValueError`` when the command
+    failed or the output was cut short -- "could not read" must never look like
+    "nothing pending"."""
     text = strip_terminal_codes(output)
-    statuses = _APT_STATUS.findall(text)
+    statuses = status.findall(text)
     if not statuses:
-        raise ValueError("apt list --upgradable reported no exit status; the output was cut short")
+        raise ValueError(f"{what} reported no exit status; the output was cut short")
     if statuses[-1] != "0":
-        raise ValueError(f"apt list --upgradable failed with exit status {statuses[-1]}")
-    return _APT_STATUS.sub("", text)
+        raise ValueError(f"{what} failed with exit status {statuses[-1]}")
+    return status.sub("", text)
 
 
 def parse_apt_upgradable(output: str) -> List[UpdateDict]:
@@ -71,7 +76,7 @@ def parse_apt_upgradable(output: str) -> List[UpdateDict]:
     :raises ValueError: when apt failed or its exit status never arrived.
     """
     by_name: Dict[str, UpdateDict] = {}
-    for line in _joined_lines(_apt_listing(output)):
+    for line in _joined_lines(_listing(output, _APT_STATUS, "apt list --upgradable")):
         match = _APT_LINE.match(line)
         if not match:
             continue
@@ -102,9 +107,12 @@ def parse_dnf_security(output: str) -> Set[str]:
     """The names of the packages a pending security advisory covers.
 
     Parses :data:`DNF_SECURITY_COMMAND`'s ``ADVISORY  SEVERITY/Sec.  NEVRA`` lines.
+
+    :raises ValueError: when dnf failed or its exit status never arrived -- every
+        update would otherwise read as not security.
     """
     names: Set[str] = set()
-    for line in output.splitlines():
+    for line in _listing(output, _DNF_STATUS, "dnf updateinfo list --security").splitlines():
         parts = line.split()
         if len(parts) >= 3 and parts[1].endswith("/Sec."):
             names.add(nevra_name(parts[-1]))

napalm_device_types/roles.py

@@ -40,9 +40,13 @@ def roles_of(driver_cls: type) -> List[Type[Any]]:
     """Every role base in ``driver_cls``'s MRO, most significant first.
 
     MRO order is inheritance order, so the list reflects exactly what the driver
-    author wrote in the class definition.
+    author wrote in the class definition. A class that sets ``ROLE`` itself
+    replaces the role it inherits rather than adding one: a Windows driver on
+    :class:`~napalm_device_types.os.OSDriver` sets ``ROLE = "windows"`` and is
+    not also ``"linux"``.
     """
-    return [cls for cls in driver_cls.__mro__ if _is_role_base(cls)]
+    roles = [cls for cls in driver_cls.__mro__ if _is_role_base(cls)]
+    return [cls for cls in roles if not any(o is not cls and issubclass(o, cls) for o in roles)]
 
 
 def role_keys_of(driver_cls: type) -> List[str]:

napalm_device_types/ssh_server.py

@@ -22,7 +22,9 @@ Only without such an ``Include`` is sshd_config itself rewritten, after a copy.
 
 1. The plan never removes the last algorithm of a class, and -- given the
    lists of the client that manages the host -- never the last one that client
-   speaks. Each one kept back is reported, with why.
+   speaks. Each one kept back is reported, with why. For host key algorithms
+   only those the host has a key for count (``ssh-keygen -l`` on each
+   ``HostKey``); when the keys cannot be read, the host key list is not touched.
 2. ``sshd -t`` must accept the result, and ``sshd -T`` must show the new lists.
    A file read before ours can set the same directive; ours is then valid and
    ignored, and that is reported rather than called a success.
@@ -36,7 +38,7 @@ from __future__ import annotations
 import time
 from dataclasses import dataclass, field
 from shlex import quote
-from typing import Callable, Dict, Iterable, List, Mapping, Optional, Sequence, Tuple, TYPE_CHECKING
+from typing import Callable, Dict, Iterable, List, Mapping, Optional, Sequence, Set, Tuple, TYPE_CHECKING
 
 if TYPE_CHECKING:  # pragma: no cover - for type checkers only
     from napalm_device_types.channel import CommandResult
@@ -61,6 +63,17 @@ _HEADER = (
     "# taken out of what this server offers. Delete this file to undo it.\n"
 )
 
+#: Host key type, as ``ssh-keygen -l`` prints it -> the algorithms that key serves.
+#: Certificate algorithms are left out: they also need a HostCertificate.
+_KEY_TYPE_ALGORITHMS: Dict[str, Tuple[str, ...]] = {
+    "ED25519": ("ssh-ed25519",),
+    "RSA": ("rsa-sha2-512", "rsa-sha2-256", "ssh-rsa"),
+    "DSA": ("ssh-dss",),
+    "ED25519-SK": ("sk-ssh-ed25519@openssh.com",),
+    "ECDSA-SK": ("sk-ecdsa-sha2-nistp256@openssh.com",),
+}
+_ECDSA_CURVES = ("256", "384", "521")
+
 _RELOAD = (
     "if systemctl is-active --quiet ssh 2>/dev/null; then systemctl reload ssh;"
     " elif systemctl is-active --quiet sshd 2>/dev/null; then systemctl reload sshd;"
@@ -100,12 +113,46 @@ def parse_sshd_effective(text: str) -> Dict[str, List[str]]:
     return found
 
 
+def parse_sshd_hostkeys(text: str) -> List[str]:
+    """The ``hostkey`` paths from ``sshd -T`` output, in order."""
+    paths: List[str] = []
+    for raw in text.splitlines():
+        parts = raw.strip().split(None, 1)
+        if len(parts) == 2 and parts[0].lower() == "hostkey":
+            paths.append(parts[1].strip())
+    return paths
+
+
+def host_key_algorithms(fingerprints: str) -> Optional[Set[str]]:
+    """The host key algorithms the keys in ``ssh-keygen -l`` output can serve.
+
+    None when a line is not understood or there is no key: then nothing is known.
+    """
+    usable: Set[str] = set()
+    for raw in fingerprints.splitlines():
+        parts = raw.split()
+        if not parts:
+            continue
+        kind = parts[-1].strip("()")
+        if len(parts) < 3 or not parts[-1].startswith("("):
+            return None
+        if kind == "ECDSA" and parts[0] in _ECDSA_CURVES:
+            usable.add(f"ecdsa-sha2-nistp{parts[0]}")
+        elif kind in _KEY_TYPE_ALGORITHMS:
+            usable.update(_KEY_TYPE_ALGORITHMS[kind])
+        else:
+            return None
+    return usable or None
+
+
 @dataclass(frozen=True)
 class Refusal:
     """One algorithm that was asked to go and stays, with why.
 
-    *reason* is ``"last"`` (the class would be empty), ``"client"`` (the managing
-    client would share none of the class with the server) or ``"not_offered"``
+    *reason* is ``"last"`` (the class would be empty -- for host keys: have no
+    algorithm the host has a key for), ``"client"`` (the managing client would
+    share none of the class with the server), ``"host_keys_unknown"`` (the host
+    keys could not be read, so the host key list stays) or ``"not_offered"``
     (the server does not offer it, so there is nothing to remove).
     """
 
@@ -131,12 +178,18 @@ def plan_ssh_hardening(
     remove: Mapping[str, Iterable[str]],
     *,
     client: Optional[Mapping[str, Iterable[str]]] = None,
+    host_keys: Optional[Iterable[str]] = None,
 ) -> SshHardeningPlan:
     """What the lists become when *remove* is taken out of *effective*.
 
     *client* is what the client managing this host speaks, per class. A class it
     has no list for is not guarded beyond "never empty".
 
+    *host_keys* are the host key algorithms the host has a key for (see
+    :func:`host_key_algorithms`). Only those count as a host key algorithm that
+    stays: sshd offers the others but cannot use them. Without it nothing is
+    known, and no host key algorithm is removed.
+
     When a removal would leave a class empty, or without an algorithm *client*
     speaks, the first of the asked-for algorithms in the server's order that
     fixes it stays, and is reported in ``refused``.
@@ -150,21 +203,32 @@ def plan_ssh_hardening(
     removed: Dict[str, List[str]] = {}
     refused: List[Refusal] = []
     not_offered: List[Refusal] = []
+    usable_keys = set(host_keys) if host_keys is not None else None
     for kind in SSH_ALGORITHM_CLASSES:
         offered = list(effective.get(kind, []))
         asked = set(remove.get(kind, []))
         not_offered += [Refusal(kind, a, "not_offered") for a in sorted(asked - set(offered))]
         going = [a for a in offered if a in asked]
-        staying = [a for a in offered if a not in asked]
+        if kind == "hostkey" and usable_keys is None:
+            refused += [Refusal(kind, a, "host_keys_unknown") for a in going]
+            going = []
+        # A host key algorithm without a key is offered, but nobody gets in with it.
+        usable = usable_keys if kind == "hostkey" and usable_keys is not None else set(offered)
+        staying = [a for a in offered if a not in going and a in usable]
         speaks = set(client[kind]) if client and kind in client else None
         if going and speaks is not None and not speaks & set(staying):
-            spared = next((a for a in going if a in speaks), None)
+            spared = next((a for a in going if a in speaks and a in usable), None)
             if spared:
                 going.remove(spared)
+                staying.append(spared)
                 refused.append(Refusal(kind, spared, "client"))
-        if going and not [a for a in offered if a not in going]:
-            spared = going.pop(0)
-            refused.append(Refusal(kind, spared, "last"))
+        if going and not staying:
+            spared = next((a for a in going if a in usable), None)
+            if spared is None and not [a for a in offered if a not in going]:
+                spared = going[0]
+            if spared is not None:
+                going.remove(spared)
+                refused.append(Refusal(kind, spared, "last"))
         keep[kind] = [a for a in offered if a not in going]
         if going:
             removed[kind] = going
@@ -284,14 +348,17 @@ class OpenSshServerMixin:
         result = self._ssh_run(f"cat -- {quote(path)}")
         return result.stdout if result.exit_code == 0 else None
 
-    def _ssh_write(self, path: str, content: str) -> None:
+    def _ssh_write(self, path: str, content: str, mode_of: Optional[str] = None) -> None:
         """Replace *path* with *content*, as root, without root reading stdin.
 
         A driver gains root with ``sudo -S`` and puts the password in front of
         stdin. A sudo that does not prompt -- NOPASSWD, a cached timestamp --
         leaves it there, and a ``cat`` under sudo writes it into the file. So the
         content goes, without privilege, to a file only the login user can read;
-        root then installs that file (mode 644, owned by root) and removes it.
+        root then installs that file, owned by root, and removes it.
+
+        The mode is that of *mode_of* (default: *path* itself), so a 0600
+        sshd_config stays 0600 and so does its copy; 644 for a new file.
         """
         staged = self.run_command(
             'umask 077 && t=$(mktemp /tmp/netork-ssh.XXXXXX) && cat > "$t" && printf %s "$t"',
@@ -304,7 +371,10 @@ class OpenSshServerMixin:
             raise RuntimeError(f"could not stage {path}: {(staged.stderr or staged.stdout).strip()}")
         part = path + ".netork-tmp"
         result = self._ssh_run(
-            f"install -m 644 -- {quote(tmp)} {quote(part)} && mv -f -- {quote(part)} {quote(path)};"
+            # GNU stat, then BSD's; 644 for a file that does not exist yet.
+            f"r={quote(mode_of or path)}; m=$(stat -c %a -- \"$r\" 2>/dev/null"
+            f' || stat -f %Lp -- "$r" 2>/dev/null) || m=644;'
+            f' install -m "$m" -- {quote(tmp)} {quote(part)} && mv -f -- {quote(part)} {quote(path)};'
             f" s=$?; rm -f -- {quote(tmp)} {quote(part)}; exit $s"
         )
         if result.exit_code != 0:
@@ -324,10 +394,29 @@ class OpenSshServerMixin:
 
         :raises RuntimeError: if sshd -T fails -- no OpenSSH, or no root.
         """
+        return parse_sshd_effective(self._sshd_effective_text())
+
+    def _sshd_effective_text(self) -> str:
         result = self._ssh_run("PATH=$PATH:/usr/sbin:/sbin sshd -T")
         if result.exit_code != 0:
             raise RuntimeError(f"sshd -T failed: {(result.stderr or result.stdout).strip()}")
-        return parse_sshd_effective(result.stdout)
+        return result.stdout
+
+    def _ssh_host_key_algorithms(self, sshd_t: str) -> Optional[Set[str]]:
+        """The host key algorithms the host has a key for; None when that cannot be read.
+
+        A ``HostKey`` whose file does not exist is skipped, as sshd skips it.
+        """
+        paths = parse_sshd_hostkeys(sshd_t)
+        if not paths:
+            return None
+        result = self._ssh_run(
+            f"for k in {' '.join(quote(p) for p in paths)}; do"
+            ' [ -e "$k" ] || continue; ssh-keygen -l -f "$k" || exit 1; done'
+        )
+        if result.exit_code != 0:
+            return None
+        return host_key_algorithms(result.stdout)
 
     def _sshd_accepts(self) -> Optional[str]:
         """None when ``sshd -t`` accepts the configuration, else what it said."""
@@ -357,9 +446,20 @@ class OpenSshServerMixin:
         """
         say = log or (lambda _line: None)
         say("Reading the algorithms sshd offers (sshd -T)")
-        plan = plan_ssh_hardening(self.get_ssh_server_algorithms(), remove, client=client)
+        sshd_t = self._sshd_effective_text()
+        host_keys: Optional[Set[str]] = None
+        if remove.get("hostkey"):
+            say("Reading which host keys the server has (ssh-keygen -l)")
+            host_keys = self._ssh_host_key_algorithms(sshd_t)
+        plan = plan_ssh_hardening(parse_sshd_effective(sshd_t), remove, client=client, host_keys=host_keys)
+        whys = {
+            "last": "it is the last one",
+            "client": "netOrk could no longer connect",
+            "host_keys_unknown": "the host keys could not be read",
+        }
         for r in plan.refused:
-            why = "it is the last one" if r.reason == "last" else "netOrk could no longer connect"
+            last_key = r.reason == "last" and r.kind == "hostkey"
+            why = "it is the last one the host has a key for" if last_key else whys[r.reason]
             say(f"Keeping {r.algorithm}: {why}")
         kinds = plan.changed_classes
         result = SshHardeningResult(plan=plan)
@@ -396,7 +496,7 @@ class OpenSshServerMixin:
 
             return restore
         backup = f"{MAIN_CONFIG}.netork-{time.strftime('%Y%m%d-%H%M%S')}.bak"
-        self._ssh_write(backup, main)
+        self._ssh_write(backup, main, mode_of=MAIN_CONFIG)
         self._ssh_write(MAIN_CONFIG, render_main_config(main, plan.keep, plan.changed_classes))
         result.path, result.backup = MAIN_CONFIG, backup
         say(f"{MAIN_CONFIG} has no Include of {DROPIN_DIR}; rewrote it (copy at {backup})")

napalm_device_types/virtual_ips.py

@@ -34,7 +34,8 @@ host.
   handed to a root shell.
 
 **None is "did not look".** A file that exists but cannot be read, an include
-that cannot be followed, a host without systemd: that mechanism is None, and the
+that cannot be followed, a host without systemd (no ``systemctl``, or systemd not
+running as PID 1, as in a container): that mechanism is None, and the
 caller keeps what it knew. ``[]`` is a host that has none.
 
 **Root, and without it.** The configuration may be root's alone. The whole
@@ -60,6 +61,8 @@ from napalm_device_types.terminal import strip_terminal_codes
 _BEGIN = "VIPS_BEGIN"
 _END = "VIPS_END"
 _NO_SYSTEMD = "no-systemd"
+#: Exists only while systemd runs as PID 1; a container may carry systemctl without.
+_SYSTEMD_RUN = "/run/systemd/system"
 #: A record that says a mechanism was not fully read.
 _BLIND = frozenset({"unreadable", "too-deep"})
 _KEEPALIVED_CONF = "/etc/keepalived/keepalived.conf"
@@ -72,7 +75,9 @@ _CFG_KEYS = "(ip|netmask|mask|interface|iface|trigger-key|key)"
 #: ``instance <name>`` / ``interface`` / ``virtual_router_id`` / ``vip <addr> [dev <if>]``
 #: / ``end``, and ``unreadable``/``too-deep``/``missing <path>``. No ``>`` but in
 #: ``2>/dev/null``: comparisons are written the other way round. ``\047`` is a
-#: single quote, so the program itself needs none.
+#: single quote, so the program itself needs none. A statement ends with its line
+#: unless the next one opens with ``{``: keepalived takes ``vrrp_instance VI_2``
+#: and its brace on two lines.
 _AWK = " ".join(
     (
         r'function q(p) { return "\047" p "\047" }',
@@ -102,10 +107,11 @@ _AWK = " ".join(
         r' if (4 < d || (f in seen)) { print "too-deep " f; return }'
         r" if (!ok(f)) return; seen[f] = 1;"
         r' while (0 < (r = (getline l < f))) { sub(/[#!].*/, "", l); gsub(/[{}]/, " & ", l);'
-        r' n = split(l, t, " "); nw = 0; for (k = 1; k <= n; k++) {'
+        r' n = split(l, t, " "); if (n && nw && t[1] != "{") { stmt(w, nw, f, d); nw = 0 }'
+        r' for (k = 1; k <= n; k++) {'
         r' if (t[k] == "{") { push(nw ? w[1] : "", 1 < nw ? w[2] : ""); nw = 0 }'
-        r' else if (t[k] == "}") { stmt(w, nw, f, d); nw = 0; pop() } else w[++nw] = t[k] }'
-        r" stmt(w, nw, f, d) }"
+        r' else if (t[k] == "}") { stmt(w, nw, f, d); nw = 0; pop() } else w[++nw] = t[k] } }'
+        r" stmt(w, nw, f, d);"
         r' if (r < 0) print "unreadable " f; close(f); delete seen[f] }',
         r"function inc(p, f, d,   c, g, dir, fs, n, i) {"
         r' if (p !~ /^\//) { dir = f; sub(/[^\/]*$/, "", dir); p = dir p }'
@@ -138,7 +144,7 @@ _AWK = " ".join(
 VIRTUAL_IPS_COMMAND = (
     "PATH=$PATH:/usr/sbin:/sbin; "
     "printf '%s%s\\n' VIPS_ BEGIN; "
-    "if command -v systemctl >/dev/null 2>&1; then echo '[systemd]'; "
+    f"if command -v systemctl >/dev/null 2>&1 && [ -d {_SYSTEMD_RUN} ]; then echo '[systemd]'; "
     "for u in $(systemctl list-units --all --plain --no-legend --type=service "
     "vip-manager.service 'vip-manager@*' keepalived.service 2>/dev/null | awk '{print $1}'); do "
     "case $u in keepalived*) m=keepalived;; *) m=vip-manager;; esac; "