Skip to content

Commit

Permalink
ntp: check for reasonable time and stratum < 10 (#4119)
Browse files Browse the repository at this point in the history
Context from
#4087 (comment):

> What we think happened here is the boundary NTP zones considered
themselves authoritative without having upstream connectivity, and
responded to other NTP zones with a 1980-era timestamp. The telling
factor here is the stratum 11 lines in tracking.log, which imply the
boundary NTP service was serving stratum 10.
>
> We could most easily work around this by ensuring we don't consider
the host in sync until the stratum is < 10. ... We could also set up an
epoch before which time cannot be to consider ourselves in sync, which
might be a good additional defense?

This adds these checks. The epoch I've chosen for no particular reason
is UNIX time 1234567890 (Fri Feb 13 23:31:30 2009 UTC); there are plenty
of times after this that would not make sense but it at least ensures it
is not December 1986.
  • Loading branch information
iliana authored Sep 21, 2023
1 parent 373ddbd commit 9a222ab
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 2 deletions.
13 changes: 13 additions & 0 deletions openapi/sled-agent.json
Original file line number Diff line number Diff line change
Expand Up @@ -2755,6 +2755,17 @@
"format": "uint32",
"minimum": 0
},
"ref_time": {
"description": "The NTP reference time (i.e. what chrony thinks the current time is, not necessarily the current system time).",
"type": "number",
"format": "double"
},
"stratum": {
"description": "The NTP stratum (our upstream's stratum plus one).",
"type": "integer",
"format": "uint8",
"minimum": 0
},
"sync": {
"description": "The synchronization state of the sled, true when the system clock and the NTP clock are in sync (to within a small window).",
"type": "boolean"
Expand All @@ -2764,6 +2775,8 @@
"correction",
"ip_addr",
"ref_id",
"ref_time",
"stratum",
"sync"
]
},
Expand Down
5 changes: 5 additions & 0 deletions sled-agent/src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,11 @@ pub struct TimeSync {
pub ref_id: u32,
/// The NTP reference IP address.
pub ip_addr: IpAddr,
/// The NTP stratum (our upstream's stratum plus one).
pub stratum: u8,
/// The NTP reference time (i.e. what chrony thinks the current time is, not
/// necessarily the current system time).
pub ref_time: f64,
// This could be f32, but there is a problem with progenitor/typify
// where, although the f32 correctly becomes "float" (and not "double") in
// the API spec, that "float" gets converted back to f64 when generating
Expand Down
2 changes: 2 additions & 0 deletions sled-agent/src/rack_setup/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,8 @@ impl ServiceInner {
sync: ts.sync,
ref_id: ts.ref_id,
ip_addr: ts.ip_addr,
stratum: ts.stratum,
ref_time: ts.ref_time,
correction: ts.correction,
})
}
Expand Down
20 changes: 18 additions & 2 deletions sled-agent/src/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2373,6 +2373,8 @@ impl ServiceManager {
sync: true,
ref_id: 0,
ip_addr: IPV6_UNSPECIFIED,
stratum: 0,
ref_time: 0.0,
correction: 0.00,
});
};
Expand Down Expand Up @@ -2401,6 +2403,10 @@ impl ServiceManager {
.map_err(|_| Error::NtpZoneNotReady)?;
let ip_addr =
IpAddr::from_str(v[1]).unwrap_or(IPV6_UNSPECIFIED);
let stratum = u8::from_str(v[2])
.map_err(|_| Error::NtpZoneNotReady)?;
let ref_time = f64::from_str(v[3])
.map_err(|_| Error::NtpZoneNotReady)?;
let correction = f64::from_str(v[4])
.map_err(|_| Error::NtpZoneNotReady)?;

Expand All @@ -2410,13 +2416,23 @@ impl ServiceManager {
let peer_sync = !ip_addr.is_unspecified()
|| (ref_id != 0 && ref_id != 0x7f7f0101);

let sync = peer_sync && correction.abs() <= 0.05;
let sync = stratum < 10
&& ref_time > 1234567890.0
&& peer_sync
&& correction.abs() <= 0.05;

if sync {
self.boottime_rewrite(existing_zones.values());
}

Ok(TimeSync { sync, ref_id, ip_addr, correction })
Ok(TimeSync {
sync,
ref_id,
ip_addr,
stratum,
ref_time,
correction,
})
} else {
Err(Error::NtpZoneNotReady)
}
Expand Down

0 comments on commit 9a222ab

Please sign in to comment.