On Fri, Dec 15, 2023 at 02:54:19PM +0100, Otto Moerbeek wrote:
> On Sun, Dec 10, 2023 at 09:57:08AM +0100, Otto Moerbeek wrote:
>
> > On Fri, Dec 01, 2023 at 09:18:32PM +0000, guilherme.janc...@yandex.com
> > wrote:
> >
> > > >Synopsis: Repeated NTP peers in OpenNTPD
> > > >Category: user
> > > >Environment:
> > > System : OpenBSD 7.4
> > > Details : OpenBSD 7.4 (GENERIC.MP) #0: Sun Oct 22 12:13:42 MDT 2023
> > >
> > > r...@syspatch-74-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > >
> > > Architecture: OpenBSD.amd64
> > > Machine : amd64
> > > >Description:
> > > If the same address/domain is specified multiple times in
> > > OpenNTPD's configuration file, or if multiple domains resolve
> > > to the same IP address, OpenNTPD will treat the same IP address
> > > as if it was multiple peers.
> > > >How-To-Repeat:
> > > This can be tested by appending `server 127.0.0.1` multiple
> > > times to the configuration file.
> > >
> > > Alternatively, assuming a default OpenNTPD configuration file
> > > from OpenBSD 7.4, the following entries can be added to
> > > /etc/hosts:
> > > 127.0.0.1 time.cloudflare.com
> > > 127.0.0.1 pool.ntp.org
> > >
> > > I noticed this bug using the default 7.4 configuration file. It
> > > can happen because time.cloudflare.com is part of pool.ntp.org:
> > > https://www.ntppool.org/scores/162.159.200.1
> > > https://www.ntppool.org/scores/162.159.200.123
> > > >Fix:
> > > Removing the `server time.cloudflare.com` line from the
> > > configuration file is a simple fix the user can make, but
> > > OpenNTPD should check if an IP address it tries to add to the
> > > list of peers is already a peer, and ignore it if so. If a
> > > server is added with the `server` (not `servers`) keyword in the
> > > configuration file, OpenNTPD should try the next IP the domain
> > > resolves to if applicable.
> > >
> >
> > Thanks for the report, I'll take a look.
> >
> > -Otto
> >
>
> Due to verious reasons this is all a bit complicated, I did not find a
> nice solution yet. Some patience required.
>
> -Otto
>
Found some more time. Try this. The approach is: we assume the
addresses of a pool (from the "servers" keyword) vary over time. So if
one of the pool addresses is in the address list of a peer ("server")
we skip it ad try to re-resolve the pool, lookin for a new address, as
we already did earlier when a pool member doe snot respond.
I decided to not implement "the move to abother address of the list"
in the "server" case. The address logic is already complex enough.
-Otto
Index: client.c
===================================================================
RCS file: /home/cvs/src/usr.sbin/ntpd/client.c,v
diff -u -p -r1.117 client.c
--- client.c 24 Mar 2022 07:37:19 -0000 1.117
+++ client.c 17 Dec 2023 09:13:16 -0000
@@ -108,6 +108,7 @@ client_nextaddr(struct ntp_peer *p)
return (-1);
if (p->addr_head.a == NULL) {
+ log_debug("kicking query for %s", p->addr_head.name);
priv_dns(IMSG_HOST_DNS, p->addr_head.name, p->id);
p->state = STATE_DNS_INPROGRESS;
return (-1);
@@ -145,7 +146,15 @@ client_query(struct ntp_peer *p)
}
if (p->state < STATE_DNS_DONE || p->addr == NULL)
- return (-1);
+ return (0);
+
+ /* if we're a pool member and a peer has taken our address, signal */
+ if (p->addr_head.pool != 0 && addr_already_used(&p->addr->ss)) {
+ log_debug("pool addr %s used by peer, will reresolve pool",
+ log_sockaddr((struct sockaddr *)&p->addr->ss));
+ p->senderrors++;
+ return (0);
+ }
if (p->query.fd == -1) {
struct sockaddr *sa = (struct sockaddr *)&p->addr->ss;
Index: ntp.c
===================================================================
RCS file: /home/cvs/src/usr.sbin/ntpd/ntp.c,v
diff -u -p -r1.171 ntp.c
--- ntp.c 6 Dec 2023 15:51:53 -0000 1.171
+++ ntp.c 17 Dec 2023 09:13:16 -0000
@@ -54,8 +54,6 @@ int ntp_dispatch_imsg(void);
int ntp_dispatch_imsg_dns(void);
void peer_add(struct ntp_peer *);
void peer_remove(struct ntp_peer *);
-int inpool(struct sockaddr_storage *,
- struct sockaddr_storage[MAX_SERVERS_DNS], size_t);
void
ntp_sighdlr(int sig)
@@ -524,23 +522,52 @@ ntp_dispatch_imsg(void)
return (0);
}
-int
+static int
+addr_equal(struct sockaddr_storage *a, struct sockaddr_storage *b)
+{
+ if (a->ss_family != b->ss_family)
+ return 0;
+ if (a->ss_family == AF_INET) {
+ if (((struct sockaddr_in *)a)->sin_addr.s_addr ==
+ ((struct sockaddr_in *)b)->sin_addr.s_addr)
+ return 1;
+ } else if (memcmp(&((struct sockaddr_in6 *)a)->sin6_addr,
+ &((struct sockaddr_in6 *)b)->sin6_addr,
+ sizeof(struct sockaddr_in6)) == 0) {
+ return 1;
+ }
+ return 0;
+}
+
+static int
inpool(struct sockaddr_storage *a,
struct sockaddr_storage old[MAX_SERVERS_DNS], size_t n)
{
size_t i;
for (i = 0; i < n; i++) {
- if (a->ss_family != old[i].ss_family)
+ if (addr_equal(a, &old[i]))
+ return 1;
+ }
+ return 0;
+}
+
+/* check to see af an address is already listed by a "server" peer
+ in that case, it does not get added to a pool */
+int
+addr_already_used(struct sockaddr_storage *a)
+{
+ struct ntp_peer *peer;
+ struct ntp_addr *addr;
+
+ TAILQ_FOREACH(peer, &conf->ntp_peers, entry) {
+ if (peer->addr_head.pool != 0)
continue;
- if (a->ss_family == AF_INET) {
- if (((struct sockaddr_in *)a)->sin_addr.s_addr ==
- ((struct sockaddr_in *)&old[i])->sin_addr.s_addr)
+ addr = peer->addr_head.a;
+ while (addr != NULL) {
+ if (addr_equal(a, &addr->ss))
return 1;
- } else if (memcmp(&((struct sockaddr_in6 *)a)->sin6_addr,
- &((struct sockaddr_in6 *)&old[i])->sin6_addr,
- sizeof(struct sockaddr_in6)) == 0) {
- return 1;
+ addr = addr->next;
}
}
return 0;
@@ -628,8 +655,11 @@ ntp_dispatch_imsg_dns(void)
free(h);
continue;
}
- if (inpool(&h->ss, existing,
- n)) {
+ if (addr_already_used(&h->ss) ||
+ inpool(&h->ss, existing, n)) {
+ log_debug("skipping address %s
for %s",
+ log_sockaddr((struct
sockaddr *)
+ &h->ss),
peer->addr_head.name);
free(h);
continue;
}
@@ -660,8 +690,11 @@ ntp_dispatch_imsg_dns(void)
}
if (dlen != 0)
fatalx("IMSG_HOST_DNS: dlen != 0");
- if (peer->addr_head.pool)
- peer_remove(peer);
+ if (peer->addr_head.pool) {
+ if (peercount > addrcount)
+ peer_remove(peer);
+ peer->state = STATE_NONE;
+ }
else
client_addr_init(peer);
break;
Index: ntpd.h
===================================================================
RCS file: /home/cvs/src/usr.sbin/ntpd/ntpd.h,v
diff -u -p -r1.152 ntpd.h
--- ntpd.h 27 Nov 2022 13:19:00 -0000 1.152
+++ ntpd.h 17 Dec 2023 09:13:16 -0000
@@ -371,6 +371,7 @@ int server_dispatch(int, struct ntpd_con
int client_peer_init(struct ntp_peer *);
int client_addr_init(struct ntp_peer *);
int client_nextaddr(struct ntp_peer *);
+int addr_already_used(struct sockaddr_storage *);
int client_query(struct ntp_peer *);
int client_dispatch(struct ntp_peer *, u_int8_t, u_int8_t);
void client_log_error(struct ntp_peer *, const char *, int);
