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.
_______________________________________________
I2nsf mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/i2nsf
