[karp] RtgDir review of draft-ietf-mpls-ldp-hello-crypto-auth-02

[David Sinicrope <[e-mail]>]

This time with a more relevant subject on the email.
Dave
> Hello,
> 
> I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see
> http://www.ietf.org/iesg/directorate/routing.html
> 
> Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft.
> 
> Document: draft-ietf-mpls-ldp-hello-crypto-auth-02
> Reviewer: David Sinicrope
> Review Date: 28 September 2013
> IETF LC End Date: 23 September 2013
> Intended Status: Standards Track
>  
> o Summary:
> I have some major and minor concerns about this document that I think should be resolved before publication.  While I reviewed the document from the RtgDir point of view, I would strongly suggest a thorough security related review, if not already done, before progressing to publication.  
> 
> In general, I believe the document should be progressed to publication once the issues are resolved by the authors.  The document assumes a working knowledge of KARP mechanisms.  This should be stated up front with relevant references.
> 
> I've attached a .pdf version of the document to this message for use by the authors.
> 
> Comments:
> 
> --- Page 4 ---
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Nit: sec1, para 1 1st sentence : c/runs/run
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor- sec 1, para 2: start the second sentence with "Since the Hello messages are sent with UDP and not TCP..."
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor - sec 1, para 2: change "Besides a note that some  configuration may help protect against bogus discovery messages,  [RFC5036] does not really provide any security mechanism to protect  the Hello messages." To "While some configuration guidance is given in [RFC5036] to help protect against false discovery messages, it does not provide an explicit security mechanism to protect the Hello messages."
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor - global: c/Spoofing/Falsifying/
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor - sec 1, para 3: please give an example of a falsified Hello with a different transport address.
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Nit - sec 1, para 4: remove first "that" in the fist sentence
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor - sec 1 para 4, 1st sentence and global: remove "Basic". The text is introducing a new term of "Basic Hellos" that doesn't exist in RFC 5036.  You may be referring to "Link Hellos".  See RFC 5036 sec 2.4.
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor - sec 1 para 4 & global: remove "Extended" and replace with "Targeted". The text is introducing a new term of "Extended Hellos" that doesn't exist in RFC 5036.  You may be referring to "Targeted Hellos".  See RFC 5036 sec 2.4.
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Nit - sec 1, para 5, sentence 1: c/message/messages/
> 
> 
> --- Page 5 ---
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Nit - global: refer consistently to "Hello message"s vs "Hellos" and "Hello packets".
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Major - Sec 1, para 6, last sentence: remove thisMinor - sec 1 para 4, 1st sentence and global: remove "Basic". The text is introducing a new term of "Basic Hellos" that doesn't exist in RFC 5036.  You may be referring to "Link Hellos".  See RFC 5036 sec 2.4. last sentence, it is redundant with section 3.
> 
> 
> --- Page 6 ---
> 
> Note (yellow), Sep 27, 2013, 3:04 PM:
> Minor - sec 2.1, 1st para: remove 3rd sentence.  Adds no value.
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Major- s2.2, Auth Key ID: this field is conveying the algorithm AND the the secret key.  It should be atomic, I.e., the algorithm ID should be one field the key another.
> 
> Major - s2.2, Auth Key ID: the composition of this field is not clear to the casual reader.  Exactly how is the algorithm identified and what portion is the algorithm I'd and what part is the secret key?
> 
> 
> --- Page 7 ---
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Nit - s2.2, p6: remove "really" in the penultimate sentence.
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Major - s2.2, p6: what are the "available mechanisms" that are being required? I.e., if some one was to test compliance to the requirements of this draft what would they test for this?  Without more specifics on how to meet the sequence number requirement, the strictly increasing requirement in the paragraph above is sufficient.
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Major - s2.3, p1: it should be clearly stated that the high order 32 bits are  incremented on device boot, and low order 32 bit wrap.  "This is currently on given subtly in the previous section as "wrap/boot". 
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Nit - s2.3, p1: "If by some chance..."  Should start a new paragraph,
> 
> 
> --- Page 8 ---
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Major - s3, p1, sentence 1:  "... the Auth Key ID maps to the authentication algorithm and the secret key used to..."  Maps how exactly?  As noted in earlier comments it seems as those this field should be broken into two atomic fields Auth Algorithm and Auth Key.
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Nit - s3, p4: might be easier to see requirements as a list. 
> "Of the above, implementations of this specification:
> - MUST include support for at least HMAC-SHA-256
> - SHOULD include support for HMAC-SHA-1
> - MAY include support for HMAC-SHA-384 or HMAC-SHA-512 or both."
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Nit- s3,p3:remove this paragraph/list, it is superfluous, with the paragraph below and section 2.2 last paragraph.  Further the word "includes" implies the list is not exhaustive.  i.e., I can use MD5 if I wish.
> 
> Question- s3,p3: Are other authentication hashes prohibited?  If not how are they identified and encoded?
> 
> 
> --- Page 9 ---
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Major- s4:  this is difficult to follow.  
> - the section makes reference to "the two octet LDP Cryptographic Protocol ID" this field is not previously mentioned in this document.  What is the reference to this ID? Where is it defined?
> 
> - The section makes reference to the "authentication key". Is this the same as the Authentication Key ID in section 2.2?
> 
> -"Other protocols  using cryptographic authentication as specified herein MUST similarly  append their respective Cryptographic Protocol IDs to their keys in  this step."  This document only specifies one protocol using crypto authentication, I.e., LDP What are the others being referred to?  
> 
> - This text first uses the term "Cryptographic Protocol ID". What is this?  Where is it defined?  From the IANA considerations it is clearer this refers to a KARP mechanism.  Please add some explanation of how this mechanism fits into the KARP framework.
> 
> - could use more explanation or a reference to additional information on the cross protocol attack problem being addressed. 
> 
> - reference to "protocols sharing common keys," what does this mean?  Please elaborate. Perhaps an example would help.
> 
> 
> --- Page 10 ---
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> General- the RtgDir 
> 
> Note - s5: I don't see any routing protocol issues with this.  It should be reviewed for security and crypto issues.
> 
> 
> --- Page 12 ---
> 
> Note (yellow), Sep 28, 2013, 1:29 PM:
> Nit - s6.1, p1, 1st sentence: add "the" after "transmitting"
> Nit - s6.1, p1, last sentence: make this a list
> Nit - s6.1, p2, 1st sentence: make this a separate paragraph
> 
> 
> 
> (report generated by GoodReader)
> 
> 
> 
> 
> 
> Sent from my iPad. Please excuse abbreviations, terse replies and auto-correct errors.