[I2nsf] Benjamin Kaduk's No Objection on draft-ietf-i2nsf-sdn-ipsec-flow-protection-13: (with COMMENT)

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

Benjamin Kaduk has entered the following ballot position for
draft-ietf-i2nsf-sdn-ipsec-flow-protection-13: 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 IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-i2nsf-sdn-ipsec-flow-protection/



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

A huge thanks for the many thoughtful updates in the -13; they do a great
job of resolving my discuss and comment points from the -12 and have really
improved the document a lot.

Section 5

   In terms of security, IKE case provides better security properties
   than IKE-less case, as discussed in section Section 8.  The main
   reason is that the NSFs generate the session keys and not the I2NSF
   Controller.

nit: s/section Section/Section/

Section 5.2

   In the IKE case, the I2NSF Controller MAY need to configure the
   affected NSF with the new IKEv2, SPD and PAD information.

nit: s/MAY/may/

   imply avoiding contact with the I2NSF Controller.  Finally, the I2NSF
   Controller MAY also need to send new parameters (e.g., a new fresh
   PSK for authentication) to the NSFs which had IKEv2 SAs and IPsec SAs
   with the affected NSF.

nit: s/MAY/may/

Section 6.1.2

      grouping lifetime {
        [...]
        leaf bytes {
          type yang:counter64;
          default 0;
          description
             "If the IPsec SA processes the number of bytes
             expressed in this leaf, the IPsec SA expires and
             SHOULD be rekeyed. The value 0 implies
             infinite.";

Since this is the *limit*, it should just be a uint64; the counter type
should be used for the statistics, not the configured limit.
My apologies for being sloppy in in writing my earlier comment; I can
see how you thought that I was suggesting to use the "counter64" type
here, but that was not my intent.

        list dscp-mapping {
          [...]
          leaf inner-dscp {
            type inet:dscp;
            description
              "The DSCP value of the inner IP packet. If this
              leaf is not defined it means ANY inner DSCP value";
          }

We should probably specify one way or the other if it's allowed to have
(e.g.) one element of this list that has both inner+outer DSCP values,
and another element that has only the outer value (to match "everything
else").  That is, is the "ANY inner DSCP value" really "any", or "any
not matching other configuration"?

Section 6.2.3

      typedef autostartup-type {
        [...]
          enum start {
            description
              "IKE/IPsec configuration is loaded
              and transferred to the NSF's kernel, and the
              IKEv2 based IPsec SAs are established
              immediately without waiting any packet.";

nit: "waiting for"

              container eap-method {
                when "../auth-method = 'eap'";
                leaf eap-type {
                  type uint64 {range "1 .. 4294967295"; }

[I don't really understand why we need uint64 vs uint32 with the
explicit range limit like that, but it's not harmful.]

Section 6.3.3

              container tunnel {
                when "../mode = 'tunnel'";
                uses nsfikec:tunnel-grouping;
                leaf-list dscp-values {
                  type inet:dscp;
                  description
                    "DSCP values allowed for packets carried over
                    this IPsec SA. If no values are specified, no
                    DSCP-specific filtering is applied";

nit: It might be worth a couple more words to clarify that this is for
ingress packets received, and thus the value that would be the "inner"
DSCP value in the other configuration we have for the DSCP mapping.

                container replay-window {

If I understand correctly, the 'w', 't', and 'b' paremeters herein are
related, with the gap t-b usually (always?) being of value w.  I don't
know enough about YANG constraints to say whether they should be used to
recognize this relationship, but even if not, some prose might be in
order.

Section 8.2

When people say that "encryption algorithms [...] MUST have, at least,
the same strength as the algorithms used [...]" we like to see clarity
on how the "strength" is measured.  In this case it's probably fairly
clear to most readers, but spelling out the bit strength of the
symmetric cipher is probably still worthwhile.