Last Call Review of draft-ietf-sidr-bgpsec-protocol-20
review-ietf-sidr-bgpsec-protocol-20-secdir-lc-housley-2016-12-08-00

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?


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.

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.

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.


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.

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.
         
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.

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.

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


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.

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 ...

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.

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