Subject: [Ntp] Warren Kumari's Discuss on draft-ietf-ntp-interleaved-modes-05: (with DISCUSS and COMMENT)

From: Warren Kumari via Datatracker <[email-masked]>
Date: Mon, 28 Jun 2021 08:21:15 -0700

Warren Kumari has entered the following ballot position for
draft-ietf-ntp-interleaved-modes-05: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-ntp-interleaved-modes/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I'm quite uncomfortable with the "we'll just change the meaning of some fields,
and implementations will be fine with this" tone. The document says: "An
explicit negotiation would require a new extension field, which would not work
well with implementations that do not respond to requests with unknown
extension fields.", and so this plays some tricks to encode information in
existing fields. This feels incredibly hacky, but I figured that it's probably
all fine if it has been shown to work well across a very very large set of
implementations without causing backward issues.

The Shepherd Writeup
(https://datatracker.ietf.org/doc/draft-ietf-ntp-interleaved-modes/shepherdwriteup/
) says: "There are multiple implementations of the document. They are discussed
in Section 5, entitled "Implementation Status".", so I felt hopeful that there
had been extensive testing, but I was not able to find a section entitled
"Implementation Status" in any version of draft-mlichvar-ntp-interleaved-modes
or draft-ietf-ntp-interleaved-modes.

What I *did* find was:
5.  Acknowledgements

   The interleaved modes described in this document are based on the
   implementation written by David Mills in the NTP project [1].  The
   specification of the broadcast mode is based purely on this
   implementation.  The specification of the symmetric mode has some
   modifications.  The client/server mode is specified as a new mode
   compatible with the symmetric mode, similarly to the basic symmetric
   and client/server modes.

So, from what I understand, the broadcast mode is based in the linked
implementation (a pointer to where in the codebase this is implemented would
have been much more helpful than just a link to http://www.ntp.org), but the
more complex, and (FWICT) likely to cause interoperability issues pars have 
"some modifications" and "a new mode compatible with the symmetric mode".

So, are there in fact "multiple implementations of the document."? Has there
been interoperability testing to confirm that there are no backwards
compatibility issues? Much of the document feels somewhat hand-wavy regarding
the compatibility with existing implementations, as well as in things like:
"The client SHOULD limit the number of requests in the interleaved mode between
server responses to prevent processing of very old timestamps in case a large
number of consecutive requests is lost." -- SHOULD limit to what number? 1? 3?
17?

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

When the document says things like:
"A peer A SHOULD send a peer B a packet in the interleaved mode only when the
following conditions are met:" I'm assuming that this means "only when the
**all** following conditions are met:"? If so, it needs to be explicit.

To be clear, when the document says:
"1.  The peer A has an active association with the peer B which was
       specified with the option enabling the interleaved mode, OR the
       peer A received at least one valid packet in the interleaved mode
       from the peer B."

If "peer A has an active association with the peer B which was **not**
specified with the option enabling the interleaved mode" and "peer A receive[s]
at least one valid packet in the interleaved mode from the peer B." should it
enable interleaved mode? How is an operator supposed to know if they should
enable this mode when talking to a peer?

As far as I can tell, this adds a significant amount of state keeping to server
implementations. The document does somewhat mention this ("A server which
supports the interleaved mode needs to save pairs of local receive and transmit
timestamps.  The server SHOULD discard old timestamps to limit the amount of
memory needed to support clients using the interleaved mode."), but it seems
like this is a fairly large change, and this bit of text doesn't really cover
the state requirements.

---

From: Miroslav Lichvar <[email-masked]>
Date: Tue, 29 Jun 2021 10:36:28 +0200

Thanks for the review.

On Mon, Jun 28, 2021 at 08:21:15AM -0700, Warren Kumari via Datatracker wrote:
> So, are there in fact "multiple implementations of the document."?

There are two widely-used implementations that support some of the
interleaved modes: ntp (from ntp.org) and chrony. There are few other
implementations I saw, but those were rather experimental. FWIW, a
minimalistic one is included as a proof-of-concept in the repository
of the draft [1].

The first ntp version marked as stable with this feature was 4.2.6p1
released in 2010. It implements the symmetric mode and broadcast mode.
The draft has some requirements that this implementation doesn't
follow yet. This causes problems when the polling intervals of the
peers don't match, even when it is interoperating with itself. Other
than that it interoperates with chrony, even as a client, although
that doesn't seem to be officially supported.

The first chrony version was 3.0 released in 2016. It added the
client/server mode and symmetric interleaved modes. The server support
is enabled by default.

> Has there
> compatibility issues?

Yes, it has, quite extensively.

There is a large number of public NTP servers that have the server
support enabled. I have also captured and analyzed terabytes of NTP
traffic on several public servers in different regions to confirm that
it doesn't break any existing implementations.

There was only one implementation that had an issue and that was a
fork of ntp that had partially removed support for the interleaved
mode. It basically had the xleave option permanently set, but it was
no longer able to process an interleaved response, which caused the
protocol to switch between the two modes. IIRC it still synchronized,
but used only every second or third response. Obviously, this wouldn't
happen if the implementation didn't already support the interleaved
mode at some point in its history and it could still happen even if
the protocol used a different mechanism to switch between the modes.

> Much of the document feels somewhat hand-wavy regarding

That is an implementation detail with no impact on interoperability.
For different implementations different values might work better
depending on how they control the clock and filter measurements etc,
so I'd rather not suggest any specific values.

> **all** following conditions are met:"? If so, it needs to be explicit.

Ok, I'll fix that.

> enable interleaved mode?

Yes, it should.

> How is an operator supposed to know if they should

They are expected to know whether the peer supports it. Or they can
try it and see if it works. Symmetric mode is typically used in local
networks.

> like this is a fairly large change, and this bit of text doesn't really cover

I think that is an implementation detail. The only requirement from
the protocol point of view is that server is able to keep at least one
pair of timestamps across requests. It can keep as many of them as it
likes and it can drop them at any time for any reason. A minimal
implementation would be a fixed-size hashtable, overwriting old
timestamps with new ones as they come. No additional state is
required.

[1] https://github.com/mlichvar/draft-ntp-interleaved-modes

-- 
Miroslav Lichvar