Integrity Protection for the Network Service Header (NSH) and Encryption of Sensitive Context Headers
draft-ietf-sfc-nsh-integrity-09

Note: This ballot was opened for revision 06 and is now closed.

Martin Vigoureux Yes

Alvaro Retana No Objection

Comment (2021-07-12 for -06)
(1) Given the required behavior specified in the Security Considerations section...

   NSH data are exposed to several threats:

   o  A man-in-the-middle attacker modifying the NSH data.

   o  Attacker spoofing the NSH data.

   o  Attacker capturing and replaying the NSH data.

   o  Data carried in Context Headers revealing privacy-sensitive
      information to attackers.

   o  Attacker replacing the packet on which the NSH is imposed with a
      bogus packet.

   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.  The Base and Service Path headers are not
   encrypted.


Why doesn't this document formally update rfc8300?  Concerns that eventually led to this solution have been expressed for several other documents, including rfc8459 and rfc8979.

It looks like the WG didn't consider the question of Updating the base NSH specification.  I believe that this document specifies a required update to NSH, and would like the WG to consider formally Updating rfc8300.  [My search of the archive didn't find any related discussion -- did I miss it?]

[Even though I consider this omission a serious oversight, I don't think this issue raises to the level of a DISCUSS.]


(2) §3: I find the use of normative language to describe requirements (that are met in this same document) not the best use of rfc2119 language because any interoperability concerns would result from the specification itself and not the requirements.

The use of rfc2119 keywords to describe requirements may result in confusion.  For example, "The solution MAY provide integrity protection for the Base Header." -- as described later, protecting the Base Header is optional, but the solution *does* provide integrity protection for it.  IOW, the specification is what is reflected in the requirement, but referring to the solution, not the protection: providing integrity protection is not optional, using it is.  A better working would be: "The solution must provide optional integrity protection for the Base Header."

Benjamin Kaduk (was Discuss) No Objection

Comment (2021-09-14 for -08)
Thanks for the updates in the -07 and -08; I think we found a reasonable
way to address the issues that came up in my earlier review.

I turned most of my comments into a github PR against
https://github.com/boucadair/draft-ietf-sfc-nsh-integrity/ since that
repo was referenced in previous responses to comments.  The tip of the
branch I forked off seems to have corresponded to only the -07, though,
so I'm not sure if I should have gone somewhere else.  (I made a
separate first commit that just syncs down the -08 from the
datatracker.)
The PR is
https://github.com/boucadair/draft-ietf-sfc-nsh-integrity/pull/6 and the
actual "new" (non-08) changes are viewable via
https://github.com/boucadair/draft-ietf-sfc-nsh-integrity/compare/99c6484fbe85af7179086ab243c88813e2b47b74..kaduk:kaduk-review?expand=1

Section 4.4

   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
   addresses the problem of synchronization of keying material.

I included a suggestion in my pull request, but I want to call out as a
potentially significant change that the key identifier should identify
not just the key material but also the algorithms that they key is to be
used with.

Section 5.1

   Nonce Length:  Carries the length of the Nonce.  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 depending on the encryption algorithm used to encrypt
      the Context Headers.

I included this in my PR, but want to call out that this last sentence
seems harmful.  It seems vastly preferrable to have "the nonce length
field is zero" indicate one specific thing, rather than having two
different possibilities for that situation, as this text appears to do.
I know that my earlier comments proposed a scenario where an AEAD might
validly encrypt with a zero-length nonce, but I don't think we should
support that at the cost of losing the clear signal that the encrypted
context headers are (not) present.

Section 7.3

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

I included some text in my pull request, but want to call out as
important that this description does not specify what is to be used as
the "additional authenticated data" AEAD input.  (I assume the empty
string is intended, though other choices are valid.)

Section 7.5

   When an SFC data plane element conforms to this specification, it
   MUST ensure that a "MAC and Encrypted Metadata" Context Header is
   included in a received NSH packet.  [...]

This doesn't quite seem like the right condition -- this text sounds
like once you implement this context header you have to require that it
appears in all incoming packets, which breaks interoperability with
senders that don't produc this contxt header.

Section 9

   The secret key (K) must have an expiration time assigned as the
   latest point in time before which the key may be used for integrity
   protection of NSH data and encryption of Context Headers.  Prior to
   the expiration of the secret key, all participating NSH-aware nodes
   SHOULD have the control plane distribute a new key identifier and
   associated keying material so that when the secret key is expired,
   those nodes are prepared with the new secret key.  This allows the
   NSH imposer to switch to the new key identifier as soon as necessary.
   It is RECOMMENDED that the next key identifier and associated keying
   material be distributed by the control plane well prior to the secret
   key expiration time.

I note that draft-irtf-cfrg-aead-limits offers some guidance on how
often to rekey (though it gives data-based criteria, not time-based
ones), if that is potentially relevant.

Erik Kline No Objection

Comment (2021-07-12 for -06)
[S7.{2,3}] [question]

* Is the timestamp a part of the input to the MAC/encrypted metadata
  generation?  If so, perhaps consider adding an explicit mention
  (perhaps I missed it).

  For that matter, I can't quite tell of some normalized version of the
  MAC#1/2 header overall is part of the input to the MAC/AAD generation
  or not.

Francesca Palombini No Objection

John Scudder No Objection

Comment (2021-07-14 for -06)
1. Section 4.2

   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.

As written, this means that each of these quantities is a 32 bit string, even P and A. I think you don’t mean that. If you mean each quantity is a string of octets, then move the hyphen: “takes as input four octet-strings“. (In the unlikely event you really do mean each quantity is a string of four octets, then “… takes as input four strings, each of four octets.”)

2. Section 9

Regarding your use of the term “man-in-the-middle attacker”, you might want to take into consideration that this language may be seen as exclusionary by some readers, see also https://www.ietf.org/about/groups/iesg/statements/on-inclusive-language/. I’ve seen “on-path attacker“ used as a replacement in some cases, but I understand there may be nuances that make this inappropriate for some uses; it also appears you might just be able to use “attacker” in your case. The RFC Editor may have further guidance.

3. Section 9.1

You use “should“ in several places. As written, it isn’t clear if you’re indicating expectation, or requirement. After reading the whole section, I think you’re indicating requirement. This seems like a good place for use of the RFC 2119 style SHOULD keyword.

Lars Eggert No Objection

Comment (2021-07-12 for -06)
Section 1. , paragraph 6, 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).  Thus, the NSH
>    does not have to rely upon an underlying transport encapsulation for
>    security and confidentiality.

Given that, I am surprised this document doesn't formally update RFC8300?

Section 6. , paragraph 16, comment:
>       This timestamp format is affected by leap seconds.  The timestamp
>       represents the number of seconds elapsed since the epoch minus the
>       number of leap seconds.

Any particular reason why leap seconds are being excluded here? This is unusual
and also requires care with synchronized clocks (as identified below).

Found terminology that should be reviewed for inclusivity:
 * Term "master"; alternatives might be "active", "central", "initiator",
   "leader", "main", "orchestrator", "parent", "primary", "server".
 * Term "man"; alternatives might be "individual", "people", "person".
See https://www.rfc-editor.org/part2/#inclusive_language for background and
more guidance.

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

"SFC ", paragraph 2, nit:
>  Also, this specification allows to encrypt sensitive metadata that is carrie
>                                  ^^^^^^^^^^
Did you mean "encrypting"? Or maybe you should add a pronoun? In active voice,
"allow" + "to" takes an object, usually a pronoun.

Section 2. , paragraph 7, nit:
> ng a service path. The NSH allows to share context information (a.k.a., metad
>                                   ^^^^^^^^
Did you mean "sharing"? Or maybe you should add a pronoun? In active voice,
"allow" + "to" takes an object, usually a pronoun.

Section 9. , paragraph 10, nit:
> ts This document was edited as a follow up to the discussion in IETF#104: ht
>                                  ^^^^^^^^^
This noun is spelled as one word.

Martin Duke No Objection

Comment (2021-07-12 for -06)
Two nits:

Section 3 frequently uses the passive voice (“is instructed” “may be instructed”) and that makes it hard to understand who is doing the instruction. Please add subjects to at least the first of these sentences.

You frequently use the structure “allows to [verb]”. There are many ways to fix this gramatically, but recommend simply deleting “allows to” and conjugating the verb correctly.

Murray Kucherawy (was Discuss) No Objection

Comment (2021-07-26 for -07)
Thanks for the discussion about updating RFC 8300.

Only nits to add, given the thorough treatment already given by others:

Section 4.1.2: "The first level of assurance where all NSH data ..." -- add "is" before "where"?  And the same issue in the next paragraph.

Section 5.2: s/Coves/Covers/

Robert Wilton No Objection

Roman Danyliw (was Discuss) No Objection

Comment (2021-09-18 for -08)
No email
send info
Thanks for addressing my COMMENT and DISCUSS items.

Warren Kumari No Objection

Comment (2021-07-14 for -06)
I support Roman and Eric's DISCUSS points.

I also found:
"Note that some transport encapsulations (e.g., IPsec) only provide hop-by-hop security between two SFC data plane elements (e.g., two Service Function Forwarders (SFFs), SFF to SF) and do not provide SF-to-SF security of NSH metadata.  For example, if IPsec is used, SFFs or SFs within a Service Function Path (SFP) that are not authorized to access the privacy-sensitive metadata will have access to the metadata."
to be incredibly hard to read/parse. I think that my confusion comes in around the "that are not authorized to access the privacy-sensitive metadata will have access to the metadata." test, and thing that the last sentence should be rewritten to start with "Because IPsec does not... it exposes privacy-sensitive metadata to..."

Also, thanks to Jürgen Schönwälder for the OpsDir review, and to the authors for addressing the comments.

Zaheduzzaman Sarker No Objection

Comment (2021-07-13 for -06)
Thanks for the efforts on this specification.

I have following non-blocking comments those I believe would improve the document if addressed -- 

* I agree with Alvaro and Lars's comment about updating 8300. Would like to get response(s) to their comments.

* I think it will be helpful to explicitly mention if integrity and confidentiality by the transport encapsulation is needed or not when this specification is in use. This specification definitely says that one does not need to relay on the service provided by the transport encapsulation but it does not says that those services are not longer required.  

* Section 1 : says -
    "This specification fills that gap.  Concretely, this document adds
   integrity protection and optional encryption of sensitive metadata
   directly to the NSH (Section 4);"
  
  Does this specification extends the use of NSH in multiple SFC domain? My little understanding of NSH says it is SFC domain specific and within one SFC domain the devices a vetted to be trusted. I think it will be very helpful to add zest from the section 3.2.1. of I-D.arkko-farrell-arch-model-t here.

* Section 6 : 

   The epoch is 1970-01-01T00:00Z in UTC time.  Note this epoch value
      is different from the one used in Section 6 of [RFC5905].
   
   It would be great if we can add the implications of the difference. Now I don't know what it means.

Éric Vyncke (was Discuss) No Objection

Comment (2021-09-13 for -08)
Thank you for the work put into this document. As already communicated by email, I am now clearing my previous DISCUSS ballot (kept below only for archiving purposes), I am also trusting the responsible AD about my previous COMMENT points.

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

I hope that this helps to improve the document,

Regards,

-éric

PS: your reply was lost in the ‘fury’ ot the IETF week, sorry about that and please thank your AD, Martin Vigoureux, for sending me today a gentle nudge...

== previous DISCUSS (for archive only as the current revision addresses all the points) ==

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 ?

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 ?