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

From: Russ Housley <[email-masked]>
Date: Thu, 8 Dec 2016 11:26:26 -0500

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.

---

From: "Sriram, Kotikalapudi (Fed)" <[email-masked]>
Date: Thu, 22 Dec 2016 15:50:55 +0000

Hi Russ,

Thank you for your comments.
I am preparing a version 21 of the document to be submitted soon.
I have updated the document already to include all your comments 
except the following thee:
(need your further guidance on these)

#1

[Sriram] I agree. However, this change would require modifications with two existing
implementations. If you say it doesn't matter, just do it; let the implementations catch up --
then sure I will have no problem. I'll make the change in the spec. 

#2

[Sriram] I spoke with Oliver Borchert. He thought there was some merit to
implementing it the way it currently is in the spec
(he already has the implementation in Quagga BGPsec).
This change would also require modifications with the two existing
implementations (NIST's and Parson's). 
Again, if you feel strongly, please let me know.

#3

[Sriram] Is this not already taken care of by using the same approach that is
described in Section 6.2?  Section 6.2 says:

"In the case that such a change to BGPsec were deemed desirable, it is
   expected that a subsequent version of BGPsec would be created and
   that this version of BGPsec would specify a new BGP path attribute,
   let's call it BGPsec_Path_Two, which is designed to accommodate the
   desired changes to BGPsec."

[Sriram] BGPsec_Path_Two structure can be anything you want. 
This is no way tied to computing a digest and signing it.
It can be designed/structured specifically for the "signature that works differently". No?   

For the first two (#1 and #2), perhaps I can wait a bit and make the changes 
closer to the final publication of the RFC (in January).
(I mean we perhaps need not rush at this time to make these two changes?)

Regards,
Sriram

---

From: Russ Housley <[email-masked]>
Date: Thu, 22 Dec 2016 11:31:23 -0500

> then sure I will have no problem. I'll make the change in the spec. 

Better to point this out on the mail list and let the implementors have a say.

> Again, if you feel strongly, please let me know.

Again, please point this out on the mail list and let the implementors have a say.

>   desired changes to BGPsec.”

When I read that paragraph, I did not think it was talking about the hash and sign paradigm.

Russ

---

From: "Sriram, Kotikalapudi (Fed)" <[email-masked]>
Date: Wed, 28 Dec 2016 17:30:53 +0000

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:[email-masked]] 
Sent: Thursday, December 08, 2016 11:26 AM
To: [email-masked]
Cc: IETF SecDir <[email-masked]>
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.

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

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:

[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

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:

[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:

[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:

NEW:

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

---

From: Russ Housley <[email-masked]>
Date: Wed, 28 Dec 2016 12:46:22 -0500

Sriram:

Thanks for your very comprehensive response to my review.  I think it is ready to move forward,

Russ