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

[Benjamin Kaduk via Datatracker <[e-mail]>]

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.