Hello Authors/WG, Thanks for the work put into this document. I know this has been in the works for a long time but there is still some more work needed before it can be taken up for IESG evaluation.
I would like to share my review of the v20 of this document. General Comment/Suggestion: This is about the contents of this document and its relationship with draft-ietf-bfd-stability and draft-ietf-bfd-optimizing-authentication. Please make this document self-sufficient for all things related to the two auth types that it introduces (incl IANA and YANG). It can refer normatively to draft-ietf-bfd-optimizing-authentication (as it sometimes does today but not consistently?) for the optimized mode operation and the transition back/forth. It can refer normatively to draft-ietf-bfd-stability for the BFD packet loss monitoring feature that can be enabled with this auth type. If you see this along with my suggestions for the other two documents, I hope you will see a clear dependency graph between these documents. Major Concern: The reference to ISAAC has to be normative for this document and that reference is today a private webpage. This is not something that could be considered a stable reference for an RFC publication. We will need to figure this out and please check in my detailed comments for some suggestions. Please find below my comments in the idnits output of v20 and look for <EoRv20> at the very end of the review. If you don't see that, then likely the email has been truncated by your email client and you should look at the BFD WG email archive for the full version. Thanks, Ketan 85 1. Introduction 87 BFD [RFC5880] (Section 6.7) defines a number of authentication 88 mechanisms, including Simple Password, and various other methods 89 based on MD5 and SHA1 hashes. The benefit of using cryptographic 90 hashes is that they are secure. The downside to cryptographic hashes 91 is that they are expensive and time consuming on resource-constrained 92 hardware. <minor> The term "Meticulous Keyed" that is used in the abstract and further in this section and document may trip readers. It would be good to have some text here to explain that term along with the reference to the specific sections of RFC5880. 117 2. Requirements Language 119 The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", 120 "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this 121 document are to be interpreted as described in RFC 2119 [RFC2119]. <major> Please align the above blob with BCP14 including RFC8174. 136 Some of the state variables in BFD [RFC5880] (Section 6.8.1), are 137 related to the authentication type being used for a particular 138 session. However, the definitions given in BFD [RFC5880] are 139 specific to Keyed MD5 or SHA1 Authentication, which limit their 140 utility for new authentication types. For the purpose of the 141 experiment, this specification updates the definition of some of the 142 state variables as given below. <minor> Perhaps rephrase the above sentence to convey that this document does not "update" RFC5880 since it is narrowly scoped to the types introduced in this document alone and to this experiment only? 185 4. Architecture of the Auth Type Method 187 When BFD uses authentication, methods using MD5 or SHA1 are CPU 188 intensive, and can negatively impact systems with limited 189 computational power. <minor> Do we want to add that the BFD function has been offloaded to hardware or lower powered CPUs or something like that in some router implementations ? Or, better still move all these discussions to the optimizing authentication document since those considerations apply there and are not specific to these two new types. 252 Opt Mode: 253 The Optimized Authentication Mode, as defined in 254 [I-D.ietf-bfd-optimizing-authentication]. allows for systems to 255 switch to a less computationally intensive authentication method, 256 without changing the main bdf.AuthType. <major> Define this field here and specific to the new types introduced. The reference to optimizing-auth draft is perhaps only to the extent of indicating that this field is where the 1 or 2 values are encoded to indicate the shift in modes. 271 Auth-Key: 273 This field carries the 32-bit (4 octet) ISAAC output which is 274 associated with the Sequence Number. The ISAAC PRNG MUST be <minor> Here, a forward reference is needed to the section where the description of the ISAAC Page and how to use the sequence number to pick the 32-bit value from it is described. 296 The receiving system accepts the packet if the key ID matches one of 297 the configured Keys, and the Auth-Key derived from the selected Key, 298 Seed, and Sequence Number matches the Auth-Key carried in the packet, 299 and the sequence number is strictly greater than the last sequence 300 number received (modulo wrap at 2^32) <nit> missing a "." above. There are multiple such instances in this document where the trailing period has been missed. Please check and fix them all. 304 The Auth Type field MUST be set to one of two TBDs (Optimized MD5 305 Meticulous Keyed ISAAC Authentication or Optimized SHA-1 306 Meticulous Keyed ISAAC Authentication). <minor> Could we avoid the use of "TBDs" here? IOW, can we reference them by their names that are in the brackets? 332 If the received BFD Control packet does not contain an 333 Authentication Section, or the Auth Type is not correct (one of 334 two possible TBDs, Optimized MD5 Meticulous Keyed ISAAC <minor> Same as the previous comment - avoiding the "TBDs". 367 Note that in some cases, calculating the expected output of ISAAC 368 will result in the creation of a new "page" of 256 numbers. This 369 process will be irreversible, and will destroy the current "page". 370 As a result, if the generation of a new output will create a new 371 "page", the receiving party MUST save a copy of the entire ISAAC 372 state before proceeding with this calculation. If the outputs 373 match, then the saved copy can be discarded, and the new ISAAC 374 state is used. If the outputs do not match, then the saved copy 375 MUST be restored, and the modified copy discarded, or cached for 376 later use. <major> Can we have some text in section 4 to describe this thing with "pages" so the reader has that context before they come to this section? IOW describe the term ISAAC Page and how these pages are generated and used. Then the text in this section can be simplified. We can also use the term ISAAC Page without any quotes? Taking it further (as you will see in further comments), please consider describing and specifying all ISAAC working and procedures up front. 404 bfd.MetKeyIsaacRcvAuthData: 405 A data structure which contains the ISAAC data for the received 406 Auth Type method. <minor> Is this data structure the figure in section 10? If so, please put a forward reference to it. Same goes for Xmit. 439 A particular Secret Key is identified via the Auth Key ID field. 440 This Auth Key ID is either placed in the packet by the sender, or 441 verified by the receiver. Meticulous Keyed ISAAC Authentication 442 permits systems to have multiple Secret Keys configured, but we do 443 not discuss how those keys are managed or used. We do, however, <minor> Please find all usage of "we" in this document and replace it with something more suitable for an RFC. E.g. use "this document". 454 For interoperability, the management interface by which the key is 455 configured MUST accept ASCII strings, and SHOULD also allow for the <major> Is the terminating \0 of the ASCII string considered as part of the seed or not? 475 There is no negotiation as to when authentication switches from the 476 original type, to using Meticulous Keyed ISAAC. The sender simply 477 begins authentication with a relevant Auth-Type, and with the 478 Optimized Authentication Mode field set to 1. When the sender 479 switches to using using Meticulous Keyed ISAAC Authentication, it 480 sets the Optimized Authentication Mode field to 2, and starts 481 performing the ISAAC calculations as described here. <major> If the switch to ISAAC is not strictly specified, and implementations are free to switch back/forth whenever they desire, does that not open up a security hole where an attacker could exploit these transitions themselves or cause a transition? Why not mandate that switch to/from happen immediately after a state transition? Better still why not all this is specified in the optimizing-auth draft and this document only points to that spec? Am I missing something? 517 The ISAAC PRNG is initialized by setting all internal variables and 518 data structures to zero (0). The PRNG is then seeded by using the 519 the following structure: 521 0 1 2 3 522 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 523 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 524 | Seed | 525 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 526 | Your Discriminator | 527 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 528 | Secret Key ... | Counter | 529 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ <minor> Please insert a figure name and reference for the picture. It will be a helpful reference. This goes for all figures in the document. 534 systems, the field are taken from the received packet. The length of 535 the Secret Key MUST be 1016 octets or less. The Counter field is use <major> This conflicts with the limits of the secret key in section 8. Or am I missing something? 541 Counter field is incremented, starting from zero (0). This process 542 may finish with a partial copy of the above structure, in which case 543 only a prefix of the structure is copied. <major> What is a "prefix of the structure"? Do you mean how much ever part of the structure that can be accomodated in what remains of that page? 545 Once the ISAAC "page" is initialized, the data is processed throught <nit> s/throught/through ... there are several other spelling mistakes that a spell check can help find/fix easily. 546 the "randinit()" function of ISAAC. Pseudo-random numbers are then 547 produced 32 bits at a time by calling the "isaac()" function. <minor> Please put a reference to ISAAC here. 555 The following figure gives Seed and Your-Discriminator as 32-bit hex <nit> expand hex to hexadecimal 556 values, and the Secret Key as an eleven-character string. The 557 subsequent figure shows the first eight Sequence numbers and <minor> There is no figure. It would be better to either put this example blob in a figure or suitably formatted so it is distinguished from the normative text of the document. 561 Seed 0x0bfd5eed 562 Y-Disc 0x4002d15c 563 Key RFC5880June <major> Isn't this Key the Secret Key? If so, please correct. 575 Note that this construct requires that the "Your Discriminator" field 576 not change during a session. However, it does allow the "My 577 Discriminator" field to change as permitted by RFC5880 Section 6.3 578 [RFC5880] <major> If My Discriminator is changed, then does that not result in the change of "Your Discriminator" for the other end? RFC5880 says the implications of discriminator change are outside scope. Perhaps the same can be said here or better still make it a prerequisite that there is no discriminator change while the session is in UP state (somewhat similar to what is indicated for the Seed a couple of paragraphs below)? 581 controlled by each party in a BFD session. For security, each 582 implemention SHOULD randomize their discrimator fields at the start 583 of a session, as discussed in RFC5880 Section 10 [RFC5880]. <nit> fix redundant use of RFC5880 above. 650 If, however, the packet's Sequence Number differs from the expected 651 value, then the difference "N" indicates how many packets were lost. 652 The receiver then has to search through the first "N" Auth Keys 653 derived from its calculated ISAAC state in order to find one which 654 matches. If no key matches the Auth Key in the packets, the packet 655 is deemed to be inauthentic, and is discarded. <major> If the sequence is incrementing monotonically, then why is there a need to look at multiple records instead of only the offset between the previous and the received number? Perhaps there is something here that I have not understood ... 657 If a calculated key at index "I" does match the Auth Key in the <major> what is this index "I"? 659 this value. The bfd.MetKeyIsaacRcvAuthBase field is then initialized 660 to contain the value of bfd.RcvAuthSeq, minus the value of 661 bfd.MetKeyIsaacRcvAuthIndex. This process allows the pseudo-random 662 stream to be re-synchronized in the event of lost packets. <question> Isn't it problematic to do this? What if it is an attack? Or, likely I may be missing something ... 669 This document does not make provisions for dealing with the case of 670 losing more than 256 packets. Implementors should limit the value of 671 "Detect Multi" to a small number in order to keep the number of lost 672 packets within an acceptable limit. <major> Isn't this a MUST? Also, this is something for the operator and is better if placed in an operational considerations section of its own for proper focus. Same goes for other similar considerations. 674 11. Operation <major-editorial> This is buried far too deep in the document. It will be easier, if this were all explained towards the start or at an appropriate point in the document. Assume that the reader is familiar with BFD - what is new is this ISAAC thing. So, perhaps explaining that at the beginning will be helpful. 696 The receiving party can then look at the Sequence Number to determine 697 which particular PRNG value is being used in the packet. By 698 subtracting the bfd.MetKeyIsaacAuthBase from the Sequence Number 699 (with possible wrapping), an expected "Index" can be derived, and a 700 corresponding Auth Key found. This process thus permits the two 701 parties to synchronize if/when a packet or packets are lost. <major> Please see my previous comment about index I - I assume the above is the description. Do consider consolidating the procedure and descriptions for a consistent flow for the reader. 722 11.1. Page Flipping 724 Once all 256 Auth Keys from the current page have been used, the 725 "next" page is calculated by calling the isaac() function. This 726 function processes the current "page" to create the "next" page, and <minor> Is this "processes the current page" or "flushes the current page"? 759 12. Transition away from using ISAAC <major> Don't we want to put a reference to [I-D.ietf-bfd-optimizing-authentication] to indicate how the switching to/from ISAAC is done? 767 Since Meticulous Keyed ISAAC Authentication does not provide for full 768 packet integrity checks, it may be desirable for a party to 769 periodically use a strong Auth Type. The switch to a different Auth 770 Type can be done at any time during a session. The different Auth 771 Type can signal that the session is still in the Up state. <major> This seems somewhat conflicting with the previous section where the start of ISAAC auth mode was not immediately after a state change or the use of strong Auth Type. What is stated here is much more desirable than doing a switch where there is no auth mode enabled. 778 The nature of Meticulous Keyed ISAAC Authentication means that there 779 is no issue with this switch, so long as it is for a small number of 780 packets. From the point of view of the Meticulous Keyed ISAAC state 781 machine, this switch can be handled similarly to a lost packet. The 782 state machine simply notices that instead of Sequence Number value 783 being one more than the last value used for ISAAC, it is larger by 784 two. The ISAAC state machine then calculates the index into the 785 current "page", and uses the found number to validate (or send) the 786 Auth Key. <major> I find this strange and one is expected to retain the ISAAC pages even when the auth mode changes. Based on previous section, the receiver initializes when it receives the first packet with the ISAAC auth mode. Am I missing something? The clean start also enables one to change the seed, discriminator, etc. 788 If the non-ISAAC Auth Type instead runs for extended periods of time, 789 then the ISAAC process must continue "in the background" in order to 790 maintain synchronization. This process is needed because this method 791 does not provide for a way to reinitialize the ISAAC method with new 792 Seed value. <major> Seems like a contradiction to me ... see previous comment. 794 13. IANA Considerations 796 For IANA Consideration, please refer to the IANA Considerations 797 section of Optimizing BFD Authentication 798 [I-D.ietf-bfd-optimizing-authentication]. 800 Note to RFC Editor: this section may be removed on publication as an 801 RFC. <major> I am not sure why this complication is needed with creating this IANA dependency with the optimizing authentication draft. Please just ask for allocating the code points 7 and 8 for the two new ISAAC auth types in this document. What would be the issue with doing so? 818 The security of this proposal depends strongly on ISAAC. This 819 generator has been analyzed for almost three decades, and has not 820 been broken. Research shows that there are few other CSRNGs which 821 are as simple and as fast as ISAAC. For example, many other 822 generators are based on AES, which is infeasibe for resource 823 constrained systems. <minor> perhaps provide some BFD offload context here? 825 In a keyed algorithm, the key is shared between the two systems. 826 Distribution of this key to all the systems at the same time can be 827 quite a cumbersome task. BFD sessions running a fast rate may 828 require these keys to be refreshed often, which poses a further 829 challenge. Therefore, it is difficult to change the keys during the 830 operation of a BFD session without affecting the stability of the BFD 831 session. Therefore, it is recommended to administratively disable 832 the BFD session before changing the keys. <minor> Isn't this an operational consideration? 838 The Auth Type method defined here allows the BFD end-points to detect 839 a malicious packet, as the calculated hash value will not match the 840 value found in the packet. The behavior of the session, when such a 841 packet is detected, is based on the implementation. A flood of such 842 malicious packets may cause a BFD session to be operationally down. <major> Could you please elaborate on why this would be the case? And why the behavior is based on the implementation when this spec says that those packets are to be discarded? 871 However, the usual actual attack which we are protecting BFD from is 872 availability. That is, the attacker is trying to shut down then 873 connection when the attacked parties are trying to keep it up. As a 874 result, the attacks here seem to be irrelevant in practice. <major> I don't buy this argument, giving the false impression that a link is up when it is down will result in dropping of packets and is equally harmful. Is it necessary to call this "irrelevant"? 925 16.2. Informative References 927 [ISAAC] Jenkins, R. J., "ISAAC", 928 http://www.burtleburtle.net/bob/rand/isaac.html, 1996. 930 [ISAAC_] Aumasson, J-P., "On the pseudo-random generator ISAAC", 931 https://eprint.iacr.org/2006/438.pdf, 2006. <major> The first reference has got to be normative. Additionally, the reference is to a private blog site and its availability is suspect post publication as an RFC. Please see if there is a better/stable reference. Another option is if the actual algorithm or code could be placed in the appendix (subject to copyright considerations - I am not an expert on this and so we will need to consult - but if the inventor could agree for the portion of the paper/code can be included in the appendix). There may be other options/ways. The 2nd one is at least an organization and it is informational - so I am not worried about that. <EoRv20>
