Re: [spring] Benjamin Kaduk's Discuss on draft-ietf-spring-srv6-network-programming-26: (with DISCUSS and COMMENT)

["Pablo Camarillo (pcamaril)" <[e-mail]>]

Hi Ben,

I know it’s a busy time of the year, but I'm hopeful for an early Christmas present reply. ;-)
Whenever you have some time, can you please verify whether the rev27 (posted one week ago) addresses your last DISCUSS point? I believe it should be addressed as we added your proposed text to the document. 

Many thanks,
Pablo.

-----Original Message-----
From: Pablo Camarillo (pcamaril) <[e-mail]> 
Sent: jueves, 10 de diciembre de 2020 19:02
To: Benjamin Kaduk <[e-mail]>; The IESG <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; Bruno Decraene <[e-mail]>; Joel Halpern <[e-mail]>
Subject: RE: Benjamin Kaduk's Discuss on draft-ietf-spring-srv6-network-programming-26: (with DISCUSS and COMMENT)

Hi Ben,

Many thanks for your time on this document.
We’ve published rev27 of the document with the changes.  Details inline with [PC].
https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming-27

Cheers,
Pablo.

-----Original Message-----
From: Benjamin Kaduk via Datatracker <[e-mail]> 
Sent: miércoles, 9 de diciembre de 2020 18:05
To: The IESG <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; Bruno Decraene <[e-mail]>; Joel Halpern <[e-mail]>; [e-mail]
Subject: Benjamin Kaduk's Discuss on draft-ietf-spring-srv6-network-programming-26: (with DISCUSS and COMMENT)

Benjamin Kaduk has entered the following ballot position for
draft-ietf-spring-srv6-network-programming-26: Discuss

When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-spring-srv6-network-programming/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Thanks for the updates through to the -26; they help a lot.
However, I do think there is one final Discuss-level point that needs to be resolved: it's mostly a process point, to make sure that what we say in this document complies to the requirements that were laid out in RFC
8754 for the procedure we're trying to follow.  Specifically, in the process of trying to finalize my review comments, I ended up doing a lot of background reading, in which I noticed that RFC 8754 says:

   New SIDs defined in the future MUST specify the mutability properties
   of the Flags, Tag, and Segment List and indicate how the Hashed
   Message Authentication Code (HMAC) TLV (Section 2.1.2) verification
   works.  Note that, in effect, these fields are mutable.

This is a bit confusing to me, in that the SIDs themselves appear as entries in the Segment List, and it's not quite clear when or how a per-SID behavior relating to fields in the containing SRH might come into play.  However, given that we allocate a behavior codepoint for "the SID defined in RFC 8754", I am forced to conclude that the behaviors specified in this document meet the definition of "new SIDs"
that are being defined "in the future" (from the reference point of RFC 8754), and therefore that they must specify the indicated properties.
I'm told out of band that the intent is to do the same thing that RFC
8754 does for the SID it defines, and so this should be trivial to resolve just by adding a brief note that (e.g.) "the SIDs specified in this document have the same HMAC TLV handling and mutability properties of the Flags, Tag, and Segment List field as the SID specified in RFC 8754".  However, I believe that such an explicit statement is required, and that we would introduce an internal inconsistency between this document and RFC 8754 if we say nothing on this topic.  In particular, I think that we would not inherit that behavior as some kind of default behavior if we make no statement at all.

I am sorry that I did not notice this earlier, but I feel that it is important to remain consistent with the requirements of RFC 8754 and thus that this is appropriate to raise as a Discuss-level point, even if I have previously reviewed the text in question.

[PC] We have added your suggested sentence to the Section 9. (I’ve s/SIDs/SID Behaviors/). Thanks.
<NEW>
The SID Behaviors specified in this document have the same HMAC TLV handling and mutability properties of the Flags, Tag, and Segment List field as the SID Behavior specified in RFC 8754.
</NEW>

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

[note: I originally prepared these comments when looking at the -24.  I tried to remove comments about things fixed in the -25 or -26, but may have missed a couple; please ignore any such already-addressed comments.
There are also a couple points that we have already been discussing in the other thread but I retain as a "placeholder"; hopefully we can keep the actual discussion about them in just one place.]

As mentioned in the Discuss section, I did a lot of background reading while preparing this updated ballot position.  Another thing I noticed while doing that reading is that the pseudocode in
https://tools.ietf.org/html/rfc8754#section-4.3.1.1 explicitly mentions "perform TLV processing"; we might consider repeating that step in our pseudocode procedures, and otherwise making our procedures as analogous as possible to the RFC 8754 procedures, just from the perspective of keeping the writing style as consistent as possible across the related documents.  However, that's entirely an editorial matter and thus left to the discretion of the authors/AD.

[PC] Ack

Section 2

   The following terms used within this document are defined in
   [RFC8754]: SRH, SR Source Node, Transit Node, SR Segment Endpoint
   Node, Reduced SRH, Segments Left and Last Entry.

It's slightly unfortunate that 8754 didn't have a dedicated terminology section containing these, though it's too late to really do anything about it now.

[PC] Indeed, but nothing I can do about it...

Section 3

   The term "function" refers to the bit-string in the SRv6 SID.  The
   term "behavior" identifies the behavior bound to the SID.  The
   behaviors are defined in Section 4 of this document.

(nit) using "the behaviors" to some extent implies that these are the only ones allowed or defined, which is not true.  Perhaps "some behaviors" would be more accurate (or some other phrasing would also cover the expected evolution of the ecosystem)?

[PC] Changed to "some behaviors" as suggested.

Section 3.3

   A packet could be steered to a non-routed SID 2001:db8:b:2:101:: by
   using a SID list <...,2001:db8:b:1:100::,2001:db8:b:2:101::,...>
   where the non-routed SID is preceded by a routed SID to the same
   node.  Routed and non-routed SRv6 SIDs are the SRv6 instantiation of
   global and local segments, respectively [RFC8402].

If it's (also) possible to steer a packet to a non-routed SID without a preceding routed SID for the same node (e.g., via End.X), it seems like that might be worth listing an example of as well.  Otherwise a reader might assume that the global segment is a necessary part of using the non-routed SID.

[PC] Indeed. We've added the following. Thanks.
<OLD>
A packet could be steered to a non-routed SID 2001:db8:b:2:101:: by
   using a SID list <...,2001:db8:b:1:100::,2001:db8:b:2:101::,...>
   where the non-routed SID is preceded by a routed SID to the same
   node.  
</OLD>
<NEW>
A packet could be steered through a non-routed SID 2001:db8:b:2:101:: by
   using a SID list <...,2001:db8:b:1:100::,2001:db8:b:2:101::,...>
   where the non-routed SID is preceded by a routed SID to the same
   node.  A packet could also be steered to a node instantiating a 
   non-routed SID by preceding it in the SID-list with an Adjacency SID to that node.
</NEW>

Section 4

  End.DT2U           Endpoint with decaps and unicast MAC L2table lookup
                     e.g. EVPN Bridging unicast use-case

(nit) we seem to put a space in "L2 table" the other times it appears.

[PC] Fixed

   The list is not exhaustive.  In practice, any function can be
   attached to a local SID: e.g. a node N can bind a SID to a local VM
   or container which can apply any complex processing on the packet.

(nit) the preceding list was a list of well-known behaviors, not a list of functions.  IIUC, it is more appropriate to use "behavior" here than "function", since the "function" is just the opaque bitstring.

[PC] Indeed. Fixed.

Section 4.1.1, ...

   When processing the Upper-layer Header of a packet matching a FIB
   entry locally instantiated as an SRv6 End SID do the following:

(editorial, I think) I find it interesting to compare the phrasing here to what was used in §4.1 (when processing an SRH), where the text is "receives a packet whose IPv6 DA is S and S is a local End SID".  Why the distinction between "End SID" and 'SRv6 End SID"?  IIUC the distinction between checking "IPv6 DA" and "matching a FIB entry locally instantiated" is important and the language should not be harmonized between occurrences.
The "..." in the Section number listing indicates that this (or similar) phrasing appears throughout, whenever we talk about processing an upper-layer header.

[PC] The phrasing of 4.1.1 is the same as the one used in RFC8754.
[PC] The diff between "SRv6 End SID" and "End SID" is an editorial nit. I agree that should be harmonized. I've removed "SRv6" in all instances.

Section 4.2

   Note that if N has an outgoing interface bundle I to a neighbor Q
   made of 10 member links, N may allocate up to 11 End.X local SIDs:
   one for the bundle itself and then up to one for each Layer-2 member
   link.  The flows steered using the End.X SID corresponding to the

(nit) I think that while "up to 11" might be the situation that makes the most sense (in that having many distinct subgroups with 1 < n < 10 member links doesn't make sense), it is not strictly speaking a physical or protocol requirement.  Perhaps "might allocate 11" is better than "may allocate up to 11" for that reason.

[PC] Changed to might.

Section 4.4, ...

   When N receives a packet destined to S and S is a local End.DX6 SID,
   N does the following processing:

(nit) we have a mismatch of "N does the following processing" (like appears here) and "N does" or similar; it is probably worth normalizing on one phrasing.

[PC] Changed to "N does"

   When processing the Upper-layer header of a packet matching a FIB
   entry locally instantiated as an SRv6 End.DX6 SID, the following is
   done:

Similarly here, we use "the following is done" but the "N does the following" phrasing used in some other sections is probably preferred, as it avoids the passive voice.

[PC] Changed to "N does"

Section 4.12

We might give some mnemonic explanation for how the name "FE2" was chosen to identify the argument value.

[PC] Initially we referred to it as “L2 argument specific to EVPN ESI filtering”. I don’t see a straightforward mnemonic explanation.

   table T flooding.  The allocation of the argument values is local to
   the SR Endpoint Node instantiating this behavior and the signaling of
   the argument to other nodes for the EVPN functionality via control
   plane.

nit(?): s/via control plane/occurs via the control plane/?

[PC] Changed

Section 4.13

  S05.   If (IPv6 Hop Limit <= 1) {
  S06.       Send an ICMP Time Exceeded message to the Source Address,
               Code 0 (Hop limit exceeded in transit),

(nit) the indentation seems off by one space in the first line of S06 (it doesn't match the other two places where this chunk occurs).

[PC] Fixed

   S14.  The SRH MAY be omitted when the SRv6 Policy B only contains one
   SID and there is no need to use any flag, tag or TLV.
   S17.  The Payload Length, Traffic Class, Hop Limit and Next-Header
   fields are set as per [RFC2473].  The Flow Label is computed as per
   [RFC6437].

(These look to be S15 and S18, respectively, now.)

[PC] Fixed

Section 4.14

   The SRH MAY be omitted when the SRv6 Policy only contains one segment
   and there is no need to use any flag, tag or TLV.

(nit) it's probably worth harmonizing the phrasing between here and the note on S15 in §4.13 (specifically, "only contains one SID" vs "only contains one segment").

[PC] Indeed. Changed.

Section 4.15

(nit) there's a blank line at the end of S06 that doesn't occur in the other two locations where this pseudocode appears.

  S16.   Submit the packet to the MPLS engine for transmission to the
            topmost label.

(nit) I suggest rewording slightly so as to not imply that the node to transmit to is determined by the topmost label -- IIUC it's determined by the MPLS policy, since the interpretation of the label is in general local to the receiving node.

[PC] Removed "to the topmost label".

Section 4.16.1.2

   As a reminder, [RFC8754] defines in section 5 the SR Deployment Model
   within the SR Domain [RFC8402].  Within this framework, the
   Authentication Header (AH) is not used to secure the SRH as described
   in Section 7.5 of [RFC8754].

(repeating from the previous thread as a placeholder) I think we need another sentence or clause here to clarify why this statement is relevant, e.g., "Thus, while the AH can detect changes to the IPv6 header chain, it will not be used in combination with the SRH, so use of PSP will not cause delivery failure due to AH validation checks."

[PC] What about this?
<OLD>
Within this framework, the Authentication Header (AH) is not used to secure the SRH as described in Section 7.5 of [RFC8754].
</OLD>
<PROPOSAL>
Within this framework, the Authentication Header (AH) is not used to secure the SRH as described in Section 7.5 of [RFC8754]. Hence, the discussion of applicability of PSP along with AH usage is beyond the scope of this document.
</PROPOSAL>

Section 5

(editorial) This is the first place in the document that we talk about the "headend" or its policy at all, so a bit of background on why it's useful to tabulate potential headend policy/behaviors might be helpful.

[PC] I've added a reference to RFC8402.

Section 5.x

Tying the other policies more precisely to the pseudocode for H.Encaps (e.g., replacing S01) seems like it would be useful, to avoid the appearance of specifying behavior by appealing to examples.

Section 5.1

   Note:
   S03: As described in [RFC6437] (IPv6 Flow Label Specification).

We need to pull in RFC 2473 for payload length, traffic class, and next-header, IIUC.  (hop-limit is covered a few paragraphs down.) Also to say how the next-header value is selected.

[PC] Added reference to RFC2473.

Section 8.1

   The presence of SIDs in the IGP does not imply any routing semantics
   to the addresses represented by these SIDs.  The routing reachability
   to an IPv6 address is solely governed by the non-SID-related IGP
   prefix reachability information that includes locators.  Routing is
   neither governed nor influenced in any way by a SID advertisement in
   the IGP.

It seems like this is trying to say "the IGP must not advertise prefixes contained within the LOC part of an SID", but in a very roundabout way.

[PC] Not at all. This is saying that in your IGP you have two different things: prefix reachability information -including locators-; and SID advertisements. And routing in your network is derived from the first one.

Section 8.3

   The End.DX4, End.DX6, End.DT4, End.DT6, End.DT46, End.DX2, End.DX2V,
   End.DT2U and End.DT2M SIDs can be signaled in BGP.

Since we said earlier that the signaling of SIDs needs to include the behavior codepoint for each function bitstring, it seems like we should provide a reference to how BGP will encode the behavior codepoint.

[PC] Informative references to these other routing protocol drafts were removed as part of earlier review comments. We can add it back if Martin (responsible AD) wants us to do so.

Section 9

There seem to be some security considerations relating to the use of PSP, in that the egress node loses visibility into which policy was used for a given packet, so all packets from all policies that egress via that SID are in the same anonymity (and policy!) set.  In particular, even if an HMAC TLV was present in the SRH, it is not available and cannot be validated.  I recognize that the headend (or whatever entity is assigning SR policy) should know when such validation would be intended to occur and not assign a policy incompatible with it, but there are still new considerations in the sense that the headend needs to be aware of these considerations.

[PC] As part of the comments from Alvaro we added this text which I believe covers that point:
“The headend policy definition should be consistent with the specific
   behavior used and any local configuration”

(repeating as a placeholder from the previous thread) I think we should also say that in the absence of the HMAC TLV, valid FUNC and ARG values on any given node may be guessable and spoofable, along with the standard disclaimer that risks are minimal since all nodes in the SR domain are assumed to be trusted.  This is distinct from the already-extant ability to spoof a SID in that the underlying structure in the SID may allow the attacker to induce behavior that was never intended to be a SID, for example if the implementation logically separates FUNC and ARG processing and the attacker makes a combination that was never advertised.

[PC] I believe you are making an implementation assumption on how SIDs are matched at a node. As a reminder RFC8754 states that “When an SRv6-capable node receives an IPv6 packet, it performs a longest-prefix-match lookup on the packet's destination address.”. Thus, the ability to spoof a particular FUNCT is the same as the ability to spoof any other IP address on the node. In the context of SRv6, this is mitigated by using the SR Domain as described in RFC8754 and -in particular Section 7.2-.

Also, IIUC, the "Segments Left == 0" handling for, e.g., End.X is important to prevent traffic loops -- if a node fails to perform that check and blindly sends the packet to the interconnect it will get returned to that node/SID and loop until the IP hop limit is exhausted.

[PC] Indeed, following all the pseudocodes are important and not skipping instructions is important. But this could be said for many lines of pseudocode -not only for that check-. In the same line that it could be said that decrementing the IPv6 hop limit when processing an IPv6 packet is critical. In other words, it feels awkward to add a particular reminder only on that line and not on any other. All are important.