Subject: Comments on draft-ietf-bfd-generic-crypto-auth

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Mon, 14 Apr 2014 17:23:17 +0000

Hi Authors,

I have reviewed your draft (draft-ietf-bfd-generic-crypto-auth ), comments below.
Since document is nearing expiration, perhaps you can address some of these comments and re-publish?

(1) Abstract

s/arbitary/arbitrary/

(2) Section 2

Before listing "Not Before Time" and "Not After Time", it'll be great if the document can provide some words to better transition from previous paragraph. 

(3) Section 3.3

[snip]
   The device MUST fill the Auth Type and the Auth Len fields before the
   authentication data is computed.  The Sequence Number field MUST be
   set to bfd.XmitAuthSeq.
[snip]

The paragraph is slightly confusing wrt whether Sequence Number field need to be set before or after authentication data is computed. It'll be good to clarify this.

(4) Section 3.4, second paragraph

[snip]
   If the Authentication Present (A) bit is set in the packet header and
   the receiver will try to find a appropriate BFD SA in its local key
   table to process the packet.
[snip]

I believe you meant to say:

   If the Authentication Present (A) bit is set in the packet header and
   Auth Type is TBD1 or TBD2, the receiver is to find an appropriate BFD
   SA in its local key table to process the packet.

See below for usage of TBD1/TBD2.

(5) Section 3.4, fourth paragraph

Is it better to say "unsigned 64 bit circular number space" here, or keep it as "unsigned 32 bit circular number space"? If latter, then the document should clarify that it's low order 32 bits.

(6) Section 3.4, sixth paragraph

[snip]
  In such a case, an error event SHOULD be logged.
[snip]

Such log could become DoS attack point? Rate limiting of such log is outside the scope of this document, but it could be beneficial to explain this in the Security Considerations section ... optional comment though.

(7) IANA Considerations

Indeed values 6 and 7 are next, but it's technically not correct to just use them [yet].
Can we use TBD1 and TBD2? If someone is implementing and early allocation is required, then we can do that.

Also it'll be beneficial for this section to clearly describe what is needed from where. Maybe something like:

  IANA is requested to assign two authentication types from the
  "BFD Authentication Types" sub-registry within the "Bidirectional
  Forwarding Detection (BFD) Parameters" registry.

  Address  BFD Authentication Type Name             Reference
  -------  ---------------------------------------  -------------
     TBD1  Cryptographic Authentication             this document
     TBD2  Meticulous Cryptographic Authentication  this document

(8) Security Considerations

s/reestablishing/re-establishing/

(9) References

RFC5880 should be a normative reference instead of informative, as document is even referencing a variable from RFC5880 (ex: bfd.XmitAuthSeq).

-Nobo

---

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 16 Apr 2014 23:18:16 -0700

Nobo,

See comments inline.

> Since document is nearing expiration, perhaps you can address some of these comments and re-publish?

Fixed.

> Before listing "Not Before Time" and "Not After Time", it'll be great if the document can provide some words to better transition from previous paragraph. 

Those are the parameters associated with a BFD SA just like Authentication Algorithm etc. The indentation might have thrown you off. Let me see if I can fix it.

> The paragraph is slightly confusing wrt whether Sequence Number field need to be set before or after authentication data is computed. It'll be good to clarify this.

I was looking at RFC 5880 for guidance on this. It says "The Sequence Number field MUST be set to bfd.XmitAuthSeq." Period. No mention of before or after.

How about this:
"The device MUST fill the Auth Type, the Auth Len fields and set the Sequence Number field to bfd.XmitAuthSeq before the authentication data is computed."

> See below for usage of TBD1/TBD2.

Ok. Will fix it.

> Is it better to say "unsigned 64 bit circular number space" here, or keep it as "unsigned 32 bit circular number space"? If latter, then the document should clarify that it's low order 32 bits.

Ok. Will change it to say "unsigned 64 bit circular number space"

> Such log could become DoS attack point? Rate limiting of such log is outside the scope of this document, but it could be beneficial to explain this in the Security Considerations section ... option…

How about this:

"In such a case, an error event SHOULD be logged. Rate limiting of such a log to prevent a DoS attack is outside the scope of this document."

>  Address  BFD Authentication Type Name             Reference

Ok. Will make the suggested changes.

> s/reestablishing/re-establishing/

Will fix.

> RFC5880 should be a normative reference instead of informative, as document is even referencing a variable from RFC5880 (ex: bfd.XmitAuthSeq).

That is correct. Consider it done.

Thanks

> -Nobo

Mahesh Jethanandani
[email-masked]

---

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Thu, 17 Apr 2014 13:02:02 +0000

Hi Mahesh,

Thanks for considering my comments.

> > The paragraph is slightly confusing wrt whether Sequence Number field
> need to be set before or after authentication data is computed. It'll be good
> to clarify this.
> 
> 
> I was looking at RFC 5880 for guidance on this. It says "The Sequence
> Number field MUST be set to bfd.XmitAuthSeq." Period. No mention of
> before or after.
> 
> How about this:
> "The device MUST fill the Auth Type, the Auth Len fields and set the
> Sequence Number field to bfd.XmitAuthSeq before the authentication data
> is computed."

That is much clearer.

> > Such log could become DoS attack point? Rate limiting of such log is
> outside the scope of this document, but it could be beneficial to explain this
> in the Security Considerations section ... optional comment though.
> >
> 
> How about this:
> 
> "In such a case, an error event SHOULD be logged. Rate limiting of such a log
> to prevent a DoS attack is outside the scope of this document."

Changes does imply that rate limiting should be applied. However, considering these logs could be generated for every BFD packet received by the system (which can be significant amount), it may be better to explicitly have a paragraph on this in the Security Consideration section. Perhaps this can be taken up as a next next revision?

-Nobo