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

[tirumal reddy <[e-mail]>]

Hi Ben,

Please see inline

On Thu, 22 Jul 2021 at 04:16, Benjamin Kaduk <[e-mail]> wrote:

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

Sure, we will publish the revised draft next week.


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

Done, will add a line.

Cheers,
-Tiru


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