Re: [Buildroot] [PATCH 1/1] package/ntpsec: new package

[Peter Seiderer <ps.report@gmx.net>]

Hello Guillaume,

thanks of interest, testing and improvement of the ntpsec patch, some comments below...

On Sun, 12 Dec 2021 21:07:41 +0100, guillaume.bressaix@gmail.com wrote:

> From: Peter Seiderer <ps.report@gmx.net>
> 
> - set 'CC=gcc' to avoid cross-compile failure (see [1]):
> 
>   /bin/sh: line 1: .../build/ntpsec-1_2_0/build/host/ntpd/keyword-gen: cannot execute binary file: Exec format error
> 
>   Waf: Leaving directory `.../build/ntpsec-1_2_0/build/host'
>   Build failed
>    -> task in 'ntp_keyword.h' failed with exit status 126 (run with -v to display more information)  
> 
> - set '-std=gnu99"' to avoid compile failure with old compilers
> 
> - explicit set PYTHON_CONFIG
> 
> - add patch 001-ntptime-fix-jfmt5-ofmt5-jfmt6-ofmt6-related-compile-.patch to
>   fix ntptime jfmt5/ofmt5 jfmt6/ofmt6 related compile failure
> 
> - add SYSV init file (S49ntp)
> 
> - add example ntpd.conf (with legacy option enabled and provide skeleton
>   for NTS configuration)
> 
> - add config option for NTS support
> 
> - depend on python3 (omit python2 to reduce test effort)
> 
> - add ntp user/group and run ntpd as restricted user
> 
> - add libcap dependency (compile time optional but needed for droproot
>   support)
> 
> - submit latest ntpsec version 1.2.1

Interesting, not listed as release, but tagged and downloadable...

> 
> - lib ntpc import in python is fixed by specifying the --libdir flag.
>   -> removed the symlink trick  

Fine..., will test it...

> 
> - add --refclock=all flags to configure, see notes down below
> 
> - add leap second management options & ntpviz
> 
> - improved Config.in:
>   libbsd is required

Are you sure? With the original patch ntpsec builds fine without libbsd available...

>   openssl is only needed when NTS encryption is enabled

A build without NTS and without openssl gives:

	Checking for OpenSSL/libcrypto (via pkg-config)                 : not found 
	Checking for OpenSSL's crypto library                           : not found 
	The configuration failed

>   (depend on python3 only to simply things)
>   improved classic mode help description
>   improved early-drop-root feature description
> 
> - early droproot should be an option: adapt libcap accordingly

This will break (the hard coded) '-u npt:npt' option from package/ntpsec/S49ntp,
any reason to avoid the security feature?

> 
> - corrected CC=gcc to CC=$(HOSTCC) in ntpsec.mk

Good point...

> 
> - provide service script for systemd infra along sysv infra

Better done as an extra patch (easier to review)....

> 
> - I don't think we need the patch if we restrict to !BR2_TOOLCHAIN_UCLIBC
>   IMO it's better to keep the patch and allow all toolchains.
>   I usually have glibc, but I just ran a sanity check on my zedboard with uclibc,
>   it passed.

Not important for the commit log...

> 
> - used on zynq_zed_defconfig and beaglebone_defconfig
>   daemon automatically started
>   ntpq works fine
> 
> [1] https://gitlab.com/NTPsec/ntpsec/-/issues/694
> 
> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
> 
> ---
> notes on refclocks: https://docs.ntpsec.org/latest/refclock.html
>   "For security reasons, we will no longer support any refclock
>   that requires a closed-source driver to run", see webpage.
> 
>   --refclock=all is hardcoded at the moment
> 
>   One must compile ntpsec with the 'refclock' option
>   if they want to drive or interact with hardware.
> 
>   In any case, refclocks are not critical for both buildtime & runtime:
> 
>      [+] ./configure is smart enough to disable a refclock
>      if requirements are not met. In the submitted context,
>      this happens for refclock=gpsd without BR2_PACKAGE_GPSD
>      selected by user
> 
>      [+] some refclocks naturally require a specific hw support
>      with related kernel driver.
>      This is not buildtime critical because build does not care
>      about hw support.
>      This is not runtime critical either because any missing
>      hw support or unfeasible hardware access ends up as a logged
>      error message. It is up to the user to correct it in the
>      submitted context: example: 'nmea/gps' receivers without kernel support
>      or hardware not plugged in.

Would prefer one option per refclock to reduce dependencies...

> 
> ntpd / ntpsec should be mutualy exclusive if we harcode S49ntp as the service script
> 
> ---
>  DEVELOPERS                                         |  1 +
>  package/Config.in                                  |  1 +
>  ...-jfmt5-ofmt5-jfmt6-ofmt6-related-compile-.patch | 61 ++++++++++++++++
>  package/ntpsec/Config.in                           | 68 +++++++++++++++++
>  package/ntpsec/S49ntp                              | 58 +++++++++++++++
>  package/ntpsec/ntpd.etc.conf                       | 33 +++++++++
>  package/ntpsec/ntpd.service                        | 15 ++++
>  package/ntpsec/ntpsec.hash                         |  4 +
>  package/ntpsec/ntpsec.mk                           | 85 ++++++++++++++++++++++
>  9 files changed, 326 insertions(+)
>  create mode 100644 package/ntpsec/0001-ntptime-fix-jfmt5-ofmt5-jfmt6-ofmt6-related-compile-.patch
>  create mode 100644 package/ntpsec/Config.in
>  create mode 100644 package/ntpsec/S49ntp
>  create mode 100644 package/ntpsec/ntpd.etc.conf
>  create mode 100644 package/ntpsec/ntpd.service
>  create mode 100644 package/ntpsec/ntpsec.hash
>  create mode 100644 package/ntpsec/ntpsec.mk
> 
> diff --git a/DEVELOPERS b/DEVELOPERS
> index 3023526..32b5e87 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -2196,6 +2196,7 @@ F:	package/iwd/
>  F:	package/libevdev/
>  F:	package/libuev/
>  F:	package/log4cplus/
> +F:	package/ntpsec/
>  F:	package/postgresql/
>  F:	package/python-colorzero/
>  F:	package/python-flask-wtf/
> diff --git a/package/Config.in b/package/Config.in
> index 5720830..544a0fd 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -2271,6 +2271,7 @@ endif
>  	source "package/nmap/Config.in"
>  	source "package/noip/Config.in"
>  	source "package/ntp/Config.in"
> +	source "package/ntpsec/Config.in"
>  	source "package/nuttcp/Config.in"
>  	source "package/odhcp6c/Config.in"
>  	source "package/odhcploc/Config.in"
> diff --git a/package/ntpsec/0001-ntptime-fix-jfmt5-ofmt5-jfmt6-ofmt6-related-compile-.patch b/package/ntpsec/0001-ntptime-fix-jfmt5-ofmt5-jfmt6-ofmt6-related-compile-.patch
> new file mode 100644
> index 0000000..c2838fe
> --- /dev/null
> +++ b/package/ntpsec/0001-ntptime-fix-jfmt5-ofmt5-jfmt6-ofmt6-related-compile-.patch
> @@ -0,0 +1,61 @@
> +From 4015a1183d2f79dad6dd675ca5e0d329825f3fa3 Mon Sep 17 00:00:00 2001
> +From: Peter Seiderer <ps.report@gmx.net>
> +Date: Mon, 4 Oct 2021 22:25:58 +0200
> +Subject: [PATCH] ntptime: fix jfmt5/ofmt5 jfmt6/ofmt6 related compile failure
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +Use same define guard for definiton as for usage ('HAVE_STRUCT_NTPTIMEVAL_TAI'
> +instead of 'NTP_API && NTP_API > 3').
> +
> +Fixes:
> +
> +  ../../ntptime/ntptime.c: In function ‘main’:
> +  ../../ntptime/ntptime.c:349:17: error: ‘jfmt5’ undeclared (first use in this function); did you mean ‘jfmt6’?
> +    349 |   printf(json ? jfmt5 : ofmt5, (long)ntv.tai);
> +        |                 ^~~~~
> +        |                 jfmt6
> +  ../../ntptime/ntptime.c:349:17: note: each undeclared identifier is reported only once for each function it appears in
> +  ../../ntptime/ntptime.c:349:25: error: ‘ofmt5’ undeclared (first use in this function); did you mean ‘ofmt6’?
> +    349 |   printf(json ? jfmt5 : ofmt5, (long)ntv.tai);
> +        |                         ^~~~~
> +        |                         ofmt6
> +  ../../ntptime/ntptime.c:321:15: warning: unused variable ‘jfmt6’ [-Wunused-variable]
> +    321 |   const char *jfmt6 = "";
> +        |               ^~~~~
> +  ../../ntptime/ntptime.c:311:15: warning: unused variable ‘ofmt6’ [-Wunused-variable]
> +    311 |   const char *ofmt6 = "\n";
> +        |               ^~~~~
> +
> +[Upstream: https://gitlab.com/NTPsec/ntpsec/-/merge_requests/1245]
> +Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> +---
> + ntptime/ntptime.c | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/ntptime/ntptime.c b/ntptime/ntptime.c
> +index ff861cb..5d58593 100644
> +--- a/ntptime/ntptime.c
> ++++ b/ntptime/ntptime.c
> +@@ -305,7 +305,7 @@ main(
> + 		const char *ofmt2 = "  time %s, (.%0*d),\n";
> + 		const char *ofmt3 = "  maximum error %lu us, estimated error %lu us";
> + 		const char *ofmt4 = "  ntptime=%x.%x unixtime=%x.%0*d %s";
> +-#if defined NTP_API && NTP_API > 3
> ++#if defined(HAVE_STRUCT_NTPTIMEVAL_TAI)
> + 		const char *ofmt5 = ", TAI offset %ld\n";
> + #else
> + 		const char *ofmt6 = "\n";
> +@@ -315,7 +315,7 @@ main(
> + 		const char *jfmt2 = "\"time\":\"%s\",\"fractional-time\":\".%0*d\",";
> + 		const char *jfmt3 = "\"maximum-error\":%lu,\"estimated-error\":%lu,";
> + 		const char *jfmt4 = "\"raw-ntp-time\":\"%x.%x\",\"raw-unix-time\":\"%x.%0*d %s\",";
> +-#if defined NTP_API && NTP_API > 3
> ++#if defined(HAVE_STRUCT_NTPTIMEVAL_TAI)
> + 		const char *jfmt5 = "\"TAI-offset\":%d,";
> + #else
> + 		const char *jfmt6 = "";
> +-- 
> +2.33.0
> +
> diff --git a/package/ntpsec/Config.in b/package/ntpsec/Config.in
> new file mode 100644
> index 0000000..9044aa4
> --- /dev/null
> +++ b/package/ntpsec/Config.in
> @@ -0,0 +1,68 @@
> +comment "ntpsec needs a toolchain w/ wchar, thread, dynamic library"
> +	depends on BR2_STATIC_LIBS
> +	depends on !BR2_USE_WCHAR
> +	depends on !BR2_TOOLCHAIN_HAS_THREADS # libbsd
> +
> +comment "ntpsec needs libbsd"
> +	depends on !BR2_PACKAGE_LIBBSD_ARCH_SUPPORTS
> +	depends on !BR2_STATIC_LIBS # libbsd
> +
> +comment "ntpsec needs python3"
> +	depends on !BR2_PACKAGE_PYTHON3
> +
> +config BR2_PACKAGE_NTPSEC
> +	bool "ntpsec"
> +	depends on !BR2_STATIC_LIBS # libbsd
> +	depends on BR2_PACKAGE_LIBBSD_ARCH_SUPPORTS
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # libbsd
> +	depends on BR2_PACKAGE_PYTHON3
> +	select BR2_PACKAGE_LIBCAP
> +	select BR2_PACKAGE_LIBBSD
> +	select BR2_PACKAGE_PPS_TOOLS # refclock(pps)
> +	help
> +	  NTPsec is a secure, hardened, and improved
> +	  implementation of Network Time Protocol derived
> +	  from NTP Classic, Dave Mills’s original.
> +
> +	  Provides things like ntpd, ntpdate, ntpq, etc...
> +
> +	  https://www.ntpsec.org/
> +
> +if BR2_PACKAGE_NTPSEC
> +
> +config BR2_PACKAGE_NTPSEC_CLASSIC_MODE
> +	bool "ntpsec-classic"
> +	help
> +	  Enable strict configuration and log-format compatibility
> +	  with NTP Classic.
> +	  This option is not recommended as it makes the module
> +	  less efficient.
> +
> +config BR2_PACKAGE_NTPSEC_NTS
> +	bool "ntpsec-nts"
> +	select BR2_PACKAGE_OPENSSL
> +	help
> +	  Enable Network Time Security (NTS) support.
> +
> +comment "ntpsec-ntploggpsd needs gpsd"
> +	depends on !BR2_PACKAGE_GPSD
> +
> +config BR2_PACKAGE_NTPSEC_LEAP_SMEAR
> +	bool "ntpsec-leap-smear"
> +	help
> +	  Activates leap second smearing,
> +	  https://docs.ntpsec.org/latest/leapsmear.html
> +
> +config BR2_PACKAGE_NTPSEC_LEAP_TESTING
> +	bool "ntpsec-leap-testing"
> +	help
> +	  Enables leap seconds on other than 1st day of month
> +
> +config BR2_PACKAGE_NTPSEC_EARLY_DROPROOT
> +	bool "ntpsec-early-droproot"
> +	help
> +	  Drops root privileges as early as possible.
> +	  This requires the refclock devices to be owned
> +	  by owner/group running 'ntpd'
> +
> +endif
> diff --git a/package/ntpsec/S49ntp b/package/ntpsec/S49ntp
> new file mode 100644
> index 0000000..f3db514
> --- /dev/null
> +++ b/package/ntpsec/S49ntp
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +#
> +# Starts Network Time Protocol daemon
> +#
> +
> +DAEMON="ntpd"
> +PIDFILE="/var/run/$DAEMON.pid"
> +
> +NTPD_ARGS="-g -u ntp:ntp -s /var/run/ntp"
> +
> +# shellcheck source=/dev/null
> +[ -r "/etc/default/$DAEMON" ] && . "/etc/default/$DAEMON"
> +
> +mkdir -p /var/run/ntp && chown ntp:ntp /var/run/ntp
> +
> +start() {
> +	printf 'Starting %s: ' "$DAEMON"
> +	# shellcheck disable=SC2086 # we need the word splitting
> +	start-stop-daemon -S -q -p "$PIDFILE" -x "/usr/sbin/$DAEMON" \
> +		-- $NTPD_ARGS -p "$PIDFILE"
> +	status=$?
> +	if [ "$status" -eq 0 ]; then
> +		echo "OK"
> +	else
> +		echo "FAIL"
> +	fi
> +	return "$status"
> +}
> +
> +stop() {
> +	printf 'Stopping %s: ' "$DAEMON"
> +	start-stop-daemon -K -q -p "$PIDFILE"
> +	status=$?
> +	if [ "$status" -eq 0 ]; then
> +		rm -f "$PIDFILE"
> +		echo "OK"
> +	else
> +		echo "FAIL"
> +	fi
> +	return "$status"
> +}
> +
> +restart() {
> +	stop
> +	sleep 1
> +	start
> +}
> +
> +case "$1" in
> +	start|stop|restart)
> +		"$1";;
> +	reload)
> +		# Restart, since there is no true "reload" feature.
> +		restart;;
> +	*)
> +		echo "Usage: $0 {start|stop|restart|reload}"
> +		exit 1
> +esac
> diff --git a/package/ntpsec/ntpd.etc.conf b/package/ntpsec/ntpd.etc.conf
> new file mode 100644
> index 0000000..e0f45c1
> --- /dev/null
> +++ b/package/ntpsec/ntpd.etc.conf
> @@ -0,0 +1,33 @@
> +#
> +# legacy NTP configuration
> +#
> +pool 0.pool.ntp.org iburst
> +pool 1.pool.ntp.org iburst
> +pool 2.pool.ntp.org iburst
> +pool 3.pool.ntp.org iburst
> +
> +#
> +# NTS configuration
> +#
> +# Notes:
> +#  - uncomment the following lines to enable NTS support (but
> +#    make sure the initial clock is up-to-date (otherwise the
> +#    NTS certificate validation will fail with 'NTSc: certificate invalid:
> +#    9=>certificate is not yet valid' as on boards without RTC support)
> +#    and/or keep at least one line from the legacy NTP lines
> +#  - enable BR2_PACKAGE_CA_CERTIFICATES to gain access to the certificate
> +#    files
> +#
> +# server time.cloudflare.com nts  # Global, anycast
> +# server nts.ntp.se:4443 nts      # Sweden
> +# server ntpmon.dcs1.biz nts      # Singapore
> +# server ntp1.glypnod.com nts     # San Francisco
> +# server ntp2.glypnod.com nts     # London
> +#
> +# ca /usr/share/ca-certificates/mozilla
> +
> +# Allow only time queries, at a limited rate, sending KoD when in excess.
> +# Allow all local queries (IPv4, IPv6)
> +restrict default nomodify nopeer noquery limited kod
> +restrict 127.0.0.1
> +restrict [::1]
> diff --git a/package/ntpsec/ntpd.service b/package/ntpsec/ntpd.service
> new file mode 100644
> index 0000000..b7db4a2
> --- /dev/null
> +++ b/package/ntpsec/ntpd.service
> @@ -0,0 +1,15 @@
> +[Unit]
> +Description=Network Time Service
> +After=network.target
> +
> +[Service]
> +Type=forking
> +PIDFile=/run/ntpd.pid
> +# Turn off DNSSEC validation for hostname look-ups, since those need the
> +# correct time to work, but we likely won't acquire that without NTP. Let's
> +# break this chicken-and-egg cycle here.
> +Environment=SYSTEMD_NSS_RESOLVE_VALIDATE=0
> +ExecStart=/usr/sbin/ntpd @NTPD_EXTRA_ARGS@ -g -p /run/ntpd.pid

The @NTPD_EXTRA_ARGS@ handling needs the sed command instead of the
simple install one (see package/ntp/ntp.mk)...

> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/package/ntpsec/ntpsec.hash b/package/ntpsec/ntpsec.hash
> new file mode 100644
> index 0000000..49dc4e4
> --- /dev/null
> +++ b/package/ntpsec/ntpsec.hash
> @@ -0,0 +1,4 @@
> +# Locally calculated
> +sha256  71c9f4bde6953bbc048bbaf278da81c451a56cc08d6772542b4ad37c67d72e89  ntpsec-NTPsec_1_2_1.tar.bz2
> +sha256  b4db4de3317c3b0554ed91eb692968800bdfd6ad2c16ffbeee8ce4895ed91da4  LICENSE.adoc
> +sha256  d3b21470adadd9abd9c6d675378f8c371ac5a4ea6dbec91859e02fadca3c0856  docs/copyright.adoc
> diff --git a/package/ntpsec/ntpsec.mk b/package/ntpsec/ntpsec.mk
> new file mode 100644
> index 0000000..55b4bb0
> --- /dev/null
> +++ b/package/ntpsec/ntpsec.mk
> @@ -0,0 +1,85 @@
> +################################################################################
> +#
> +# ntpsec
> +#
> +################################################################################
> +
> +NTPSEC_VERSION_MAJOR = 1
> +NTPSEC_VERSION_MINOR = 2
> +NTPSEC_VERSION_POINT = 1
> +NTPSEC_VERSION = $(NTPSEC_VERSION_MAJOR)_$(NTPSEC_VERSION_MINOR)_$(NTPSEC_VERSION_POINT)
> +NTPSEC_SOURCE = ntpsec-NTPsec_$(NTPSEC_VERSION).tar.bz2
> +NTPSEC_SITE = https://gitlab.com/NTPsec/ntpsec/-/archive/NTPsec_$(NTPSEC_VERSION)
> +NTPSEC_LICENSE = BSD-2-Clause NTP BSD-3-Clause MIT
> +NTPSEC_LICENSE_FILES = LICENSE.adoc docs/copyright.adoc
> +
> +NTPSEC_CPE_ID_VENDOR = ntpsec
> +NTPSEC_CPE_ID_VERSION = $(NTPSEC_VERSION_MAJOR).$(NTPSEC_VERSION_MINOR)
> +NTPSEC_CPE_ID_UPDATE = $(NTPSEC_VERSION_POINT)
> +
> +NTPSEC_DEPENDENCIES = \
> +	$(if $(BR2_PACKAGE_PYTHON),python,python3) \
> +	libbsd \
> +	pps-tools
> +
> +NTPSEC_PYVER = $(if $(BR2_PACKAGE_PYTHON),python$(PYTHON_VERSION_MAJOR),python$(PYTHON3_VERSION_MAJOR))
> +
> +NTPSEC_CONF_OPTS = \
> +	CC=$(HOSTCC) \
> +	PYTHON_CONFIG="$(STAGING_DIR)/usr/bin/$(if $(BR2_PACKAGE_PYTHON),python,python3)-config" \
> +	--cross-compiler="$(TARGET_CC)" \
> +	--cross-cflags="$(TARGET_CFLAGS) -std=gnu99" \
> +	--cross-ldflags="$(TARGET_LDFLAGS)" \
> +	--notests \
> +	--disable-mdns-registration \
> +	--enable-pylib=ffi \
> +	--nopyc \
> +	--nopyo \
> +	--nopycache \
> +	--disable-doc \
> +	--disable-manpage \
> +	--refclock=all \
> +	--libdir=/usr/lib/$(NTPSEC_PYVER)/site-packages/ntp
> +
> +ifeq ($(BR2_PACKAGE_NTPSEC_CLASSIC_MODE),y)
> +NTPSEC_CONF_OPTS += --enable-classic-mode
> +endif
> +
> +ifeq ($(BR2_PACKAGE_NTPSEC_NTS),y)
> +NTPSEC_DEPENDENCIES += openssl
> +else
> +NTPSEC_CONF_OPTS += --disable-nts
> +endif
> +
> +ifeq ($(BR2_PACKAGE_NTPSEC_EARLY_DROPROOT),y)
> +NTPSEC_DEPENDENCIES += libcap
> +NTPSEC_CONF_OPTS += --enable-early-droproot
> +endif
> +
> +ifeq ($(BR2_PACKAGE_NTPSEC_LEAP_SMEAR),y)
> +NTPSEC_CONF_OPTS += --enable-leap-smear
> +endif
> +
> +ifeq ($(BR2_PACKAGE_NTPSEC_LEAP_TESTING),y)
> +NTPSEC_CONF_OPTS += --enable-leap-testing
> +endif
> +
> +define NTPSEC_INSTALL_NTPSEC_CONF
> +	$(INSTALL) -m 644 package/ntpsec/ntpd.etc.conf $(TARGET_DIR)/etc/ntp.conf
> +endef
> +NTPSEC_POST_INSTALL_TARGET_HOOKS += NTPSEC_INSTALL_NTPSEC_CONF
> +
> +define NTPSEC_INSTALL_INIT_SYSV
> +	$(INSTALL) -D -m 755 $(NTPSEC_PKGDIR)/S49ntp $(TARGET_DIR)/etc/init.d/S49ntp
> +endef
> +
> +define NTPSEC_INSTALL_INIT_SYSTEMD
> +	$(INSTALL) -D -m 644 $(NTPSEC_PKGDIR)/ntpd.service \
> +		$(TARGET_DIR)/usr/lib/systemd/system/ntpd.service
> +endef
> +
> +define NTPSEC_USERS
> +	ntp -1 ntp -1 * - - - ntpd user
> +endef
> +
> +$(eval $(waf-package))

Will prepare an update of my original patch with the version update and some of
your findings..., feel free to provide your improvements as add-on patches ;-)

Regards,
Peter

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot