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

From: Éric Vyncke via Datatracker <[email-masked]>
Date: Tue, 13 Jul 2021 05:17:41 -0700

Éric Vyncke 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:
----------------------------------------------------------------------

Thank you for the work put into this document.

Special thanks to Greg Mirsky for his shepherding especially about his summary
of the WG consensus.

Please find below some blocking DISCUSS point (which should be easy to fix),
some non-blocking COMMENT points (but replies would be appreciated), and one
nit.

I hope that this helps to improve the document,

Regards,

-éric

== DISCUSS ==

I failed to spot the order of the operations for the integrity and
confidentiality operations, e.g., I did not find on what the HMAC is computed:
in the unencrypted or encrypted field ?

-- Section 5.1 --
What is the unit of "key length", I assume a length expressed in octets but it
is not specified.

-- Section 7.2 --
What is the "A" used in the HMAC computation ?

The formula specifies HMAC-SHA-256-128() but what if another HMAC is used ?
Section 7.3 use MAC() which is more flexible.

As the MAC field is included in the integrity protected header, please specify
the value of this field when computing the HMAC (I assume 0 but let's be clear)

-- Section 7.5 --
What is the expected behavior when a NSH does not contain a "MAC and Encrypted
Metadata" Context Header ? §1 hints to packet drop ? Should there be a local
policy for this case ?

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I second Alvaro's and Lars' point about formally updating RFC 8300.

Quite often in the text "privacy-sensitive metadata" is used but encryption is
not only required for privacy but also to protect operational data from
attackers (i.e., not linked to privacy), is there a real need to qualify
"metadata" with "privacy-sensitive", i.e., "confidential metadata" may be more
appropriate ?

-- Section 4.1.1 --
The use of 'metadata' (notably in table 1) for 'context headers' is confusing
for a first-time reader. Any chance to be consistent and only use 'context
headers' ?

-- Section 4.2 --
At first reading, I am confused by the use of the word 'secret key' for what
appears to be a 'shared key'. Or is this 'secret key' never shared and simply
used to generate the secondary keys, which are then shared ? Even if discussed
later, some early explanations would be welcome.

-- Section 5.1 --
Is there a good reason why the field name "key length" is not "key ID length" ?

I also find very strange to define "Length: variable" as the header field
itself as a fixed length, simply state "unsigned integer".

As timestamp can include fractions of second, which is a good thing, then
please reword "expressed in seconds relative" to indicate that fraction of
second is included.

The 32-bit epoch will wrap around in 2036, should this I-D already consider
what to do at that moment ?

-- Section 8 --
Indeed MTU is always an interesting 'problem' but how is this extension
different than plain NSH ? The NSH neither increases nor decreases on the fly
with this document.

== NITS ==

-- Section 3 --
Should 'figure 1' be 'table 1' per consistency with section 4.1.1 ?

---

From: <[email-masked]>
Date: Tue, 13 Jul 2021 16:54:40 +0000

Hi Eric, 

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Éric Vyncke via Datatracker [mailto:[email-masked]]
> Envoyé : mardi 13 juillet 2021 14:18
> À : The IESG <[email-masked]>
> Cc : [email-masked]; [email-masked];
> [email-masked]; [email-masked]; [email-masked]
> Objet : Éric Vyncke'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.)
> 
> 
> Special thanks to Greg Mirsky for his shepherding especially about
> his summary of the WG consensus.
> 
> Please find below some blocking DISCUSS point (which should be easy
> to fix), some non-blocking COMMENT points (but replies would be
> appreciated), and one nit.
> 

[Med] Isn't this covered by this text: 

   Using the secret key (K) and authenticated encryption algorithm, the
   NSH imposer encrypts the Context Headers (as set, for example, in
   Section 3), computes the message integrity for the target NSH data,
   and inserts the resulting payload in the "MAC and Encrypted Metadata"
   Context Header (Section 5).  The entire Context Header carrying a
   privacy-sensitive metadata is encrypted (that is, including the MD
   Class, Type, Length, and associated metadata of each Context Header).

> 
> -- Section 5.1 --
> What is the unit of "key length", I assume a length expressed in
> octets but it is not specified.

[Med] Yes. Fixed. Thank you for catching this. 

> What is the "A" used in the HMAC computation ?

[Med] This is: Additional Authenticated Data (A) (mentioned in 4.2).

> The formula specifies HMAC-SHA-256-128() but what if another HMAC is

[Med] Please note that the text says: "can be illustrated as follows:". 

That is for illustration purposes. 

> As the MAC field is included in the integrity protected header,
> please specify the value of this field when computing the HMAC (I
> assume 0 but let's be clear)
> 
> -- Section 7.5 --
> What is the expected behavior when a NSH does not contain a "MAC and
> Encrypted Metadata" Context Header ? §1 hints to packet drop ?
> Should there be a local policy for this case ?

[Med] This is covered here: 

   It MUST log an error at least once per the
   SPI for which the "MAC and Encrypted Metadata" Context Header is
   missing.

> Quite often in the text "privacy-sensitive metadata" is used but
> encryption is not only required for privacy but also to protect
> operational data from attackers (i.e., not linked to privacy), is
> there a real need to qualify "metadata" with "privacy-sensitive",
> i.e., "confidential metadata" may be more appropriate ?

[Med] We use this term because we can easily argue why we included statement such as:

 " In an SFC-enabled domain where the above attacks are possible, (1)
   NSH data MUST be integrity-protected and replay-protected, and (2)
   privacy-sensitive NSH metadata MUST be encrypted for confidentiality
   preservation purposes."

Other types can be encrypted as per: 

   Classifier(s), NSH-aware SFs, and SFC proxies are instructed with the
   set of Context Headers (privacy-sensitive metadata, typically) that
   must be encrypted.

> 
> -- Section 4.1.1 --
> The use of 'metadata' (notably in table 1) for 'context headers' is
> confusing for a first-time reader. Any chance to be consistent and
> only use 'context headers' ?

[Med] Will see how to make this better, but please that we do have the following before table 1: 

"Context Header(s): Carries metadata..."

> 
> -- Section 4.2 --
> At first reading, I am confused by the use of the word 'secret key'
> for what appears to be a 'shared key'. Or is this 'secret key' never
> shared and simply used to generate the secondary keys, which are
> then shared ? Even if discussed later, some early explanations would
> be welcome.

[Med] We are using similar wording as in RFC7518.

> Is there a good reason why the field name "key length" is not "key

[Med] Only for esthetic matters of the figures. Can fix that if you prefer. 

|Key ID Length| vs | Key Length |

> 
> I also find very strange to define "Length: variable" as the header
> field itself as a fixed length, simply state "unsigned integer".

[Med] We are echoing Section 2.5.1 of RFC8300.

> 
> As timestamp can include fractions of second, which is a good thing,
> then please reword "expressed in seconds relative" to indicate that
> fraction of second is included.

[Med] OK.

> 
> The 32-bit epoch will wrap around in 2036, should this I-D already
> consider what to do at that moment ?

[Med] Hmm. We say that it wraps in 2106 :-)

> 
> -- Section 8 --
> Indeed MTU is always an interesting 'problem' but how is this
> extension different than plain NSH ? The NSH neither increases nor
> decreases on the fly with this document.

[Med] It varies as we can add/strip context headers as the packet progress in the service chain. 

> Should 'figure 1' be 'table 1' per consistency with section 4.1.1 ?

[Med] It should. Will be fixed. Thanks. 

_________________________________________________________________________________________________________________________

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: "Eric Vyncke (evyncke)" <[email-masked]>
Date: Wed, 14 Jul 2021 11:11:57 +0000

Hello Med,

Thank you for your prompt reply.

See below for EV>, still holding my DISCUSS mainly around the "how to compute the MAC as it includes the MAC field itself"

Regards

-éric

-----Original Message-----
From: "[email-masked]" <[email-masked]>
Date: Tuesday, 13 July 2021 at 18:54
To: Eric Vyncke <[email-masked]>, The IESG <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: RE: Éric Vyncke's Discuss on draft-ietf-sfc-nsh-integrity-06: (with DISCUSS and COMMENT)

    Hi Eric, 

    Please see inline. 

    Cheers,
    Med

EV> the text could be made clearer by using "then" rather than simple ","
EV> but I STRONGLY suggest to move this explanation earlier at the very beginning of section 7 (or 7.1) and not like now in section 7.3
EV> BTW, this is indeed the expected order to avoid DoS

EV> __

EV> fair enough, suggest to write that the terminology of section 4.2 is used there.

EV> could also be made more obvious with " The Message Authentication Code (T) computation process for additional authentication text (A) with HMAC-SHA-256-128() can be illustrated as follows:"

    > assume 0 but let's be clear)

EV> still waiting for an answer on this issue (so keeping my DISCUSS ballot)

EV> honestly, not clear from the text. Please consider adding "In this case, it MUST..."

EV> let's say that we disagree __ but this is non-blocking anyway

EV> I fail to see this wording in RFC 7518

EV> this would be clearer IMHO but editorial hence non-blocking

EV> like above, I fail to see this similarity in the section 2.5.1 of RFC 8300 "Indicates the length of the variable-length metadata, in bytes."

    [Med] OK.

EV> I should have spot the different epoch ;-)

EV> Amazing that this was accepted by the IESG in RFC 8300 years ago... when net-pgm RFC had to remove the header insertion.

---

From: tirumal reddy <[email-masked]>
Date: Wed, 14 Jul 2021 17:28:21 +0530

Hi Eric,

We will update text in Section 7.2 as follows to address the 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 then
inserts the computed digest in the MAC field in the "MAC and Encrypted
Metadata" Context Header.

Cheers,
-Tiru

On Wed, 14 Jul 2021 at 16:42, Eric Vyncke (evyncke) <[email-masked]>
wrote:

> See below for EV>, still holding my DISCUSS mainly around the "how to
> compute the MAC as it includes the MAC field itself"
>
> Regards
>
> -éric
>
> Cc: "[email-masked]" <
> [email-masked]>, "[email-masked]" <
> [email-masked]>, "[email-masked]" <[email-masked]>, "
> [email-masked]" <[email-masked]>
> EV> but I STRONGLY suggest to move this explanation earlier at the very
> beginning of section 7 (or 7.1) and not like now in section 7.3
> EV> could also be made more obvious with " The Message Authentication Code
> (T) computation process for additional authentication text (A) with
> HMAC-SHA-256-128() can be illustrated as follows:"
>
> EV> like above, I fail to see this similarity in the section 2.5.1 of RFC
> 8300 "Indicates the length of the variable-length metadata, in bytes."
>     >
> EV> Amazing that this was accepted by the IESG in RFC 8300 years ago...
> when net-pgm RFC had to remove the header insertion.
>
>     >
>     > == NITS ==
>     >
>     > -- Section 3 --
>     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,
>     This message and its attachments may contain confidential or
> privileged information that may be protected by law;

---

From: <[email-masked]>
Date: Thu, 15 Jul 2021 04:57:46 +0000

Hi Eric,

A version that integrates your review can be tracked at: https://tinyurl.com/nsh-integrity-latest. 

Please see inline for more context. 

Cheers,
Med

> De : Eric Vyncke (evyncke) [mailto:[email-masked]]
> Envoyé : mercredi 14 juillet 2021 13:12
> À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>; The
> IESG <[email-masked]>
> [email-masked]; [email-masked]
> Objet : Re: Éric Vyncke's Discuss on draft-ietf-sfc-nsh-integrity-
> 06: (with DISCUSS and COMMENT)
> 
> Hello Med,
> 
> See below for EV>, still holding my DISCUSS mainly around the "how
> to compute the MAC as it includes the MAC field itself"

[Med] Updated the text to:

   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.

> Cc: "[email-masked]" <draft-ietf-sfc-nsh-
> [email-masked]>, "[email-masked]" <[email-masked]>,
> "[email-masked]" <[email-masked]>, "[email-masked]"
> <[email-masked]>
[..]
>     > confidentiality operations, e.g., I did not find on what the
> HMAC is
>     > computed:
> EV> beginning of section 7 (or 7.1) and not like now in section 7.3
> BTW,
> EV> this is indeed the expected order to avoid DoS
> 

[Med] I really struggled to see how to mention this text earlier, but it is currently where it should be. It can be in 7.1 is this is about generic considerations. It is in this section because this is where we discussion encryption. 

> EV> fair enough, suggest to write that the terminology of section

[Med] Updated the text for better clarity. 

>     >
>     > The formula specifies HMAC-SHA-256-128() but what if another
> HMAC is
>     > used ?
> 
> EV> could also be made more obvious with " The Message
> Authentication Code (T) computation process for additional
> authentication text (A) with HMAC-SHA-256-128() can be illustrated
> as follows:"

[Med] Deal. 

> EV> still waiting for an answer on this issue (so keeping my DISCUSS
> EV> ballot)
> 

[Med] Added this text: 

   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.

> EV> honestly, not clear from the text. Please consider adding "In

[Med] Updated the text. It reads now:

   When an SFC data plan element conforms to this specification, it MUST
   ensure that a "MAC and Encrypted Metadata" Context Header is included
   in a received NSH packet.  The imposer MUST silently discard the
   packet and MUST log an error at least once per the SPI if at least
   one of the following is observed:

   o  the "MAC and Encrypted Metadata" Context Header is missing,

   o  the enclosed key identifier is unknown or invalid (e.g., the
      corresponding key expired),

   o  the timestamp is invalid (Section 7.4).

> 
>     >
>     >
>     > --------------------------------------------------------------
> ------
>     > --
>     > COMMENT:
>     > --------------------------------------------------------------
> ------
>     > --
>     >
>     >

[Med] I changed many occurrences of privacy-sensitive to sensitives but maintained them when zooming on privacy was the intent. 

> EV> I fail to see this wording in RFC 7518

[Med] RFC 7518 says for example:

   The authenticated encryption algorithm takes as input four octet
   strings: a secret key K, a plaintext P, Additional Authenticated Data
            ^^^^^^^^^^^^^^ 
   A, and an Initialization Vector IV.  The authenticated ciphertext
   value E and the Authentication Tag value T are provided as outputs.
   The data in the plaintext are encrypted and authenticated, and the
   Additional Authenticated Data are authenticated, but not encrypted.

> 
>     >
>     > -- Section 5.1 --
>     > Is there a good reason why the field name "key length" is not
> "key
>     > ID length" ?
> 

[Med] Made the change. Thanks. 

> EV> like above, I fail to see this similarity in the section 2.5.1
> of RFC 8300 "Indicates the length of the variable-length metadata,
> in bytes."

[Med] that's basically says variable. Updated the text to use exact wording in 8300.

> EV> Amazing that this was accepted by the IESG in RFC 8300 years
> ago... when net-pgm RFC had to remove the header insertion.
> 
>     >
>     > == NITS ==
>     >
>     > -- Section 3 --
> ____________________________________________________________________
> _____________________________________________________
> 
>     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
>     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.
>