Re: [I2nsf] Benjamin Kaduk's Discuss on draft-ietf-i2nsf-sdn-ipsec-flow-protection-12: (with DISCUSS and COMMENT)

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

Dear Benjamin:

Thank you very much for your very detailed answer. It is an impressive job and truly helpful. Please see our comments inline.

> El 5 nov 2020, a las 5:53, Benjamin Kaduk via Datatracker <[e-mail]> escribió:
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-i2nsf-sdn-ipsec-flow-protection-12: Discuss
> 
> 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/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Let's discuss whether it's appropriate to include vendor-specific
> functionality (e.g., linux NETKEY/XFRM marking) in a standards-track
> protocol/model.

[Authors] Yes, we tend to think the same. We believe it would be better to stay away of vendor-specific functionality. In fact, for example, we have suggested to Erik to remove container spd-mark (where this text appears) since now its purpose can be replaced by the new dscp-mapping format. 

> 
> The ASN.1 GeneralName type is an abstract type; in order to represent it
> in a string we must have some discussion of how it is encoded.  (A
> similar concern might apply to the other ASN.1 types used, such as
> DistinguishedName, though the latter does have a fairly well-established
> string presentation form, so the concern is of lesser magnitude there.
> That said, RFC 5280 is not a suitable reference for the
> DistinguishedName string presentation form.)

[Authors] We definitely need your guidance here. We believe this is also related to the e-mail exchange with Tom Petch about the DistinguisedName. In the DistinguisedName, it seems we have to wait to know the proper reference since RFC 2247 is not valid.

The rest of places in the I-D where ASN.1 is used has been fixed based on your comments (e.g. your suggestion about using type binary for the DER format in ca-data, crl-data)

However, regarding ASN.1 GeneralName, we must admit we need your guidance here. What do you advise about the encoding? Is it proper to define it such as:

case gnx509 {
             leaf gnx509 { 
               type binary; 
               description 
                 "ASN.1 X.509 GeneralName structure as
                 specified in RFC 5280,
                 encoded using ASN.1
                 distinguished encoding rules
                 (DER), as specified in ITU-T
                 X.690.”;
           reference
                  “ RFC 5280";
             }
 }

Is this acceptable?



> 
> In a similar vein, the 'id-key' identity representation is listed as
> type 'string' but the description lists it as an "opaque octet string".
> YANG strings are not directly suitable for holding binary content (which
> is what an opaque octet string is), so either a scheme for encoding
> arbitrary binary content as a string is needed, the YANG 'binary' type
> should be used, or this node needs to be documented as only allowing
> valid Unicode (IIUC, in UTF-8 encoding, though
> https://tools.ietf.org/html/rfc6020#section-9.4 is not as clear about
> this as I would like).

[Authors] Yes, let’s use the binary type.


> 
> The two 'anti-replay-window' leafs are (1) using different-width types,
> and (2) do not have enough of a description to indicate what content
> they hold, especially whe combined with a default value of 32.
> (I mention both locations in the COMMENT.)

[Authors] Yes this needs clarification and it is definitely confusing. 

It turns out that in section 4.4.2.1 Data Items in the SAD (RFC 4301) mentions:

"Anti-Replay Window: a 64-bit counter and a bit-map (or equivalent)
used to determine whether an inbound AH or ESP packet is a replay."

This is reasonable since it is information associated with a particular SAD entry. However this woud be the anti-reply window itself. However, our intention (poorly defined) was to allow the configuration of the window size. That is why we had default 32 (packets). Nevertheless, though the default value set to 32 came from this text in RFC 4303:

"A minimum window size of 32 packets MUST be supported when 32-bit
   sequence numbers are employed". 

However it also mentions 

  "a window size of 64 is preferred and SHOULD be employed as the default"

It seems that 32 (packets) MUST be supported but it recommends 64. Taking into account this, we think we could change both containers as follows:

leaf anti-replay-window-size {
       type uint32;
       default 64;  
       description 
         "To set the anti-replay window size. 
         The default value is set
         to 64 following RFC 4303 recommendation.";
       reference
         "Section 3.4.3 in RFC 4303”; 
}

What do you think?


> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Abstract
> 
> We should not use unexpanded acronyms like SPD, SAD, and PAD in the
> abstract unless there are very well-known.

[Authors] Agree. Let’s expand them.

> 
> Section 1
> 
>   [RFC6071].  In these cases, hosts, gateways or both may act as NSFs.
>   Consideration for the host-to-gateway scenario is out of scope.
> 
> Is there anything to say about what gaps would need to be filled in
> order to address the host-to-gateway case (vs how much of the mechanisms
> defined herein could be reused)?

[Authors] At the very beginning (first version of personal I-D), we included host-to-gateway case but also realized that this case is much complex. After some discussions in the I2NSF WG, it was agreed to focus only on host-to-host and gateway-to-gateway case. Roughly speaking, the complexity is coming specially for one reason: in the host-to-gateway (road warrior configuration) it is assumed one of the nodes (the mobile) may be out of the management of the I2NSF controller, in other words it is not considered as an NSF in the context of the I2NSF work. 

The interfaces defined here have still value for configuration some of the aspects of the IKEv2 configuration for the “road warrior” node but it is more limited since the I2NSF controller cannot send any configuration to the other side (the “road warrior”). It was agreed that this deserved further discussion so that this I-D should focus on two valid scenarios such as host-to-host and gateway-to-gateway. We preferred to state that this work was out of scope and avoid complicated explanations for the reader.

Maybe we could include this text:

"Due to its complexity, consideration for the host-to-gateway scenario is out of scope. The source of this complexity comes from the fact that, in this scenario,  the host may not be under the control of the I2NSF controller and, therefore, it is not configurable. Nevertheless, the I2NSF interfaces defined in this I-D can be considered as a starting point to analyze and provide a solution for the host-to-gateway scenario."




> 
>   the I2NSF Controller and the NSF.  Using YANG data modelling language
>   version 1.1 [RFC7950] and based on YANG models defined in
>   [netconf-vpn], [I-D.tran-ipsecme-yang], RFC 4301 [RFC4301] and RFC
>   7296 [RFC7296], this document defines the required interfaces with a
>   YANG model for configuration and state data for IKE, PAD, SPD and SAD
> 
> RFCs 4301 and 7296 do not define YANG models.  Is the intent to say that
> this is based on YANG models defined in netconf-vpn and
> I-D.tran-ipsecme-yang, and the data structures defined in RFC 4301 and
> RFC 7296?

[Authors] Correct. Let’s change the sentence as you suggest.


> 
>   o  To define the interfaces required to manage and monitor the IPsec
>      SAs in the NSF from a I2NSF Controller.  YANG data models are
>      defined for configuration and state data for IPsec and IKEv2
>      management through the I2NSF NSF-Facing interface.  Thus, this
>      document does not define any new protocol.
> 
> It might perhaps be worth saying that the YANG models can be used via
> existing protocols such as NETCONF and RESTCONF.

[Authors] Agree. Let’s include them.


> 
> Section 3
> 
>   The following term is defined in [RFC6437]:
> 
>   o  Flow/traffic flow.

> 
> That does not seem to be used as a single term in RFC 6473; perhaps it's
> best to say that they are two terms that are related or have identical
> definition/usage.

[Authors] Ok, let’s rephrase this to:

"The following two terms that are related or have identical definition/usage in RFC6437.

	• Flow or traffic flow."

> 
> Section 4.1
> 
>   flexible management of IPsec SAs in flow-based NSFs.  In order to
>   support this capability in the IKE case, a YANG data model for IKEv2,
>   SPD and PAD configuration data, and for IKEv2 state data MUST be
>   defined for the I2NSF NSF-Facing Interface.
> 
> I think this is probably better as a descriptive "needs to" than a BCP
> 14 "MUST”.

[Authors] Ok. Changed.


> 
> Section 4.2
> 
> We should perhaps say something before the figure about what the I2NSF
> User does, in order to justify their inclusion in the figure (or
> otherwise introduce the figure before it appears).

[Authors] Correct. We have moved now the paragraph where the I2NSF User appears before the figure 2 and not before.

> 
> Section 5.1
> 
>   IPsec SAs management.  With the YANG data models defined in this
>   document the I2NSF Controller can configure and conduct the rekey
> 
> Does the "and conduct" apply only to the IKE-less case, or the IKE-ful
> one as well?  If only the IKE-less case we might consider some
> additional notation such as "and, as needed, conduct”.

[Authors] Yes, it is for the IKE-less case. In the IKE case the rekey process is managed by IKEv2. How about this change?

"Performing a rekey for IPsec SAs is an important operation during the
   IPsec SAs management.  With the YANG data models defined in this
   document the I2NSF Controller can configure parameters of the rekey process (IKE case) or conduct the rekey
   process (IKEless case).  Indeed, depending on the case, the rekey process is different."



> 
> Section 5.2
> 
>   In the IKE case, the I2NSF Controller will configure the affected NSF
>   with the new IKEv2, SPD and PAD information.  It has also to send new
>   parameters (e.g. a new fresh PSK for authentication) to the NSFs
>   which have IKEv2 SAs and IPsec SAs with the affected NSF.  Finally,
> 
> It does not seem intuitively obvious to me that the controller *always*
> has to send new parameters to the other NSFs (that have SAs with the
> affected NSF).  E.g., for IKE-ful SAs that use certificate
> authentication, what would need to change on the other NSFs?

[Authors] Your comment is proper. It is true that in certain cases, the non affected NSF already has the information it needs, as happens in the example you mention, if authentication is based on certificates. 

How about this rephrase?

"In the IKE case, the I2NSF Controller MAY need to configure the affected NSF with the new IKEv2, SPD and PAD information.  Alternatively, IKEv2 configuration MAY be made permanent between NSFs reboots without compromising security by means of the startup configuration datastore in the NSF. This way, each time a NSF reboots it will use that configuration for each rebooting. It would 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."



> 
>   Alternatively, IKEv2 configuration MAY be made permanent between NSFs
>   reboots without compromising security by means of the startup
> 
> Similarly, this MAY seems to require that the previous paragraph has
> some hedging langauge that the procedures it describes are only one
> possible option (with this paragraph being the other option).

[Authors] We have merged both paragraphs as described above. Does it sound reasonable?


> 
> Section 5.3
> 
>   some of the peers or both are located behind a NAT.  If there is a
>   NAT network configured between two peers, it is required to activate
> 
> I am not familiar with "NAT network" as a term; is there a reference for
> it?

[Authors] True. The purpose was to refer that in the path between two peers there is a NAT configured. So we can rephrase this as 

"In the IKE case, IKEv2 already provides a mechanism to detect whether some of the peers or both are located behind a NAT. In this case, UDP or TCP encapsulation for ESP packets ([RFC3948], [RFC8229]) is required."


> 
>   the usage of UDP or TCP/TLS encapsulation for ESP packets ([RFC3948],
> 
> Why is TLS needed?  I understand that RFC 8229 talks about how it can be
> done, but why is that necessary here?

[Authors] We tend to agree with you here. Strictly speaking RFC 8229 speaks about TCP therefore we should only mention TCP. Thus, let’s specify only TCP.

Also then we should remove

enum espintls { 
         description 
           "ESP in TCP encapsulation using TLS.";
         reference 
           "RFC 8229 - TCP Encapsulation of IKE and 
           IPsec Packets.";
       } 

And just leave:

enum espintcp { 
         description 
           "ESP in TCP encapsulation.";
         reference 
           "RFC 8229 - TCP Encapsulation of IKE and 
           IPsec Packets.";
 } 



> 
>   configured between two hosts, and apply the required policies to both
>   NSFs besides activating the usage of UDP or TCP/TLS encapsulation of
> 
> (TLS again)

[Authors] Same here.


> 
> Section 6
> 
>   In order to support the IKE and IKE-less cases we have modeled the
>   different parameters and values that must be configured to manage
> 
> The use of the first person is uncommon in RFCs; phasing like "models
> are provided for the different parameters and values [...]" is more
> common.

[Authors] Ok, let’s modify this sentence to avoid the use of first person.

"In order to support the IKE and IKE-less cases, models are provided for the different parameters and values…"
> 
> Section 6.1
> 
> We have a 'spd' tree in the IKE-ful model, so it's a bit surprising that
> we don't match the treatment of the IKE-less model and say that the SPD
> model has mainly been extracted from [parts of] RFC 4301.


[Authors] True. We have now included the following paragraph:

"The definition of the SPD model has been mainly extracted from the specification in section 4.4.1 and Appendix D in RFC 4301."


> 
> (side note?) The tree diagram apparently doesn't have a good way of
> showing the "when" relationship between (e.g.) auth-method and the
> sibling nodes (eap-method, pre-shared, etc.), which confused me briefly.

[Authors] That’s right but the tree diagrams have been generated with the pyang command tool and follow the notation syntax for YANG tree diagrams described in RFC8340. AFAIK there is no way to show these clauses.

> 
> Using string names instead of YANG references for correlating the PAD
> entries is rather surprising to me.

[Authors] In reality, “name" has been just used as a key of the list. In other words to univocally to identify a PAD entry. Same happens with conn-entry, SPD entries, for example. 

> 
>   to be secured (local/remote subnet and ports, etc.) and how must be
>   protected (AH/ESP, tunnel/transport, cryptographic algorithms, etc.);
> 
> I think that AH support was present in a previous version of the
> document but has since been removed, so we should not mention AH here.

[Authors] Yes, you are right. We will revise the document to remove any reference to AH protocol, including the sentence you mention.


> 
> Section 6.2
> 
>   o  Each IPsec policy (spd-entry) contains one traffic selector,
>      instead of a list of them.  The reason is that we have observed
>      actual kernel implementations only admit a single traffic selector
>      per IPsec policy.
> 
> It's not entirely clear that we need to cripple the overall model just
> because some implementations would not support the full functionality.
> My understanding is that having many traffic selectors for a single
> policy (or SA, which has similar text later in this section) can be
> quite useful in some cases.

[Authors] This question was precisely discussed within the I2NSF WG at IETF 104. You may want to consult slide 8 of the presentation we prepared for that meeting: https://www.ietf.org/proceedings/104/slides/slides-104-i2nsf-sdn-based-ipsec-flow-protection-00 

It is true that the idea of only allowing one traffic selector per policy was taken from the real implementations. But we decided to propose the adoption of this change to the I2NSF WG because it simplified the model while, at the same time, we were not losing functionality. The point of view about this is that having one policy with multiple traffic selectors is equivalent to having several policies, each one configured with a different traffic selector. 

Perhaps to avoid any confusion about implementation we can just simply state:

“For simplicity, each IPsec policy (spd-entry) contains one traffic selector”


> 
>   o  Combined algorithms have been removed because encryption
>      algorithms MAY include authenticated encryption with associated
>      data (AEAD).
> 
> A reference here would be helpful (IIUC, the "MAY" here is in effect
> quoting the external document, and not a new requirement).
> (Likewise for the corresponding SAD-model item.)

[Authors] Our text is certainly very poorly written. Let’s clarify this part.

In RFC 4301 it is mentioned that:

- ESP Encryption algorithm, key, mode, IV, etc.  If a combined mode
      algorithm is used, these fields will not be applicable.


- ESP integrity algorithm, keys, etc.  If the integrity service is
      not selected, these fields will not be applicable.  If a combined
      mode algorithm is used, these fields will not be applicable.

- ESP combined mode algorithms, key(s), etc.  This data is used when
      a combined mode (encryption and integrity) algorithm is used with
      ESP.  If a combined mode algorithm is not used, these fields are
      not applicable.

It seems we have like three possibilities. However, according to RFC 8221

"Encryption without authentication is not effective and MUST NOT be
   used.  IPsec offers three ways to provide both encryption and
   authentication:

   o  ESP with an Authenticated Encryption with Associated Data (AEAD)
      cipher

   o  ESP with a non-AEAD cipher + authentication

   o  ESP with a non-AEAD cipher + AH with authentication"

Therefore AEAD is the combined mode (first choice) or we have encryption algorithm (non-AEAD) + authentication/integrity algorithm.  
Since we do not have AH, third option is not considered. In summary, that text should say:

"The model allows specifying the algorithm for encryption. This can be an Authenticated Encryption with Associated Data (AEAD) or non-AEAD. If an AEAD is specified the integrity algorithm is not required. If an non-AEAD algorithm is specified the integrity algorithm is required [RFC8221]"


> 
>   The notifications model has been defined using as reference the
>   PF_KEYv2 standard in [RFC2367].
> 
> RFC 2367 is only an Informational document, so calling it a "standard"
> (as opposed to a "protocol" or "specification" may not be the best
> choice.

[Authors] Agree. Let’s replace “standard” with "specification". 


> 
>    |     |   +--rw protocol-parameters? ipsec-protocol-parameters
> 
> Isn't this type nsfikec:ipsec-protocol-parameters?

[Authors] Certainly, the ipsec-protocol-parameters is a new data type (defined in ietf-i2nsf-ikec module) which, in turn, is used within this module to define “ipsec-policy-grouping”. In reality, the "ipsec-policy-grouping" is the one imported from ietf-i2nsf-ikeless. Consequently, the YANG tree diagram does not depict "nsfikec:ipsec-protocol-parameters" because the leaf “protocol-parameters” is not a data element defined in the ietf-i2nsf-ikeless module, it is a data element defined within the "ipsec-policy-grouping" imported. 

Analyzing this part of the yang model, we believe the confusion is motivated by the definition of the algorithm-type node, just a couple of lines below this one:

+--rw protocol-parameters? ipsec-protocol-parameters
    |     |   +--rw esp-algorithms
    |     |   | +--rw integrity* integrity-algorithm-type
    |     |   | +--rw encryption* [id]
    |     |   | |+--rw id                uint8
    |     |   | |+--rw algorithm-type? nsfikec:encryption-algorithm-type
    |     |   | |+--rw key-length?       uint16
    |     |   | +--rw tfc-pad?      boolean

In fact, we detected here that nsfikec prefix is redundant for the same reason previously explained. 


> 
>   absence of an IKE software in the NSF: traffic direction to apply the
>   IPsec policy, and a value to link an IPsec policy with its associated
>   IPsec SAs.  [...]
> 
> Maybe "a 'requid' value to link [...]" since it is otherwise a little
> hard to find by searching?

[Authors] Ok, it's helpful for the reader to mention the specific parameter in the model used for linking a policy and IPsec SAs: 

"…a “reqid" value to link since it is otherwise a little hard to find by searching."


> 
> Section 8
> 
>   On the one hand, it is important to note that there MUST exist a
>   security association between the I2NSF Controller and the NSFs to
>   protect the critical information (cryptographic keys, configuration
>   parameter, etc.) exchanged between these entities.
> 
> We should probably state explicitly that the nature of and means to
> create that SA is out of scope for this specification (i.e., it's part
> of device provisioning or onboarding).

[Authors] ok, we can clarify that as follow:

"On the one hand, it is important to note that there MUST exist a
  security association between the I2NSF Controller and the NSFs to
  protect the critical information (cryptographic keys, configuration
  parameter, etc.) exchanged between these entities. The nature of and means 
  to create that security association is out of the scope of this document 
 (i.e., it is part of device provisioning or onboarding)."


> 
>   this information.  Although we can assume this attack is not likely
>   to happen due to the assumed security measurements to protect the
>   I2NSF Controller, it still deserves some analysis in the hypothetical
>   case that the attack occurs.  The impact is different depending on
> 
> I don't think this sentence actually adds any value; whether or not the
> attack is likely, the potential consequences are significant enough that
> we have to document it.

[Authors] ok, we can remove that. The last paragraph would look like that:

"Finally, we have divided this section in two parts in order to
   analyze different security considerations for both cases: NSF with
   IKEv2 (IKE case) and NSF without IKEv2 (IKE-less case).  In general,
   the I2NSF Controller, as typically in the SDN paradigm, is a target
   for different type of attacks [SDNSecServ] and [SDNSecurity].  Thus,
   the I2NSF Controller is a key entity in the infrastructure and MUST
   be protected accordingly.  In particular, the I2NSF Controller will
   handle cryptographic material thus the attacker may try to access
   this information. The impact is different depending on the IKE case 
   or the IKE-less case."


> 
> Section 8.1
> 
>                                                        Alternatively,
>      the NSF could generate the private key and export only the public
>      key to the I2NSF Controller.
> 
> (Again, the mechanisms by which to do this are outside the scope of this
> specification.  We do have some text like this for the following bullet
> regarding certificates, which could easily be repurposed for use here.)

[Authors] ok, we agree.
This bullet is rewritten as:

"Alternatively, the NSF MAY generate the private key and export only the
public key to the I2NSF Controller. How the NSF generates these cryptographic material (public key/ private keys) and
exports the public key is out of scope of this document."



> 
> Section 10.2
> 
> Many of these entries don't really contain sufficient information to
> unambiguously locate the intended reference document.  E.g.,
> [libreswan], [netconf-vpn], [ONF-SDN-Architecture], [SDNSecServ],
> [SDNSecurity], [strongswan].  For many of them it seems like it would be
> quite straightforward to include URLs and/or publisher information,
> e.g., https://ieeexplore.ieee.org/document/6702553 and
> https://datatracker.ietf.org/meeting/87/materials/slides-87-sdnrg-2 seem
> fairly likely to remain stable references.

[Authors] Certainly, most of the informative references can be completed to include more information helping the reader to locate them. All of them have been extended to include the concrete URL. In the concrete case of journal/conference papers, we’ve included all the information (authors, journal name, DOI, etc.). 

> 
> We use RFC 3948 as reference for the UDP encapsulation facility, so it
> probably should be a normative reference; likewise for RFC 8229 and
> TCP(/TLS) encapsulation.

[Authors] You’re absolutely right. We will move both RFCs to the normative references section.

> 
> Appendix A
> 
>      typedef ipsec-protocol-parameters {
>        type enumeration {
>          enum esp { description "IPsec ESP protocol."; }
>        }
>        description
>          "Only the Encapsulation Security Protocol (ESP) is
>          supported but it could be extended in the future.";
> 
> I assume that the usual caveats about extension via identity vs enum are
> well-understood by the WG (i.e., a full revision of the module would be
> needed to enable use of a new value; augmentation would not suffice).
> (This potentially applies to many of the enumerated types in this
> document, but I will only note it this one time.)

[Authors] Yes, that was accepted in the WG, as far as we can remember.

> 
>        description
>          "IPsec traffic direction is defined in two
>          directions: inbound and outbound. From a NSF
>          perspective inbound means the traffic that enters
>          the NSF and outbound is the traffic that is sent
>          from the NSF.";
> 
> This is perhaps not the greastest wording, since in the gateway case any
> given traffic both enters and is sent from the NSF.  I think it is
> probably tolerable, since we refer specifically to the IPsec traffic,
> but might be worth further thought.

[Authors] This text follows the notion of inbound and outbound defined in RFC 4301 (Section 3.1)

"In this document, the term "inbound" refers to traffic entering an IPsec implementation via the
   unprotected interface or emitted by the implementation on the
   unprotected side of the boundary and directed towards the protected
   interface.  The term "outbound" refers to traffic entering the
   implementation via the protected interface, or emitted by the
   implementation on the protected side of the boundary and directed
   toward the unprotected interface"

This text is referring to the IPsec processing a NSF performs for both received packets (decryption, integrity checking, etc.) and sent packets (encryption, integrity calculation, etc.).  Perhaps we may only say that is defined in RFC 4301 as follows:

description
  "IPsec traffic direction is defined in two directions: inbound and outbound.  
  From a NSF perspective,  inbound and outbound are defined as
  mentioned in RFC 4301 (Section 3.1).";


> 
>        description
>          "IPsec protection can be applied to specific IP
>          traffic and layer 4 traffic (TCP, UDP, SCTP, etc.)
>          or ANY protocol in the IP packet payload. We
>          specify the IP protocol number with an uint8 or
>          ANY defining an enumerate with value 256 to
>          indicate the protocol number. NOTE: In case
>          of IPv6, the protocol in the IP packet payload
>          is specified in the Next Header field of the IPv6
>          packet.";
> 
> [There are perhaps some complications around this description when
> extension headers are in use.]

[Authors] This note mentioning IPv6 extension headers was recently included in response to a comment received. You can consult it in the following link:

https://mailarchive.ietf.org/arch/msg/i2nsf/cSbDzzvFEzs3qrB7gTWvoMOVy0Y/

What kind of complications do you have in mind? Do you recommend extending this explanation about extension headers somehow?


> 
>        leaf bytes {
>          type uint32;
>          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.";
> 
> 32 bits for a byte lifetime is perhaps overly restrictive; some
> encryption schemes do not have cryptographic need to rekey that
> frequently, so a 64-bit counter would be more appropriate for them.

[Authors] We agree on this. Let’s change it.


> 
>            is a waste of resources. If the IPsec SA is idle
>            during this number of seconds the IPsec SA
>            should be removed. The value 0 implies
>            infinite.";
>        }
>        reference
>          "Section 4.4.2.1 in RFC 4301.";
> 
> I don't see any mention of "idle" in RFC 4301.

[Authors] Yes, that is correct. We have observed though this is a non-standard feature but is supported by current IPsec implementations. In fact, we included it as a response of a comment received from Paul Wouters. You can access to his review through the following link:

https://mailarchive.ietf.org/arch/msg/i2nsf/QJrBFpUwsuCpo4PeTMc4gtGvPLc/

In any case, if it is not considered appropriate including it, we can remove this type of IPsec SA lifetime. 


> 
>        leaf bypass-dscp {
>          type boolean;
>          default true;
>          description
>            "If DSCP (Differentiated Services Code Point)
>            values in the inner header have to be used to
>            select one IPsec SA among several that match
>            the traffic selectors for an outbound packet";
> 
> This doesn't really give me a clear picture of what action to take when
> it has value 'true' (vs 'false’).

[Authors] The same comment has been raised by Erik Kline. In the following link you can access the email we sent him proposing some changes not only clarifying this issue but also improving DSCP configuration:

https://mailarchive.ietf.org/arch/msg/i2nsf/66JG9Isj7QHRF1OC3isWlAngpWg/

In summary , we proposed:

leaf bypass-dscp { 
       type boolean;
       default true; 
       description 
         "If True to copy the DSCP value from inner header
         to outer header. If False to map DSCP values 
         from an inner header to values in an outer header 
         following ../dscp-mapping";
       reference
         "Section 4.4.1.2. in RFC 4301.";
     }


> 
>        list local-ports {
>          key "start end";
>          uses port-range;
>          description
>            "List of local ports. When the inner
>            protocol is ICMP this 16 bit value
>            represents code and type.
>            If this list is not defined
>            it is assumed that start and
>            end are 0 by default (any port).";
> 
> Shouldn't these extra semantics be attached to the port-range grouping
> itself?  Also, the repurposing of start/end for ICMP code/type is quite
> unconventional.

[Authors] The port-range grouping defines how to create a port range. However, the concrete application of this grouping arises in the concrete list (in our case) where it is used. While in the list local-ports it contains a list of local port ranges, in the list remote-ports it contains a list of remote port ranges. Thus, we believe this would be the correct place to explain the semantics of the port ranges but not in the port range-grouping. 

With regards to ICMP type/code, we agree with you. But it certainly simplifies the definition. In fact, it follows what we saw in RFC 4301. For example, in section 4.4.1.3 page 34

Local's
             next layer protocol = ICMP
             "port" selector     = <specific ICMP type & code>

           Remote's
             next layer protocol = ICMP
             "port" selector     = OPAQUE


As you can see it considers that as part of the “port” selector (ICMP type/code is not a port obviously but we interpret that we can leverage the 16-bit used for the ports hence the description we have included.



> 
>        leaf anti-replay-window {
>          type uint64;
>          default 32;
>          description
>            "A 64-bit counter used to determine whether an
>            inbound ESP packet is a replay.";
>          reference
>            "Section 4.4.2.1 in RFC 4301.";
> 
> Is this the counter or the bit-map or the size of the bit-map?
> I don't think a uint64 combined with default 32 makes sense.

[Authors] We added a discussion about this in the DISCUSS section

Our proposal was to change this node to:

leaf anti-replay-window-size {
       	type uint32;
      	default 64;  
           description 
              "To set the anti-replay window size. 
              The default value is set
              to 64 following RFC 4303 recommendation.";
       reference
         "Section 3.4.3 in RFC 4303”;
}



> 
>            leaf seq-overflow {
>              type boolean;
>              default false;
> 
> I think we may want to give a stronger indication that setting this to
> 'true' is strongly discouraged.

[Authors] Ok. We can include the following text in the description associated to this leaf to warn the reader about this issue:

"Setting this flag to true is strongly discouraged."


> 
>                leaf id {
>                  type uint8;
>                  description
>                    "The index of list with the
>                    different encryption algorithms and
>                    its key-length (if required).";
> 
> I'm not sure what this id is indexing into and how it would indicate key
> length.  Is the idea that this is a sequential index for the very 'list
> encryption' that it is contained in?  If so, I think we would need to
> write more specifics about its semantics and how it is assigned.

[Authors] The purpose of this id is to be a simple index that unequivocally identifies each entry of the list. It can be implemented as a sequential number monotonically incremented by one, but other schemes are also valid. The id does not indicate the key-length, because this information is stored in the key-length leaf. To clarify the semantics of this id, we will update its description as follows:

"An identifier that unequivocally identifies each entry of the list, i.e., an encryption algorithm and its key-length (if required)” 

Also, after your comment, we believe that having an uint16 is better since algorithms and its corresponding key-length may be beyond 255 cases."


> 
> Appendix B
> 
>        "This module contains IPsec IKE case model for the SDN-based
>        IPsec flow protection service. An NSF will implement this
>        module.
> 
> Why do we have the last sentence?  IIUC we do not mandate implementation
> of the IKE case, since the IKE-less case is seen as making this more
> accessible for lower-resourced NSFs.

[Authors] Yes, we will remove this sentence.


> 
>          enum on-demand {
>            description
>              "IKE/IPsec configuration is loaded
>              into IKE implementation. The IPsec policies
>              are transferred to the NSF's kernel but the
>              IPsec SAs are not established immediately.
>              The IKE implementation will negotiate the
>              IPsec SAs when the NSF's kernel requests it
>              (i.e. through an ACQUIRE notification).";
> 
> I'm not sure I understand the intended semantics of the ACQUIRE
> notification, here -- the sadb-acquire notification is part of this YANG
> module, which would typically imply that it is send from the NSF to the
> I2NSF Controller.  I'm not sure where the NSF's kernel is, in that path.
> (We also haven't really talked about "NSF kernel"s previously, though
> we did have significant discussion of kernel IPsec/IKE implementations.)

[Authors] 

Yes, you’re right. This is confusing. IKE implementation and NSF’s kernel are in the NSF for the IKE case. So this ACQUIRE is after all, an internal notification from the NSF’s kernel to the IKEv2 implementation. However the YANG sadb-acquire notification is defined for the IKE-less case (module ietf-i2nsf-ikeless) because, in that case, we need a way to notify the I2NSF controller to trigger the IPsec SA installation. However, it is worth noting that this enum is for the IKE case so that we can simplify this description as follows:

enum on-demand {
           description
             "IKE/IPsec configuration is loaded
             into IKE implementation. The IPsec policies
             are transferred to the NSF but the
             IPsec SAs are not established immediately.
             The IKE implementation will negotiate the
             IPsec SAs only when they are required.”


> 
>      typedef pfs-group{
> 
> "pfs-group" (and the "pfs-groups" nodes of this type) calls to mind
> "perfect forward secrecy", but the guidance from SAAG is moving towards
> dropping the "perfect" part since it is a bit misleading and its meaning
> is not terribly well-defined.

[Authors] We have opted for the use of “perfect forward secrecy” because it is the term used in RFC 7296 (IKEv2 protocol standard). In any case, we can follow your guidance and rename it as “fs-group”, is that ok?



> 
>          list pad-entry {
>            [...]
>            leaf name {
>            [...]
>            choice identity {
> 
> I guess I would have expected some discussion somewhere (possibly up in
> §6.1) about why there is both a 'name' and an 'identity'.  (I might
> surmise that it is to allow differently-named-and-configured SAs to the
> same remotem peer, but that is just supposition if there's no text in
> the document about it.)

[Authors] We use the leaf name as a key for a YANG list. In other words, to  univocally identify a PAD entry. Originally, we wanted to use the choice identity as a key following what is mentioned in RFC 4301 but that was not allowed in YANG. Moreover, we follow the same line that happens with SPD entries in RFC 4301 where each entry has a name.

Moreover, in the past, we found a problem to store the credentials of the local NSF (i.e. local NSF’s private key) and the identity to be used for a particular remote NSF.  We came to the conclusion that PAD could also be a proper place to store this information. In such a case, we may have several entries with the same (local) identity and therefore, it cannot be used as key. For example, the same (local) identity could have different certificates or the same local identity could be used with different authentications methods. In fact, we have this text in the description of the container pad that you may have read:

“
Configuration of the Peer Authorization Database (PAD). 
The PAD contains information about IKE 
peers (local and remote). Therefore, the Security
 Controller also stores authentication
information for this NSF and can include
         several entries for the local NSF, not only
         remote peers. Storing local and remote
         information makes it possible to specify that this
         NSF with identity A will use some particular 
         authentication with remote NSF with identity B
         and what are the authentication mechanisms
         allowed for communication to B.
“
However, we believe this text can be improved in the following way:

“Configuration of the Peer Authorization Database 
(PAD). Each entry of PAD contains authentication 
information of either the local peer or the remote peer. 
Therefore, the I2NSF Controller stores authentication
information (and credentials) not only for the remote NSF 
but also for the local NSF. The local NSF MAY use the 
same identity for different types of authentication
and credentials. Pointing to the entry for a local NSF
(e.g., A) and the entry for remote NSF (e.g., B)
is  possible to specify all the required information to 
carry out the authentication between A and B (see
../conn-entry/local and ../conn-entry/remote)”.

We also have the following text in section 6.1:

“Firstly, it contains a "pad" container that serves to
 configure the Peer Authentication Database with authentication
information about local and remote peers.  More precisely, it
consists of a list of entries, each one indicating the identity,
authentication method and credentials that will use a particular peer.”

We can improve it in the following way:

“Firstly, it contains a "pad" container that serves to
configure the Peer Authentication Database with authentication
information about local and remote peers. More precisely, it
consists of a list of entries, each one indicating the identity,
authentication method and credentials that a particular peer (local or remote) will use. 
Therefore, each entry contains identity, authentication information, and credentials of 
either the local NSF or the remote NSF. Therefore, the I2NF Controller can store identity, 
authentication information and credentials
for the local NSF and the remote NSF.”

Therefore we centralize all the authentication information, credentials and identities to be used for local and remote NSF in the PAD. This simplifies the module since for each conn-entry we can just simply state the local NSF identity and credentials and the remote NSF identity and credentials just pointing to the entries in the PAD (as we already had)

container local {
         leaf local-pad-entry-name { 
           type string; 
           mandatory true; 
           description 
             "Local peer authentication information.
             This node points to a specific entry in
             the PAD where the authorization
             information about this particular local
             peer is stored. It MUST match a
             pad-entry-name.";
         } 
         description 
           "Local peer authentication information.";
       }

     container remote {
         leaf remote-pad-entry-name { 
           type string; 
           mandatory true;
           description 
             "Remote peer authentication information.
             This node points to a specific entry in
             the PAD where the authorization
             information about this particular
             remote peer is stored. It MUST match a 
             pad-entry-name.";
         }
         description 
           "Remote peer authentication information.";   
       }   

In summary, those are the two reasons why the choice identity is not used as key in this list. 




> 
>                leaf dnx509 {
>                  type string;
> 
> I assume from the 'string' type and the example that we use the
> presentation form and not (e.g.) the DER encoded form.  This is probably
> worth stating explicitly.

[Authors] Ok, we can extend the description to clarify this issue:

           description 
                "Specifies the identity as a
                ASN.1 X.500 Distinguished
                Name using the presentation 
                form (i.e. not encoded). 
                An example is C=US,O=Example
                Organisation,CN=John Smith.";



> 
>                leaf eap-type {
>                  type uint8;
>                  mandatory true;
>                  description
>                    "EAP method type. This
>                    information provides the
>                    particular EAP method to be
>                    used. Depending on the EAP
>                    method, pre-shared keys or
>                    certificates may be used.";
> 
> A reference to where the actual method type values are obtained (e.g.,
> https://www.iana.org/assignments/eap-numbers/eap-numbers.xhtml#eap-numbers-4)
> seems in order.  I note that there is a range allocatable via
> "Specification Required" in that registry that is not representable in a
> uint8.

[Authors] Yes, it is convenient to clarify the source from where valid eap method values can be obtained. We will update the description of the eap-type leaf and add a reference clause pointing to the IANA registry. Regarding Specification Required part , yes , we agree. As a consequence we should include a uint64 and establish a range. 

          container eap-method {
             when "../auth-method = 'eap'";
             leaf eap-type { 
               type uint64 {range "1 .. 4294967295";}
               mandatory true; 
               description 
                 "EAP method type specified with 
		 a value extracted from the 
		 IANA Registry. This
                 information provides the
                 particular EAP method to be
                 used. Depending on the EAP
                 method, pre-shared keys or
                 certificates may be used."; 
             }
             description 
               "EAP method description used when
               authentication method is 'eap'.";
             reference 
               "IANA Registry; Extensible Authentication 
               Protocol (EAP); Registry; Method Types. 
	       Section 2.16 in RFC 7296.";    
           }

           description
                   "EAP method description used when
                   authentication method is 'eap'.";
                reference
                   "Section 2.16 in RFC 7296.";
      }

Moreover we have added the reference to this IANA Registry in the I-D as normative reference. 




> 
>              container pre-shared {
>                when
>                  "../auth-method[.='pre-shared' or
>                  .='eap']";
> 
> This seems a little unfortunate, as I don't think that it is appropriate
> to require ("mandatory true") the pre-shared secret leaf to be present,
> universally, for all EAP methods.o

[Authors] Yes, we agree with you. We will remove the mandatory statement. In any case we have extended the description a little bit:

leaf secret {
               nacm:default-deny-all; 
               type yang:hex-string; 
               description 
                  "Pre-shared secret value. The
                 NSF has to prevent read access
                 to this value for security
                 reasons. This value MUST be 
                 set if the EAP method uses a 
		    pre-shared key or pre-shared 
		    authentication has been chosen.";
}





> 
>              container digital-signature {
>                when
>                  "../auth-method[.='digital-signature'
>                  or .='eap']";
> 
> Similarly here, for the mandatory 'public key' choice -- if I am reading
> this correctly the YANG would require us to have both pre-shared *and*
> public-key values present for the 'eap' auth-method.

[Authors] Yes, here the same, we will remove the mandatory statement, otherwise we will be enforcing the configuration of both pre-shared key and digital certificate when eap authentication is used.


> 
>                leaf ds-algorithm {
>                  type uint8;
>                  default 1;
> 
> My understanding is that algorithm 14 (the generic "digital signature")
> is currently preferred as the default algorithm, since it is not tied to
> a particular public key type.  See
> https://tools.ietf.org/html/rfc8247#section-3.1 for discussion of why it
> is expected to be the way of the future.

[Authors] Thanks. Without a doubt, it is advisable to set as default algorithm the generic Digital Signature. We will modify it:

 leaf ds-algorithm {
               type uint8;
               default 14; 
               description 
                 "The digital signature 
                 algorithm is specified with a
                 value extracted from the IANA
                 Registry. Default is the generic
		 Digital Signature method. Depending 
		 on the algorithm, the following leafs
                 MUST contain information. For
                 example if digital signature or the 
		 EAP method involves a certificate 
		 then leaf 'cert-data' and 'private-key'
                 will contain this information.";
             reference
               "IANA Registry; Internet Key
               Exchange Version 2 (IKEv2);
               Parameters; IKEv2 Authentication Method.";
             }


> 
>                      signature algorithm. For
>                      example, an RSA key is
>                      represented as RSAPublicKey as
>                      defined in RFC 8017, and an
>                      Elliptic Curve Cryptography
>                      (ECC) key is represented
>                      using the 'publicKey'
>                      described in RFC 5915.";
> 
> This is perhaps a bit under-specified without a clear procedure for
> going from a digital signature algorithm codepoint to the specification
> of the public key encoding.  That said, I'm not sure that there is a
> very good thing to say here, and even the RFC 7427 generic "Digital
> Signature" type is effectively still deferring to the ASN.1
> AlgorithmIdentifier.  (Similarly for the 'private-key', of course.)

[Authors] Any suggestion to improve this?


> 
>                  leaf cert-data {
>                    type binary;
>                    description
>                      "X.509 certificate data -
>                      PEM4. If raw-public-key
>                      is defined this leaf is
>                      empty.";
> 
> If the leaf it of type binary we don't need the (ASCII) PEM encoding.
> (Also, why the '4' in "PEM4”?)

[Authors] Yes, you are right. We will remove this text from the description since no PEM encoding is necessary to store the certificate given the assigned binary type. 

We can change the descriptions as follows:

leaf cert-data {
                 type binary;
                 description 
                   "X.509 certificate data in DER format.  
                   If raw-public-key
                   is defined this leaf is
                   empty.";
                }


> 
>                leaf crl-uri  {
>                  type inet:uri;
>                  description
>                    "X.509 CRL certificate URI.
>                    If it is not defined
>                    the default value is empty.";
> 
> An RFC 5280 reference would probably be appropriate here as well.

[Authors] Thanks, we have included it.


> 
>                leaf oscp-uri {
>                  type inet:uri;
>                  description
>                    "OCSP URI.
>                    If it is not defined
>                    the default value is empty.";
> 
> and maybe 2560 or 5280 here.

[Authors] Thanks, it is really appropriate the inclusion of both references.


> 
>          leaf-list authalg {
>            type nsfikec:integrity-algorithm-type;
>            default 12;
>            ordered-by user;
>            description
>              "Authentication algorithm for establishing
> 
> Using "authentication algorithm" in the description here (and "authalg"
> as the node name) seems needlessly confusing, since these are the IKEv2
> integrity algorithms, and there is a separate thing that is the IKE
> authentication methods.

[Authors] Yes, the description of this leafl-list needs to be improved to avoid confusion. Additionally, we agree with the necessity of improving the node naming (authalg and encalg) to emphasize we are referring to the algorithm configuration of the IKE SA. Thus, we propose the following modifications:

leaf-list ike-sa-intr-alg {  // Old authalg
        type nsfikec:integrity-algorithm-type;
        default 12; 
        ordered-by user; 
        description 
          "Integrity algorithm for establishing 
          the IKE SA. This list is ordered following
          from the higher priority to lower priority.
          First node of the list will be the algorithm
          with higher priority.";
      }

list ike-sa-encr-alg {  // Old encalg
            …..
      }



> 
>          list encalg {
>            key id;
>            min-elements 1;
>            ordered-by user;
>            leaf id {
>              type uint8;
>              description
>                "The index of the list with the
>                different encryption algorithms and its
>                key-length (if required). E.g. AES-CBC,
>                128 bits";
> 
> As was the case for esp-algorithms/encryption, I'm not sure what this id
> is indexing into and how it would indicate key length.  Is the idea that
> this is a sequential index for the very 'list encryption' that it is
> contained in?  If so, I think we would need to write more specifics
> about its semantics and how it is assigned.


[Authors] We have a similar comment when you mentioned the esp-algorithms/encryption node. To clarify the semantics of this id, we will update its description as follows:

"An identifier that unequivocally identifies each entry of the list, i.e., an encryption algorithm and its key-length (if required)"

On the other hand, the list is ordered-by user, which means (in our case) that it is the I2NSF controller which establishes the order. First node in the list has higher priority and so on. We mention this in the list.

"Encryption or AEAD algorithm for the IKE
           SAs. This list is ordered following
           from the higher priority to lower priority.
           First node of the list will be the algorithm
           with higher priority."


> 
>          leaf half-open-ike-sa-timer {
>            type uint32;
>            default 0;
>            description
>              "Set the half-open IKE SA timeout
>              duration.";
> 
> What are the units?  Does 0 really mean 0 (vs infinity)?

[Authors] Thanks, we forgot including the units statement. Yes, 0 means infinity we can clarify it in the description as follows:

leaf half-open-ike-sa-timer { 
        type uint32; 
        units "seconds";
        default 0; 
        description 
          "Set the half-open IKE SA timeout 
          duration. The value 0 means infinite."; 
        reference
          "Section 2 in RFC 7296.";
 }


> 
>          container child-sa-info {
>            leaf-list pfs-groups {
> 
> ["pfs" again, just noting the location; no new comment other than the
> one above]

[Authors] Ok. If you agree, we will replace "pfs-groups" with "fs-groups".


> 
>            leaf current-rekey-time {
>              type uint64;
>              units "seconds";
>              description
>                "Seconds before IKE SA MUST be rekeyed.";
>            }
>            leaf current-reauth-time {
>              type uint64;
>              units "seconds";
>              description
>                "Seconds before IKE SA MUST be
>                re-authenticated.";
> 
> I don't think either of these strictly speaking need to be RFC 2119
> "MUST”s.

[Authors] Yes, we can just simply say:

 description
     "Seconds before IKE SA is re-authenticated.";


> 
>        container number-ike-sas {
> 
> In general it seems like these counters could be gauge64's instead of
> uint64’s.


[Authors] Yes, it makes sense. 


> 
> Appendix C
> 
>            container ipsec-sa-config {
>              [...]
>              leaf spi {
>                type uint32 { range "0..max"; }
> 
> I would have expected a clear indication of whether this value is for
> sending or receiving.

[Authors] The direction of the SA where this SPI is defined can be inbound or outbound depending on the traffic selector elements in the SA entry. In fact, this information (inbound or outbound) is not included explicitly in the SAD, as described in RFC4301. 

> 
>              leaf seq-number-counter {
> 
> Why is this leaf writable?

[Authors] Yes you're completely right. We can remove it. In reality, this counter appeared as a data item for an sad entry but it refers to the actual current sequence number (state). There is no need to configure it since, as RFC4303 mentions: 

"The sender's counter and the receiver's counter are initialized to 0 when an SA is established."	
> 
>              leaf anti-replay-window {
>                type uint32;
>                default 32;
>                description
>                  "A 32-bit counter and a bit-map (or
>                  equivalent) used to determine
>                  whether an inbound ESP packet is a
>                  replay. If set to 0 no anti-replay
> 
> First, in the common module the anti-replay-window was a uint64, but
> here it's uint32.  Second, my confusion/comment from the common module
> seem to also apply here.

[Authors] Fully agree. See comment in the DISCUSS section.

> 
>                leaf replay-window {
>                  type uint64;
>                  description
>                    "Current state of the replay
>                    window.";
> 
> Probably a little bit more description and/or reference is in order
> here.

[Authors] Fully agree. In fact we believe it is a bit under specified. According to Appendix A in RFC 4303, as state data, we could have something like the following to model the replay window as observed in rfc 4303. 


container replay-window {
               
               leaf w {
                 type uint32; 
                 description 
                   "Size of the replay window.";
               }
                 
               leaf t {
                 type uint64; 
                 description 
                   "Highest sequence number 
                   authenticated so far,
                   upper bound of window ";
               }
               
               leaf b {
                 type uint64; 
                 description 
                   "Lower bound of window.";
               }
                 
               description 
                 "This container contains three 
                 parameters that defines the state 
                 of the replay window: window size (w), 
                 highest sequence number authenticated (t) 
                 lower bound of the window (b).";
               reference 
                 "Appendix A in RFC 4303.”;
             }




> 
>                leaf packet-dropped {
>                  type uint64;
> 
> IIUC this could be a counter64.

[Authors] Yes, it could be counter64. 


> 
>                leaf failed {
>                  type uint32;
>                  description
>                    "Number of packets detected out
>                    of the replay window.";
> 
> How is this different from 'packet-dropped' (and why does it only need
> to be a 32-bit counter)?  Also, would a 'counter32' (or 'counter64')
> type be appropriate?

[Authors] Yes, there is a subtle difference. Packets dropped indicates the number of packets which have been dropped due to a replay. On the other hand, failed indicates the number of packets that have arrived outside of the replay window (old ones). Yes, they should have both counter64 (though it is likely that a counter32 should be enough for regular communication.)

As a consequence we have:

leaf packet-dropped {
               type yang:counter64;  
               description 
                 "Packets dropped
                 because they are 
                 replay packets.";
             }
             leaf failed {
               type yang:counter64; 
               description 
                 "Number of packets detected out
                 of the replay window.";
             }



> 
>      notification sadb-acquire {
>        if-feature ikeless-notification;
>        description
>          "An IPsec SA is required. The traffic-selector
>          container contains information about the IP packet
>          that triggers the acquire notification.";
> 
> If I understand correctly, the scenario here is that an inbound packet
> arrives at (or is generated by) the NSF and it is determined that the
> packet needs IPsec protection to proceed.  Some guidance on whether we
> should use exact-address match to determine the source and destination
> prefixes or some heuristic to select a less-specific prefix seems like
> it would be in order.  (Or is this all supposed to be specified in the
> corresponding SPD entry?)

[Authors] The SADB_ACQUIRE (RFC2367) event is raised when an outbound IP packet is first analysed by the SPD. This packet matches one of the traffic selectors established by a policy and the IPsec implementation (in the NSF) looks for an entry in the SAD for this policy. If this entry is not found in the system (NSF’s SAD)  the acquire event is raised. The process about matching the packet is described in RFC 4301. The traffic-selector container in this notification contains the IP packet information that matched the traffic-selector defined in the policy. We can improve the description in this way:

 	"The NSF detects and notifies that an IPsec SA is required for an 
	 outbound IP packet that has matched a SPD entry. 
	 The traffic-selector container in this notification contains information about 
the IP packet that triggered this notification."



> 
>        container lifetime-current {
>          description
>            "IPsec SA current lifetime. If
>            soft-lifetime-expired is true this container is
>            set with the lifetime information about current
>            soft lifetime.";
> 
> I'm not 100% sure I understand what goes in this field -- intuitively,
> though, it seems like what would be most useful to the I2NSF Controller
> would be the time before the *hard* lifetime expires (since we already
> know that the soft lifetime has expired by virtue of this being a
> sadb-expire notification with soft-lifetime-expired set to true).


[Authors] RFC2367 says: "The CURRENT lifetime extension will indicate the current status, and comparisons to the HARD or SOFT lifetime will indicate which limit was reached.”

This container includes a nsfikec:lifetime in order to represent this "current status". It could help the controller to know which of the (soft)lifetime limits raised the event: time, bytes, packets.... We can improve the descriptions with this last sentence: 

"IPsec SA current lifetime. If soft-lifetime-expired is true this container is set with the lifetime information about current soft lifetime. It can help the NSF Controller to know which of the (soft)lifetime limits raised the event: time, bytes, packets or idle.";


> 
>      notification sadb-seq-overflow {
>        if-feature ikeless-notification;
>        description "Sequence overflow notification.";
>        leaf ipsec-sa-name {
>          type string;
>          mandatory true;
>          description
>            "It contains the SAD entry name (unique) of
>            the IPsec SA that is about to have sequence
>            number overflow and rollover is not permitted.
>            It is assumed the I2NSF Controller will have
>            a copy of the IPsec SA information (except the
>            cryptographic material and state data) indexed
>            by this name (unique identifier) so the it can
>            know all the information (crypto algorithms,
>            etc.) about the IPsec SA that has expired in
>            order to perform a rekey of the IPsec SA.";
>        }
> 
> There seems to be internal disagreement here ("about to have ...
> overflow" vs "that has expired") -- it seems more useful for this to be
> the "about to" behavior, but in that case we should give some guidance
> about how to measure "about to”.

[Authors] Yes, you are right, it is confusing.

Regarding the text, we can remove the sentence "that has expired" in order to avoid confusion.

Regarding the “about to”, it is implementation specific. 

RFC4301 just says “Sequence Counter Overflow: a flag indicating whether overflow of
the sequence number counter should generate an auditable event and
prevent transmission of additional packets on the SA, or whether
rollover is permitted”

The IPsec implementation should issue this event “some time” before the overflow takes place, so it is out of the scope of this document. 

What about this text to clarify:

"It contains the SAD entry name (unique) of
the IPsec SA that is about to have sequence
number overflow and rollover is not permitted.
When the NSF issues this event before reaching 
a sequence number overflow is implementation 
specific and out of scope of this specification.  
It is assumed the I2NSF Controller will have
a copy of the IPsec SA information (except the
cryptographic material and state data) indexed
by this name (unique identifier) so the it can
know all the information (crypto algorithms,
etc.) about the IPsec SA in
order to perform a rekey of the IPsec SA.";


> 
> Appendix D
> 
>   This example shows a XML configuration file sent by the I2NSF
>   Controller to establish a IPsec Security Association between two NSFs
>   (see Figure 3) in tunnel mode (gateway-to-gateway) with ESP,
>   authentication based on X.509 certificates and applying the IKE case.
> 
> Maybe note that some (base64) values have been simplified and/or
> truncated for brevity?  ("base64encodedvalue==" is not a valid base64
> encoding of a DER-encoded X.509 certificate, for example)

[Authors] Yes, for brevity. We can clarify that in the text as follow:

"This example shows a XML configuration file sent by the I2NSF
  Controller to establish a IPsec Security Association between two NSFs
  (see Figure 3) in tunnel mode (gateway-to-gateway) with ESP,
  authentication based on X.509 certificates (simplified for 
 brevity with "base64encodedvalue==") and applying the IKE case."



> 
>        <!--AUTH_HMAC_SHA1_160-->
>        <authalg>7</authalg>
> 
> Perhaps a non-SHA1 example would age better (even though HMAC-SHA1 is
> not known to be weak at this point).

[Authors] Yes, you are right.
“12” is the default value (AUTH_HMAC_SHA2_256_128), 
so it doesn’t not make sense to use it in the example. 
We can use, for example, “14” (AUTH_HMAC_SHA2_512_256).


> 
>                  <local-ports>
>                    <start>0</start>
>                    <end>0</end>
>                  </local-ports>
>                  <remote-ports>
>                    <start>0</start>
>                    <end>0</end>
>                  </remote-ports>
> 
> These are the default values and could be omitted, if I understand
> correctly.

[Authors] Yes, you are right, they can be omitted.

> 
> Appendix E
> 
> I would prefer if the examples did not show seq-overflow as true.

[Authors] Ok, we can modify the examples as follow:

<seq-overflow>false</seq-overflow>


> 
>                 <inner-protocol>any</inner-protocol>
> 
> Is this encoded as "any" as shown, or as 256?

[Authors]

A typedef ipsec-inner-protocol is defined as:

typedef ipsec-inner-protocol {
        type union {
          type uint8;
          type enumeration {
            enum any {
              value 256;
              description
                "Any IP protocol number value.";
            }
          }
        }
    …..
      }

As described in RFC 7950 (section 9.6.5 shows an example), it is encoded with the label, in our case “any".



> 
> Appendix G.1.2
> 
>       *  The I2NSF Controller chooses two random values as SPIs: for
>          example, SPIa1 for NSF A and SPIb1 for NSF B.  These numbers
>          MUST NOT be in conflict with any IPsec SA in NSF A or NSF B.
> 
> Is this restriction for the respective SPI<X>/NSF<X> pairs, or for the
> full matrix of combinations?
> (Similarly in G.2)

[Authors] The restriction is that SPIa1 cannot be the same as any inbound SPI in A. In the same way, SPIb1 cannot be the same as any inbound SPI in B.

Let’s clarify this point in the text in G.1.2 

The I2NSF Controller chooses two random values as SPIs: for example, SPIa1 for the inbound IPsec SA in the NSF A and SPIb1 for the inbound IPsec SA in NSF B. The value of the SPIa1 MUST NOT be the same as any inbound SPI in A. In the same way, the value of the SPIb1 MUST NOT be the same as any inbound SPI in B. Moreover, the SPIa1 MUST be used in B for the outbound IPsec SA to A, while SPIb1 MUST be used in A for the outbound IPsec SA to B. 

Similarly in G.2:

The I2NSF Controller chooses two random values as SPI for the new inbound IPsec SAs: 
for example, SPIa2 for the inbound IPsec SA in A and SPIb2 for the inbound IPsec SA in B. The value of the SPIa1 MUST NOT be the same as any inbound SPI in A. In the same way, the value of the SPIb1 MUST NOT be the same as any inbound SPI in B.

> 
> Appendix G.2
> 
>   If step 1 is successful but some of the operations in step 2 fails
>   (e.g. the NSF A reports an error when the I2NSF Controller is trying
>   to install the new outbound IPsec SA), the I2NSF Controller MUST
>   perform a rollback operation by deleting any new outbound SA that had
>   been successfully installed during step 2 and by deleting the inbound
>   SAs created in step 1.
> 
> I think it's important that the two rollbacks are done in the order
> listed here.
> 

[Authors] ok, we can clarify this point: 

"If step 1 is successful but some of the operations in step 2 fails
  (e.g. the NSF A reports an error when the I2NSF Controller is trying
  to install the new outbound IPsec SA), the I2NSF Controller MUST
  perform a rollback operation by deleting any new outbound SA that had
  been successfully installed during step 2 and by deleting the inbound
  SAs created in step 1, in that order."


The NITS below has been fixed.

> 
> NITS
> 
> Section 1
> 
>   resources through software.  The SDN paradigm relocates the control
>   of network resources to a centralized entity, namely SDN Controller.
> 
> "the SDN Controller".
> 
>                                         For example, Software-Defined
>   WANs (SD-WANs).  [...]
> 
> incomplete sentence (no verb).
> 
>   can be established using IPsec.  The management of IPsec SAs in data
>   centers using a centralized entity is a scenario where the current
>   specification maybe applicable.
> 
> s/maybe/may be/
> 
>   I2NSF NSF-Facing interface.  In this document we define a service
>   allowing the I2NSF Controller to carry out the key management
>   procedures.  More specifically, we define YANG data models for I2NSF
> 
> I'm not sure that "service" is the best word, here.
> 
>   NSF-Facing interface that allow the I2NSF Controller to configure and
>   monitor IPsec-enabled flow-based NSFs.
> 
> missing article ("the" or "a") before "I2NSF NSF-Facing interface".
> (Also, it's weird that "Facing" is capitalized but "interface" is not.)
> 
>   IPsec architecture [RFC4301] defines clear separation between the
> 
> more articles: "The IPsec architecture", "a clear separation"
> 
>   which allows to centralize the key management procedures in the I2NSF
> 
> s/to centralize/centralizing/
> 
>       charge of provisioning the NSF with the required information in
>       the SPD, PAD (e.g.  IKE credential) and IKE protocol itself (e.g.
>       parameters for the IKE_SA_INIT negotiation).
> 
> comma after "e.g." (twice)
> "credentials" plural
> There's amismatch between "in the [databases] and "in IKE protocol
> itself" -- it should more properly be "for the IKE protocol itself",
> which makes the list non-parallel, so it would be "in the SPD and PAD
> (...), and for the IKE protocol itself".
> 
>       required parameters to create valid entries in the SPD and the
>       SAD into the NSF.  Therefore, the NSF will have only support for
> 
> "into" is not right; maybe "in" or "of".
> s/have only/only have/
> 
>   o  To describe the architecture for the I2NSF-based IPsec management,
> 
> s/the I2NSF-based/I2NSF-based/
> 
> Section 4
> 
>   As mentioned in Section 1, two cases are considered, depending on
>   whether the NSF implements IKEv2 or not: IKE case and IKE-less case.
> 
> more articles: "the" for both cases
> 
>   parameters (e.g.  cryptographic algorithms for establishing an IKEv2
> 
> comma after "e.g."
> 
>   information about the end points information (through the I2NSF
> 
> redundant "information"s
> 
> Section 4.2
> 
>   o  Cryptographic algorithm/s selection.
> 
> Just "algorithm" works fine for both the singular and plural case.
> 
> Section 5
> 
>   IKEv2/IPsec implementation, hosts can easily install it.  As
>   downside, the NSF needs more resources to hold IKEv2 such as memory
> 
> "As a downside"
> "to use IKEv2"
> 
>   Alternatively, IKE-less case benefits the deployment in resource-
> 
> "the IKE-less case"
> 
>   centralized controller (like the I2NSF Controller) is aware about
> 
> s/about/of/
> 
>   provided and discussed (e.g.  hierarchical controllers; having
>   multiple replicated controllers, dedicated high-speed management
>   networks, etc).  In the context of I2SNF-based IPsec management, one
> 
> I think s/;/,/ to be consistent about the list separator within a single
> list.
> 
>   waiting for notifications (e.g. a notification sadb-acquire when a
> 
> comma after "e.g."
> 
> Section 5.1
> 
>   the old one (if a IPsec SA lifetime has not been defined).  This
>   rekeying process starts when the I2NSF Controller receives a sadb-
>   expire notification or it decides so, based on lifetime state data
> 
> s/or it decides so/or on the I2NSF Controller's initiative/
> 
> Section 5.2
> 
>   with the new IKEv2, SPD and PAD information.  It has also to send new
> 
> s/has also/also has/
> 
> Section 5.3
> 
>   some of the peers or both are located behind a NAT.  If there is a
>   NAT network configured between two peers, it is required to activate
>   the usage of UDP or TCP/TLS encapsulation for ESP packets ([RFC3948],
> 
> The "it is required" could be misread to have "it" mean "the NAT
> network"; rewording the second clause to be "UDP or TCP/TLS
> encapsulation for ESP packets ([RFC3948]) is required" would help with
> that.
> 
>   [RFC8229]).  Note that the usage of IPsec transport mode when NAT is
>   required MUST NOT be used in this specification.
> 
> "usage" and "used" seem redundant.
> 
> Section 5.4
> 
>   NSF registration refers to the process of facilitating the I2NSF
>   Controller information about a valid NSF such as certificate, IP
> 
> s/facilitating/providing/ or similar
> 
>   address, etc.  This information is incorporated in a list of NSFs
>   under its control
> 
> full stop at end of sentence.
> 
>   capabilities of this NSF, such as what is the cryptographic suite
>   supported, authentication method, the support of the IKE case and/or
> 
> s/is/are/, s/suite/suites/
> 
> Section 6.1
> 
>   configure the Peer Authentication Database with authentication
>   information about local and remote peers.  More precisely, it
> 
> "authentication information about local peers" reads a bit oddly.
> 
>   authentication method and credentials that will use a particular
>   peer.
> 
> s/that will use a particular peer/that a particular peer will use/
> 
> Section 6.2
> 
>   The data model consists of a unique "ipsec-ikeless" container which,
>   in turn, is integrated by two additional containers: "spd" and "sad".
> 
> "integrated by" is probably not the right term here; "composed of" might
> be better.
> 
>   The "spd" container consists of a list of entries that conform the
> 
> s/conform/form/ (or similar), I think.
> 
>   IPsec SAs.  The "sad" container is a list of entries that conform the
> 
> (ditto)
> 
>   Security Association Database.  In general, each entry allows to
>   specify both configuration information (SPI, traffic selectors,
> 
> s/to specify/specifying/
> 
> Section 8.3
> 
>         to be used for the authentication (e.g. to impersonate a NSF),
> 
> comma after "e.g." (occurs several times in this section).
> 
>         changing to a allow policy), and in general changing the IKE SA
> 
> s/a/an/
> 
>         example, encryption/key contains a ESP encryption key value and
> 
> s/a ESP/an ESP/
> 
>         encryption/iv contains a initialization vector value.
> 
> s/a initialization/an initialization/
> 
>         Similarly, integrity/key has ESP integrity key value.  Those
> 
> s/has/has an/
> 
> Appendix A
> 
>          "The encryption algorithm is specified with a 16-bit
>          number extracted from IANA Registry. The acceptable
> 
> missing article for "IANA Registry" (so probably "the", since only one
> registry is listed).
> (similarly for integrity-algorithm-type)
> 
>          "The integrity algorithm is specified with a 16-bit
>          number extracted from IANA Registry.
>          The acceptable values MUST follow the requirement
>          levels for encryption algorithms for ESP and IKEv2.";
> 
> s/encryption/integrity/, right?
> 
>          enum none {
>            description
>              "NOT ESP encapsulation.";
> 
> "Not" probably doesn't need to be capitalized.
> 
>        description
>          "Types of ESP encapsulation when Network Address
>          Translation (NAT) is present between two NSFs.";
> 
> Perhaps s/is/may be/, as it doesn't seem like an intrinsic requirement
> for the use of encapsulation.
> 
>        description
>          "When the lifetime of an IPsec SA expires an action
>          needs to be performed over the IPsec SA that
> 
> "over" doesn't seem like the right word; perhaps "for"?
> 
>        description
>          "This group of nodes allows to define the type of
>          encapsulation in case NAT traversal is
>          required and port information.";
> 
> s/and/and includes/
> 
>          description
>            "When a NSF stores an IPsec SA, it
>            consumes system resources. In an idle NSF this
> 
> Maybe s/In an idle NSF/For an idle SA/?
> 
>            enum clear {
>              description
>                "Disable the DF (Don't Fragment) bit
>                from the outer header. This is the
> 
> s/from/in/
> 
>        container traffic-selector {
>          description
>            "Packets are selected for
>            processing actions based on the IP and inner
>            protocol header information, selectors,
>            matched against entries in the SPD.";
> 
> The grammar here doesn't make sense, to the extent that I can't really
> even guess the intended meaning.  It makes the most sense if I just end
> the sentence after "inner protocol header information".  Perhaps the
> remaining bit was intended to be a separate statement that "selectors
> are matched against" something?
> 
>              description
>                "True if this IPsec SA is using extended
>                sequence numbers. True 64 bit counter,
>                False 32 bit.";
> 
> How about (for the second sentence): "If true, the 64-bit extended
> sequence number counter is used; if false, the normal 32-bit sequence
> number counter is used"?
> 
>                    "Configuration of ESP authentication
>                    based on the specified integrity
>                    algorithm. With AEAD algorithms,
>                    the integrity node is not used.";
> s/AEAD algorithms/AEAD encryption algorithms/
> 
> Appendix B
> 
>        description
>          "IKE authentication protocol version specified in the
>          Peer Authorization Database (PAD). It is defined as
>          enumerate to allow new IKE versions in the
> 
> s/enumerate/enumerated/
> 
>          enum digital-signature {
>            description
>              "Select digital signature method.";
> 
> s/method/as the authentication method/ to match the others.
> 
>      container ipsec-ike {
>        description
>          "IKE configuration for a NSF. It includes PAD
>          parameters, IKE connections information and state
> 
> s/connections/connection/
> 
>            "Configuration of Peer Authorization Database
>            (PAD). The PAD contains information about IKE
> 
> s/Peer/the Peer/.
> 
>            peer (local and remote). Therefore, the Security
> 
> s/peer/peers/
> 
>            Controller also stores authentication
>            information for this NSF and can include
>            several entries for the local NSF not only
>            remote peers. Storing local and remote
> 
> comma before "not only".
> 
>            information makes possible to specify that this
> 
> s/makes possible/makes it possible/
> 
>            NSF with identity A will use some particular
>            authentication with remote NSF with identity B
>            and what are the authentication mechanisms
>            allowed to B.";
> 
> s/allowed/allowed for communication/
> 
>                  description
>                    "Specifies the identity as a
>                    single four (4) octet.";
> 
> "four-octet value".  (The current text says that it is a single byte
> with value 0x4, which is not a very useful identifier.)
> 
>                     "Specifies the identity as a
>                     Fully-QualifiedDomain Name
> 
> space in "Qualified Domain".
> 
>              case rfc822-address-string {
> 
> (I'm still a little confused why email address identifiers are useful
> enough to be in the core spec, but I think I am behind on the email
> thread so maybe it is answered there.)
> 
>                  description
>                    "Specifies the identity as a
>                    ASN.1 X.500 Distinguished
> 
> s/a/an/
> 
>                    "ID_NULL identification used
>                    when IKE identification payload
> 
> "The ID_NULL identification method is used when the IKE identification
> payload [...]".
> 
>                "This container allows the Security
>                Controller to configure the
>                authentication method (pre-shared key,
>                eap, digitial-signature, null) that
>                will use a particular peer and the
> 
> "will be used with"
> 
>                credentials, which will depend on the
> 
> "credentials to use"
> 
>                     "If the I2NSF Controller
>                     knows that the NSF
>                     already owns a private key
>                     associated to this public key
>                     (the NSF generated the pair
>                     public key/private key out of
>                     band), it will only configure
> 
> I think this is better with "e.g.," at the start of the parenthetical.
> 
>            "IKE peer connection information. This list
>            contains the IKE connection for this peer
>            with other peers. This will be translated in
>            real time by IKE Security Associations
>            established with these nodes.";
> 
> Is "translated" the right word here?
> 
>          leaf autostartup {
>            type autostartup-type;
>            default add;
>            description
>              "By-default: Only add configuration
>              without starting the security
>              association.";
> 
> This seems to be describing the default behavior, not the function of
> the leaf overall.
> 
>          leaf version {
>            type auth-protocol-type;
>            default ikev2;
>            description
>              "IKE version. Only version 2 is supported
>              so far.";
> 
> "so far" is perhaps a bit colloquail for RFC style.
> 
>                "Time in seconds between each IKE SA
>                rekey.The value 0 means infinite.";
> 
> Space at the sentence break.
> 
>                "If non-zero, perfect forward secrecy
>                is required when requesting new
>                IPsec SA. The non-zero value is
>                the required group number. [...]
> 
> How about:
> 
> % If non-zero, forward secrecy is required when a new IPsec SA is being
> % created.  The (non-zero) value indicates the group number to use for
> % the key exchange process used to achieve forward secrecy.
> 
>                "Soft IPsec SA lifetime soft.
> 
> redundant "soft"s.
> 
>                description
>                  "True if this IPsec SA is using
>                  extended sequence numbers. True 64
>                  bit counter, false 32 bit.";
> 
> [same comment as previously]
> 
>                    description
>                      "ESP encryption IV value. If
>                      this leaf is not defined the
>                      IV is not defined (e.g.
>                      encryption is NULL)";
>                  }
> 
> Maybe say how the IV length is determined?
> 
>                    IPsec Encapsulation Security
>                    Payload (ESP). This container
>                    allows to configure integrity
>                    algorithm when no AEAD
> 
> s/allows to configure/allows configuration of/
> s/algorithm/algorithms/
> 
>                      the key is not defined (e.g.
> 
> comma after "e.g.".
> 
>                    "Packets detected out of the
>                    replay window and dropped
>                    because they are replay
>                    packets.";
> 
> s/they are/they are assumed to be/
> 
>            description
>              "List of SAD entries that conforms the SAD.";
> 
> s/conforms/forms/ again, I think.
> 
> 
> 
> 

-------------------------------------------------------
Rafa Marin-Lopez, PhD
Dept. Information and Communications Engineering (DIIC)
Faculty of Computer Science-University of Murcia
30100 Murcia - Spain
Telf: [phone] Fax: [phone] e-mail: [e-mail]
-------------------------------------------------------