Subject: Re: [sidr] draft-ietf-sidr-bgpsec-protocol

From: Keyur Patel <[email-masked]>
Date: Wed, 4 Jan 2017 22:53:01 +0000

[adding routing-ads]

From: Keyur Patel <[email-masked]>
Date: Wednesday, January 4, 2017 at 2:17 PM
To: Jonathan Hardwick <[email-masked]>, "Alvaro Retana (aretana)" <[email-masked]>, Zhangxian Xian <[email-masked]>, Jon Hudson <[email-masked]>
Cc: rtg-dir <[email-masked]>, "[email-masked]" <[email-masked]>, sidr <[email-masked]>, "Sriram, Kotikalapudi (Fed)" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: draft-ietf-sidr-bgpsec-protocol

Hello,

Apologies for the delayed response.

I have been selected as the Routing Directorate QA reviewer for draft-ietf-sidr-bgpsec-protocol.

The Routing Directorate QA reviews are intended to be a support to improve the quality of RTG Area documents as they pass through the IETF process. This is the QA review at the time of the WG document adoption poll.

Summary:

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.
The document is well written, easy to read and follow. Some minor comments are listed below:

Comments for the authors:

1)      Section 4.1 “The BGPsec Path attribute and the AS_PATH attribute are mutually exclusive. That is, any update message containing the BGPsec Path attribute MUST NOT contain the AS_PATH attribute”.  For any restarting speakers in a GR mode, where the bgp capability is not exchanged, the existing stale routes won’t have an AS_PATH attribute. We could add some clarifying that helps to indicate that such routes should be considered valid in stale mode (till they get refreshed)?

2)       4.1 4th paragraph: “Note also that new signatures are only added to a BGPsec update message when a BGPsec speaker is generating an update message to send to an external peer (i.e., when the AS number of the peer is not equal to the BGPsec speaker's own AS number).  Therefore, a BGPsec speaker who only sends BGPsec update messages to peers within its own AS does not need to possess any private signature keys.” This text doesn’t seem to apply to confed peers? If so, it would be nice to clarify that this text doesn’t apply to any confed peers.

3)      Section 5 and Section 5.2, 1st paragraph: RFC4271 considers update message received without a wellknown AS_PATH attribute as an error.  We need some text to clarify the (error handling if any) behavior when an update message is received without a bgpsec and an aspath attribute. The current draft text seems unclear about generation of bgpsec attribute as well (in a ibgp scenario). Is it a requirement to generate an empty bgpsec attribute?

4)      With an AS_PATH attribute in 4271 there was loop detection in place.  With BGPSec I don’t see that being called explicitly other than a passing remark in section 5. Section 5.2 should have a check that allows a BGPsec speaker to bail out of a validation procedure when a aspath loop is detected.

Best Regards,
Keyur

---

From: Randy Bush <[email-masked]>
Date: Fri, 06 Jan 2017 07:54:49 +0900

did i miss the response to this?  i think some of the points are
important.

randy

---

From: "Sriram, Kotikalapudi (Fed)" <[email-masked]>
Date: Fri, 6 Jan 2017 05:40:09 +0000

Keyur,

Thank you for taking the time to read and offer comments. 
I find the comments very insightful and helpful. 
My comments are marked with [Sriram] below.

>From: Keyur Patel [email-masked]

>Sent: Wednesday, January 4, 2017 5:53 PM

>The document is well written, easy to read and follow.
 
>Some minor comments are listed below:

>1)  Section 4.1 “The BGPsec Path attribute and the AS_PATH attribute are mutually exclusive. That is, any update message containing the BGPsec Path attribute MUST NOT contain the AS_PATH attribute”.…

[Sriram]  As you have clarified for me on the phone, what you are saying here is that the two BGPsec peers lost the BGPsec session and now restarting in GR mode, but they have not exchanged BGPsec capability this time. Hence, they are now simple BGP (non-BGPsec) peers in GR mode. RFC4271 considers update message received without a well-known AS_PATH attribute as an error, and unfortunately in this case the cached BGPsec updates do not have AS_PATH (albeit they have BGPsec_Path). So you are saying "the router should not panic" and instead simply treat each cached update as NOT-IN-ERROR even though it is missing AS_PATH attribute. This way the GR can work properly. Of course, shortly the updates will have AS_PATH (and not considered in error) when they get refreshed (over the new simple BGP session). Per your suggestion, I will include new text in Section 7 to describe this required behavior for the GR mode.         

>2)   4.1 4th paragraph: "Note also that new signatures are only added to a BGPsec update message when a BGPsec speaker is generating an update message to send to an external peer (i.e., when the AS number of the peer is not equal to the BGPsec speaker's own AS number).  Therefore, a BGPsec speaker who only sends BGPsec update messages to peers within its own AS does not need to possess any private signature keys." This text doesn't seem to apply to confed peers? If so, it would be nice to clarify that this text doesn't apply to any confed peers.

[Sriram] You have clarified in our phone conversation that you consider the inter-AS-member sessions as "iBGP" since they are all within a confederation AS domain. The BGPsec document considers the inter-AS-member sessions as "eBGP" (not "iBGP") and intra-Member-AS sessions as "iBGP".  You also clarified that you may call inter-AS-member sessions as "confederation-eBGP" sessions. Obviously, private key is required to sign over such "confederation-eBGP/BGPsec" sessions. I understand your point. I will put in new text (notes) to clarify this in the document.

>3)  Section 5 and Section 5.2, 1st paragraph: RFC4271 considers update message received without a well-known AS_PATH attribute as an error.  We need some text to clarify the (error handling if any) behavior when an update message is received without a bgpsec and an aspath attribute. The current draft text seems unclear about generation of bgpsec attribute as well (in a ibgp scenario). Is it a requirement to generate an empty bgpsec attribute?

[Sriram]  As you have clarified for me over the phone, RFC 4271 (page 26) says the following :

   "When a BGP speaker originates a route then:
   b) the originating speaker includes an empty AS_PATH attribute in
         all UPDATE messages sent to internal peers.  (An empty AS_PATH
         attribute is one whose length field contains the value zero)."

[Sriram]  So what needs to be said in the BGPsec document is the following:  The BGPsec_Path attribute is not attached in updates originated inside an AS and propagated to BGPsec capable internal peers. However, when a route is originated inside an AS and propagated to non-BGPsec internal peers, an empty AS_PATH attribute is included in the update (see [RFC 4271], page 26).

>4)   With an AS_PATH attribute in 4271 there was loop detection in place.  With BGPSec I don’t see that being called explicitly other than a passing remark in section 5. Section 5.2 should have a ch…

[Sriram]  I agree. I will include loop detection in the list of error checks in Section 5.2.

Sriram

---

From: "Sriram, Kotikalapudi (Fed)" <[email-masked]>
Date: Fri, 6 Jan 2017 16:27:50 +0000

My previous post of this message seems to have line wrap issue.

Resending ... hopefully this avoids that issue.   - Sriram

-------------------

Keyur,

Sriram

---

From: "Alvaro Retana (aretana)" <[email-masked]>
Date: Fri, 6 Jan 2017 23:10:17 +0000

On 1/6/17, 12:40 AM, "Sriram, Kotikalapudi (Fed)" <[email-masked]> wrote:

[Cut the distribution list a little.]

Sriram:

Hi!  Happy New Year!

I have some comments on this, please see below.

Thanks!

Alvaro.

…

| | 1)  Section 4.1 “The BGPsec Path attribute and the AS_PATH attribute are mutually

| | exclusive. That is, any update message containing the BGPsec Path attribute MUST NOT

| | contain the AS_PATH attribute”.  For any restarting speakers in a GR mode, where the bgp

| | capability is not exchanged, the existing stale routes won’t have an AS_PATH attribute. We

| | could add some clarifying that helps to indicate that such routes should be considered

| | valid in stale mode (till they get refreshed)?

|

| [Sriram]  As you have clarified for me on the phone, what you are saying here is that the

| two BGPsec peers lost the BGPsec session and now restarting in GR mode, but they have not

| exchanged BGPsec capability this time. Hence, they are now simple BGP (non-BGPsec) peers

| in GR mode. RFC4271 considers update message received without a well-known AS_PATH

| attribute as an error, and unfortunately in this case the cached BGPsec updates do not have

| AS_PATH (albeit they have BGPsec_Path). So you are saying "the router should not panic"

| and instead simply treat each cached update as NOT-IN-ERROR even though it is missing

| AS_PATH attribute. This way the GR can work properly. Of course, shortly the updates will

| have AS_PATH (and not considered in error) when they get refreshed (over the new simple

| BGP session). Per your suggestion, I will include new text in Section 7 to describe this

| required behavior for the GR mode.

I don’t have an objection for this behavior, but I think we should make the WG (and idr!) aware of the change and get their comments (if any) before I approve the publication.

…

| | 3)  Section 5 and Section 5.2, 1st paragraph: RFC4271 considers update message received

| | without a well-known AS_PATH attribute as an error.  We need some text to clarify the

| | (error handling if any) behavior when an update message is received without a bgpsec and

| | an aspath attribute. The current draft text seems unclear about generation of bgpsec

| | attribute as well (in a ibgp scenario). Is it a requirement to generate an empty bgpsec

| | attribute?

|

| [Sriram]  As you have clarified for me over the phone, RFC 4271 (page 26) says the

| following :

|

|  "When a BGP speaker originates a route then:

|   b) the originating speaker includes an empty AS_PATH attribute in

|        all UPDATE messages sent to internal peers.  (An empty AS_PATH

|         attribute is one whose length field contains the value zero)."

|

|

| [Sriram]  So what needs to be said in the BGPsec document is the following:  The

| BGPsec_Path attribute is not attached in updates originated inside an AS and propagated to

| BGPsec capable internal peers. However, when a route is originated inside an AS and

| propagated to non-BGPsec internal peers, an empty AS_PATH attribute is included in the

| update (see [RFC 4271], page 26).

The Route Selection Section (9.1.2) in RFC4271 is not explicit about performing loop detection only on eBGP sessions – the criteria is generic to any route, so there is a possibility that a BGPsec-capable router may want to perform loop detection on an iBGP-received Update.  Given this text from Section 5 in the BGPsec spec:

   Whenever the use of AS path information is called for

   (e.g., loop detection, or use of AS path length in best path

   selection) the externally visible behavior of the implementation

   shall be the same as if the implementation had run the algorithm in

   Section 4.4 and used the resulting AS_PATH attribute as it would for

   a non-BGPsec update message.

…how should an iBGP speaker perform loop detection if there’s no BGPsec_Path attribute?  In other words, there is no defined mechanism to run the algorithm in 4.4 without it.

I’m not suggesting that you include an empty attribute, but that you indicate in 4.4 that no BGPsec_Path attribute is equivalent to an empty AS_PATH.

Thanks!

Alvaro.

---

From: Randy Bush <[email-masked]>
Date: Sat, 07 Jan 2017 10:32:22 +0900

re keyur's point 4.  the issue is that 4271 has semantics of AS_PATH,
bgpsec replaces it with BGPSEC_PATH, but bgpsec does not explicitly
repeat things such as loop detection using BGPSEC_PATH.  instead of this
becoming another text explosion, a simple statement that, in bgpsec, the
following AS_PATH semantics of 4271: <list>, hold analogously for
BGPSEC_PATH.

randy

---

From: Keyur Patel <[email-masked]>
Date: Wed, 11 Jan 2017 16:54:02 +0000

Hi Alvaro,

Comments inlined #Keyur

From: "Alvaro Retana (aretana)" <[email-masked]>
Date: Friday, January 6, 2017 at 3:10 PM
To: "Sriram, Kotikalapudi (Fed)" <[email-masked]>, Keyur Patel <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, sidr <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re: draft-ietf-sidr-bgpsec-protocol

Sriram:

Hi!  Happy New Year!

Thanks!

Alvaro.

…

|

#Keyur: Ack. Though I was only requesting some text clarification so that it is very clear to the implementers.

Regards,

Keyur

…

| | attribute?

|

| following :

|

|

|

Thanks!

Alvaro.

---

From: "Sriram, Kotikalapudi (Fed)" <[email-masked]>
Date: Tue, 17 Jan 2017 03:10:25 +0000

Hi Alvaro,

... snip ...
Alvaro wrote:
>>I don’t have an objection for this behavior, but I think 
>>we should make the WG (and idr!) aware of the change 
>>and get their comments (if any) before I approve the publication.

Keyur responded:
>#Keyur: Ack. Though I was only requesting some text clarification 
>so that it is very clear to the implementers.

So there was no change required as Keyur points out. 
Oliver also agreed with Keyur's observation when I ran this by him last week.
Per Keyur's request, I have added the following text clarification in Section 7:

   During Graceful Restart (GR), restarting and receiving BGPsec
   speakers MUST follow the procedures specified in [RFC4724] for
   restarting and receiving BGP speakers, respectively.  In particular,
   the behavior of retaining the forwarding state for the routes in the
   Loc-RIB [RFC4271] and marking them as stale as well as not
   differentiating between stale and other information during forwarding
   will be the same as specified in [RFC4724].

...snip...
Alvaro wrote:
>>…how should an iBGP speaker perform loop detection 
>>if there’s no BGPsec_Path attribute?  In other words, 
>>there is no defined mechanism to run the algorithm in 4.4 without it.
>>
>>I’m not suggesting that you include an empty attribute, 
>>but that you indicate in 4.4 that no BGPsec_Path attribute 
>>is equivalent to an empty AS_PATH.

Per your suggestion, I have added the following text in Section 4.4: 

   Finally, one special case of reconstruction of AS_PATH is when the
   BGPsec_Path attribute is absent.  As explained in Section 4.1, when a
   BGPsec speaker originates a prefix and sends it to a BGPsec-capable
   iBGP peer, the BGPsec_Path is not attached.  So when received from a
   BGPsec-capable iBGP peer, no BGPsec_Path attribute in a BGPsec update
   is equivalent to an empty AS_PATH [RFC4271].

Please let me know if you have any comments/questions. 

Thank you.

Sriram

---

From: "Alvaro Retana (aretana)" <[email-masked]>
Date: Tue, 17 Jan 2017 15:49:23 +0000

Sriram:

Hi!

Yes, I think I may have read more into Keyur’s comment than what he was asking for, so I think we’re ready to go. ☺

I’ll approve publication.

Thanks!!

Alvaro.

On 1/16/17, 10:10 PM, "Sriram, Kotikalapudi (Fed)" <[email-masked]<mailto:[email-masked]>> wrote:

Hi Alvaro,

... snip ...
Alvaro wrote:

Keyur responded:

...snip...
Alvaro wrote:

Thank you.

Sriram