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

From: Sandra Murphy <[email-masked]>
Date: Wed, 4 Mar 2020 19:06:46 -0500

I am not an NTP participant but curious about vulnerabilities of NTP so I looked at NTS on last Fri, 28 Feb.

Unfortunately, that was the last day of the IETF last call, so I have comments and they are arriving after the last call.

So give these comments the consideration they deserve - they come late from someone who lacks context.

Detailed comments attached.  Overall comments here:

Section 1 makes it clear that there is an intentional separation between the NTS-KE server and the NTP server, although it is allowed that both services could be resident on the same host.  Figure 1 also makes it appear that the NTS-KE server may be associated with a large number of NTP servers, with unspecified means of communicating between them.  Reading through, I was struck by how much the NTS-KE server must know about the NTP server - in Section 4.1.5 what algorithms it would support, in Section 6 the keying material it would accept.  I was puzzled at how this could work in the post.ntp.org set of volunteer NTP servers.  Then in Section 6, I saw a reference to “load-balanced cluster”, which would easily account for the NTS-KE server’s knowledge of the NTP servers’ configurations.  More explanations of the supportable (current and potential) architectures would be very helpful.  

I believe it would be helpful for the implementers to have an explicit list of what the NTS-KE server must know about the NTP servers (address, algorithms supported, shared keys in use, cookie construction, protocol IDs accepted, anything else?) and what the communication between the NTS-KE server and the NTP server must provide (what Figure 1 calls “Shared cookie encryption parameters”).

Some of the text concerns the interactions with the NTS-KE server, some concerns the interactions with the NTP server.  The word “server” is used throughout.  There are cases where it is not clear which type of server is meant from context.

I found several cases where I was not sure what the error conditions should be handled, when requirements are not met, when an exchange fails, when an error condition is received, etc.  In some sections, the receiver’s action upon receiving one message is explained but for other messages is not.  

Normative language: There are places where the text says “should” not “SHOULD” and it was not clear the RFC2119 language was intended.  In particular, Section 6 uses “should” many times.  Perhaps that was intentional since this section is not normative.  I’m not sure what should (sic) be the usage in that case.  “In general” appears many times, which begs the question of when it is or is not the case.  On page 21, the text says “In general, however, the server MUST”.  I’m not sure what that means to an implementer.  The following language talks about exceptions, so perhaps the “In general” means “where there is no exception”.

The document sets up an IANA registry for the Protocol ID.  Is it necessary to stipulate what a specification of a new Protocol ID must include?  Must it include a cookie format, for example?

On page 21, the caption for Figure 5 should be “NTS-protected NTP Time Synchronization Messages”.  It is an NTP exchange, not an NTS exchange.

Section 9.4 page 36 - Is there a reference for “NTP_PHASE_MAX?”  A web search found only references to this draft and to a “kernel constant” in various *nx man pages.

In Section 6, the identifier “I” is used as a salt.  RFC5869 says the salt is random.  Should the text about choosing the “non-secret, unique” identifier mention random / unpredictable as well?

—Sandy

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 5 Mar 2020 09:57:10 -0500

I realize that I did not include one concern about the security considerations.  Section 9.7 NTS Stripping talks about the client being deceived into reverting to plain NTP.  There’s no discussion of the server being deceived into responding with plain NTP.  An adversary who stripped the NTS extensions from the a NTP request would lead the server to reply without the expected NTS extensions in the response.  What should/would the client do if it receives a non-NTS enhanced response to a NTS enhanced request?

[Steve Bellovin would frequently suggest that there should be a way for a recipient to know to expect security protections to prevent such downgrade attacks.  I don’t think there is a way in this case.]

—Sandy

> On Mar 4, 2020, at 7:06 PM, Sandra Murphy <[email-masked]> wrote:
> 
> <draft-ietf-ntp-using-nts-for-ntp.detailedcomments.txt>
> 
> 
> _______________________________________________
> ntp mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ntp

---

From: Ragnar Sundblad <[email-masked]>
Date: Thu, 12 Mar 2020 02:20:13 +0100

Hello Sandy,

Thank you very much for your thorough review and all your insightful comments!

As Daniel mentioned in another thread, we are working on a new revision of the draft, were we intend to addresses these issues.

Since there were so many detailed comments, to be able to keep track of them and any discussions around them, I have now made “issues” of them, here: <https://github.com/dfoxfranke/nts/issues>
I have also started to insert comments from previous discussions, and pull requests to fix them.
Feel free to comment them there too.

Best regards,

Ragnar

> On 5 Mar 2020, at 15:57, Sandra Murphy <[email-masked]> wrote:
>

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 26 Mar 2020 08:03:23 -0400

My responses took the form of general comments in the message text and a bunch of detailed comments in an attachment.  I’m going to respond to whether they were were addressed in v26 in two messages.  One will cover the detailed comments, this time inline the message text (an attachment was a mistake).  The other will cover the general comments.

—Sandy

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 26 Mar 2020 08:04:06 -0400

Here are my general comments and my responses.  As for the detailed comments, I have added the authors' github issue number to each.  Where one of the general comments duplicates one of the detailed comments, I note that, and do not repeat the response.

    Section 1 makes it clear that there is an intentional separation between the
    NTS-KE server and the NTP server, although it is allowed that both services
    could be resident on the same host.  Figure 1 also makes it appear that the
    NTS-KE server may be associated with a large number of NTP servers, with
    unspecified means of communicating between them.  Reading through, I was
    struck by how much the NTS-KE server must know about the NTP server - in
    Section 4.1.5 what algorithms it would support, in Section 6 the keying
    material it would accept.  I was puzzled at how this could work in the
    post.ntp.org set of volunteer NTP servers.  Then in Section 6, I saw a
    reference to “load-balanced cluster”, which would easily account for the
    NTS-KE server’s knowledge of the NTP servers’ configurations.  More
    explanations of the supportable (current and potential) architectures would be very helpful.
    (github issue #65)

The working group has begun work on the use of NTS with the NTP pool (pool.net.org).  That work could be what I was looking for here.

I accept the v26 text as is.

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

    I believe it would be helpful for the implementers to have an explicit
    list of what the NTS-KE server must know about the NTP servers (address,
    algorithms supported, shared keys in use, cookie construction, protocol
    IDs accepted, anything else?) and what the communication between the NTS-KE
    server and the NTP server must provide (what Figure 1 calls “Shared cookie encryption parameters”).
    (github issue #66)

I still think that would be a boon to implementers.  I had in mind something like the following (with any additions of something I missed.)

Implementers should note that the NTS-KE server must have knowledge, in some way, of the following information about the NTP servers it supports:
- NTS enabled
- IP address
- list of Protocol ID's accepted
- list of AEAD algorithms accepted
- the NTP server's cookie format and content
- the AEAD algorithm to use in protecting the cookie (generally)
- an AEAD key and parameters for the protection of the cookie (generally)
- for those that support Section 6's suggested mechanism, the NTP server's key rotation and expiration schedules

The authors responded that this was implementation dependent.  Eric Vyncke and I noted on the mailing list that how this information is known is implementation dependent, but what must be known is not.

The list is derived from various places in the existing text, but assembling the information in one point in the text would IMHO be a benefit to implmenters who will have to decide on the means of getting this information known.

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

    Some of the text concerns the interactions with the NTS-KE server, some
    concerns the interactions with the NTP server.  The word “server” is used
    throughout.  There are cases where it is not clear which type of server is
    meant from context.
    (github issue #67)

The v26 text changes the one place that I noted a potential ambiguity.

I accept the v26 text

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

    I found several cases where I was not sure what the error conditions should
    be handled, when requirements are not met, when an exchange fails, when an
    error condition is received, etc.  In some sections, the receiver’s action
    upon receiving one message is explained but for other messages is not.
    (github issue #68)

This was a general comment, and in most places the authors response was that it is implementation dependent.  However, in some cases the implementation response to error or failures could result in additional load on either the NTS-KE sever or the NTP server.  Perhaps a general statement in the security considerations section would be useful and help avoid implementation behaviors that would be harmful.

Like, maybe, something similar to the following could be considered:

There are various places in this document where message errors or failures might be detected by the recipient. Where the response is left to implementation choice, implementers are urged to consider the potential for their implementation's behavior to overload the NTS-KE server or NTP server.  Consider, for example, the discussion of retrying the NTS-KE protocol in Section 5.7.  The implementer's choices must also be guided by the intended application, in choosing outcomes that might range over abandoning time synchronization, accepting insecure time, retrying attempts to achieve secure time synchronization, aborting the application, etc.

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

    Normative language: There are places where the text says “should” not
    “SHOULD” and it was not clear the RFC2119 language was intended.  In
    particular, Section 6 uses “should” many times.  Perhaps that was
    intentional since this section is not normative.  I’m not sure what should
    (sic) be the usage in that case.  “In general” appears many times, which
    begs the question of when it is or is not the case.  On page 21, the text
    says “In general, however, the server MUST”.  I’m not sure what that means
    to an implementer.  The following language talks about exceptions, so
    perhaps the “In general” means “where there is no exception”.
    (github issue #69) duplicates detailed comment issues #53 and #56

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

    The document sets up an IANA registry for the Protocol ID.  Is it necessary
    to stipulate what a specification of a new Protocol ID must include?  Must it include a cookie format, for example?
    (github issue #70) duplicate of detailed comments issue #49

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

    On page 21, the caption for Figure 5 should be “NTS-protected NTP Time
    Synchronization Messages”.  It is an NTP exchange, not an NTS exchange.
    (github issue #71) duplicate of detailed comments issue #51

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

    Section 9.4 page 36 - Is there a reference for “NTP_PHASE_MAX?”  A web
    search found only references to this draft and to a “kernel constant” in
    various *nx man pages.
    (github issue #72) duplicate of detailed comments issue #61

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

    In Section 6, the identifier “I” is used as a salt.  RFC5869 says the salt
    is random.  Should the text about choosing the “non-secret, unique”
    identifier mention random / unpredictable as well?
    (duplicate of detailed comments issue #57)

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

    Section 9.7 NTS Stripping talks about the client being deceived into
    reverting to plain NTP.  There’s no discussion of the server being deceived
    into responding with plain NTP.  An adversary who stripped the NTS
    extensions from the a NTP request would lead the server to reply without the
    expected NTS extensions in the response.  What should/would the client do if
    it receives a non-NTS enhanced response to a NTS enhanced request?
    (github issue #64)

v26 now has a sentence:

                                          In particular, the client MUST
   discard unprotected responses to NTS-protected requests.

v26 addresses my comment.

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

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 26 Mar 2020 08:03:33 -0400

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.

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

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 26 Mar 2020 11:33:55 -0400

Gad.  I was so absorbed in constructing a detailed reply that I missed the fact that publication had been decided.

—Sandy

> On Mar 26, 2020, at 8:03 AM, Sandra Murphy <[email-masked]> wrote:
>

---

From: Ragnar Sundblad <[email-masked]>
Date: Thu, 26 Mar 2020 16:49:58 +0100

Hi Sandy,

Your input is still valuable, we do want to understand if there is
anything broken, now and as long as this spec is in use.

Thanks for all your work!

We were going to prepare a reply to you, but maybe it is not needed.
It seems you are ok with most solutions.

Regarding how the different errors should be handled, there was some
extra text added in Section 4.1.3 in v27:
<https://tools.ietf.org/html/draft-ietf-ntp-using-nts-for-ntp-28#section-4.1.3>

We will go through your comments and address those that remain.

Ragnar

> On 26 Mar 2020, at 16:33, Sandra Murphy <[email-masked]> wrote:
>

---

From: Sandra Murphy <[email-masked]>
Date: Thu, 26 Mar 2020 11:58:06 -0400

Thanks, and sorry it took me so long to go through your very hard work to address my comments and construct an organized reply.  Perhaps quick and dirty would have been better.

I did point near the beginning of the detailed comments to two GitHub issue numbers that I hoped would get some attention.

And I added a new comment about the “forward security” statement in the text.  I did check with Ben first to see if I was plain clueless, and he seemed to agree.  At least he offered to make the comment himself.  That was yesterday.

That’s about it.

I’m glad to see the discussion about retries, as it sort of goes to my concern about error conditions.

—Sandy

> On Mar 26, 2020, at 11:49 AM, Ragnar Sundblad <[email-masked]> wrote:
> 
> 
> Hi Sandy,
>