Subscriber and Performance Policy Identifier Context Headers in the Network Service Header (NSH)
draft-ietf-sfc-serviceid-header-14

[[ questions ]]

[ section 1 ]

* Is section 3 of 8300 really the right reference for MTU considerations?

  8300 section 5, perhaps?  Or 7665 section 5.6?


[[ nits ]]

[ section 6 ]

* s/within from/within/

I support Ben's DISCUSS position.

Thanks for document updates in Section 7 to address my DISCUSS item.

Thank you for the work put into this document. 

Please find below a couple of non-blocking COMMENT points and I would appreciate it if the authors answered to my points that were closed to be a DISCUSS.

I hope that this helps to improve the document,

Regards,

-éric

-- Section 1 --
Please clarify the following text as NAT only applies to IPv4 and as there is no private addresses in IPv6
"  Typically,
   because of the widespread use of private addressing in those
   networks, if SFs to be invoked are located after a NAT function, the
   identification based on the internal IP address is not possible once
   the NAT has been crossed. "

-- Section 3 --
While I am not familiar with NSH RFC 8300 and the concept of updating the NSH context, the following text makes me wonder:
"  The classifier and NSH-aware SFs MAY inject or strip a Subscriber
   Identifier Context Header as a function of a local policy."
   
It appears to me that a SF would actually increase/decrease the size of NSH which could lead to two issues:
1) the actual MTU is decreased on the path *after* encapsulation
2) if ICMP is generated back to the source of the NSH header, then will the source recognize the NSH packet inside the ICMP message as it is different ?

[I also suport Ben's DISCUSS.]

I found this to be hard to read, despite its short length.  I hope my comments below can help with that.

— Abstract —

   This document defines Subscriber and Performance Policy Identifiers
   Network Service Header Variable-Length Context Headers to inform

That string of capitalized words really is indecipherable.  I think that’s because it’s talking about three separate things (Identifiers, Network Service Headers, and Context Headers) with nothing to separate them.  Can you reword the sentence to fix that?

Maybe, “This document defines Subscriber and Performance Policy Identifiers that can be put into variable-length Context Headers within the Network Service Header to inform...”?

— Section 1 —

   NAT functionality can reside in a distinct
   node which for 3GPP network can be the Packet Data Network (PDN)
   Gateway (PGW) as specified in [TS23401] in case of 4G or the User
   Plane Function (UPF) facing the external Data Network (DN) [TS23501]
   in case of 5G, respectively.

That’s a long sentence with at least one grammatical error, and it’s hard to read.  May I suggest splitting it?:

NEW
   NAT functionality can reside in a distinct node.  For a 4G 3GPP
   network, that node can be the Packet Data Network (PDN)
   Gateway (PGW) as specified in [TS23401].  For a 5G 3GPP
   network it can be the User Plane Function (UPF) facing the
   external Data Network (DN) [TS23501].
END

   This approach avoids
   to require additional internal structures of the Context Headers

You can’t “avoid to require”.  You can say that the approach “avoids the requirement of additional...”, or, better, simply “avoids additional...”.

— Section 3 —

   In order
   to prevent interoperability issues, the type the identifiers carried
   in the Subscriber Identifier Context Headers and their format to be
   injected in such header should be configured to nodes authorized to
   inject and consume such headers.

I can’t parse that sentence, and I can’t suggest a fix because I have no idea what it’s trying to say.  I don’t know whether the issue is grammar or difficult wording.  Can you please fix it?

   Local policies may require running additional validation checks on
   the content of these Context Headers (e.g., accept only some lengths,
   types) and the behavior to adopt at NSH-aware SFs.

You lose me here at “and the behavior”: I don’t see how it’s supposed to connect to the rest of the sentence.  Are you trying to say that:
1. Local policies may require running validation checks, and
2. Local policies may define the behavior to adopt ?

Or is it something else?

   However, this specification does not require nor preclude
   such NSH-aware SFs may be instructed to ignore the Context Header

This also doesn’t parse.  I think you can fix this by adding “that” after “preclude“.

— Section 4 —

   Dedicated service-specific performance identifiers are defined to
   differentiate between services requiring specific treatment to
   exhibit a performance characterized by, e.g., ultra-low latency (ULL)
   or ultra-high reliability (UHR).

Another sentence I can’t make sense of.  Can you please re-word it?

   adapted dynamically by on-the move SFC path and SF instance

Nit: you need one more hyphen, “on-the-move”.

   Multiple Performance Policy Identifier Context Headers MAY be present
   in the NSH; each carrying an opaque value

Nit: change the semicolon to a comma.

   o  Performance Policy Identifier: Represents an opaque value pointing
      to specific performance policy to be enforced.  The structure and
      semantic of this field is deployment-specific.

Nit: “semantics” as a noun is always used in plural form.

— Section 6 —

   Access to subscriber data usually requires specific access privilege
   levels.  To avoid bypassing this guard, it is not advised that an SF
   maintaining logs for operational reasons to also log the content of
   Subscriber and Performance Policy Context Headers received in NSH
   packets if the SF does not use the content of that header for its
   operation.

The second sentence has some grammatical errors and is hard to read, partly because of a chain of negatives (avoid bypassing, not advised, does not use).  Please reword this to say things more directly.  Maybe something like this?:

NEW
   Access to subscriber data usually requires specific access privilege
   levels.  To maintain that protection, an SF keeping operational logs
   should not log the content of a Subscriber and Performance Policy
   Context Header received in an NSH packet unless the SF actually
   uses the content of that particular header for its operation.
END

(And can you also eliminate “received in an NSH packet” from that sentence?  I think so.)

This is just an observation, since we don't seem to have talked about it
the first time around, but this document seems to leave a lot of details
up to the deployment (or implementation?); I wonder how much thought
went into targeting Proposed Standard vs Informational.


Section 3

   per-subscriber policies.  The assessment whether a method for
   defining a subscriber identifier provides the required functionality
   and that it is compliant with SFs capabilities with the intended
   performance levels is deployment specific.

Some nits here; I propose

NEW:
The assessment of whether a method for defining a subscriber identifier
provides the required functionality and whether it is compatible with the
capabilities of the SFs to the intended performance level, is deployment
specific.

   if the NSH-aware SF is instructed to do so.  For example, an SF that
   expects a subscriber identifier generated from an internal IP address
   will discard Subscriber Identifier Context Headers conveying
   identifiers generated from Mobile Subscriber ISDN Number (MSISDN),
   International Mobile Subscriber Identity (IMSI), or malformed IP
   addresses.

It feels a bit odd for a PS document to talk about various types of
identifier like this when any type indicator would be part of the
internal site-specific structure of the opaque data.

Section 4

   The Performance Policy Identifier allows for the distributed
   enforcement of a per-service policy such as a service function path
   to only include specific SFs instances (e.g., SFs located within the
   same DC or those that are exposing the shortest delay from an SFF).

nit: there looks to be a grammar issue here; adding "requiring" before
"a service function path to only include" would probably fix it, but
there are many other options.

   The default behavior of intermediary NSH-aware nodes have to preserve
   such Context Headers (i.e., the information can be passed to next hop
   NSH-aware nodes), but local policy may require an intermediary NSH-
   aware node to strip one after processing it.

Please align the wording here with the (quite well done) version in
the previous section (e.g., s/have to preserve/is to preserve/).

   Nodes that are involved in an SFC-enabled domain are assumed to be
   trusted (Section 1.1 of [RFC8300]).  Means to check that only
   authorized nodes are solicited when a packet is crossing an SFC-
   enabled domain are out of scope of this document.

Sorry for repeating this from my earlier comments, but I would suggest
to s/solicited/traversed/ here.

                                                                If
   encryption is not enabled, the use of non persistent identifiers as
   well as the removal of the Subscriber Identifier Context Headers from
   the NSH by the last SF making use of such headers soften this issue
   (see "data minimization" discussed in Section 3 of [RFC8165]).

I would double-check the location of the "if encryption is not enabled"
clause, here -- use of encryption would be a different partial
mitigation, but the indicated concerns could still apply in an encrypted
setting, just in a different way (depending on which nodes had access to
the decrypted results, etc.).

   If no secure transport encapsulation is enabled, the use of trivial
   subscriber identifier structures together with the presence of
   specific SFs in a service function chain may reveal sensitive
   information to every on-path device (but also to teams managing these
   devices).  [...]

I would suggest promoting the parenthetical to a full clause (offset by
a comma) and to s/but/and/; the consideration seems of similar import to
the exposure to devices.

Considering the concerns of the other ADs, I think it would be helpful to clarify "subscriber identifier".  I may be wrong in my interpretation but I didn't assume this as being used to identify an individual subscriber but as a context label for a group of users. Both for the obvious privacy reasons and scaling, a mapping context label. Wording to clarify and as Ben noted, guidance on encrypting this value (obvious to an operator but may not be so obvious to software implementers). Even for a group context label, depending on the information, there are different levels of security which are applied.

Thanks for addressing my issue.

I support Ben's DISCUSS.

Thank you for this document.

A few minor comments/questions:

3.  Subscriber Identification NSH Variable-Length Context Header

   This document adheres to the recommendations in [RFC8300] for
   handling the Context Headers at both ingress and egress SFC boundary
   nodes (i.e., to strip such Context Headers).  Revealing any personal
   and subscriber-related information to third parties is avoided by
   design to prevent privacy breaches in terms of user tracking.
   
This paragraph doesn't appear to be specific to "Subscriber Identification NSH Variable-Length Context Header", yet appears under section 3.  Perhaps this paragraph would be better at the end of the introduction, may be continuing on from "This document assumes the NSH is used exclusively within a single administrative domain."?

3.  Subscriber Identification NSH Variable-Length Context Header

       When multiple subscriber
       identifier Context Headers are present and an SF is instructed to
       strip the Subscriber Identifier Context Header, that SF MUST remove
       all Subscriber Identifier Context Headers.

Is a local policy allowed to remove a single subscriber identifier Context Header?  This text implies that it must always be the case that if one is being removed, they are always all removed.  Is that the intention?  E.g., I was wondering whether the intent here is to explain that their might be multiple context headers in a packet, and hence if a node is instructed to generically remove the subscriber identifier then it better ensure that it removes all subscriber identifier context headers and not just the first one that it finds.

A few nits:

"As such, means to allow" => "As such, methods to allow"
"out of the scope of this" => "outside the scope of this" or "out of scope for this"
"an additional internal structures" => "additional internal structures"
"decide unambiguously whether" => "specify whether"