[yang-doctors] Yangdoctors early review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-04

[Martin Björklund via Datatracker <[e-mail]>]

Reviewer: Martin Björklund
Review result: Not Ready

This is my YANG doctor review of
draft-ietf-i2nsf-sdn-ipsec-flow-protection-04.  The review focuses on
YANG-specifics only, since I am not very familiar with the technology
that is modelled.


o  All three YANG modules should follow the common template for IETF
   YANG modules found in RFC 8407.

   Specifically, fix the copyright text and template for RFC 2119
   langauge.  (if you want help with this, download the latest checked
   in version of pyang from github, and run pyang --ietf
   --max-line-length 69)


o  The YANG modules are difficult to read b/c of the formatting.  I
   had to run:

     pyang -f yang --yang-line-length 69 <FILE>

   Even after this, you need to manually break long lines so that they
   fit into the I-D / RFC format.

   Note that the RFC editor now enforce this format, so that all IETF
   modules have a consistent look and feel.   I suggest you run this
   command, fix the long lines manually, and put the result in the
   draft.


o  In general, it would help if the definitions had "reference"
   statements to the relevant sections in the underlying RFCs.

   Also use references rather than comments or descriptions:

    /* This is defined by XFRM */

    /* RFC 4301 ASN1 notation. Annex C*/

        description
          "RFC 7619";

      description
        "List of local ports. When the upper-layer-protocol is ICMP
        this 16 bit value respresents code and type as mentioned in
        RFC 4301";



o  In general, almost all nodes need better descriptions.  For example

        leaf initiator-ikesa-spi {
          type uint64;
          description
            "Initiator's IKE SA SPI";
        }

   This description doesn't add much info...


o  A general comment; in IETF YANG modules, lower-case-with-hyphens
   are preferred.  In your module you sometimes use lowCamelCase,
   sometimes underscore_as_separator and sometimes UPPERCASEONLY.

   You can use pyang --lint --lint-ensure-hyphenated-names to find all
   occurrences.


o  Top-level structure and naming

  Your models define this top-level structure:
  
  +--rw ikev2
  +--rw ietf-ipsec

  First of all, is the intention that a device implements one of these
  moules, or can it implement both?

  I wonder if a better structure would be:

  +--rw ipsec      // note name suggestion
     +--rw ikev2


  The name "ietf-ipsec-ikeless" sounds a bit odd.  It seems to imply
  that if IKEv2 is implemented, this module is not used.  But I
  suspect that is not true?


o  Use of groupings

  In the module ietf-ipsec-ike you have a set of groupings that are
  used only once.  I suggest you remove these groupings.  I assume
  that they are not intended to be re-used by other modules.  (if they
  are, they need better descriptions for how they are supposed to be
  used)


o  PAD

  RFC 4301 says that the PAD is a user-ordered database.  You use a
  uint64 as the key into the "pad-entry" list.

  I suggest that you make this list "ordered-by user", and use a
  symbolic string as key instead:

    list pad-entry {
      key name;
      ordered-by user;

      leaf name {
        type string;
        ...
      }
      ...
    }


o  PAD leafs

  I notice that in your model, all leafs in the pad-entry list are
  optional, except for "my-identifier", and w/o default values.  Is
  this correct?  If it is, I suggest you add description text that
  explains what the behavior is if these leafs are not configured.
  For example, what does it mean if "pad-auth-protocol" isn't
  configured.


o  Names...

  Unless "my-identifier" is a well-known term in this domain, I
  suggest "auth-identifier".

  Instead of "pad-auth-protocol" I suggest "auth-protocol" - it is
  already scoped in a pad-entry so we know it is a pad.

  Some leafs in "ike-conn-entry" are called "ike-..."
  (e.g. ike-fragmentation).   This seems a bit random, e.g., "version"
  is not called "ike-version".  I suggest you remove the "ike-" prefix
  from these nodes.

  Some abbreviations should probably be expanded, e.g., "oaddr",
  "bmp", "dport", "spi", "espencap" etc.


o  auth-method model

  Perhaps change:

    container auth-method {
      leaf auth-m { ... }
      ...
    }
     
  to:

    container peer-authentication {
      leaf auth-method { ... }
      ...
    }
     

  In the case of "digitial-signature", are all leafs in that container
  optional?  Is is ok to configure none of them?

  Some of these leafs refer to file names.  How are these files
  supposed to be handled - specifically, if I configure a file name
  here, how do I control the file?


o  NACM

  Consider using nacm:default-deny-all on the "secret" and "key-data"
  etc leafs.


o  pad-auth-protocol

  This leaf can currently only have one value, "ikev2".  But at the
  same time, it is located under the top-level container "/ikev2".  Is
  the intention that there might be other pad-auth-protocols in the
  future?  If so, is the top-level name "/ikev2" a good name?

  Perhaps remove this leaf?  And/or rename the top-level container.


o  SPD

  RFC 4301 says that the SPD is ordered, consistent with the use of
  Access Control Lists (ACLs) or packet filters in firewalls,
  routers, etc.  You use a uint64 as the key into the "spd-entry" list.

  I suggest that you make this list "ordered-by user", and use a
  symbolic string as key instead:

    list spd-entry {
      key name;
      ordered-by user;

      leaf name {
        type string;
        ...
      }
      ...
    }

  (Compare also with RFC 8519)


o  anti-replay-window

  Should this leaf have a default value?


o  SPD model

  You have:

          +--rw names* [name]
          |  +--rw name-type?   ipsec-spd-name
          |  +--rw name         string

  This is not easy to guess what it is.  The description gives at
  least a hint, so perhaps:

     list policy {
       key name;

       leaf name { ... }
       leaf type { ... }
     }

  ... but this probably needs a better description as well.


o  ike-conn-entry/version

  This leaf (if it really is needed) should have a default statement.


o  Usage of yang:timestamp

  There are a couple of nodes that use yang:timestamp, both for
  configuration and operational state.  In both cases it is probably
  not the right the type to use, but the description of these nodes
  are not precise enough for me to recommend another type.  As a first
  step, please re-read the definition of yang:timestamp in RFC 6991.