Subject: [I2nsf] Benjamin Kaduk's No Objection on draft-ietf-i2nsf-sdn-ipsec-flow-protection-13: (with COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Mon, 01 Mar 2021 09:43:58 -0800

Benjamin Kaduk has entered the following ballot position for
draft-ietf-i2nsf-sdn-ipsec-flow-protection-13: No Objection

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/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

A huge thanks for the many thoughtful updates in the -13; they do a great
job of resolving my discuss and comment points from the -12 and have really
improved the document a lot.

Section 5

   In terms of security, IKE case provides better security properties
   than IKE-less case, as discussed in section Section 8.  The main
   reason is that the NSFs generate the session keys and not the I2NSF
   Controller.

nit: s/section Section/Section/

Section 5.2

   In the IKE case, the I2NSF Controller MAY need to configure the
   affected NSF with the new IKEv2, SPD and PAD information.

nit: s/MAY/may/

   imply avoiding contact with the I2NSF Controller.  Finally, the I2NSF
   Controller MAY also need to send new parameters (e.g., a new fresh
   PSK for authentication) to the NSFs which had IKEv2 SAs and IPsec SAs
   with the affected NSF.

nit: s/MAY/may/

Section 6.1.2

      grouping lifetime {
        [...]
        leaf bytes {
          type yang:counter64;
          default 0;
          description
             "If the IPsec SA processes the number of bytes
             expressed in this leaf, the IPsec SA expires and
             SHOULD be rekeyed. The value 0 implies
             infinite.";

Since this is the *limit*, it should just be a uint64; the counter type
should be used for the statistics, not the configured limit.
My apologies for being sloppy in in writing my earlier comment; I can
see how you thought that I was suggesting to use the "counter64" type
here, but that was not my intent.

        list dscp-mapping {
          [...]
          leaf inner-dscp {
            type inet:dscp;
            description
              "The DSCP value of the inner IP packet. If this
              leaf is not defined it means ANY inner DSCP value";
          }

We should probably specify one way or the other if it's allowed to have
(e.g.) one element of this list that has both inner+outer DSCP values,
and another element that has only the outer value (to match "everything
else").  That is, is the "ANY inner DSCP value" really "any", or "any
not matching other configuration"?

Section 6.2.3

      typedef autostartup-type {
        [...]
          enum start {
            description
              "IKE/IPsec configuration is loaded
              and transferred to the NSF's kernel, and the
              IKEv2 based IPsec SAs are established
              immediately without waiting any packet.";

nit: "waiting for"

              container eap-method {
                when "../auth-method = 'eap'";
                leaf eap-type {
                  type uint64 {range "1 .. 4294967295"; }

[I don't really understand why we need uint64 vs uint32 with the
explicit range limit like that, but it's not harmful.]

Section 6.3.3

              container tunnel {
                when "../mode = 'tunnel'";
                uses nsfikec:tunnel-grouping;
                leaf-list dscp-values {
                  type inet:dscp;
                  description
                    "DSCP values allowed for packets carried over
                    this IPsec SA. If no values are specified, no
                    DSCP-specific filtering is applied";

nit: It might be worth a couple more words to clarify that this is for
ingress packets received, and thus the value that would be the "inner"
DSCP value in the other configuration we have for the DSCP mapping.

                container replay-window {

If I understand correctly, the 'w', 't', and 'b' paremeters herein are
related, with the gap t-b usually (always?) being of value w.  I don't
know enough about YANG constraints to say whether they should be used to
recognize this relationship, but even if not, some prose might be in
order.

Section 8.2

When people say that "encryption algorithms [...] MUST have, at least,
the same strength as the algorithms used [...]" we like to see clarity
on how the "strength" is measured.  In this case it's probably fairly
clear to most readers, but spelling out the bit strength of the
symmetric cipher is probably still worthwhile.

---

From: Rafa Marín López <[email-masked]>
Date: Wed, 17 Mar 2021 17:28:26 +0100

Dear Ben:

Thank you very much for putting all this effort in reviewing the document.

Please, see our answers inline.

> El 1 mar 2021, a las 18:43, Benjamin Kaduk via Datatracker <[email-masked]> escribió:
> 

[Authors] Thanks to you for the comments.

> nit: s/section Section/Section/

[Authors] Fixed
>   affected NSF with the new IKEv2, SPD and PAD information.

[Authors] Fixed
>   PSK for authentication) to the NSFs which had IKEv2 SAs and IPsec SAs

[Authors] Fixed
> here, but that was not my intent.

[Authors] No problem at all. We have now included uint64. It is worth noting that since this is a grouping lifetime that is used in different places, this change has been automatically applied to places where the grouping is used.

> not matching other configuration"?

[Authors] If we focus on the leaf inner-dscp “ANY inner DSCP value” really means “any”. Having said that, after your comment, another clarification is required in the description of the list dscp-mapping. The reason is that it is important to express “any not matching other configuration” case. In this sense, let’s imagine we want to express the following mappings  (x1,y1), (*,y2) . Last node implies that any value for inner-dscp (except x1 that maps to y1)  will map to y2. The order is important here.

<dscp-mapping>
             <id>1</id>
             <inner-dscp>x1</inner-dscp>
             <outer-dscp>y1</outer-dscp>
             <id>2</id> 
             <outer-dscp>y2</outer-dscp>

</dscp-mapping>

If we want to map any inner-dscp value to 0, then adding a node to the list without an inner-dscp leaf (ANY) and with one outer-dscp leaf without specific value will make it, because outer-dscp leaf has a default value of 0. In other words, 

<dscp-mapping>
             <id>1</id>
             <inner-dscp>x1</inner-dscp>
             <outer-dscp>y1</outer-dscp>
             <id>2</id> 
             <outer-dscp></outer-dscp>
</dscp-mapping>

 Thus, following your comments, All this would require to extend the description part in the list dscp-mapping as follows:

“A list that represents an array with the mapping from the inner DSCP value to outer 
 DSCP value when bypass-dscp is False. To express a default mapping in the list where any other inner dscp value is not matching a node in the list, a new node has to be included at the end of the list where the leaf inner-dscp is not defined (ANY) and the leaf outer-dscp include the value of the mapping. If there is no value set in the outer-dscp leaf the default value for this leaf is 0.”

Is this reasonable?

>              immediately without waiting any packet.";

[Authors] Done.

> explicit range limit like that, but it's not harmful.]

[Authors] Fully agree. We can change it to uint32, which is enough.

> DSCP value in the other configuration we have for the DSCP mapping.

[Authors] Yes, you’re right. We are assuming that when you mention “ingress packets received” you mean the packets that ingress to the tunnel from one side and have to be sent to the other endpoint of the tunnel.

Having said this, it is worth mentioning that, when you comment “in the other configuration we have for the DSCP mapping”,  it is happening when bypass-dscp is false (in that case the dscp-mapping is applied). In this way, we could modify the description as follows:

"DSCP values allowed for ingress packets carried over this IPsec SA. If no values are specified, no DSCP-specific filtering is applied. When ../bypass-dscp is false and a dscp-mapping is defined, each value here would be the same as the “inner" DSCP value for the DSCP mapping (list dscp-mapping)". 
 
What do you think?

> recognize this relationship, but even if not, some prose might be in

[Authors] According to Appendix A2.1 in  RFC 4303 ,  “...W = T - B + 1 and cannot
   exceed 2^32 - 1 in value.”

We can include some text in the description adding this information. It is worth mentioning these data are state data so this information will specify what the NSF returns must follow this rule according to A2.1 RFC 4303.

The text could be something like:

“This container contains three parameters that define the state of the replay window: window size (w), highest sequence number authenticated (t) and lower bound of the window (b). According to Annex 2.1 - RFC 4303  w = t-b+1“

> symmetric cipher is probably still worthwhile.

[Authors] Right. We can say at least 128 bits taking into account the default length along the YANG model is also 128 bit. Therefore we could change the text as follows:

“Similarly, the encryption algorithms used in the security association between I2NSF Controller and the NSF MUST have, at least, the same strength (minimum strength of a 128-bit key) as the algorithms used to establish the IPsec SAs.”

Many thanks again.

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