Subject: [sfc] Benjamin Kaduk's No Objection on draft-ietf-sfc-nsh-integrity-08: (with COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Tue, 14 Sep 2021 14:45:20 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-sfc-nsh-integrity-08: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-sfc-nsh-integrity/

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

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.

---

From: <[email-masked]>
Date: Wed, 15 Sep 2021 12:00:17 +0000

Hi Ben,

The suggestions look good to me. 

I made some additional edits + added a pointer to the cfrg I-D. The full diff can be seen at: https://tinyurl.com/nsh-integrity-latest 

Will proceed with the submission of this version tomorrow.

Many thanks for your effort. 

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk via Datatracker [mailto:[email-masked]]
> Envoyé : mardi 14 septembre 2021 23:45
> À : The IESG <[email-masked]>
> Cc : [email-masked]; [email-masked];
> [email-masked]; [email-masked]; [email-masked]
> Objet : Benjamin Kaduk's No Objection on draft-ietf-sfc-nsh-integrity-
> 08: (with COMMENT)
> 
> https://github.com/boucadair/draft-ietf-sfc-nsh-
> integrity/compare/99c6484fbe85af7179086ab243c88813e2b47b74..kaduk:kaduk-
> review?expand=1
> 
> Section 4.4
> 

_________________________________________________________________________________________________________________________

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: Benjamin Kaduk <[email-masked]>
Date: Wed, 15 Sep 2021 15:52:48 -0700

Hi Med,

Thanks for taking the PR and making the further edits.

I don't see any changes in §7.5, though -- is it really the expectation
that any node that implements this specification at all is going to be
required to expect that all incoming NSH-bearing messages include the new
context header?  That doesn't really seem deployable.

-Ben

---

From: <[email-masked]>
Date: Thu, 16 Sep 2021 06:39:28 +0000

Hi Ben, 

I didn't touch that part given that sentence was added to address a comment from Roman and other ADs to further insist that this spec is optional in the overall SFC arch.  

The text does not say "implements" but "conforms" which includes also the instructions received from the control plane to enable the validating checks (see Section 3, for example). Overall, 
Section 7 should be positioned in the context called in Section 4.5:

==
   ...
   An SFC data plane element that needs to check the integrity of the
                             ^^^^^^^^^^
   NSH data uses MAC_KEY and the HMAC algorithm for the key identifier
   being carried in the NSH.

   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.

   Section 7 specifies the detailed procedure.
==

Cheers,
Med

> De : sfc [mailto:[email-masked]] De la part de Benjamin Kaduk
> Envoyé : jeudi 16 septembre 2021 00:53
> À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>
> Cc : [email-masked]; [email-masked]; sfc-
> [email-masked]; The IESG <[email-masked]>; [email-masked]
> Objet : Re: [sfc] Benjamin Kaduk's No Objection on draft-ietf-sfc-nsh-
> integrity-08: (with COMMENT)
> 
> Hi Med,
> 
> required to expect that all incoming NSH-bearing messages include the
> new context header?  That doesn't really seem deployable.
> 
> -Ben
> 
> > I made some additional edits + added a pointer to the cfrg I-D. The
> > full diff can be seen at: https://tinyurl.com/nsh-integrity-latest
> >
> Envoyé
> > > : mardi 14 septembre 2021 23:45 À : The IESG <[email-masked]> Cc :
> > > [email-masked]; [email-masked];
> > > [email-masked]; [email-masked]; [email-masked] Objet :
> > > Benjamin Kaduk's No Objection on draft-ietf-sfc-nsh-integrity-
> > > 08: (with 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.)
> > >
> > >
> > > 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.
> > >
> > > 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
> > > 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
> > >
> > > 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
> > >
> > > 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
> > >
> > > 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
> > >
> > ______________________________________________________________________
> > ___________________________________________________
> >
> > 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.
> _______________________________________________
> sfc mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/sfc