[mpls] Benjamin Kaduk's Discuss on draft-ietf-mpls-sfc-05: (with DISCUSS and COMMENT)

[Datatracker on behalf of Benjamin Kaduk <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-mpls-sfc-05: 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-mpls-sfc/



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

Thank you for this easy-to-read document!  The design is pretty elegant,
but I think there are a few places that remain insufficiently specified
so as to admit interoperable implementation.

In particular, I don't think the TTL-handling behavior for the SF Label
in the "label stacking" case is completely specified, though an
apparent typo in Section 7 (see COMMENT) makes it hard to tell what was
intended.

In Section 8, we see that:
   When an SFF receives a packet containing an MPLS label stack, it
   checks whether it is processing an {SFP, SI} label pair for label
   swapping or a {context label, SFI index} label pair for label
   stacking. [...]

What data source is consulted to make this check?  The control plane?
Is this distinction made on a per-node basis or a per-packet basis or
otherwise?  My first thought was that it's per-SFC-Context-Label, but
we're not guaranteed that those values will be interpretable
equivalently in the swapping or stacking usages, IIUC.

I also support Alissa's DISCUSS (and the secdir reviewer makes similar
comments).


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

Am I supposed to assume that the control plane sufficiently informs
everyone whether they're an SFF, SFI, or neither?  (Or does any given
(virtual) device just inherently know?)

Abstract (and Introduction)

   Multiprotocol Label Switching (MPLS) is a widely deployed forwarding
   technology that uses labels placed in a packet in a label stack to
   identify the forwarding actions to be taken at each hop through a
   network.  Actions may include swapping or popping the labels as well,
   as using the labels to determine the next hop for forwarding the
   packet.  [...]

nit: no comma after "as well".

   This document describes how Service Function Chaining can be achieved
   in an MPLS network by means of a logical representation of the NSH in
   an MPLS label stack.  That is, the NSH is not used, but the fields of
   the NSH are mapped to fields in the MPLS label stack.  It does not
   deprecate or replace the NSH, but acknowledges that there may be a
   need for an interim deployment of SFC functionality in brownfield
   networks.

Am I supposed to read this as that the NSH is the way of the future, and
this mechanism is just a temporary interim measure?

Section 4.3

   It may be that a service function chain (as described in Section 4.1
   allows some leeway in the choice of service function instances along
   the chain.  However, it may be that a service classifier wishes to
   constrain the choice and this can be achieved using chain
   concatenation so that the first chain ends at the point of choice,

This does not give any motivation for why a classifier might wish to
constrain the choice.

Section 5

The decision to use the control plane to indicate "label stacking" vs.
"label swapping" semantics as opposed to an in-band signal seems to
create a new opportunity for misconfiguration and consequent service
misbehavior.  I suppose it's not appreciably worse than any other way to
configure the interpretation of the label field, though.

   S: The bottom of stack bit has its usual meaning in MPLS.  It MUST be
      clear in the SFC Context label stack entry and MAY be set in the
      SF label stack entry depending on whether the label is the bottom
      of stack.

I don't understand why this is only a MAY.

Section 6

      Under normal circumstances (with the exception of branching and
      reclassification - see [I-D.ietf-bess-nsh-bgp-control-plane]) the

The word "reclassification" does not appear in the indicated reference.

   The following processing rules apply to the TTL field of the SF label
   stack entry, and are derived from section 2.2 of [RFC8300]:

nit: we seem to capitalize "SF Label"

Section 7

It's a bit unusual, style-wise, to have Section 6 introduce new terms
for the SPI Label and SI Label that are specific to the swapping usage,
and then have Section 7 just reuse the nominally generic terms from
Section 5 as its own for the stacking usage.

   SFC Context Label:  The Label field of the SFC Context label stack
      entry contains a label that delivers SFC context.  This label may
      be used to indicate the SPI encoded as a 20 bit integer using the
      semantics of the SPI is exactly as defined in [RFC8300] and noting
      that in this case a system using MPLS representation of the
      logical NSH MUST NOT assign SPI values greater than 2^20 - 1 or
      less than 16.  This label may also be used to convey other SFC
      context-speific semantics such as indicating how to interpret the
      SF Label or how to forward the packet to the node that offers the
      SF.

This "may also be used" behavior seems rather under-specified.

   TTL:  The TTL fields in the SFC Context label stack entry SF label
      stack entry SHOULD be set to 1 as stated in Section 5, but MAY be
      set to larger values if the label indicated a forwarding operation
      towards the node that hosts the SF.

What is "SFC Context label stack entry SF label stack entry"?  It seems
like there's a missing word or something.  I note that section 5 defers
to here for the TTL of the SF Label and we are either not saying
anything or attempting to defer to Section 5, so this seems
under-specified.

Section 8

   o  If the current hop requires an {SFP, SI} and the next hop requires
      an {SFP, SI}, it selects an instance of the SF to be executed at
      the next hop, sets the SI label to the SI value of the next hop,
      and tunnels the packet to the SFF for that SFI.

nit: I know the default behavior is to use the same SFP value, but (1)
this should probably be stated explicitly, and (2) we've already talked
about branching/etc. that could cause it to change.

      *  If the top of the MPLS label stack contains a {context label,
         SFI label}, it tunnels the packet to the SFF indicated by the
         context label.

nit: probably best to use "new top" for consistency with the preceding
sub-bullet.

Section 12.1

   The SFC Metadata Label (as a set of three labels as indicated in
   Figure 4) may be present zero, one, or more times in an MPLS SFC
   packet.  For MPLS label swapping, the SFC Metadata Labels are placed
   immediately after the basic unit of MPLS label stack for SFC as shown
   in Figure 5.  For MPLS label stacking, the SFC Metadata Labels can be
   present zero, one, or more times and are placed at the bottom of the
   label stack as shown in Figure 6.

The "may be present zero, one, or more times" appears twice.
(I actually don't mind the internal redundancy of that phrase here, though,
since all three cases are potentially relevant.)

Section 12.2

   Metadata Type:  The type of the metadata present.  Values for this
      field are taken from the "MD Types" registry maintained by IANA
      and defined in [RFC8300].

The "MD Types" registry I'm finding in RFC 8300 is defined to hold
four-bit values; why do we need a 16-bit field to hold it here in the
TLV?  (I mean, "to keep the metadata itself aligned", sure, but having
12 reserved bits would do that, too.)

Section 15

I agree with the secdir reviewer that the "trusted" nature of the
classifier as a "trusted resource" could be further clarified.

                                                 Where an SF is not
   encapsulation aware the encapsulation may be stripped by an SFC proxy

nit: "encapsulation-aware"

Thank you for the new text in the -05 prompted by the secdir review; it
is a huge improvement!
In addition to encryption, I'd probably also note that MPLS at present
doesn't provide any cryptographic integrity protection on the headers.

   o  The SFC-capable devices participating in an SFC system are
      responsible for verifying and protecting packets and their
      contents as well as providing other security capabilities that
      might be required in the particular system.

Do I recall correctly that we currently don't specify any mechanisms for
them to do so?  If so, we probably need to note that this is currently
in implementation-specific territory.

I'd also suggest
OLD:
   Thus, the security vulnerabilities are addressed (or should be
   addressed) in all the underlying technologies used by this design.
NEW:
   Thus, this design relies on the component underlying technologies to
   address the potential security vulnerabilities, and documents the
   necessary protections (or risk of their absence) above.  It does not
   include any native security mechanisms in-band with the MPLS
   encoding of the NSH functionality.
(since "are addressed (or should be addressed)" is rather wishy-washy
language).

   No known new security vulnerabilities are introduced by this design,

It's probably worth stating what the reference point for the comparison
is, just for clarity.  (I assume it's the RFC8300 NSH.)

Section 19.2

It seems that RFC 6790 needs to be a normative reference, since we are
RECOMMENDED to insert Entropy Labels.