Re: [sfc] Benjamin Kaduk's Discuss on draft-ietf-sfc-nsh-integrity-06: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

Hi Tiru,

Also inline...

On Mon, Jul 19, 2021 at 01:20:11PM +0530, tirumal reddy wrote:
> Hi Ben,
> 
> Please see inline
> 
> On Thu, 15 Jul 2021 at 02:55, Benjamin Kaduk via Datatracker <
> [e-mail]> wrote:
> 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-sfc-nsh-integrity-06: 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 DISCUSS and COMMENT positions.
> >
> >
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-sfc-nsh-integrity/
> >
> >
> >
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> >
> > (0) (I came to this realization rather late in my review process, so
> > there may be places where the COMMENT and this discuss point are in
> > disagreement; this DISCUS takes precedence.)
> > I fear that the construction that separately distributes ENC_KEY and
> > MAC_KEY in an attempt to achieve privilege separation is fatally flawed.
> > In particular, the CBC encryption mode is a malleable encryption mode,
> > in that flipping a bit of ciphertext will filp the corresponding bit of
> > the next block of recovered plaintext (at the cost of completely
> > garbling the recovered block containing the bit that was modified).
> > Subsequent blocks are unaffected.  Typically we combine CBC mode with a
> > MAC such as HMAC in order to prevent such modifications from being
> > exposed as attack vectors, and while we do use HMAC here for that
> > purpose, we also introduce a separate class of actors that have access
> > to the HMAC key but not the encryption key.  Accordingly, those actors
> > can produce a new, valid, integrity tag after making a modification to
> > the ciphertext, allowing them to engage in attacks that make use of
> > ciphertext malleability.  Ciphertext malleability is particularly useful
> > as an attack vector when the structure of the plaintext being encrypted
> > is known, and there are portions of the plaintext that the application
> > will either ignore if they are garbled or are expected to be near random
> > in the normal case (and thus for which garbled output does not cause
> > rejection by the application).  In a SFC environment it seems highly
> > likely that the structure of the plaintext will be known or guessable,
> > and we don't have any real mechanisms to control what types of metadata
> > go into encrypted context headers, so it seems that we must act as if we
> > are exposed to this risk.
> >
> > While §4.3 does have a note that use of GCM with HMAC is undesirable due
> > to the additional authentication tag, it may be unavoidable in order to
> > provide the properties that we need.
> >
> 
> Thanks for explaining the attack. We will modify the text in Section 4.3 as
> follows to address the comment:
> 
>    Classifiers, NSH-aware SFs, and SFC proxies MUST implement the
>    AES_128_GCM [GCM][RFC5116] algorithm and SHOULD implement the
>    AES_192_GCM and AES_256_GCM algorithms.
> 
>    Classifiers, NSH-aware SFs, and SFC proxies MUST implement the HMAC-
>    SHA-256-128 algorithm and SHOULD implement the HMAC-SHA-384-192 and
>    HMAC-SHA-512-256 algorithms.
> 
>    SFFs MAY implement the aforementioned cipher suites and HMAC
>    algorithms.
> 
> Note:
> The use of AES_128_CBC_HMAC_SHA_256 algorithm would have avoided the need
> for a second 128-bit authentication tag but due to the nature of how CBC
> mode operates, the mode allows for malleability of plaintexts. This
> malleability allows for attackers to make changes in the ciphertext and, if
> parts of the plaintext are known, create arbitrary blocks of
> plaintext. This specification mandates the use of AES-GCM to prevent this
> type of attack.

This seems okay on first read, thanks.  I will probably end up reading it
again once the revised I-D is posted, in the full context of the draft.

> 
> >
> > (1) Section 5.1 describes the MAC as:
> >
> >    Message Authentication Code:   Covers the entire NSH data, excluding
> >       the Base header.  The Additional Authenticated Data (defined in
> >       [RFC7518]) MUST be the Service Path header, the unencrypted
> >       Context headers, and the inner packet on which the NSH is imposed.
> >
> > This description seems to exclude from the MAC most of the MAC context
> > header itself (if we go by the corresponding figure), which is very bad
> > for security.  We definitely need to include under the MAC the MAC
> > context header bits from metadata class through and including at least
> > timestamp, and I think IV length as well.  (The IV itself would be
> > incorporated via the ciphertext, since the IV is an input to encryption,
> > but since the IV length field indicates whether or not encryption was
> > performed, we'd need to protect that information.)
> >
> 
> No, the MAC context header is also used for generating the digest.

Ok.

> 
> >
> > Similarly, Section 5.2 has the description:
> >
> >    Message Authentication Code:  Coves the entire NSH data.  The
> >       Additional Authenticated Data (defined in [RFC7518]) MUST be the
> >       entire NSH data (i.e., including the Base Header) excluding the
> >       Context Headers to be encrypted.
> >
> > which on the face of it includes the field that holds the MAC itself
> > (and is not yet populated), i.e., is self-referential.
> >
> 
> The below update will address this comment:
> 
> The NSH imposer sets the MAC field to zero and then computes the message
> integrity for the target NSH data (depending on the integrity protection
> scope discussed in Section 5) using MAC_KEY and HMAC algorithm. It inserts
> the computed digest in the MAC field in the "MAC and Encrypted Metadata"
> Context Header.
> 

Thanks!  (I think Roman may have goten this before me, so sorry for the
duplication.)

> 
> >
> > I think we need to be much more precise about the construction of the
> > AAD in both cases.  It's possible that the HMAC construction for the
> > no-encrypted-context-headers case can inherit a definition from the AAD
> > description, but if not we'll need to have some more precision there as
> > well.
> >
> > (2) In order for the MAC-only construction in §7.2 to be compatible with
> > the AEAD integrity tag construction, we would need to include the 64-bit
> > AL after A.  While HMAC is intrinsically immune to length-extension
> > attacks, I think that having the explicit AL is useful to avoid any risk
> > of malleability, since the same MAC_KEY is used for constructing both
> > types of MACs.
> >
> 
> I don't think this comment is applicable after AES_128_CBC_HMAC_SHA_256 is
> removed.

I think so, yes.

> 
> >
> > (3) Section 5.1 describes the Timestamp field as an "unsigned 64-bit
> > integer value", which is inconsistent with the actual format given in
> > Section 6.
> >
> 
> Thanks, we will fix it in the next revision.
> 
> 
> >
> > (4) Section 7.5 directs the verifier to check if "the value of the newly
> > generated digest is identical to the one enclosed in the NSH".  It is
> > critical for the security of the system that this comparison be done in
> > a constant-time manner that does not provide a side channel into whether
> > the generated digest and the value in the NSH share a common substring.
> >
> 
> Agreed, will update text as follows:
> 
> After storing the value of the MAC field in the "MAC and Encrypted
> Metadata" Context Header, the SFC data plane element fills the MAC
> field with zeros.  Then, the SFC data plane element generates the message
> integrity
> for the target NSH data (depending on the integrity protection scope
> discussed in Section 5) using MAC_KEY and HMAC algorithm.  If the value of
> the newly generated digest is identical to the stored one, the SFC data
> plane element is certain that the NSH data has not been tampered and
> validation is therefore successful. Otherwise, the NSH packet MUST be
> discarded. The comparison of the computed HMAC value to the store value MUST
> be done in a constant-time manner to thwart timing attacks.

Thanks!

> 
> >
> > (5) Do the MAC context headers always have to be the last metadata
> > entries in the packet (to simplify the cryptographic calculations)?
> >
> 
> Yes.
> 

Okay.  I suspected that, but it is probably worth a MUST-level requirement
in the document to make it clear to everyone.

Thanks again,

Ben

> 
> > Certainly the diagrams only show "unencrypted context headers" appearing
> > prior to the MAC context header, so if we expect unencrypted context
> > headers to appear after the MAC context header as well, we should be
> > clear about that both in the figures and in the specification for how to
> > prepare the AAD.
> >
> >
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > Section 3
> >
> >    o  Both encrypted and unencrypted Context Headers MAY be included in
> >       the same NSH.  That is, some Context Headers may be protected
> >       while others do not need to be protected.
> >
> > In the security area we're pretty strongly moved towards a stance of
> > "encrypt everything that doesn't have a strong need to be plaintext
> > (since we don't want to assume that we can predict all threats and
> > attacks in advance), and QUIC has even taken that to new levels.  It's
> > quite surprising to see "don't need to be protected" as a justification
> > for lack of encryption, rather than "need to be unprotected".  Given
> > that we're going to be solving key distribution for integrity protection
> > anyway, it remains unclear to me why we wouldn't just encrypt
> > everything.
> >
> > Section 4.1.1
> >
> > Is the column heading for the middle column of Table 1 referring to the
> > Base Header and "Service Path Header"?  I'm not sure how to interpret
> > just "Service Header" other than the entire NSH, which doesn't seem
> > right...
> >
> >    The Service Path Header (Section 2 of [RFC8300]) is not encrypted
> >    because SFFs use Service Index (SI) in conjunction with Service Path
> >    Identifier (SPI) for determining the next SF in the path.
> >
> > It's good to see this spelled out like this; I'd love to have this kind
> > of explanation for every un-encrypted protocol element.
> >
> > Section 4.1.2
> >
> >    The solution provides integrity protection for the NSH data.  Two
> >    levels of assurance (LoAs) are supported.
> >
> > I see (reading onward) that the two LoAs just differ in what is covered
> > by the integrity tag, but each involve only a single MAC.  There's a
> > reasonable argument that when there are overlapping needs for what data
> > needs protecting and who needs access to it, that multiple MACs (or
> > encryption, if needed) are most appropriate, which can be tailored to
> > the respective needs.  This is something that Phillip Hallam-Baker has
> > ascribed as the "short-blanket theory" of cryptography, in that it's
> > like a blanket that's too short to cover everything you want, so you
> > need to have more than one and put them in partially overlapping layers.
> > Perhaps this "short-blanket theory" is more applicable to Discuss point
> > (0) than what is covered by the MAC, though...
> >
> >    The first level of assurance where all NSH data except the Base
> >    Header are integrity protected (Figure 2).  In this case, the NSH
> >    imposer may be a Classifier, an NSH-aware SF, or an SFC Proxy.  SFFs
> >    are not thus provided with authentication material.  [...]
> >
> > There seems to be a gap in the chain of reasoning as to why SFFs are not
> > provided with authentication material.  It seems that the intent is
> > something along the lines of considering which entities need access to
> > the key material in order to successfully add an NSH or update context
> > headers, but that alone is not itself cause to say that other entities
> > can never be provided with the keys (subject to the security
> > considerations of group-shared PSKs, of course).  Furthermore, if a
> > particular SFF is trusted, it may have grounds to verify the integrity
> > tag even if it is administratively prohibited from writing such a tag.
> >
> >    In both levels of assurance, the unencrypted Context Headers and the
> >    packet on which the NSH is imposed are subject to integrity
> >    protection.
> >
> > Surely any encrypted Context Headers are also given integrity
> > protection, as excluding them from the MAC computation would be quite
> > complex.  I see that the encrypted context headers are not part of the
> > AAD input, but in the AEAD model the ciphertext is still given integrity
> > protection!
> >
> > Section 4.2
> >
> >    The authenticated encryption algorithm defined in [RFC7518] is used
> >    to provide NSH data integrity and to encrypt the Context Headers that
> >    carry privacy-sensitive metadata.
> >
> > RFC 7518 (JSON Web Algorithms) is a very surprising reference for
> > generic AEAD encryption.  Continuing on, it becomes clear that we in
> > fact do not want a generic AEAD operation here, but instead have need
> > for separation of encryption and integrity protection due to the various
> > separations of roles that are possible.  Because this is a divergence
> > from current best practices (use of native AEAD cipher modes like GCM,
> > SIV, etc.), I might suggest a rather different introductory material:
> >
> > % The AEAD interface specified in RFC 5116 is a very powerful tool for
> > % providing strong confidentiality and integrity protection for protocol
> > % messages, and has become quite widely adopted as a result.  For
> > % protecting NSH data we adhere to the spirit of the AEAD interface but
> > % use a slightly modified version in order to remain consistent with the
> > % privilege separation needed (e.g., in the second level of assurance,
> > % SFFs need to be able to update the message integrity tag but do not
> > % need access to the decrypted plaintext).  Accordingly, a compound AEAD
> > % mechanism combining CBC encryption and HMAC integrity tag is used, so
> > % that distribution of the encryption key can be limited.  Rather than
> > % define a new scheme from scratch, the AES_CBC_HMAC_SHA2 scheme from
> > % [RFC7518] is used.  In order to simplify key distribution, a single
> > % secret key material seed K is used, which can be kept private
> > % centrally or distributed to all parties that will have access to
> > % decrypted plaintext, and the secondary keys MAC_KEY and ENC_KEY are
> > % derived from K as discussed in Section 5.2.2.1 of [RFC7518].  Entities
> > % that need to update the integrity tag but are not authorized to see
> > % the decrypted plaintext will only receive MAC_KEY and not K or
> > % ENC_KEY.
> >
> >    o  If the Context Headers are not encrypted, the Hashed Message
> >       Authentication Mode (HMAC) algorithm discussed in [RFC4868] is
> >       used to protect the integrity of the NSH data.
> >
> > It's not clear to me that this is adding much value; HMAC is used
> > whether or not there are encrypted headers, and the RFC 4868 reference
> > is rather odd (see also my Discuss point (2)).
> >
> >    The advantage of using the authenticated encryption algorithm is that
> >    NSH-aware SFs and SFC proxies only need to re-compute the message
> >    integrity of the NSH data after decrementing the Service Index (SI)
> >    and do not have to re-compute the ciphertext.  The other advantage is
> >    that SFFs do not have access to the ENC_KEY and cannot act on the
> >    encrypted Context Headers and, only in case of the second level of
> >    assurance, SFFs do have access to the MAC_KEY.  Similarly, an NSH-
> >    aware SF or SFC Proxy not allowed to decrypt the Context Headers will
> >    not have access to the ENC_KEY.
> >
> > [If my above suggestion is used, this text has probably become mostly
> > redundant and could be trimmed down or removed.]
> >
> >    The authenticated encryption process takes as input four-octet
> >    strings: a secret key (K), a plaintext (P), Additional Authenticated
> >    Data (A) (which contains the data to be authenticated, but not
> >    encrypted), and an Initialization Vector (IV).  The ciphertext value
> >    (E) and the Authentication Tag value (T) are provided as outputs.
> >
> >    In order to decrypt and verify, the cipher takes as input K, IV, A,
> >    T, and E.  The output is either the plaintext or an error indicating
> >    that the decryption failed as described in Section 5.2.2.2 of
> >    [RFC7518].
> >
> > This seems true for the abstract AEAD interface, but since we're peeling
> > off the HMAC integrity tag for standalone use, we may need to say
> > something about what inputs/outputs are needed for just using the
> > integrity tag without doing decryption.  In particular, we cannot use K
> > as an input for standalone HMAC verification!
> >
> > Also, this is just the generic AEAD interface, so RFC 5116 is probably a
> > more apropos reference than RFC 7518.
> >
> > Section 4.3
> >
> >       Note: The use of Advanced Encryption Standard (AES) in Galois/
> >       Counter Mode (GCM) + HMAC may have CPU and packet size
> >       implications (need for a second 128-bit authentication tag).
> >
> > [This might also be redundant if my earlier proposal is accepted.]
> >
> > Section 4.4
> >
> >    The document does not assume nor preclude the following:
> >
> >    o  The same keying material is used for all the service functions
> >       used within an SFC-enabled domain.
> >
> >    o  Distinct keying material is used per SFP by all involved SFC data
> >       path elements.
> >
> >    o  Per-tenant keys are used.
> >
> > I suggest a forward reference to the security considerations with a
> > brief note that the risk involved in using a group-shared symmetric key
> > increases with the size of the group to which it is shared.
> >
> >    In order to accommodate deployments relying upon keying material per
> >    SFC/SFP and also the need to update keys after encrypting NSH data
> >    for a certain amount of time, this document uses key identifiers to
> >    unambiguously identify the appropriate keying material.  Doing so
> >    allows to address the problem of synchronization of keying material.
> >
> > I'd suggest making a statement about the scope in which key identifiers
> > need to be unique.
> >
> > Section 4.5
> >
> >    An NSH-aware SF or SFC Proxy that needs to decrypt some Context
> >    Headers uses ENC_Key and the decryption algorithm for the key
> >    identifier being carried in the NSH.
> >
> > I think we need to say that decryption MUST NOT proceed unless the
> > integrity tag has been successfully validated.  To decrypt without
> > checking the integrity tag violates the AEAD interface.  Alternately, we
> > could only ever describe decryption as the AEAD operation that uses K as
> > input (not just ENC_KEY) -- §7.6 seems to attempt to take this latter
> > approach, but the document as a whole is not consistent about it.
> >
> > Section 5, 5.x
> >
> > I strongly suggest changing "Key Length" to "keyid length", since there
> > is already convention on "key length" (e.g., for AES-128 vs AES-192 vs
> > AES-256).
> >
> > Section 5.1
> >
> >    Key Identifier:  Carries a variable-length Key Identifier object used
> >       to identify and deliver keys to SFC data plane elements.  This
> >       identifier is helpful to accommodate deployments relying upon
> >       keying material per SFC/SFP.  The key identifier helps in
> >       resolving the problem of synchronization of keying material.
> >
> > I suggest clarifying that a single key identifier is used to lookup both
> > the associated ENC_KEY and the associated MAC_KEY (or, alternately, that
> > it is logically bound to K and remind the reader that ENC_KEY and
> > MAC_KEY are extracted from K and might be separately distributed).
> >
> >    Key Length:  Variable.  Carries the length of the key identifier.
> >    [...]
> >    IV Length:  Carries the length of the IV (Section 5.2 of [RFC7518]).
> >       If encryption is not used, IV length is set to zero (that is, no
> >       "Initialization Vector" is included).
> >
> > Please also specify that the Key/IV lengths are measured in bytes (the
> > reference for IV length, at least, talks about IVs in bits).
> >
> > I strongly suggest using a value other than zero as the sentinel value
> > for "no encryption" -- there already exist modes like AES-SIV (RFC 5297)
> > that do not require an IV input, which would be a natural use for
> > zero-length IV.
> >
> > Additionally, one might imagine using a non-zero-length IV as input to
> > an HMAC calculation as a per-message nonce.  On cursory examination,
> > since we are already using timestamp-based replay detection, the
> > additional nonce is unlikely to add much value, but I'm not willing to
> > completely rule it out on the basis of the minimal analysis I performed.
> >
> > Section 7.1
> >
> >    Only one instance of "MAC and Encrypted Metadata" Context Header
> >    (Section 5) is allowed.  If multiple instances of "MAC and Encrypted
> >    Metadata" Context Header are included in an NSH packet, the SFC data
> >
> > I think Roman may have mentioned this already, but please clarify if
> > this is one instance per NSH or per packet (e.g., in hierarchical SFC we
> > might have more than one NSH in a packet).
> >
> > Please also clarify it if is permitted to have both MAC#1 and MAC#2
> > present in the same packet.  (IIUC there are not obvious situations
> > where such a setup would be useful, but it's still good to be clear
> > about expectations.)
> >
> > Section 7.2
> >
> >    If the Context Headers are not encrypted, the HMAC algorithm
> >    discussed in [RFC4868] is used to integrity protect the target NSH
> >    data.  [...]
> >
> > It's not really clear to me why RFC 4868 is the right reference here, as
> > opposed to (e.g.) RFC 2104.  In particular, see my Discuss point (2).
> >
> >    The Message Authentication Code (T) computation process can be
> >    illustrated as follows:
> >
> >          T = HMAC-SHA-256-128(MAC_KEY, A)
> >
> > I agree with the other reviewer that wondered about just using "MAC" (or
> > maybe "HMAC") rather than "HMAC-SHA-256-128" here (I do see the "can be
> > illustrated as" qualifier).
> >
> > It's also a little weird to reuse T and A from the AEAD construction (or
> > RFC 4868) here without additional discussion on how they apply and how
> > the value of A is constructed.
> >
> >    An entity in the SFP that intends to update the NSH MUST follow the
> >    above behavior to maintain message integrity of the NSH for
> >    subsequent validations.
> >
> > I'm not sure that "intends to" is needed; the requirement applies to
> > actions, not intent.
> >
> > Section 7.3
> >
> >    In an SFC-enabled domain where pervasive monitoring [RFC7258] is
> >    possible, all Context Headers carrying privacy-sensitive metadata
> >    MUST be encrypted; [...]
> >
> > I agree with the other reviewer that noted that this seems to
> > effectively be an unqualified "MUST", since there is always some
> > possibility of monitoring.
> >
> >          MAC_KEY = initial MAC_KEY_LEN octets of K,
> >          ENC_KEY = final ENC_KEY_LEN octets of K,
> >          E = CBC-PKCS7-ENC(ENC_KEY, P),
> >          M = MAC(MAC_KEY, A || IV || E || AL),
> >          T = initial T_LEN octets of M.
> >          MAC and Encrypted Metadata = E || T
> >
> > I recognize that this is just copy+paste from RFC 7518 (with the
> > addition of the last line), but it seems a glaring omission that the
> > CBC-PKCS7-ENC() invocation does not include the IV as an input!
> > (In §5.2.2.1 of RFC 7518 we do see that "The plaintext is CBC encrypted
> > using PKCS #7 padding using ENC_KEY as the key and the IV", so it's just
> > the figure/formula that omits the IV.)
> >
> > We also don't define or mention T_LEN in this document other than in
> > this formula and the analogous one for MAC-without-encryption, which
> > might be worth remedying.
> >
> > Section 7.4
> >
> > I suggest adding another word (like "Prevention") to the section title.
> >
> >    The received NSH is accepted by an NSH-aware node if the Timestamp
> >    (TS) in the NSH is recent enough to the reception time of the NSH
> >    (TSrt).  The following formula is used for this check:
> >
> >              -Delta < (TSrt - TS) < +Delta
> >
> > (There's no real reason why the accepted timestamp skew parameter Delta
> > needs to be symmetric in the positive and negative directions.)
> >
> >    Replay attacks within the Delta window may be detected by an NSH-
> >    aware node by recording a unique value derived, for example, from the
> >    NSH data and Original packet (e.g., using SHA2).  [...]
> >
> > Note that the unique value MUST NOT use as input any bits that are not
> > covered by the MAC.  Indeed, just using the MAC itself as the unique
> > value would be a fairly straightforward scheme, I think.
> >
> > Section 7.5
> >
> >    When an SFC data plane element receives an NSH packet, it MUST first
> >    ensure that a "MAC and Encrypted Metadata" Context Header is
> >    included.  [...]
> >
> > This sort of requirement (as written) would seem to support the position
> > of some other reviewers that this document would need to formally update
> > RFC 8300.
> >
> > Section 9
> >
> > We might discuss that this mechanism is only for metadata type 2 and not
> > type 1 (as well as that with the relatively small fixed length of type 1
> > metadata, it's quite challenging to provide useful cryptographic
> > protection), so that type-1 metadata still has no generic protection
> > mechanism.  A given metadata element could define its own protection
> > scheme, of course.
> >
> >    The attacks discussed in [I-D.nguyen-sfc-security-architecture] are
> >    handled owing to the solution specified in this document, except for
> >    attacks dropping packets.  [...]
> >
> > >From a very quick read, draft-nguyen-sfc-security-architecture proposes
> > a probe-based scheme that monitors all middleboxes (or at least those in
> > the expected path) to detect a middlebox-bypass attack and verify that
> > the probe traversed the expected SFP.  I am not sure that this document
> > on its own suffices to prevent the middlebox-bypass attack for all
> > traffic (i.e., the probe-based scheme might still be needed).  If some
> > network entities know what the expected service index will be for a
> > given SPI at a given node, then middlebox-bypass attacks could be
> > natively detected, but my understanding was that such advance knowledge
> > of expected service index was not going to be available.
> >
> >    The solution specified in this document does not provide data origin
> >    authentication.
> >
> > I would have expected more in-depth discussion of the security
> > considerations for a group-shared PSK scheme, including how the risk of
> > key loss/compromise goes up when the key is distributed to more
> > locations.
> >
> > Section 9.1
> >
> >    No device other than the NSH-aware SFs in the SFC-enabled domain
> >    should be able to update the integrity protected NSH data.
> >
> > What about classifiers and SFC proxies?
> >
> > Section 9.2
> >
> > I expected to see some remarks here analogous to the second paragraph of
> > §9.1 (even if just by reference to that section).
> >
> > Section 9.3
> >
> >    Section 5.6 of [RFC8633] describes best current practices to be
> >    considered in deployments where SFC data plane elements use NTP for
> >    time synchronization purposes.
> >
> > The referenced section covers NTP symmetric mode only.  Is client/server
> > mode not a BCP for trusted networks and broadcast/symmetric the only
> > options?
> >
> > NITS
> >
> > Section 1
> >
> >    network infrastructures against them, network slicing, etc.  Because
> >    of the proliferation of such advanced SFs together with complex
> >    service deployment constraints that demand more agile service
> >    delivery procedures, operators need to rationalize their service
> >    delivery logics and master their complexity while optimising service
> >    activation time cycles.  The overall problem space is described in
> >    [RFC7498].
> >
> > The "their" in "master their complexity" is a bit ambiguous between
> > "operators" and "service delivery logics" (noting that the previous use
> > of "their" did refer to "operators").  (The sentence as a whole reads a
> > bit like marketing language, but that's not intended to be an actionable
> > comment.)
> >
> >    This specification fills that gap.  Concretely, this document adds
> >    integrity protection and optional encryption of sensitive metadata
> >    directly to the NSH (Section 4); integrity protects the packet
> >    payload and provides replay protection (Section 7.4).  [...]
> >
> > The clause after the semicolon needs to be able to stand alone as a
> > complete sentence in order to be grammatical, but it currently has no
> > subject.  Maybe prefixing "it also" would be enough to fix it.
> >
> > Section 4.1.2
> >
> >    The integrity protection scope is explicitly signaled to NSH-aware
> >    SFs and SFC proxies in the NSH by means of a dedicated MD Type
> >    (Section 5).
> >
> > Signaled to SFFs as well, since in the second LoA they can use it.
> >
> > Section 4.6
> >
> >    As discussed in [RFC8459], an SFC-enabled domain (called, upper-level
> >    domain) may be decomposed into many sub-domains (called, lower-level
> >    domains).  In order to avoid maintaining state to restore back upper-
> >    lower NSH information at the boundaries of lower-level domains, two
> >
> > I think s/upper-lower/upper-layer/ (across the line break)?
> >
> > Section 5.1
> >
> >    MAC#1 Context Header is a variable-length Context Header that carries
> >    the Message Authentication Code (MAC) for the Service Path Header,
> >    Context Headers, and the inner packet on which NSH is imposed,
> >    calculated using MAC_KEY and optionally Context Headers encrypted
> >    using ENC_KEY.  [...]
> >
> > Comma after "calculated using MAC_KEY", please.  ("optionally Context
> > Headers [...]" are not used along with MAC_KEY in the calculation)
> >
> > Section 5.2
> >
> >    Message Authentication Code:  Coves the entire NSH data.  The
> >
> > s/Coves/Covers/
> >
> > Section 6
> >
> >       The epoch is 1970-01-01T00:00Z in UTC time.  [...]
> >
> > (IIUC, the "in UTC time" is redundant with the trailing 'Z'.)
> >
> > Section 7.1
> >
> >    element.  The details of sending notification alarms (i.e., the
> >    parameters affecting the transmission of the notification alarms
> >    depend on the information in the context header such as frequency,
> >    thresholds, and content in the alarm) SHOULD be configurable.
> >
> > The grammar in the parenthetical seems off to me.  Possibly
> > s/depend/depending/ would be a fix, but I'm not sure.
> >
> >    NSH-aware SFs and SFC proxies MUST re-apply the integrity protection
> >    if any modification is made to the Context Headers (strip a Context
> >    Header, update the content of an existing Context Header, insert a
> >    new Context Header).
> >
> > I think maybe an "e.g.," in the parenthetical is in order, since this
> > doesn't seem to include things like "decrement the TTL".
> >
> > Section 9
> >
> >    The interaction between the SFC data plane elements and a key
> >    management system MUST NOT be transmitted in clear since this would
> >    completely destroy the security benefits of the integrity protection
> >    solution defined in this document.  The secret key (K) must have an
> >    expiration time assigned as the latest point in time before which the
> >
> > This first sentence seems like it could probably be a paragraph of its
> > own; the subsequent discussion of expiration time seems to be mostly
> > unrelated.
> >
> > We might also add another sentence to reiterate that "the security and
> > integrity of the key-distribution mechanism is vital to the security of
> > the system as a whole".
> >
> >    o  Attacker replacing the packet on which the NSH is imposed with a
> >       bogus packet.
> >
> > I'd probably say "modified or bogus".
> >
> >    In an SFC-enabled domain where the above attacks are possible, (1)
> >
> > This is perhaps ambiguous about whether an "any" or "all" criterion
> > should be used.  Perhaps that's desired :)
> >
> >
> >
> >