A YANG Data Model for IPsec Flow Protection Based on Software-Defined Networking (SDN)
draft-ietf-i2nsf-sdn-ipsec-flow-protection-14

Firstly, thank you for addressing the OpsDir review comments, and also to Menachem Dodge for having provided them. This week has been hectic, and so I've been relying on the directorate to help with my balloting...

I support Erik's DISCUSS, and think it should be easy to address...

Thank you for the work put into this document.

Please find below one blocking DISCUSS point, some non-blocking COMMENT points, and some nits.

I support Erik Kline's DISCUSS points as well as Ben Kaduk's DISCUSS point about vendor-specific.

I hope that this helps to improve the document,

Regards,

-éric
== COMMENTS ==

-- Section 5 --
Isn't there also the 'load sharing by IPSEC-only NSF' a use case ? Where simple ECMP could split traffic to several IPSec-only NSFs configured with the same parameters (SADB, SPD, ...) (anti-reply being probably impossible to offer though)

-- Section 6 --
It seems that this section sometimes uses the term 'model' instead of 'module'.

-- Section 6.1 --
I would have preferred to use 'local-prefix' rather than 'local-subnet' as the latter is old looking IPv4 subnet.

The "state" part does not seem to contain the current state of the IKE finite state machine, is it on purpose ?

-- Section 8.2 --
As IKEv2 provides "perfect forward secrecy" should this section mandating the use of a secure channel between SDN and the NSF also with "perfect forward secrecy" and same reasoning for the encryption algorithms of course.

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.

Thanks for the discussion that resulted in that we could actually simplify things and avoid need for configuration.

There has already been some extensive discussion regarding the YANG models and the YANG model structure, so thank you for accommodating these changes and making the modules more widely reusable.

I note that you have included the YANG modules as appendices rather than in the main body of the document.  Given that the YANG module text is normative, and the main component of this specification, it would be better to follow the convention from other existing RFCs containing YANG modules and document the YANG modules in the main body of the document, probably as a new section before section 7 (IANA considerations).

Regards,
Rob