Re: [secdir] SecDir Review of draft-ietf-sidr-bgpsec-protocol-20

["Sriram, Kotikalapudi (Fed)" <[e-mail]>]

Russ,

Thanks once again for your detailed review and very helpful comments.
The recently submitted version-21 of the draft incorporates changes to reflect
all your comments except two minor comments that have to do with packet format tweaks.
(Please see Oliver's response on the SIDR list about the latter.)  

Please see my specific responses inline below marked with [Sriram].  
Please let me know if there is anything that I have missed.

Sriram


-----Original Message-----
From: Russ Housley [mailto:[e-mail]] 
Sent: Thursday, December 08, 2016 11:26 AM
To: [e-mail]
Cc: IETF SecDir <[e-mail]>
Subject: SecDir Review of draft-ietf-sidr-bgpsec-protocol-20

I reviewed this document as part of the Security Directorate's ongoing effort to review all IETF documents being processed by the IESG.  These comments were written primarily for the benefit of the Security Area Directors.  Document authors, document editors, and WG chairs should treat these comments just like any other IETF Last Call comments.

Document: draft-ietf-sidr-bgpsec-protocol-20
Reviewer: Russ Housley
Review Date: 2016-12-08
IETF LC End Date: 2016-12-16
IESG Telechat date: Unknown

Summary: Almost Ready

I also did a review for Gen-ART.  This SecDir review is identical to that Gen-ART review.

I was involved in the design of BGPsec, but I have not reviewed the document for a long time.


Question

I do not see any guidance on the handling of private AS numbers.  I'm not sure whether it belongs in the document or not.  I am thinking about a BGP stub customer that employs a private AS.  If I understand properly, that stub customer cannot publish a ROA for the private AS number and the prefixes that it uses.  So, the stub customer cannot become a BGPsec speaker.  So, my question is about the BGPsec speaker that receives an announcement from the stub customer.  Does that BGPsec speaker strip the private AS and sign an announcement?  Will their ROA that cover the prefixes used by the stub customer?

[Sriram] There are new paragraphs added in the ops and mgmt. section (S 7)  
to discuss proper handling of private ASNs that may be used for stub ASes 
or inside confederations.


Major Concerns

Section 2.2 says:

   ...  A BGP speaker MAY
   announce BGPsec capability only if it supports the BGP multiprotocol
   extension [RFC4760].  ...

This needs to be worded as a MUST NOT statement.  
The current wording does not align with the definition of MAY in RFC 2119.

[Sriram]: Done

I think an additional consideration is needed in Section 6.2.  This protocol design assumes a signer will compute a message digest and then digitally sign that digest.  If someone wants to use a digital signature that works differently, there may be a significant change to this protocol.

[Sriram]: Please refer to our earlier discussion in this thread on this topic. 
I have left the section as is.

It is very unusual for the IANA Considerations to contain a figure, and Figure 10 really seems out of place.  The version number can be put in an IANA registry, but there really is no need until a second value is needed.  There is no reason to put Dir in an IANA registry, there are only two values and both are fully specified in this document.  The unassigned bits must be zero until a new version is specified.

[Sriram]  I have modified Figures 10 to make clear that the Dir values 
are fully specified by the RFC-to-be.  I have similarly added more clarity in Figure 11 also.
The IANA reviewer Sabrina Tanamal has used Figure 10 in her response.
Seems like she found it useful. So I am keeping it in the document for now.
In the IANA section (including Figs. 10 and 11), it is now clearly stated
that the unassigned bits must be set to zero. 

Minor Concerns

I find the Abstract misses some important information.  Also, the old wording suggests that the attribute contains a single digital signature.
I suggest:

   This document describes BGPsec, an extension to the Border Gateway
   Protocol (BGP) that provides security for the path of autonomous
   systems (ASes) through which a BGP update message passes.  BGPsec is
   implemented via an optional non-transitive BGP path attribute that
   carries digital signatures produced by each autonomous system that
   propagates the update message.  The digital signatures provide
   confidence that every AS on the path of ASes listed in the update
   message has explicitly authorized the advertisement of the route.

[Sriram] Thanks for providing the additional wording. The document now has 
the modified version of the abstract that you've provided above. 

Section 2.1 says:

   ...  The BGP speaker
   sets this bit to 0 to indicate the capability to receive BGPsec
   update messages.  The BGP speaker sets this bit to 1 to indicate the
   capability to send BGPsec update messages.

It seems a bit wasteful to repeat the whole capability for each direction.  Wouldn't it be better to follow the example used in other capability definitions (such as RFC 7911) by using one of the unassigned bits?  The Send/Receive pair of bits would have these
semantics:

   This field indicates whether the sender is (a) able to receive
   BGPsec update messages from its peer (value 1), (b) able to send
   BGPsec update messages to its peer (value 2), or (c) both (value 3)
   for the address family identified in the AFI.

[Sriram]: I have followed your suggestion to put this out on the SIDR list for comments. 
Please see Oliver's response on the SIDR list. Let us see if anyone else comments.
         
For completeness, please add a paragraph to the end of Section 3.1 that describes the 4-octet AS Number field.  Please state how an old 16-bit AS number is carried.

[Sriram] Done.

In Section 3.2, the Signature Length within the Signature Segment does not count the length field itself.  It seems that all of the other length values in this specification count the size of the length too.
Consistency will avoid implementation errors.

[Sriram] I have followed your suggestion to put this out on the SIDR list for Comments. 
Please see Oliver's response on the SIDR list. Let us see if anyone else comments.

Section 4.2 references the MP_REACH_NLRI attribute; please add a citation to the RFC that defines that attribute.


[Sriram]  Done. RFC7460 added as citation.

Nits

Section 2.1 says:

   The second and third octets contain the 16-bit Address Family
   Identifier (AFI) which indicates the address family for which the
   BGPsec speaker is advertising support for BGPsec.  This document only
   specifies BGPsec for use with two address families, IPv4 and IPv6,
   AFI values 1 and 2 respectively.  BGPsec for use with other address
   families may be specified in future documents.

Please reference RFC 4760 for a place to learn about AFI.

[Sriram]  Done.

In Section 4.1, the comma is in the wrong place, but when I tried to offer a correction, I thought that a slight rewording would also improve the clarity:

OLD:

   If a BGPsec router has received only a non-BGPsec update message
   (without the BGPsec_Path attribute), containing the AS_PATH
   attribute, from a peer for a given prefix then it MUST NOT ...

NEW:

   If a BGPsec router has received only a non-BGPsec update message
   containing the AS_PATH attribute (instead of the BGPsec_Path
   attribute) from a peer for a given prefix, then it MUST NOT ...

[Sriram]  Done. Thank you for providing the better wording for this.

Please reword the first paragraph of Section 4.2 to avoid the phrase "said route advertisement".  While it is not wrong, it does feel very awkward.  This is not a patent claim.

[Sriram]  Done. Now the document avoids patent-like usage of "said". 

The acronym "ASN" is not used until Section 4.4, and it is not expanded there.  I suggest that all uses of ASN should be expanded to AS number.

[Sriram]  Done.