-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
NTS support for systemd-timesyncd #39010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
e276d71
to
0a23c66
Compare
|
||
int flags; | ||
if((flags = fcntl(sock, F_GETFL)) < 0 || fcntl(sock, F_SETFL, flags | O_NONBLOCK) < 0) { | ||
return close(sock), -2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ever use raw close() in our codebase. use safe_close().
and usually you don't want to use that either, use _cleanup_close_
.
finally, please just create the socket with O_NONBLOCK right-away, u.e. use SOCK_NONBLOCK.
continue; | ||
} | ||
|
||
int sock = socket(cur->ai_family, type, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all our fds must have O_CLOEXEC set, except if it's clear they should not use it. hence please use SOCK_CLOEXEC
} | ||
|
||
int status = 1; | ||
setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, &status, sizeof(status)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please debug log about unexpected failures at least. also, use setsockopt_bool()
hints.ai_socktype = type; | ||
|
||
struct addrinfo *info; | ||
if (getaddrinfo(host, NULL, &hints, &info) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please never use a blocking call like this in an event-loop based serice such as timesyncd. use sd_resolve_xyz
assert_return(nts_handshake, -ENOMEM); | ||
|
||
while ((r = NTS_TLS_handshake(nts_handshake)) > 0 && tls_patience_msec-- > 0) | ||
usleep_safe(USEC_PER_MSEC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this?
no blocking calls like this. this is event loop driven...
i did some initial review now. please have a closer look at the coding style docs. |
Thanks for the quick initial review! Note that the
They're LGPL2.1+ licensed (since they're specifically targetted for timesyncd). |
…till trigger the compiler in case of misuse
For my own reference, I'm tracking more general issues that still need attention on the forked repo: https://github.com/pendulum-project/nts-timesyncd/issues |
option('openssl', type : 'feature', deprecated : { 'true' : 'enabled', 'false' : 'disabled' }, | ||
description : 'openssl support') | ||
option('cryptolib', type : 'combo', choices : ['auto', 'openssl', 'gcrypt'], deprecated : true) | ||
option('timesync-nts', type : 'combo', choices : ['auto', 'enabled', 'disabled'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be of type "feature" rather than a manual combo
|
||
if conf.get('ENABLE_TIMESYNC_NTS') == 1 | ||
|
||
executables += [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation and whitespace above
org.freedesktop.timesync1.Manager.SystemNTPServers | ||
org.freedesktop.timesync1.Manager.RuntimeNTPServers | ||
org.freedesktop.timesync1.Manager.FallbackNTPServers | ||
org.freedesktop.timesync1.Manager.NTSKeyExchangeServers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't, add to the version history instead
<filename>systemd-timesyncd.service</filename>.</para> | ||
|
||
<para>The <filename>systemd-timesyncd.service</filename> service supports the Network Time Security (NTS) | ||
mechanism for NTP. These allow for authentication of the time source and protect against spoofing attacks.</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the plural here refer to?
mechanism for NTP. These allow for authentication of the time source and protect against spoofing attacks.</para> | |
mechanism for NTP. This allows for authentication of the time source and protect against spoofing attacks.</para> |
<command>systemd-timesyncd</command> will contact all configured servers in turn, until one responds. | ||
When the empty string is assigned, the list of NTS servers is reset, and | ||
all prior assignments will have no effect. This setting defaults to an empty list. | ||
If NTS severs have been configured, any other configured NTP servers (including per-interface NTP servers acquired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If NTS severs have been configured, any other configured NTP servers (including per-interface NTP servers acquired | |
If NTS servers have been configured, any other configured NTP servers (including per-interface NTP servers acquired |
This is intended to eventually close #9481
Some comments:
To keep the modification well-separated, the additions to timesyncd are split up two clearly distinguishable parts:
timesyncd
sources.Currently, to select an "NTS server" an
NTS=
line needs to be added to the configuration; because the intent is "secure time", enabling it will ignore all other NTP servers (except for theFallbackNTP
). Maybe in the future (if NTS gains traction) it might be wise to configure this differently (e.g.RequireSureTime=yes
or something).The function
manager_nts_obtain_agreement
currently does busy-waiting (which has the advantage of making it perhaps a bit easier to follow the logic). This still should be integrated in the async event loop! (hence the "draft" status of this PR).The current meson build system modifications take some pains to make the selection of TLS and crypto backends flexible. I've learned from @poettering that the intent is to consolidate on OpenSSL; so that part can be simplified. Do note that the minimum required OpenSSL version is v3.5 due to a bug in OpenSSL (and a workaround for that bug not being universally compatible with existing NTS servers). In any case, this PR still allows selecting
gnutls
for TLS andnettle
andgcrypt
as alternative crypto backends. Not included in this PR (but available if desired) is a crypto backend that relies on libaes_siv, which would allow using older OpenSSL versions.I'm not 100% sure I didn't miss some config file for integrating the tests and fuzzing code; if there's anything missing, a PR on our forked repo is appreciated!
To get in the weeds on NTS protocol details:
AES-GCM-SIV
crypto (because it is most space-efficient). This will only work with up-to-date Chrony implementations (4.6.1 or later, for annoying reasons). In my practical tests I haven't found any NTS servers running an older Chrony version, and I don't think it's very useful to make this fresh NTS implementation "bug-compatible" with older Chrony versions. By the time NTS lands intimesyncd
I would hope that this issue is long forgotten. But if this is seen as problematic, it's very easy to make the defaultAES-SIV-CMAC
.