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

[Rafa Marin-Lopez <[e-mail]>]

Dear Martin:

Thank you very much for your review. Our comments inline.
> El 17 abr 2019, a las 17:54, Martin Björklund via Datatracker <[e-mail] <mailto:[e-mail]>> escribió:
> 
> 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)

Yes, thank you. We have performed the command and it clearly shows the lines we have to fix. It is solved now.
> 
> 
> 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.

We’re in process now, besides the text in the different sections of the I-D.
> 
> 
> 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”;

We are in process.

> 
> 
> 
> 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…

We are improving the descriptions.

> 
> 
> 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.

Done. We have fixed already this.

> 
> 
> 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?

A device will have to implement the ietf-ipsec-common and either

- ietf-ipsec-ike (IKE case)

or

- ietf-ipsec-ikeless (IKE-less case)

> 
>  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?

Yes, it implies precisely that: if ikev2 is implemented and ietf-ipsec-ikeless is not implemented in the device. We should clarify that better in the module description.

> 
> 
> 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)

Agree.

> 
> 
> o  PAD
> 
>  RFC 4301 says that the PAD is a user-ordered database.

Done.

>  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;
>        ...
>      }
>      ...
>    }

Ok, we will have to do that in other lists as well (spd and sad) We used an uint64 since we thought it was ok to point a specific entry in a database (this is what the list represents here) with an uint64.

> 
> 
> 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”.

In reality this could be removed since it is not required anymore. 
> 
>  Instead of "pad-auth-protocol" I suggest "auth-protocol" - it is
>  already scoped in a pad-entry so we know it is a pad.

Right.
> 
>  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.

Ok.
> 
>  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 { ... }
>      ...
>    }
> 
Done.
> 
>  In the case of "digitial-signature", are all leafs in that container
>  optional?  Is is ok to configure none of them?

This is a good question. If digital-signature is the authentication method some of the leaf nodes must be configured.
For example, the configuration will depend whether the controller selects raw public key authentication or certificates for example.
> 
>  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?

Could you clarify “control the file”? But yes, we agree that setting a file name may not make sense and just sending the file should be the right choice.

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

Ok, we will check this.
> 
> 
> 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”.

We have changed the container’s name to ike 

>  Is
>  the intention that there might be other pad-auth-protocols in the
>  future?

Yes.

>  If so, is the top-level name "/ikev2" a good name?

No, it is better to name it simply ike
> 
>  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)

Ok.


> 
> o  anti-replay-window
> 
>  Should this leaf have a default value?

We have observed in implementations a default windows size of 32
> 
> 
> 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.

Yes. Actually this is extracted from the RFC 4301 where they include a name. We thought to remove it. But considering your suggestion we can include a single name as the key for the list.

> 
> 
> o  ike-conn-entry/version
> 
>  This leaf (if it really is needed) should have a default statement.

The version is there in case a new IKE version appears. We have added a default statement as well.

> 
> 
> 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.

Basically we would like to add number of seconds. For example, a lifetime of 60 seconds for a particular IPsec security association. Most probably an uint32 should be enough.

Best Regards.
> 
> 
> _______________________________________________
> I2nsf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/i2nsf