ietf-wg-idr / draft-ietf-idr-bgp-ct

1 stars 2 forks source link

Joel Halpern's review #21

Closed suehares closed 1 year ago

suehares commented 1 year ago

Section 14: Moderate issue: Section 14.1 refers to using the DSCP in a forwarding decision. I am sure that is a not-uncommon practice. However, it is a direct violation of the defining RFC. This contradiction should at least be addressed. Section 14.2 refers to using an MPLS label to communicate the classification between the CE and the PE, and carrying that in BGP. That makes sense, if one assumes that MPLS label stacks are allowed from the customer. This is quite unusual, and introduces significant security issues if it is allowed. I expect they can be addressed, but this should be discussed. Minor but confusing issues: Section 14.1 refers to the BGP MultiNextHop Attribute draft. First, that draft seems to have expired. Second, the important part of the reference is to the carriage of DSCP which it says is described in section 5.5.3.4. But there is no section 5.5.3.4

Section 20: Moderate: I think I figured out what section 20.1.3 is doing. But I am wondering, if these are really coordinating domains, wouldn't it be simpler in domains AS1 and AS3, to advertise gold1 and gold2, and simply have them both point to the gold tunnels? No custom resolution, no special handling? Is there a reason that doesn't work? I think I missed something towards the end of section 20.2.1. If so, clarification would likely help. The text says "ASBR21 readvertises the BGP LU route for endpoint ASBR11_lpbk_gold to ASBR22 with nexthop set to the unique loopback ASBR21_lpbk_gold representing gold SLA." Given that ASBR21 does not understand CT, by what means does it know to set the nexthop to ASBR21_lpbk_gold? Minor: In order to understand section 20, one needs section 6 (in and of itself, fine.) However, the beginning of section 6 needs some wordsmithing. The first paragraph treats as given that one knows how resolution of s service route to a primary color is done. Only after reading onward does one realize that this section is defining that resolution. Please clarify the opening. Section 20.1.2 in describing the non-cooperating domains says "Though the color values are different, they map to tunnels with the same TE characteristics in each domain." If these are not closely coupled domains, it seems likely that the TE characteristics used in them will not be identical. Should "the same TE characteristics" be replaced by "sufficiently similar TE characteristics"? It is possible that the draft intends to require identical TE. If so, then we should also have a description of heterogeneous non-agreeing color transport domains.

kalirajv commented 1 year ago

From: Kaliraj Vairavakkalai kaliraj@juniper.net Date: Friday, June 16, 2023 at 2:45 PM To: Joel Halpern jmh.direct@joelhalpern.com, Susan Hares shares@ndzh.com, Natrajan Venkataraman natv@juniper.net, Reshma Das dreshma@juniper.net Subject: Re: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - CE/PE MPLS issue Thanks Joel. Added a mention in sec 22 as-well.

https://raw.githubusercontent.com/ietf-wg-idr/draft-ietf-idr-bgp-ct/main/draft-ietf-idr-bgp-ct.txt

With this, I think all your comments are addressed. Please let us know if anything is pending.

We will push the changes so far to IETF soon.

Thanks Kaliraj

From: Joel Halpern jmh.direct@joelhalpern.com Date: Friday, June 16, 2023 at 5:50 AM To: Kaliraj Vairavakkalai kaliraj@juniper.net, Susan Hares shares@ndzh.com, Natrajan Venkataraman natv@juniper.net, Reshma Das dreshma@juniper.net Subject: Re: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - CE/PE MPLS issue [External Email. Be cautious of content]

14.2 That looks good. I would include a short version of the "can't inspect the insides" in 22 so that the security issue is mentioned where people would look for it. With that addition, I think 22 is also sufficient. Yours, Joel PS: I understand why you want to allow the behavior, I just want to make sure we describe the tradeoffs fairly. On 6/16/2023 5:48 AM, Kaliraj Vairavakkalai wrote: Hi Joel, agree with your assessment.

https://raw.githubusercontent.com/ietf-wg-idr/draft-ietf-idr-bgp-ct/main/draft-ietf-idr-bgp-ct.txt

I have re-written sec 14.2 and 22. Please take a look.

I think this lean-fib method cannot provide inner IP packet based checks, since it is doing mpls based forwarding. Those checks may be offloaded to a downstream box like a common firewall.

Thanks Kaliraj

Juniper Business Use Only From: Joel Halpern jmh.direct@joelhalpern.com Date: Thursday, June 15, 2023 at 9:47 AM To: Kaliraj Vairavakkalai kaliraj@juniper.net, Susan Hares shares@ndzh.com, Natrajan Venkataraman natv@juniper.net, Reshma Das dreshma@juniper.net Subject: Re: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - CE/PE MPLS issue [External Email. Be cautious of content]

Sorry, misunderstood where to look for these changes. WHile they improve the situation, I do not believe an MPLS context tables suffice. At the very least, it seems that you need to ensure there are no other labels on the packet after the permitted one. Otherwise, unexpected things can happen when the packet reaches the egress. I also wonder if there should be an actual check that the MPLS content is an IP packet, as otherwise, the packet might tickly interesting bugs in people's devices. For that matter, shouldn't that inner IP packet be checked for any other rules the PE applies to incoming packets from the customer? Yours, Joel On 6/15/2023 3:29 AM, Kaliraj Vairavakkalai wrote:

We're working on updating text for this one. Basically, Label-spoofing protection mechanisms using MPLS context tables can be used to secure this MPLS-interface towards untrusted domains.

I updated text for this issue as-well, in sec 14.2 and sec 22. Pls take a look.

Thanks Kaliraj

Juniper Business Use Only From: Kaliraj Vairavakkalai kaliraj@juniper.net Date: Wednesday, June 14, 2023 at 10:48 PM To: Joel M. Halpern jmh@joelhalpern.com, Susan Hares shares@ndzh.com, Natrajan Venkataraman natv@juniper.net, Reshma Das dreshma@juniper.net Subject: Re: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - by Hi Joel,

Please find the latest version of the ct draft with edits taking care of most of your comments.

https://github.com/ietf-wg-idr/draft-ietf-idr-bgp-ct/blob/main/draft-ietf-idr-bgp-ct.txt

For some issues, we have clarification questions. Pls see inline and reply:

  1. Section 14.1 refers to using the DSCP in a forwarding decision. I am sure that is a not-uncommon practice. However, it is a direct violation of the defining RFC. This contradiction should at least be addressed.

Could you give a pointer to which text is being violated please?

  1. BGP. That makes sense, if one assumes that MPLS label stacks are allowed from the customer. This is quite unusual, and introduces significant security issues if it is allowed.

We're working on updating text for this one. Basically, Label-spoofing protection mechanisms using MPLS context tables can be used to secure this MPLS-interface towards untrusted domains.

  1. Section 14.1 refers to the BGP MultiNextHop Attribute draft. First, that draft seems to have expired. Second, the important part of the reference is to the carriage of DSCP which it says is described in section 5.5.3.4. But there is no section 5.5.3.4

We updated the MNH draft. Sorry, I had a private change that I forgot to publish to IETF. Please look now, the section numbers are current.

  1. I think I figured out what section 20.1.3 is doing. But I am wondering, if these are really coordinating domains, wouldn't it be simpler in domains AS1 and AS3, to advertise gold1 and gold2, and simply have them both point to the gold tunnels? No custom resolution, no special handling? Is there a reason that doesn't work?

About this one, it is atually that. Just that we use the resolution schemes mechanism to achieve it, to point the gold1/gold2 to the gold tunnels. If we dont use custom resolution schemes, then color 101 routes will look for color 101 tunnels. The custom resolution scheme indicates color 101 routes to look for color 100 tunnels instead. Without that, it wouldn't work. Does that answer your question?

  1. I think I missed something towards the end of section 20.2.1. If so, clarification would likely help.

We updated the text for this section. Please take a look.

  1. In order to understand section 20, one needs section 6 (in and of itself, fine.) However, the beginning of section 6 needs some wordsmithing

We updated the text for this section. Please take a look.

  1. Section 20.1.2 in describing the non-cooperating domains says...be replaced by "sufficiently similar TE characteristics"?..

We updated the text for this section. Please take a look.

Thanks Kaliraj

From: Susan Hares shares@ndzh.com Date: Tuesday, June 13, 2023 at 4:26 PM To: Kaliraj Vairavakkalai kaliraj@juniper.net, Natrajan Venkataraman natv@juniper.net, Reshma Das dreshma@juniper.net Subject: FW: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - by [External Email. Be cautious of content]

Review by Joel Halpern.

Sue

From: Joel Halpern jmh@joelhalpern.com Sent: Tuesday, June 13, 2023 12:16 PM To: Susan Hares shares@ndzh.com; Ketan Talaulikar ketant.ietf@gmail.com; Alvaro Retana aretana.ietf@gmail.com Subject: Re: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - by

Section 14: Moderate issue: Section 14.1 refers to using the DSCP in a forwarding decision. I am sure that is a not-uncommon practice. However, it is a direct violation of the defining RFC. This contradiction should at least be addressed. Section 14.2 refers to using an MPLS label to communicate the classification between the CE and the PE, and carrying that in BGP. That makes sense, if one assumes that MPLS label stacks are allowed from the customer. This is quite unusual, and introduces significant security issues if it is allowed. I expect they can be addressed, but this should be discussed. Minor but confusing issues: Section 14.1 refers to the BGP MultiNextHop Attribute draft. First, that draft seems to have expired. Second, the important part of the reference is to the carriage of DSCP which it says is described in section 5.5.3.4. But there is no section 5.5.3.4

Section 20: Moderate: I think I figured out what section 20.1.3 is doing. But I am wondering, if these are really coordinating domains, wouldn't it be simpler in domains AS1 and AS3, to advertise gold1 and gold2, and simply have them both point to the gold tunnels? No custom resolution, no special handling? Is there a reason that doesn't work? I think I missed something towards the end of section 20.2.1. If so, clarification would likely help. The text says "ASBR21 readvertises the BGP LU route for endpoint ASBR11_lpbk_gold to ASBR22 with nexthop set to the unique loopback ASBR21_lpbk_gold representing gold SLA." Given that ASBR21 does not understand CT, by what means does it know to set the nexthop to ASBR21_lpbk_gold? Minor: In order to understand section 20, one needs section 6 (in and of itself, fine.) However, the beginning of section 6 needs some wordsmithing. The first paragraph treats as given that one knows how resolution of s service route to a primary color is done. Only after reading onward does one realize that this section is defining that resolution. Please clarify the opening. Section 20.1.2 in describing the non-cooperating domains says "Though the color values are different, they map to tunnels with the same TE characteristics in each domain." If these are not closely coupled domains, it seems likely that the TE characteristics used in them will not be identical. Should "the same TE characteristics" be replaced by "sufficiently similar TE characteristics"? It is possible that the draft intends to require identical TE. If so, then we should also have a description of heterogeneous non-agreeing color transport domains.

Yours, Joel

kalirajv commented 1 year ago

From: Joel Halpern jmh.direct@joelhalpern.com Date: Friday, June 16, 2023 at 3:10 PM To: Kaliraj Vairavakkalai kaliraj@juniper.net, Susan Hares shares@ndzh.com, Natrajan Venkataraman natv@juniper.net, Reshma Das dreshma@juniper.net Subject: Re: Early review of section 14 and 20 of draft-ietf-idr-bgp-ct-04 - CE/PE MPLS issue [External Email. Be cautious of content]

Thanks. You have addressed my concerns. Yours, Joel On 6/16/2023 5:45 PM, Kaliraj Vairavakkalai wrote: Thanks Joel. Added a mention in sec 22 as-well.

https://raw.githubusercontent.com/ietf-wg-idr/draft-ietf-idr-bgp-ct/main/draft-ietf-idr-bgp-ct.txt

With this, I think all your comments are addressed. Please let us know if anything is pending.

We will push the changes so far to IETF soon.

Thanks Kaliraj

suehares commented 1 year ago

This item is closed with the publication of draft-ietf-idr-bgp-ct-12

suehares commented 1 year ago

closing issue