Re: [Ntp] comments on draft-ietf-ntp-using-nts-for-ntp-23

[Sandra Murphy <[e-mail]>]

This is a list of my detailed comments and the status in the draft
v26. 

A new comment:

The text in Section 6 reads:

  Servers should periodically (e.g., once daily) generate a new pair
  (I,K) and immediately switch to using these values for all newly-
  generated cookies.  Following each such key rotation, servers should
  securely erase any previously generated keys that should now be
  expired.  Servers should continue to accept any cookie generated
  using keys that they have not yet erased, even if those keys are no
  longer current.  Erasing old keys provides for forward secrecy,
  limiting the scope of what old information can be stolen if a master
  key is somehow compromised.

I do not understand the "forward secrecy" comment. How does erasure of
old keys limit the scope of “what old information can be stolen”
arising from a master key compromise.  How could a new master key
compromise allow theft of old information protected with old master keys?

Is it possible that you meant a host compromise?  It is easy to see how
erasure of old keys would prevent old information being exposed upon a
host compromise.

----------------


observation:

One commenter said that "recommendation" was perhaps too strong a word
to use for Section 6.  I observe that in some places Section 6 is said
to be "suggested" and in others "recommended"/"recommendation".  Just
an observation.

————————

Many/most of my comments are addressed.

I made more detailed responses to issues #55 and #58, and make
further suggestions.

Several of these detailed comments concern the lack of indications of
how an error should be handled by the recipient.  This is also one of
my general comments, and I dealt with that general comment separately,
so I haven't repeated that response here.

The authors gave me access to their github consideration of my
comments and separated them out into separate issues.  (I admire their
diligence, btw.)  I added the appropriate github issue number to the
comments to keep track of the page containing the author response, and
I'm leaving it in - maybe it will be helpful.

My original comments are indented and my responses are not.


——————————

    Detailed comments on 

    draft-ietf-ntp-using-nts-for-ntp-22 and my reply to the changes
    made in v26.

    Section 1.2, p 6

       supply of cookies along with an IP address to the NTP server for
    ...
       Time synchronization proceeds with one of the indicated NTP servers

    The cookies are for one server, what are "the indicated NTP servers"
    (plural).
    (github issue #32)
    
v26 addresses this comment.

---------------------------

    Section 4, p 8

       in the TLS-protected channel.  Then, the server SHALL send a single
       response 
    and
       The client's request and the server's response
    but 
							Requests and
       non-error responses each SHALL include exactly one NTS Next Protocol
       Negotiation record.

    Is there a single request and single response, or plural requests
    and responses?  If there are many, do they have to have the same
    Next Protocol?
    (github issue #34)

v26 addresses this comment.

---------------------------

    Section 4, page 9

	  unrecognized Record Type MUST ignore the record if the Critical
	  Bit is 0 and MUST treat it as an error if the Critical Bit is 1.
    If it is an error, what then?  The server can reply with an Error record
    type, if so, what would the code be?  What would the client do?
    (github issue #35)

The authors' response was that this was implementation dependent, but
recognized that "Reasonably though, the client should not continue trying."

I accept the v26 text.  (My general comment about error conditions applies.)

—————————————

       significant bit of the first octet.  In C, given a network buffer

    There is a bit called "C" discussed on this page, took me aback a bit.
    Probably not a problem, but if there's an opportunity to say "In the C 
    language", maybe you should.
    (github issue #36)

v26 addresses this comment.

---------------------------

    Section 4.1.2 page 11

       Protocol IDs listed in the server's response MUST comprise a subset
       of those listed in the request and denote those protocols which the
       server is willing and able to speak using the key material
       established through this NTS-KE session.

    It is clear that the server in "server's response" is the NTS-KE server,
    but I believe that the server in "server is willing and able to speak" is
    the NTP server that the client will eventually contact.  Right?
    (github issue #37)

v26 addresses this comment

---------------------------

			    The request MUST list at least one protocol,
       but the response MAY be empty.

    If the request is empty, what is the client to do?  retry with a
    different set of protocols?  Close the TLS channel?

    [There is text saying the the TLS session has minimal impact because
    only one request and one response are exchanged, but retries change
    that. And Section 4 intro page 8 says
       client SHALL send a single request as Application Data encapsulated
       in the TLS-protected channel.  Then, the server SHALL send a single
       response followed by a TLS "Close notify" alert and then discard the
       channel state.
    which makes retries unlikely.]
    (github issue #38)

v26 retains the original language.  (This is one of my error condition
comments)

I noted above that the original text said that the NTS-KE
server would close the channel, and the v26 text says both client and
server should close the channel after one request/response exchange.
So the client's only option is to reopen the TLS negotiation.  That
raises the problem of too frequent retries producing load on the
NTS-KE server.  I think some of the discussions with Mirja and the new
rate limiting text in Section 5.7 applies here.

I accept the v26 text.  (My general comment about error conditions applies.)

---------------------------

    Protocol ID 0 is the only defined protocol now, but is it MTI for
    the server to support Protocol ID 0 always?
    (github issue #39)

v26 retains the original language.  The response from the authors was:
No, it is not, the server or the client could support only other
protocols than 0 (= "Network Time Protocol version 4 (NTPv4)"),
e.g. NTPv4++, or an NTS enabled form of PTP, or something else.

OK.

I accept the v26 text.

---------------------------

    Section 4.1.3 page 11

       Clients MUST NOT include Error records in their request.

    What if it does?  Does the server drop, respond with an Error, send the
    TLS "Close notify" alert, what? If responding with Error, what code - 
    Bad Request or Internal Error?  (I'm not sure format errors fit in the 
    Bad Request description, tho' they certainly fit the name.)
    (github issue #40)

The github response, shared with github issue #45, was to revise the
description of the Error code Bad Request.

v26 addresses this comment.

---------------------------

						      If clients
       receive a server response which includes an Error record, they MUST
       discard any negotiated key material and MUST NOT proceed to the Next
       Protocol.

    Does "negotiated key material" mean the TLS negotiated material?

    Does the client retry with a new request? (next paragraph would seem to 
    indicate retries are possible with modification, Section 4 intro pg 8 
    makes retries sound impossible.)

    If the client is discarding the TLS negotiated materials, does the
    client close the TLS channel and start over from the beginning?
    That Section 4 intro page 8 makes it sound like the client will be
    forced start over.
    (github issue #41)

v26 addresses this comment.

---------------------------

       The following error codes are defined:

    In all these cases, what does the server do after it sends the Error
    code?
    (github issue #42)

I should have noted again the Section 4 page 8 requirement that the
server always closes the connection after it responds.

v26 addresses this comment.

---------------------------


    Section 4.1.5, page 12

								 It is
       empty if the server supports none of the algorithms offered.

    If it is empty, what does the client do?  retry with a new
    request?  (yadayada about whether the Section 4 intro page 8 means
    that can't be) or declare failure and communicate with the NTP
    server without NTS protections?  should the server send the TLS
    "Close notify" alert after sending the empty algorithms list?
    (github $43)

The authors' response was that the client has several options in what
it could do, but nothing should be specified in this draft.
This is one of my error condition comments, which I addressed
generally.

I accept the v26 text.

---------------------------

       Server implementations of NTS extension fields for NTPv4 (Section 5)
       MUST support AEAD_AES_SIV_CMAC_256

    Does this apply to the client as well?  If so, is it advisable for
    the client to always include that algorithm?  Is it mandatory for the
    client to include that algorithm?
    (github issue #44)

The author's response is that the MTI algorithm requirement does not
apply to the client.  There is no change in the v26 text.  I suppose
that implementers will understand that including the MTI algorithm is
beneficial.

I accept the v26 text.

---------------------------

    Section 4.1.6 page 13

       Clients MUST NOT send records of this type.
    What does the server do if the client does?  Send an Error and if
    so with what code?
    (github issue #45)

The authors' response was covered by the change in github issue #40 above.

v26 addresses this comment.

---------------------------

    Section 4.1.7 page 13

						  Servers MUST send at
       least one record of this type
    What does the client do if the server does not?  Send a new request?
    (github issue #46)

The authors considered this an implementation detail, not to be
addressed in this document.  One did comment that it would not be good
if the client got "stuck in some retry loop".

This is one of my error condition comments, which I will address
generally.

I accept the v26 text

---------------------------

    Section 4.1.7 page 13

					       Servers MUST NOT
       send more than one record of this type.
    What does the client do if the server does?  Choose the first?
    Drop the response and retry the request?  Close the TLS channel?
    Randomly pick one?
    (github issue #47)

Same response as previous comment/issue.

---------------------------

       When this record is sent by the client, it indicates that the client
       wishes to associate with the specified NTP server.
    What if the client wishes to associate with more than one NTP server?
    (that's recommended practice, so likely.)  Does that mean multiple
    requests to the NTS-KE server under the same TLS channel?  [Section 4
    intro page 8 makes that sound impossible.] does it mean opening
    multiple TLS channels to the NTS-KE server?
    (github issue # 48)

The authors' response was that multiple TLS channels, one for each
desired NTP server, would be necessary.  The v26 text is unchanged. I
believe the text supports that expectation, so OK by me.

So I'll not pursue this comment.

----------------------------

    Section 4.2 page 14

    Inputs to the exporter function are
       to be constructed in a manner specific to the negotiated Next
       Protocol. However, all protocols which utilize NTS-KE MUST conform
       to the following two rules:

    So must all protocols with utilize NTS-KE be time protocols?
    (Github issue #49)

The authors' response was that there is no requirement that a protocol
that used NTS-KE be a time protocol.  That answers my question.  The
v26 text is unchanged.

v26 addresses this comment.

---------------------------

    There are requirements here about what a Next Protocol must provide.
    The cookie construction is another.  Should there be a statement
    about what a Next Protocol spec must include?

(No explicit response to this comment, which I think means "No".  It
would be reliant on any consideration of a Next Protocol specification
to be sure it was sufficiently complete.)

v26 text is unchanged.

I accept the v26 text.

---------------------------
 

    Section 5.7 page 20 (nit)

       already sent, it SHOULD initiate a re-run the NTS-KE protocol.  The

    "initiate a re-run of the NTS-KE protocol" or "SHOULD re-run the NTS-KE
    protocol"
    (Github issue #50)

v26 addresses this comment.

---------------------------

    Section 5.7 page 21

		    Figure 5: NTS Time Synchronization Messages
    The caption should be "NTS-protected NTP Time Synchronization Messages".
    The exchange in the figure is of NTP messages, not NTS messages.
    (github issue #51)

v26 addresses this comment.

---------------------------
								  In
       general, however, the server MUST discard any unauthenticated
       extension fields 

    I am not sure what "In general, ... the server MUST" could mean, 
    but given that later text says
    (github issue #52)

	    Servers MAY implement exceptions to this requirement for
       particular extension fields if their specification explicitly
       provides for such.

    I figure this means something like:

       The server MUST discard any unauthenticated extension fields,
       UNLESS an exception is implemented for specific extension fields
       whose specification explicitly provides for unauthenticated
       transmission.

    Maybe.
    (github issue #53)

v26 revises this text, and the authors' version is clearer and shorter
than what I suggested.

It does however eliminate the requirement that
the exception spec must "explicitly provides for unauthenticated
transmission."  Was that intentional?  Just asking.  I don't know how
the authors of an exception to this rule could miss the fact that they
were allowing unauthenticated transmission, but the fact that the
requirement was included in the earlier version sounded like a
concession to someone who found the exception troubling.  

v26 addresses this comment.

---------------------------

    Section 5.7 page 22

								  In
       general, however, the client MUST discard any unauthenticated
       extension fields and process the packet as though they were not
       present.  Clients MAY implement exceptions to this requirement for
       particular extension fields if their specification explicitly
       provides for such.

    Same complaint as on page 21 about "In general, ... the client
    MUST discard..... and "MAY implement exceptions"
    (github issue #54)

As above.

v26 addresses this comment.

---------------------------

    Section 5.7 page 23

       If the server is unable to validate the cookie or authenticate the
       request, it SHOULD respond with a Kiss-o'-Death (KoD) packet

    ...

			   A client MAY automatically re-run the NTS-KE
       protocol upon forced disassociation from an NTP server.

    Is there a suggestion there that the server may force a disassociation
    from the NTP server after sending the NTS NAK?  (I don't know what
    force that would be, NTP doesn't really have a session-connection-
    channel-state on the server side.)
    (github issue #55)

The authors' response was that the NTS-NAK is the forced
disassociation.

This is the only place in the document that uses the term "forced
disassociation".  If this said "receipt of a NTS NAK", I think I would
not have been confused (or less confused).

Trying to read the paragraph, I'm unclear here.  The paragraph
has the following sequence:
- validate the KoD packet
- discard if validation fails
- if OK, comply with RFC5905's discussion of KoD packets (which btw says
take action for DENY, RSTR, and RATE and ignore any others, which
would include this new NTS-NAK)
- may re-run the NTS-KE "upon forced disassociation"
- wait until the next poll for a validatable response (but haven't we
been disassociated at this point?)
- if none, initiate NTS-KE (by implication, if there is one, all is well?)
- while NTS-KE is in progress, continue to use old cookies etc.

That seems to be out of order.  And nowhere does this text say that
receipt of one validatable NTS-NAK should result in "forced
disassociation", i.e., "demobilize all associations with that server"
in RFC5905 Section 7.4 language.  The text seems to permit a continued
association with the NTP server, even while a new crypto environment
is being negotiated, there's not necessarily "forced disassociation".
(I'm reading "association" in the sense used in RFP5905.)

I thought maybe you needed or intended a reference to the handling of
a received NTP CRYPTO_NAK in RFC5905, but ironically I can find no
required reaction on receipt of a CRYPTO_NAK in RFC5905 normative
text, although the (non-normative) Appendix does indicate that the
client should "demobilize the association".

There are changes in this section of text but the changes don't
address my comment.  (The changes made address comments about the
potential for NTS-KE server overload from retries.)  I think the text
still needs to (1) make "forced disassociation" more clear - change
"forced disassociation" to "receipt of <validatable> NTS NAK",
presuming that is what is meant, or define it somewhere (2) clean up
the sequence of text so the order of the text matches the order of
actions (3) decide whether the NTS NAK dissolves the association in
the NTP sense of the word or (just?) starts a new crypto environment
(but only when no later NTS-protected message arrives from the server)

v26 does not address my comment.

--------------------------------

    Section 6 page 24

    This section uses "should" a lot, not SHOULD, in cases where it seems
    to be describing recommended optional behavior and sometimes when it
    is describing required behavior.  
    Perhaps that is because this is non-normative text, but distinguishing
    between the required behavior and optional behavior in this text
    is desirable, so I suggest deciding which uses of "should" are really
    RFC21119 requirements language and make it SHOULD, MUST, SHALL, etc.
    (github issue #56)

The authors pointed to RFC8174, which allows the use of
non-capitalized normative words to have a non-normative meaning.
My point was that sometimes the text specifies behavior that in this
example are required behavior and one way to indicate that is to
use the capitalized forms.  This is non-normative but it is likely
that implementers will use this text as a guide.  Having optional
behavior in this example distinguished in some way from the required
behavior in this example would be a good thing.

But the stated non-normative status of this section makes that the
issue murky at best.


So I'll not pursue this comment.

--------------------------------

       This section is non-normative.  It gives a suggested way for servers
       to construct NTS cookies.  
    and manage the server (NTS-KE and NTP) keys


       Servers should select an AEAD algorithm which they will use to
       encrypt and authenticate cookies.
    NTS-KE encrypt cookies with this algorithm, NTP servers authenticate 
    cookies with this algorithm, so in this case "servers" means both of 
    them, right?
    And both must make the same choice, right?
    (And this is one of the places where it really sounds like you mean 
    MUST or SHALL.) 
    (github issue #74)

The v26 does not change this text. The authors saw no need to change.

I accept the text of v26

--------------------------

		      Servers should additionally choose a non-secret, unique
	   value `I` as key-identifier for `K`.
	   ...
	   of this approach is to use HKDF [RFC5869] to derive new keys, using
	   the key's predecessor as Input Keying Material and its key identifier
	   as a salt.
	RFC5869 says the salt is random.  Should the text about
	choosing the “non-secret, unique” identifier mention random
	/ unpredictable as well?
	(github issue #57)

I asked Ben about the issue of using a non-random salt, as that is
beyond my crypto skill-level.  He replied that he had no concerns
about the salt in this context.

I withdraw the comment.

-----------------------
	   

	   Servers should periodically (e.g., once daily) generate a new pair
	   ...
			       Immediately following each such key rotation,
	   servers should securely erase any keys generated two or more rotation
	   periods prior.
	Does this mean that a client's cookies to its chosen NTP server will
	no longer be usable after two daily rotations?  (This presumes it does
	no polls in those two days, because any new response would provide
	a new cookie.) Must it go back to forming a TLS channel and an NTS-KE exchange?
	If the NTS-KE server gives it a cookie for a different NTP 
	server, won't its time synchronization process revert to unsynchronized?
	(github issue #58)

The v26 text has changed this from erasing keys two or more rotations
back to erasing all previous rotations' master keys (with a caveat
about non-expired keys).  I do have some questions about the resulting
text:

	   Servers should periodically (e.g., once daily) generate a new pair
	   (I,K) and immediately switch to using these values for all newly-
	   generated cookies.  Following each such key rotation, servers should
	   securely erase any previously generated keys that should now be
	   expired. 

What does "erase any previously generated keys that should now be
expired" mean?  "any previously generated keys that, btw, should at
this point be in an expired state"?  "any previously generated keys except those
that have not expired"?  "any previously generated keys that at this
point should at this point be tagged as expired"?

This is the first point that the idea of key expiration appears.


                    Servers should continue to accept any cookie generated
	   using keys that they have not yet erased, even if those keys are no
	   longer current.

The phrase "no longer current" sounds like "have expired", except that
one of my interpretations of the previous sentence says that previous
rotation keys that were expired would have been erased, so there would
be no "not yet erased" keys that are "no longer current".  Maybe "no
longer current" means "are not the master key currently in use", but
that doesn't quite work either.

                            Erasing old keys provides for forward secrecy,
	   limiting the scope of what old information can be stolen if a master
	   key is somehow compromised.

I topped this list with a comment about this sentence.

                                        Holding on to a limited number of old
	   keys allows clients to seamlessly transition from one generation to
	   the next without having to perform a new NTS-KE handshake.

This section is non-normative,  but even so, I think the clarity could be improved.

-----------------------

	Section 7.6 page 28

	   Applications for new entries SHALL specify the contents of the
	   Description, Set Critical Bit, and Reference fields as well as which
	"Set Critical Bit" is not a field in the registry entries' field 
	descriptions above and not included in the table below.
	(github issue #59)

v26 addresses this comment.

---------------------------


    Section 9.2

					      In addition to dropping
       packets and attacks such as those described in Section 9.5, an on-
       path attacker can send spoofed kiss-o'-death replies, which are not
       authenticated, in response to NTP requests.  This could result in
       significantly increased load on the NTS-KE server.

    Why?  I presume this text means that the KoD is a NTS-NAK and client 
    who sees the NTS-NAK would restart the whole process with the 
    NTS-KE server.  That is optional behavior in Section 5.7 page 23, 
    and then only "upon forced disassociation" from the server, which 
    the text does not require of the NTP server that sent an NTS-NAK.
    (github issue #60)

The authors found no reason to change this text.  I believe that my
hypothesis at to why spoofed KoD might increase load on the NTS-KE
sever is correct and supported by the rest of the text.  And the
"forced disassociation" term I interpreted above to mean a separate
action by the server, which was wrong (see the comments above about
section 5.7), so that eliminates my comment here.

So I'll not pursue this comment.

--------------------------------

    Section 9.4 page 36

	  error less than NTP_PHASE_MAX.  In this case, the clock SHOULD be
    Is there a reference for the "NTP_PHASE_MAX"? It is not in RFC5905.
    A web search finds only hits in this draft and in various *nx man
    pages as being a "kernel constant"
    (github issue # 61)

v26 no longer mentions that constant.

v26 addresses this comment.

---------------------------

    Section 9.4 page 37

							      take care not
	  to cause an inadvertent denial-of-service attack on the NTS-KE
	  server, for example by picking a random time in the week preceding
	  certificate expiry to perform the new handshake.

    This order makes it sound like the example is about the attack, not the
    way to "take care not to cause".
    (github issue #62)

I accept the v26 text.

----------------------

    Appendix A

    (I wasn't going to note this nit, but since you have a list...)

    NTS NAK should be on this list.

    Actually, NTS NAK is not defined before it is used.  In Section 5.7, page 23
    it says "NTS negative-acknowledgment (NAK)" but it does not say
    "NTS NAK".  So a search for the acronym does not find its
    definition.
    (github issue #63)

v26 addresses this comment.

---------------------------