Re: [chrony-dev] [PATCH] nm-dispatcher: handle NTP servers from DHCP

[Robert Fairley <rfairley@xxxxxxxxxx>]

On Thu, May 28, 2020 at 5:44 AM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> > From 9cf2514489352140f70d650cfc6f41f4e4f6a983 Mon Sep 17 00:00:00 2001
> > From: Robert Fairley <rfairley@xxxxxxxxxx>
> > Date: Sat, 23 May 2020 02:10:38 -0400
> > Subject: [PATCH 1/2] examples: upstream chrony.helper script
>
> This patch looks good to me, but please send them as attachments or
> separate mails, not mangled by your MUA. git am complained it is
> corrupted.

Right - apologies for that previously, had pasted them into the email
text before. Sent them as separate attachments now to this email.

> > From fe3db7858a3fd6180c89fd25fd37a5e16805b31a Mon Sep 17 00:00:00 2001
> > From: Robert Fairley <rfairley@xxxxxxxxxx>
> > Date: Wed, 20 May 2020 14:36:38 -0400
> > Subject: [PATCH 2/2] nm-dispatcher: handle NTP servers from DHCP
>
> Please change the subject prefix to "examples:".

Updated.

> > +# Make sure the two action interface and action arguments are passed by
> > +# NetworkManager before continuing.
> > +[ $# -lt 2 ] && exit 0
>
> I suspect this breaks the networkd-dispatcher support. IIRC there are no
> arguments passed to the script.

I see - I should have looked into what'd happen if less than 2
arguments were passed. Reading on networkd-dispatcher - I think to
best preserve compatibility of the original chrony.nm-dispatcher
file (e.g. so downstreams using networkd-dispatcher don't need to
patch the file), the DHCP bits should be kept in a separate
dispatcher script. I updated the patch to instead add
`chrony.nm-dispatcher.dhcp.in`. That way, `chrony.nm-dispatcher.dhcp`
(after substituting macros) can be packaged downstream as
`/usr/lib/NetworkManager/dispatcher.d/20-chrony-dhcp` or so, or
ignored if not using NetworkManager.

Alternatively, the DHCP parts could be kept in the same file
(`chrony.nm-dispatcher`) with `[ $# -lt 2 ] && exit 0` removed, then
the added DHCP parts shouldn't affect networkd-dispatcher (as DHCP
parts will only be active if two arguments are passed). Distros using
networkd would probably still want to patch out the DHCP code though
as it'll be inactive code - so I decided to make it a separate file here.
Keeping it to one dispatcher file should be fine though,
functionally, if preferred.

> > +    for server in $DHCP4_NTP_SERVERS; do
> > +        echo "$server iburst" >> "$dhcp_server_file"
> > +    done
> > +    /usr/libexec/chrony-helper update-daemon || :
>
> It would be nice to have the default options and the chrony-helper
> path in variables.

Done! I also made chronyc configurable in the existing nm-dispatcher
while at it.

Attached an interdiff of the overall changes from last comments.

I have also updated https://src.fedoraproject.org/rpms/chrony/pull-request/3
with the downstream patch that would be applied downstream to work with
Fedora/RHEL.

From 3f26a5e7c60cdaec25b69f7b4805096b0f44514d Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:24 -0400
Subject: [PATCH 1/3] examples: upstream chrony.helper script

Add an example chrony.helper file, which was initially copied from
the downstream Fedora downstream package, with distro-specific parts
taken out. Overall the following changes are made from the downstream
version:

* Remove use of `/etc/sysconfig`, which is specific to
  Fedora/RHEL.
* Make the paths in variables such as chrony_conf and
  helper_dir configurable through macros @CHRONY_CONF@ and
  @CHRONY_HELPER_DIR@. Values used here may vary across
  distributions, and should be set by downstream packages.
* Introduce the server_dir variable, where `chrony.servers.*` files
  are placed, so that a common macro @CHRONY_SERVER_DIR@ between the
  helper script and the NetworkManager dispatch script can be used.
* Remove dhclient identifiers and commands.
---
 examples/chrony.helper.in | 264 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 264 insertions(+)
 create mode 100644 examples/chrony.helper.in

diff --git a/examples/chrony.helper.in b/examples/chrony.helper.in
new file mode 100644
index 0000000..d50ccf7
--- /dev/null
+++ b/examples/chrony.helper.in
@@ -0,0 +1,264 @@
+#!/bin/bash
+# This script configures running chronyd to use NTP servers obtained from
+# DHCP and _ntp._udp DNS SRV records. Files with servers from DHCP are managed
+# externally (e.g. by a dhclient script). Files with servers from DNS SRV
+# records are updated here using the dig utility. The script can also list
+# and set static sources in the chronyd configuration file.
+
+chronyc=@CHRONYC@
+chrony_conf=@CHRONY_CONF@
+chrony_service=@CHRONY_SERVICE@
+default_options=@CHRONY_DEFAULT_OPTIONS@
+helper_dir=@CHRONY_HELPER_DIR@
+server_dir=@CHRONY_SERVER_DIR@
+
+dhcp_servers_files="${server_dir}/chrony.servers.*"
+added_servers_file=$helper_dir/added_servers
+dnssrv_servers_files="$helper_dir/dnssrv@*"
+dnssrv_timer_prefix=chrony-dnssrv@
+
+chrony_command() {
+    $chronyc -a -n -m "$1"
+}
+
+is_running() {
+    chrony_command "tracking" &> /dev/null
+}
+
+get_servers_files() {
+    echo "$dhcp_servers_files"
+    echo "$dnssrv_servers_files"
+}
+
+is_update_needed() {
+    for file in $(get_servers_files) $added_servers_file; do
+        [ -e "$file" ] && return 0
+    done
+    return 1
+}
+
+update_daemon() {
+    local all_servers_with_args all_servers added_servers
+
+    if ! is_running; then
+        rm -f $added_servers_file
+        return 0
+    fi
+
+    all_servers_with_args=$(cat $(get_servers_files) 2> /dev/null)
+
+    all_servers=$(
+        echo "$all_servers_with_args" |
+            while read -r server serverargs; do
+                echo "$server"
+            done | sort -u)
+    added_servers=$( (
+        cat $added_servers_file 2> /dev/null
+        echo "$all_servers_with_args" |
+            while read -r server serverargs; do
+                [ -z "$server" ] && continue
+                chrony_command "add server $server $serverargs" &> /dev/null &&
+                    echo "$server"
+            done) | sort -u)
+
+    comm -23 <(echo -n "$added_servers") <(echo -n "$all_servers") |
+        while read -r server; do
+            chrony_command "delete $server" &> /dev/null
+        done
+
+    added_servers=$(comm -12 <(echo -n "$added_servers") <(echo -n "$all_servers"))
+
+    if [ -n "$added_servers" ]; then
+        echo "$added_servers" > $added_servers_file
+    else
+        rm -f $added_servers_file
+    fi
+}
+
+get_dnssrv_servers() {
+    local name=$1 output
+
+    if ! command -v dig &> /dev/null; then
+        echo "Missing dig (DNS lookup utility)" >&2
+        return 1
+    fi
+
+    output=$(dig "$name" srv +short +ndots=2 +search 2> /dev/null) || return 0
+
+    echo "$output" | while read -r _ _ port target; do
+        server=${target%.}
+        [ -z "$server" ] && continue
+        echo "$server port $port $default_options"
+    done
+}
+
+check_dnssrv_name() {
+    local name=$1
+
+    if [ -z "$name" ]; then
+        echo "No DNS SRV name specified" >&2
+        return 1
+    fi
+
+    if [ "${name:0:9}" != _ntp._udp ]; then
+        echo "DNS SRV name $name doesn't start with _ntp._udp" >&2
+        return 1
+    fi
+}
+
+update_dnssrv_servers() {
+    local name=$1
+    local srv_file=$helper_dir/dnssrv@$name servers
+
+    check_dnssrv_name "$name" || return 1
+
+    servers=$(get_dnssrv_servers "$name")
+    if [ -n "$servers" ]; then
+        echo "$servers" > "$srv_file"
+    else
+        rm -f "$srv_file"
+    fi
+}
+
+set_dnssrv_timer() {
+    local state=$1 name=$2
+    local srv_file=$helper_dir/dnssrv@$name servers
+    local timer
+
+    timer=$dnssrv_timer_prefix$(systemd-escape "$name").timer || return 1
+
+    check_dnssrv_name "$name" || return 1
+
+    if [ "$state" = enable ]; then
+        systemctl enable "$timer"
+        systemctl start "$timer"
+    elif [ "$state" = disable ]; then
+        systemctl stop "$timer"
+        systemctl disable "$timer"
+        rm -f "$srv_file"
+    fi
+}
+
+list_dnssrv_timers() {
+    systemctl --all --full -t timer list-units | grep "^$dnssrv_timer_prefix" | \
+            sed "s|^$dnssrv_timer_prefix\(.*\)\.timer.*|\1|" |
+        while read -r name; do
+            systemd-escape --unescape "$name"
+        done
+}
+
+prepare_helper_dir() {
+    mkdir -p $helper_dir
+    exec 100> $helper_dir/lock
+    if ! flock -w 20 100; then
+        echo "Failed to lock $helper_dir" >&2
+        return 1
+    fi
+}
+
+is_source_line() {
+    local pattern="^[ \t]*(server|pool|peer|refclock)[ \t]+[^ \t]+"
+    [[ "$1" =~ $pattern ]]
+}
+
+list_static_sources() {
+    while read -r line; do
+        if is_source_line "$line"; then
+            echo "$line"
+        fi
+    done < $chrony_conf
+}
+
+set_static_sources() {
+    local new_config tmp_conf
+
+    new_config=$(
+        sources=$(
+            while read -r line; do
+                is_source_line "$line" && echo "$line"
+            done)
+
+        while read -r line; do
+            if ! is_source_line "$line"; then
+                echo "$line"
+                continue
+            fi
+
+            tmp_sources=$(
+                local removed=0
+
+                echo "$sources" | while read -r line2; do
+                    if [ "$removed" -ne 0 ] || [ "$line" != "$line2" ]; then
+                        echo "$line2"
+                    else
+                        removed=1
+                    fi
+                done)
+
+            [ "$sources" == "$tmp_sources" ] && continue
+            sources=$tmp_sources
+            echo "$line"
+        done < $chrony_conf
+
+        echo "$sources"
+    )
+
+    tmp_conf=${chrony_conf}.tmp
+
+    cp -a $chrony_conf $tmp_conf &&
+        echo "$new_config" > $tmp_conf &&
+        mv $tmp_conf $chrony_conf || return 1
+
+    systemctl try-restart $chrony_service
+}
+
+print_help() {
+    echo "Usage: $0 COMMAND"
+    echo
+    echo "Commands:"
+    echo "	update-daemon"
+    echo "	update-dnssrv-servers NAME"
+    echo "	enable-dnssrv NAME"
+    echo "	disable-dnssrv NAME"
+    echo "	list-dnssrv"
+    echo "	list-static-sources"
+    echo "	set-static-sources < sources.list"
+    echo "	is-running"
+    echo "	command CHRONYC-COMMAND"
+}
+
+case "$1" in
+    update-daemon)
+        is_update_needed || exit 0
+        prepare_helper_dir && update_daemon
+        ;;
+    update-dnssrv-servers)
+        prepare_helper_dir && update_dnssrv_servers "$2" && update_daemon
+        ;;
+    enable-dnssrv)
+        set_dnssrv_timer enable "$2"
+        ;;
+    disable-dnssrv)
+        set_dnssrv_timer disable "$2" && prepare_helper_dir && update_daemon
+        ;;
+    list-dnssrv)
+        list_dnssrv_timers
+        ;;
+    list-static-sources)
+        list_static_sources
+        ;;
+    set-static-sources)
+        set_static_sources
+        ;;
+    is-running)
+        is_running
+        ;;
+    command|forced-command)
+        chrony_command "$2"
+        ;;
+    *)
+        print_help
+        exit 2
+esac
+
+exit $?
-- 
2.26.2

From 1c794144c6efc0ff2a681ebe12ce18c77ce8c747 Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:35 -0400
Subject: [PATCH 2/3] examples: add dispatcher for NTP servers from DHCP

Add new NM dispatcher script for NTP servers given by DHCP through
NetworkManager in a similar way to how distributions have done in
11-dhclient, e.g. [1]. New NTP servers are written as entries to a
file per-interface in CHRONY_SERVER_DIR, which is read by
chrony.helper when updating the chrony service.

This provides a way for NTP server configuration to be carried over
from NetworkManager DHCP events to chrony, for DHCP clients other
than dhclient. Part of fixing integration where the NetworkManager
internal client is used, e.g [2].

Similarly to the chrony.helper script, make paths and options
configurable through macros that can be substituted at install time.

[1] https://src.fedoraproject.org/rpms/dhcp/blob/master/f/11-dhclient
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1800901
---
 examples/chrony.nm-dispatcher.dhcp.in | 44 +++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 examples/chrony.nm-dispatcher.dhcp.in

diff --git a/examples/chrony.nm-dispatcher.dhcp.in b/examples/chrony.nm-dispatcher.dhcp.in
new file mode 100644
index 0000000..5835e05
--- /dev/null
+++ b/examples/chrony.nm-dispatcher.dhcp.in
@@ -0,0 +1,44 @@
+#!/bin/sh
+# This is a NetworkManager dispatcher script for chronyd to update
+# its NTP sources passed from DHCP options. Note that this script is
+# specific to NetworkManager-dispatcher due to use of the
+# DHCP4_NTP_SERVERS environment variable. For networkd-dispatcher,
+# an alternative approach is external means such as a dhclient hook.
+
+export LC_ALL=C
+
+interface=$1
+action=$2
+
+chrony_helper=@CHRONY_HELPER@
+default_options=@CHRONY_DEFAULT_OPTIONS@
+server_dir=@CHRONY_SERVER_DIR@
+
+dhcp_server_file=$server_dir/chrony.servers.$interface
+# DHCP4_NTP_SERVERS is passed from DHCP options by NetworkManager.
+nm_dhcp_servers=$DHCP4_NTP_SERVERS
+
+add_servers_from_dhcp() {
+    rm -f "$dhcp_server_file"
+    for server in $nm_dhcp_servers; do
+        echo "$server $default_options" >> "$dhcp_server_file"
+    done
+    $chrony_helper update-daemon || :
+}
+
+clear_servers_from_dhcp() {
+    if [ -f "$dhcp_server_file" ]; then
+        rm -f "$dhcp_server_file"
+        $chrony_helper update-daemon || :
+    fi
+}
+
+mkdir -p $server_dir
+
+if [ "$action" = "up" ] || [ "$action" = "dhcp4-change" ]; then
+    add_servers_from_dhcp
+elif [ "$action" = "down" ]; then
+    clear_servers_from_dhcp
+fi
+
+exit 0
-- 
2.26.2

From eb5e634030e4f89567092e8afbd8ded4dd8a9d6a Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:40 -0400
Subject: [PATCH 3/3] examples/nm-dispatcher: make chronyc configurable

Use the @CHRONYC@ macro, used in the chrony.helper example, to make
the chronyc command path configurable at install time. Add the
suffix `.in` to the filename to indicate this must be substituted.
---
 examples/{chrony.nm-dispatcher => chrony.nm-dispatcher.in} | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 rename examples/{chrony.nm-dispatcher => chrony.nm-dispatcher.in} (87%)

diff --git a/examples/chrony.nm-dispatcher b/examples/chrony.nm-dispatcher.in
similarity index 87%
rename from examples/chrony.nm-dispatcher
rename to examples/chrony.nm-dispatcher.in
index 0b0c3e7..7c0e2dd 100644
--- a/examples/chrony.nm-dispatcher
+++ b/examples/chrony.nm-dispatcher.in
@@ -5,11 +5,13 @@
 
 export LC_ALL=C
 
+chronyc=@CHRONYC@
+
 # For NetworkManager consider only up/down events
 [ $# -ge 2 ] && [ "$2" != "up" ] && [ "$2" != "down" ] && exit 0
 
 # Note: for networkd-dispatcher routable.d ~= on and off.d ~= off
 
-chronyc onoffline > /dev/null 2>&1
+$chronyc onoffline > /dev/null 2>&1
 
 exit 0
-- 
2.26.2

diff --git a/examples/chrony.helper.in b/examples/chrony.helper.in
index b743a08..d50ccf7 100644
--- a/examples/chrony.helper.in
+++ b/examples/chrony.helper.in
@@ -8,6 +8,7 @@
 chronyc=@CHRONYC@
 chrony_conf=@CHRONY_CONF@
 chrony_service=@CHRONY_SERVICE@
+default_options=@CHRONY_DEFAULT_OPTIONS@
 helper_dir=@CHRONY_HELPER_DIR@
 server_dir=@CHRONY_SERVER_DIR@
 
@@ -87,7 +88,7 @@ get_dnssrv_servers() {
     echo "$output" | while read -r _ _ port target; do
         server=${target%.}
         [ -z "$server" ] && continue
-        echo "$server port $port iburst"
+        echo "$server port $port $default_options"
     done
 }
 
diff --git a/examples/chrony.nm-dispatcher.dhcp.in b/examples/chrony.nm-dispatcher.dhcp.in
new file mode 100644
index 0000000..5835e05
--- /dev/null
+++ b/examples/chrony.nm-dispatcher.dhcp.in
@@ -0,0 +1,44 @@
+#!/bin/sh
+# This is a NetworkManager dispatcher script for chronyd to update
+# its NTP sources passed from DHCP options. Note that this script is
+# specific to NetworkManager-dispatcher due to use of the
+# DHCP4_NTP_SERVERS environment variable. For networkd-dispatcher,
+# an alternative approach is external means such as a dhclient hook.
+
+export LC_ALL=C
+
+interface=$1
+action=$2
+
+chrony_helper=@CHRONY_HELPER@
+default_options=@CHRONY_DEFAULT_OPTIONS@
+server_dir=@CHRONY_SERVER_DIR@
+
+dhcp_server_file=$server_dir/chrony.servers.$interface
+# DHCP4_NTP_SERVERS is passed from DHCP options by NetworkManager.
+nm_dhcp_servers=$DHCP4_NTP_SERVERS
+
+add_servers_from_dhcp() {
+    rm -f "$dhcp_server_file"
+    for server in $nm_dhcp_servers; do
+        echo "$server $default_options" >> "$dhcp_server_file"
+    done
+    $chrony_helper update-daemon || :
+}
+
+clear_servers_from_dhcp() {
+    if [ -f "$dhcp_server_file" ]; then
+        rm -f "$dhcp_server_file"
+        $chrony_helper update-daemon || :
+    fi
+}
+
+mkdir -p $server_dir
+
+if [ "$action" = "up" ] || [ "$action" = "dhcp4-change" ]; then
+    add_servers_from_dhcp
+elif [ "$action" = "down" ]; then
+    clear_servers_from_dhcp
+fi
+
+exit 0
diff --git a/examples/chrony.nm-dispatcher.in b/examples/chrony.nm-dispatcher.in
index 2acf97f..7c0e2dd 100644
--- a/examples/chrony.nm-dispatcher.in
+++ b/examples/chrony.nm-dispatcher.in
@@ -1,49 +1,17 @@
 #!/bin/sh
 # This is a NetworkManager dispatcher / networkd-dispatcher script for
-# chronyd to handle its NTP sources. It sets the NTP sources online or
-# offline when a network interface is configured or removed. On DHCP
-# change, chrony will update its NTP sources passed from DHCP options.
+# chronyd to set its NTP sources online or offline when a network interface
+# is configured or removed
 
 export LC_ALL=C
 
-# Make sure the two action interface and action arguments are passed by
-# NetworkManager before continuing.
-[ $# -lt 2 ] && exit 0
+chronyc=@CHRONYC@
 
-interface=$1
-action=$2
+# For NetworkManager consider only up/down events
+[ $# -ge 2 ] && [ "$2" != "up" ] && [ "$2" != "down" ] && exit 0
 
-server_dir=@CHRONY_SERVER_DIR@
-dhcp_server_file=$server_dir/chrony.servers.$interface
+# Note: for networkd-dispatcher routable.d ~= on and off.d ~= off
 
-mkdir -p $server_dir
-
-add_servers_from_dhcp() {
-    rm -f "$dhcp_server_file"
-    # DHCP4_NTP_SERVERS is passed from DHCP options by
-    # NetworkManager.
-    for server in $DHCP4_NTP_SERVERS; do
-        echo "$server iburst" >> "$dhcp_server_file"
-    done
-    /usr/libexec/chrony-helper update-daemon || :
-}
-
-clear_servers_from_dhcp() {
-    if [ -f "$dhcp_server_file" ]; then
-        rm -f "$dhcp_server_file"
-        /usr/libexec/chrony-helper update-daemon || :
-    fi
-}
-
-if [ "$action" = "up" ] || [ "$action" = "dhcp4-change" ]; then
-    add_servers_from_dhcp
-elif [ "$action" = "down" ]; then
-    clear_servers_from_dhcp
-fi
-
-if [ "$action" = "up" ] || [ "$action" = "down" ]; then
-    # Note: for networkd-dispatcher routable.d ~= on and off.d ~= off
-    chronyc onoffline > /dev/null 2>&1
-fi
+$chronyc onoffline > /dev/null 2>&1
 
 exit 0