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

[Benjamin Kaduk <[e-mail]>]

Hi Med,

I see that Tiru has also replied, but I will respond inline here and then
process Tiru's comments.

On Fri, Jul 16, 2021 at 02:15:44PM +0000, [e-mail] wrote:
> Hi Ben, 
> 
> Many thanks again for the detailed review. Really helpful. 
> 
> Please see inline some replies to the COMMENT part (with references to some DISCUSS points).
> 
> Changes to address your COMMENTS/NITS can be tracked at: https://tinyurl.com/nsh-integrity-latest.
> 
> Cheers,
> Med
> 
> > -----Message d'origine-----
> > De : Benjamin Kaduk via Datatracker [mailto:[e-mail]]
> > Envoyé : mercredi 14 juillet 2021 23:26
> > À : The IESG <[e-mail]>
> > Cc : [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > Objet : Benjamin Kaduk's Discuss on draft-ietf-sfc-nsh-integrity-06:
> > (with DISCUSS and COMMENT)
> > 
> > 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/
> > 
> > 
> > 
> [...]
> > 
> > 
> > --------------------------------------------------------------------
> > --
> > 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.
> 
> [Med] This depends on the type of the context header, the capabilities of SFs, local policies, performance implications, etc. Encrypting only a subset of context headers is a feature we want to provide. 
> 
> Please note that the I-D gives a transition path for SFC from the current plain text without any confidentiality or integrity protection to start with integrity protection and eventually transition to (partial and full) confidentiality and integrity protection.
> 
> We can recommend deployments to consider using encryption for **any** context header to prevent pervasive monitoring.

That seems reasonable, essentially something analogous to my above note
about it being safest to make cleartext the exception rather than the
default, in terms of what justification is looked for (and I want to
reiterate that this whole remark is a non-blocking comment).

> We understand that the wording "do not need to be protected" may not be appropriate. We removed the sentence starting with "That is, some ...".

Thank you!

> > 
> > 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...
> 
> [Med] That is in reference to what we say in Section 3 that:
> 
>    o  Base Header: Provides information about the service header and the
>       payload protocol.

The "base header" part is not what I'm unsure about.
To clarify, https://datatracker.ietf.org/doc/html/rfc8300#section-2.1
describes the NSH (Network Service Header) as a single header that's
composed of a Base Header, a Service Path Header, and Context Header(s).
If we just write "Service Header", I have to pick whether that refers to
the (Network) Service Header or the Service (Path) Header.  The unqualified
"service header" is used a couple times in RFC 8300, but in my (hasty)
reading is referring to the entire NSH.  My understanding of the contents
of table 1 are that this column refers to the Service Path Header, but I
would like to see that written out more clearly in the text.

> > 
> >    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).
> 
> [Med] The two levels are there for several reasons:
> * ease deployment: have an option that does not involve SFFs.
> * performance considerations of SFFs

I am sure there are reasons to have this as an option; my concern here is
in some sense editorial in that the text includes a conclusion ("thus") but
does not include a chain of logical reasoning that guides the reader to
believe that conclusion.  I was hoping to have a bit more text in the
document that justifies the claim.

> 
> > 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.
> 
> [Med] Agree, but we wanted to keep only two clear LoAs. 
> 
> > 
> >    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!
> 
> [Med] The text you quoted is here to remind that the original NSH packet is integrity protected as this may be missed by the reader. Likewise, that same protection is provided to context headers, even when unencrypted. 

Hmm, maybe the sentiment is along the lines of "the integrity protection
provided by the cryptographic mechanism is extended to also provide
protection for the unencrypted Context Headers and the packet on which the
NSH is imposed".

> > 
> > 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. 
> 
> [Med] FYI, we used to have such reference in draft-rebo-sfc-nsh-integrity-00:
> 
>    The Authenticated Encryption with Associated Data (AEAD) algorithm
>    [RFC5116] is used to provide NSH data integrity and to encrypt
>    privacy-sensitive metadata.

I guess that I'm trying to make a distinction between an AEAD "interface"
as an abstraction, and a particular combination of cryptographic primtives
used to provide that interface.  It can be useful to reference RFC 5116 for
"what is an AEAD" even if a specific construction from RFC 5116 is not
used.  For example, within the boringssl cryptographic library, the TLS
ciphersuites that use AES-CBC+HMAC are still treated as providing the AEAD
interface, as a simplifying abstraction.

The AES-CBC+HMAC instantiation of AEAD is fairly widespread, and it may be
reasonable to just present it in its own right without reference to a
previous incarnation like RFC 7518.  But I don't have particularly strong
feelings in this regard.

> We changed the spec because we wanted to control by design the nodes that are allowed to access encrypted data vs integrity-only. BTW, we sent a note to the saag mailing list right after we changed our design in the hope to have more eyes on what we are doing, but unfortunately we didn't get much attention. 

I guess this was
https://mailarchive.ietf.org/arch/msg/saag/HOaeRhyJX_RdqLu-XFitNddAd-I/ and
it looks like I skimmed the draft for a quick reply but didn't look in
depth.  I'm not even sure if I even processed Tiru's reply to me at the
time that called out the separation of ENC_KEY and MAC_KEY...

> Our understanding of your review is that you are not against using ENC_KEY and MAC_KEY, but you prefer AES-GCM over CBC. We are considering the following change (not yet implemented):  

I'm not entirely sure if the summary you write reflects my sentiment, since
there's a bit of a subtlety AES-GCM+HMAC seems like it will provide the
properties we need for this mechanism, but AES-CTR+GHASH does not.  Since
you do mention HMAC still in the NEW text, that is promising.

> OLD:
> 
>    Classifiers, NSH-aware SFs, and SFC proxies MUST implement the
>    AES_128_CBC_HMAC_SHA_256 algorithm and SHOULD implement the
>    AES_192_CBC_HMAC_SHA_384 and AES_256_CBC_HMAC_SHA_512 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 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).
> 
> NEW:
> 
>    Classifiers, NSH-aware SFs, and SFC proxies MUST implement the
>    AES_128_GCM 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.
> 
> We will have to refer to RFC5116 and [GCM]
> 
>    [GCM]         Dworkin, M., "Recommendation for Block Cipher Modes of
>                  Operation: Galois/Counter Mode (GCM) and GMAC",
>                  National Institute of Standards and Technology SP 800-
>                  38D, November 2007.
> 
> With that, we don't need to cite RFC7518. 

Ok.

> 
>  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. 
> 
> [Med] That's it!
> 
>  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.
> 
> [Med] I guess, the comment will not be relevant after mandating AES-GCM.

Not in the same way, no.
In some sense we are still using an AEAD interface, but we have two layers
of integrity tag because we need to be able to have one tag that is
validatable by some parties who are not authorized to be able to read or
modify the plaintext data.

> > 
> >    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.
> 
> [Med] As suggested above, we can refer to RFC5116 instead of RFC7518.
> 
> > 
> > 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.
> 
> [Med] OK. Done. 
> 
> > 
> >    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.
> 
> [Med] That depends on the bullet mentioned above (ex. per-tenant). I'm not sure adding the sentence adds much value as the scope tweaking still deployment-specific.  

If it takes a lot of text to describe accurately it may not be worth doing,
sure.

> > 
> > 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.
> 
> [Med] This section is about introducing the new MAC context headers. I don't think this is the right place to mention this.  
> 
> BTW, the order you mentioned is what is implied by the procedure: 7.5 (NSH Data Validation) and then 7.6 (Decryption of NSH Metadata). Invalid data are discarded as part of 7.5.
> 
> No problem to add a note to 7.6 if you think this is really needed.

It may be best to wait and see what things shape out to be after other
changes.

> > 
> > 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).
> 
> [Med] Fixed as per a comment from Eric. 
> 
> > 
> > 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).
> 
> [Med] Added this NEW: 
> 
> "A single key identifier is used to lookup both the ENC_KEY and the MAC_KEY associated with a key." 
> 
> > 
> >    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
> 
> [Med] Fixed per a comment from Eric.

If he keeps this up, we'll have to nominate him for SEC AD...

> > (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.
> 
> [Med] What is the advantage of using a value other than zero when encryption is not included? 

There may arise cases in the future where it's desired to use an encryption
scheme that takes a zero-length IV.  If the zero-length IV already has the
semantics of "encryption not used", then that needlessly precludes such
future flexibility.

> > 
> > 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).
> 
> [Med] I confirm.

(I trust it will make it into the document, based on Roman's review.)

> > 
> > 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.)
> 
> [Med] It is not allowed to have both. This is why we are using the generic term: "instance of "MAC and Encrypted Metadata" Context Header". That term is used through the document when we refer indifferently to MAC#1 and MAC#2.

That is what I suspected, so thank you for confirming.  I would propose to
have in one location a clear statement that MAC#1 and MAC#2 are not allowed
together, but I do not insist on it.

> > 
> > 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).
> 
> [Med] We think that RFC 4868 is the right reference as RFC 2014 does not discuss any of the HMAC algorithms used in this specification. 

In my experience it's fine to say just that "we use HMAC [RFC 2014] as
instantiated with <some hash> [reference-for-hash]", so whether or not the
specific construction (e.g., as from RFC 4868) appears in the reference is
not critical.

> > 
> >    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.
> 
> [Med] Noted. We made already this change: s/A/target NSH data. 
> 
> > 
> >    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.
> 
> [Med] Fair. Fixed, thanks.
> 
> > 
> > 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.
> 
> [Med] If we mandate AES_GCM, all the above text will be removed.
> 
> > 
> > Section 7.4
> > 
> > I suggest adding another word (like "Prevention") to the section
> > title.
> > 
> 
> [Med] Fixed. 
> 
> >    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.)
> 
> [Med] This was to accommodate any sync issues.

Having it extend in both directions is clearly useful, to cover all kinds
of sync issues.  But the range might equally as well be -Epsilon < (TSrt -
TS) < +Delta for Delta != Epsilon.  Perhaps that's needless complexity, but
it seemed worth raising the idea for discussion.

> > 
> >    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. 
> 
> [Med] Agree, but we don't specify the bits that are used. 

This is a very tempting and easy-to-make mistake; I think we should
preemptively warn the implementor against it, regardless of whether we also
provide a specific suggestion (e.g., the next line of this email).

>  Indeed, just using the MAC itself as the
> > unique value would be a fairly straightforward scheme, I think.
> 
> [Med] Thanks.
> 
> > 
> > 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.
> 
> [Med] Added "When an SFC data plan element conforms to this specification," to address one of Roman's comments. 
> 
> > 
> > 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.
> 
> [Med] We already have the following in the introduction:
> 
>    This specification is only
>    applicable to NSH MD Type 0x02 (Section 2.5 of [RFC8300]).

Yes.
There is some security consideration to limiting this mechanism to type 2
only, in that type 1 exists and has no defined method of protection.
My proposal is to mention that this mechanism is not fully generic (i.e.,
does not cover type 1 metadata) and a little bit of explanation of why.

> > 
> >    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.
> 
> [Med] The SF bypass can be detected if SI was manipulated on path because integrity check will fail. 

Does each SF or some other on-path entity know which (SFP, SI) are
allowed/expected for traffic it receives?  If not, then just moving the
packets around differently without changing their contents is my concern.

> > 
> >    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.
> 
> [Med] That's part of the key management, declared out of scope.   

Even with the details of how keys are shared/distributed being out of
scope, I still would have expected discussion of the risks of shared keys.
Perhaps my expectations are wrong (it has happened before and will happen
again), but that's still what I expect.

> > 
> > 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?
> 
> [Med] Good catch: proxies should be mentioned as well as re-classification. We can't just talk about classification as this is about updating the MAC. 
> 
> The NEW text that also takes into account a comment from John is as follows: 
> 
>    It is expected that specific devices in the SFC-enabled domain will
>    be configured such that no device other than the classifiers (when
>    reclassification is enabled), NSH-aware SFs, and SFC proxies will be
>    able to update the integrity protected NSH data (Section 7.1), and
>    also such that no device other than the NSH-aware SFs and SFC proxies
>    will be able to decrypt and update the Context Headers carrying
>    sensitive metadata (Section 7.1).
> 
> > 
> > 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).
> 
> [Med] Added this NEW sentence: 
> 
>    Similar to Section 9.1, no device other than the NSH-aware SFs and
>    SFC proxies in the SFC-enabled domain should be able to decrypt and
>    update the Context Headers carrying sensitive metadata. 

Thanks!

> > 
> > 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.
> 
> [Med] Yes because we think that’s the symmetric mode is better vs. broadcast with trusted modes. 
> 
>   Is
> > client/server mode not a BCP for trusted networks and
> > broadcast/symmetric the only options? 
> 
> [Med] Not sure to get this comment. 

Sorry, let me try again.  "I do not operate an NTP service and have little
knowledge of NTP operational best practices.  I can easily believe that NTP
symmetric mode is a good option ("best current practice") on a trusted
network, but I could also easily believe that NTP client/server mode would
equally be a useful option on a trusted network.  Is there a reason to
prefer symmetric over client/server mode?"
It seems that you mostly answered this above, though just with a "yes" and
not the actual reason (the actual reason would just be for my own curiosity
and is not expected to appear in the document, so please feel free to drop
the topic).

Thanks,

Ben