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

[Benjamin Kaduk via Datatracker <[e-mail]>]

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.


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

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.

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)?

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.

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.

   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.

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.

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.

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.

   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.

Section 4.12

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

   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/?

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

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

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

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.

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

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.

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.

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.

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.

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.

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

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.