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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Wed, 14 Jul 2021 14:25:51 -0700

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.

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

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.

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.

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

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

(5) Do the MAC context headers always have to be the last metadata
entries in the packet (to simplify the cryptographic calculations)?
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 :)

---

From: <[email-masked]>
Date: Thu, 15 Jul 2021 13:30:12 +0000

Hi Ben, 

Thank you for the review. Much appreciated. 

Starting with the easy part (NITS) as we need more time to think about your point(0) and its impact on many COMMENT points. 

FYI, we used to have a discussion on AES-GCM in the past, but we went for the current design as we wanted to impose by design the separation between entities that access the encrypted data and avoid the extra 128 bits overhead. There was also a discussion about the parallelization provided by separating the two operations.  

Anyway, we will follow SOON on the DISCUSS and COMMENT parts with concrete proposals to hopefully fix them. 

The changes can be tracked at: https://tinyurl.com/nsh-integrity-latest 

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk via Datatracker [mailto:[email-masked]]
> Envoyé : mercredi 14 juillet 2021 23:26
> À : The IESG <[email-masked]>
> Cc : [email-masked]; [email-masked];
> [email-masked]; [email-masked]; [email-masked]
> Objet : Benjamin Kaduk's Discuss on draft-ietf-sfc-nsh-integrity-06:
> (with DISCUSS and COMMENT)
> 
> 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.)
> 
> 
[..]
[..]
> "operators" and "service delivery logics"

[Med] s/their/its

 (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.)
> 
> complete sentence in order to be grammatical, but it currently has
> no subject.  Maybe prefixing "it also" would be enough to fix it.

[Med] OK to start a new sentence. 

> Signaled to SFFs as well, since in the second LoA they can use it.

[Med] Yes. 

> I think s/upper-lower/upper-layer/ (across the line break)?

[Med] Yes, changed to "upper-level". 

> Comma after "calculated using MAC_KEY", please. 

[Med] Fixed, but that I'm afraid that we will touch this once we fix your discuss point (0).

 ("optionally
> Context Headers [...]" are not used along with MAC_KEY in the
> calculation)
> 
> Section 5.2
> 

[Med] Fixed. 

> (IIUC, the "in UTC time" is redundant with the trailing 'Z'.)

[Med] Right. 

> s/depend/depending/ would be a fix, but I'm not sure.

[Med] Made these changes: 

s/affecting/that affect
s/depend/depending/

> I think maybe an "e.g.," in the parenthetical is in order, since
> this doesn't seem to include things like "decrement the TTL".

[Med] Fixed. 

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

[Med] Works for me. Fixed.

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

[Med] Added. 

> I'd probably say "modified or bogus".

[Med] Fixed. 

> should be used.  Perhaps that's desired :)

[Med] I think you got it :-)

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

---

From: <[email-masked]>
Date: Fri, 16 Jul 2021 14:15:44 +0000

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

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

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

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

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

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

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

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

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. 

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

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.

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. 

 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.

> whether or not there are encrypted headers, and the RFC 4868
> reference is rather odd (see also my Discuss point (2)).
> 
> [If my above suggestion is used, this text has probably become
> mostly redundant and could be trimmed down or removed.]
> 
> 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.

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

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

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

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

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

> Please also specify that the Key/IV lengths are measured in bytes

[Med] Fixed per a comment from Eric.

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

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

> where such a setup would be useful, but it's still good to be clear

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

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

> I'm not sure that "intends to" is needed; the requirement applies to

[Med] Fair. Fixed, thanks.

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

[Med] If we mandate AES_GCM, all the above text will be removed.

[Med] Fixed. 

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

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

 Indeed, just using the MAC itself as the
> unique value would be a fairly straightforward scheme, I think.

[Med] Thanks.

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

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

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

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

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

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

---

From: tirumal reddy <[email-masked]>
Date: Mon, 19 Jul 2021 13:20:11 +0530

Hi Ben,

Please see inline

> https://datatracker.ietf.org/doc/draft-ietf-sfc-nsh-integrity/

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.

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.

> performed, we'd need to protect that information.)

No, the MAC context header is also used for generating the digest.

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

> of malleability, since the same MAC_KEY is used for constructing both

I don't think this comment is applicable after AES_128_CBC_HMAC_SHA_256 is
removed.

> integer value", which is inconsistent with the actual format given in

Thanks, we will fix it in the next revision.

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

> entries in the packet (to simplify the cryptographic calculations)?

Yes.

Cheers,
-Tiru

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 21 Jul 2021 15:39:57 -0700

Hi Med,

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

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

>    o  Base Header: Provides information about the service header and the

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.

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

> [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, e…

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

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

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.

> With that, we don't need to cite RFC7518. 

Ok.

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

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

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

> [Med] Fixed per a comment from Eric.

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

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

> > SFC we might have more than one NSH in a packet).

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

> [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 indi…

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.

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

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

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

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

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

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

>    update the Context Headers carrying sensitive metadata. 

Thanks!

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

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 21 Jul 2021 15:46:49 -0700

Hi Tiru,

Also inline...

> plaintext. This specification mandates the use of AES-GCM to prevent this

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.

Ok.

> the computed digest in the MAC field in the "MAC and Encrypted Metadata"

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

> I don't think this comment is applicable after AES_128_CBC_HMAC_SHA_256 is

I think so, yes.

Thanks!

> > entries in the packet (to simplify the cryptographic calculations)?

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

---

From: tirumal reddy <[email-masked]>
Date: Thu, 22 Jul 2021 15:58:10 +0530

Hi Ben,

Please see inline

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

> > > Please refer to
> https://www.ietf.org/iesg/statement/discuss-criteria.html

Sure, we will publish the revised draft next week.

> in the document to make it clear to everyone.

Done, will add a line.

Cheers,
-Tiru

> not
> peeling
> both
> (or
> (or
> position
> not

---

From: tirumal reddy <[email-masked]>
Date: Fri, 23 Jul 2021 12:40:42 +0530

Hi Ben,

Please see inline

On Thu, 22 Jul 2021 at 04:10, Benjamin Kaduk <[email-masked]> wrote:

> > Changes to address your COMMENTS/NITS can be tracked at:
> https://tinyurl.com/nsh-integrity-latest.
> >
> > Cheers,
> > Med
> >
> > [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.
>

Updated text as follows :

   In order to prevent pervasive monitoring [RFC7258], it is RECOMMENDED
   to encrypt all Context Headers.  All Context Headers carrying
   privacy-sensitive metadata MUST be encrypted; doing so, privacy-
   sensitive metadata is not revealed to attackers.  Privacy specific
   threats are discussed in Section 5.2 of [RFC6973].

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

Thanks, fixed.

> document that justifies the claim.

The primary job of SFF is to forward traffic to the appropriate SF, it does
not need access to the metadata in the context headers. However, if a
deployment requires SFF to access the metadata in the context headers, KMS
can give access to the keying material to authorized SFF.

I will add reference to
https://datatracker.ietf.org/doc/html/rfc7665#section-4.3, it discusses the
role of SFF in  SFC.

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

Looks good, I will update the draft.

> previous incarnation like RFC 7518.  But I don't have particularly strong

Removed reference to RFC 7518.

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

Yes, now we have two layers of message integrity provided by AES_GCM and
HMAC.

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

Good point, added the following line:
If the Context Headers are only integrity protected, "Nonce Length" is set
to zero (that is, no "Nonce" is included). The "Nonce Length" can be set to
zero based on the encryption algorithm used to encrypt the Context Headers.

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

Fixed.

> > > unique value would be a fairly straightforward scheme, I think.

Sure, updated text as follows:
Replay attacks within the Delta window may be detected by an NSH-aware node
by recording a unique value derived, for example, to record a unique value
derived from the MAC field value. Such an NSH-aware node will detect and
reject duplicates.

> > [Med] Added "When an SFC data plan element conforms to this
> specification," to address one of Roman's comments.
> >
> > >
> > > Section 9
> > >

Done, added following text:

This specification is not applicable to NSH MD Type 0x01 (Section TBD of
RFC8300) because it only allows a Fixed-Length Context Header whose size is
16-bytes and is not sufficient to accommodate both the metadata and message
integrity of the NSH data.

> packets around differently without changing their contents is my concern.

This attack can be detected using the proof of transit mechanism discussed
in https://datatracker.ietf.org/doc/draft-ietf-sfc-proof-of-transit/
<https://datatracker.ietf.org/doc/draft-ietf-sfc-proof-of-transit/>

> again), but that's still what I expect.

Agreed, added following line:
The group key management protocol related security considerations discussed
in Section 8 of [RFC4046] needs to be taken into consideration.

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

I think by mistake we pointed to Section 5.6, the intention was to point to
RFC8333 which discusses various modes including client/server mode with
pre-shared keys.

Fixed text.

Cheers,
-Tiru