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 protected]> > escribió: > > 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. [Authors] Thanks to you for the comments. > > 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/ [Authors] Fixed > > 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/ [Authors] Fixed > > 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/ [Authors] Fixed > > 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. [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. > > 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"? [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? > > 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" [Authors] Done. > > 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.] [Authors] Fully agree. We can change it to uint32, which is enough. > > 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. [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? > > 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. [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“ > > 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. [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 protected] -------------------------------------------------------
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
