Re: [I2nsf] Review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-03 (Appendix A)

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

Hi Paul, all:


> 
> Appendix A:
> 
> Why is it "eipsec" and not "ipsec” ?

[Authors] We have fixed this now.
> 
> Why is the organization not the IETF? Is this commonly done for yang models?

[Authors] You're right. We will change this to:

"IETF I2NSF (Interface to Network Security Functions) Working Group"
> 
> encryption-algorithm-t - I think this should only list the MUST/SOULD items, so AES_GCM (1 flavour), AES_CCM (1 flavour) CHACHAPOLY1305 and NULL
> 
> 	description "Encryption algorithms -> RFC_5996”;

[Authors] Right. Our doubt here is if we should refer instead to 

https://tools.ietf.org/html/draft-ietf-netconf-crypto-types-02

such as 

https://tools.ietf.org/html/draft-ietf-netconf-tls-client-server-08

is doing. If you check this link you will notice is referring to 

<algorithm xmlns:ct="urn:ietf:params:xml:ns:yang:ietf-crypto-t\
   ypes">ct:rsa2048</algorithm>
   
which is referring to ietf-crypto-types

> 
> RFC 5996 is an obsolete reference. It should either refer to RFC 7296, or it should simply refer to the IANA registry involved.

[Authors] OK.
> 
> integrity-algorithm-t: If the above is agreed, this enumeration should only contain "none" and the entries from aes-cmac-96 and up. That is,
> not contain des,md5,sha1 and aes_xcbc.  Same obsolete reference is used here in the description.

[Authors] Ok.

> 
> type-autostartup: Maybe instead of RESPOND-ONLY, a better word is "STANDBY"? That way, the node could decide itself to bring a connection up
> without _requiring_ a remote signal, and could be triggered locally as as well as remotely to come up.

[Authors] Sounds reasonable. We should provide a better description of each value as well.

> 
>      description "Different types of how IKEv2 starts the IPsec SAs";
> 
> I would say "different policies of when to start an IKEv2 based IPsec SA". Does it need clarification this value is ignored or unused for the
> "case 2" scenario?

[Authors] This typedef is not used in the part of the model associated to case 2 but we can something in the description. 
> 
> auth-protocol-type:  This should only contain IKEv2. The enum should remain in case we get IKEv2.1 or IKEv3 in the future.

[Author] Agree.

> 
>      description "Peer authentication protocols”;
> 
> I would say "IKE authentication protocol version", unless you want to keep the option open to have a non-IKE protocol. But I think that would
> run into many more issues of having parameters that are not defined for IKE?

[Author] Agree.
> 
> ipsec-mode: It should only contain Tunnel and Transport Mode. It would be nice to see some text that forbids using Transport Mode when NAT is
> requested, just to avoid a lot of problems and disappointments. L2TP/IPsec still has major scars due to its use of Transport Mode with NAT.

[Authors] Sounds reasonable.

> 
> esp-encap: as stated before, we are missing the port entry. I can also image that multiple encaps could be allowed, such as TCP-443 and TLS-443.
> Also note that the RFC for ESP over TCP recommends to continiously check if UDP encapsulation has become available, since it is so much prefered
> over TCP. But that only makes sense when the remote is a roaming client, and makes less sense if the network consists of data centers.

[Authors] Ok. Let's add the port entry. In any case, I believe that is something that implementation in the NSF (in particular the NETCONF server) could check (UDP encapsulation)


> 
> ipsec-protocol: I think this should only contain esp. See earlier comments. Notably, if supporting ipcomp here, the yang model must be clear
> that it could need an ipcomp + esp IPsec SA for a single 'connection'. Even if we only leave esp as a valid option, keep this enum in case
> we come up with a new esp version of some esp-lite version.

[Authors] We are in favour on leaving only ESP. If the WG agrees, let's proceed that way.

> 
> lifetime-action only has "terminate" and "rekey". For libreswan we use "clear", "hold" and "restart". The difference
> between clear and hold is what kernel policy to install for cleartext packets. For roadwarrionrs coming in, you
> want clear to leave no trace. for a site-to-site VPN you generally want "hold" to ensure you drop all packets until
> a new tunnel is established (possible ondemand, so this is different from restart)
[Authors] Basically we have "terminate" and "rekey" since RFC 4301 says:

"Lifetime of this SA: a time interval after which an SA must be
      replaced with a new SA (and new SPI) or terminated, plus an
      indication of which of these actions should occur."
	  
We are realizing that RFC 4301 should also be updated to reflect these new possibilities.

In any case, could you elaborate a little bit more about the difference between clear and hold and the effect in the kernel? We are assuming that affects the SAD.

> 
> ipsec-traffic-direction:
> 
> Traditionally, freeswan ony used inbound/outbound and not forward. Unfortunately, KAME and Linux/XFRM kept forward there. But I see no use
> case for it in these deployments. We have nothing specified in about ipsec-sa's for the forward (it's basically implied "completely open")
> But I understand it might be useful to just specify this because it is needed. But perhaps more language is then needed to clarify this?

[Authors] We think this is an implementation aspect. We think it is better to remove fwd policy. If fwd policy is required because we have an implementation based on Linux, the NETCONF server should install that fwd policy based on the in/out received.
> 
> ipsec-spd-operation:
> 
> The value BYPASS probably means "let go in the clear" but I don't find the name very obvious.

[Authors] It has been extracted from RFC 4301 definition. 

"Processing info -- which action is required -- PROTECT,
             BYPASS, or DISCARD."
			 
In relation with our previous comment we think it would be worthy to carry out an update in the RFC 4301

> The operation modes BYPASS and DISCARD cannot be configured with the current yang parameters
> for IKE? How would this be configured, eg to place a DISCARD for 192.1.2.0/24 ?

[Authors] It is not needed to configure it in the IKE model since it is already configured in the SPD model. The RFC 4301 defines how the SPD is composed, and following this, we have defined the SPD model regardless whether we are in IKE case or IKEless case. In other words, both cases uses the information contained in the SPD model. Having said this, we think it would be useful to explicitly "link" each connection in IKEv2 with the SPD entries associated to that connection. For example, per each connection we could add a list of spd-entry-id, which point to the SPD entries associated to that connection. From your comment we have realized that this issue is not well explained in the document.

Thus, regardless the case we are using, the policies establishing the type of traffic and protection to be applied are indicated in the SPD model (i.e. by means of a list of spd-entry). So the SPD model is used for both, IKE case and IKE-less case. Thus, we have the following situation. 

- For IKE case, the whole configuration is distributed in two parts. While the IKE SA configuration is represented as “ike-conn-entries”, the the CHILD SA configuration will be found as a list of “spd-entry”. 
- For IKE-less case, only the CHILD SA configuration will be accessible as a list of “spd-entry”.

What do you think?



> In libreswan
> we can configure a connection with type=transport|tunnel|drop|reject but that meshes up the
> IPsec Mode with the SPD Operation. It is also a bit odd to configure "IKE" for a "DISCARD"
> since it really does not define anything in IKE - it bypasses IKE to put in an IPsec policy
> (SPD). But if the yang model used the ipsec parts to do this, it could be mixing IKE + manual
> IPsec policies, which could really confuse the IKE daemon. I am not sure what the best fix for
> this is. Note REJECT which is DISCARD+ICMP message is not defined here. Should it? I don't
> think Linux/XFRM/netlink supports this.

[Authors] As we see it, we are not mixing IKE + manual IPsec policies. SPD is the central point of configuration for IPsec policies in the model. In any case, we talk about implementation. In particular, when the controller configures the SPD first and the IKE model with a connection, this connection will have a list of spd-entry-id to refer to the particular SPD entries that it has to configure in the IKE daemon associated to that connection.  About what is happening with drop / reject and the fix we don't really know either but I think this is not a problem for the model itself, right? As a note, ip xfrm allows : "allow" or "block" 
> 
> ipsec-next-layer-proto:
> 
> See earlier note. I think this is a really bad name.

[Authors] This is defined in RFC 4301 with this name. In ip xfrm is UPSPEC.  is Upper Layer Protocol better?
> 
> ipsec-spd-nam:
> 
> This is the first time I have seen an SPD as having a name. normally these just have numbers. Or they
> have a number that is the link between SPD and SAD (with linux, that is called reqid=)
> So maybe add an enum id_number so implementors could map this onto the linux implementation ?

[Authors] RFC 4301 says explicitly in page 28.

"- Name:  This is not a selector like the others above.  It is not
        acquired from a packet.  A name may be used as a symbolic
        identifier for an IPsec Local or Remote address."
		
"An SPD is an ordered list of entries each of which contains the
		   following fields.

		           o Name -- a list of IDs.  This quasi-selector is optional.
		             The forms that MUST be supported are described above in
		             Section 4.4.1.1 under "Name". “"


But yes, we can replace this name by other identifier, as you suggest.
> 
> auth-method-type:
> 
> See earlier comment about asymmetric authentication.
> 
> This should only contain pre-shared, null, eap, rsa, rfc7427(digital signatures). That is I think dss-signature can be removed.
> I also wonder if this entry needs to have a better difference for the source of the private/public keypair, eg raw RSA versus
> RSA certificate based authentication. I guess this is now implied by not having a certificate entry?

[Authors] Mmmmm... good question. We think we should include at least a "raw public key" leaf so if the leaf is set then we have raw public key. If we include certificate then we will have RSA certificate based authentication. In any case, if ECC-based signature is required we would also need to support ECC public and private keys. In any case, this part in the model needs to be udpated to support RFC 7670 and RFC 7427

 There is also another aspect: if eap is chosen, what eap method is allowed. I think a container mentioning the EAP method should  be included. Moreover, we should include a way to send the credentials for the particular EAP method.

> 
> Also rename description "Peer authentication method" to "IKE authentication method" (it should mention this is IKE, but also
> in case of asymmetric use, this is not neccessarilly for the peer but how we authenticate _to_ the peer)

[Authors] Sounds reasonable

> 
> sa-state:  I am not sure why this is needed? Unless someone would want to manually place larval/acquire states in the kernel
> without IKE, but then who is going to listen to these ACQUIRE messages? A non-IKE daemon? See earlier discussion on this topic.
> If you do want to get netlink/pf_key messages for max-counter, max-bytes etc without IKE, then this would be required. Otherwise
> this should be removed.

[Authors] This is precisely what we want: "to get netlink/pf_key messages for max-counter, max-bytes etc without IKE" . This is captured by the NETCONF server to send notifications to the controller in IKEless case. Having said this, most probably this state is not completely required though as we mentioned in section 6's comments , the IPsec SA appears in the state. It seems too much detail for the regular operation of the controller, even considering IKEless case.
> 
> lifetime: See earlier comments.
> 
> grouping auth-method-grouping: See earlier comments about asymmetric authentication and raw vs cert based signatures. Note based
> on above entries, this is missing a dss-signature section. So if you agree with earlier comment to remove dss-signature, then
> it not appearing here is fine.

[Authors] Yes, we are going to remove dss-signature and extend the auth-method container to contemplate the use of raw public keys as well as generic digital signatures (rfc 7427).
> 
> grouping identity-grouping:  See earlier comments on ID_NULL / AUTH_NULL support

[Authors] We failed to see this earlier comments about ID_NULL and AUTH_NULL, could you point us to them?


> 
> grouping port-range: I don't see anywhere that protocol and ports together are "overloaded" for things like ICMP message types.
> Should language be added for that?

[Authors] Not sure what are you are referring here... could you elaborate? We have seen that description should be changed as well.
> 
> SAD and SPD grouping : See earlier note about Security Labels. It would need to be added here. Linux/XFRM already supports these.

[Authors] Not sure about this. Right now, the usage of security labels seems associated to specific implemenation (Linux/XFRM) and https://tools.ietf.org/html/draft-sprasad-ipsecme-labeled-ipsec-00 has not been evolved since March 2018 (is it going to be adopted?).
> 
> I see container-ah and container-esp, but what about container-ipcomp? Again, if we remove IPCOMP as I'd like, then we are fine
> here. but if not, then something needs to be added here. If AH is removed as I'd recommended, it needs to be removed here.

[Authors] That's right. We think we could remove that ipcomp and ah if the WG is agree.
> 
> 	container sad-lifetime-soft {
>         	description "SAD lifetime hard state data";
> 
> that "hard" should be "soft”.

[Authors] Right.
> 
> I am confused that "hard" and "soft" have an action that can be similar. soft could have an action to just notify userland via
> XFRM/PF_key, but "hard" can only have an option "terminate” ?

[Authors] Your comment makes sense. 
> 
> container encap: Now I see the sport / dport, but I'm still confused how this is populated from the higher level Security Controller
> communication (if not IKE). Note also the Security Controllers cannot know the port numbers without sending any traffic so the
> NAT gateway crates a mapping.

[Authors] As mentioned earlier, this can be informed by the NSF through a YANG module related with NAT management. 
> 
> list traffic-selector-list: Can a traffic selector have a direction of fwd ? Not in the IKE protocol.

[Authors] fwd direction has been removed now.
> 
>            leaf mode { type ipsec-mode; description "transport/tunnel"; }

[Authors] True. We will add them but removing BEET.
> 
> This "description" does not match all the available options defined before.
> 
> container ah-algorithms: See AH comments before. Also see IPCOMP comments before.

[Authors] As mentioned we are in favour of removing AH and IPCOMP if the WG agrees on that.

> 
> 	description "Configure ESP encryption"
> 
> Maybe say "Configure ESP/AEAD encryption”

[Authors] This would be applicable if we finally agree that only AEAD will be used. But in the last e-mail exchange in the mailing list, it looks like we are not limiting us to a case where only AEAD is allowed. 
> 
> spd-mark:
> 
> Note that the IKE case cannot negotiate this spd-mark value. Is such a capability needed? It is similar to negotiating a Security Context.

[Authors] It is in linux kernels and handled by XFRM but we think you're right and we could remove it from the model. In any case, do you think there is an important example case for its usage?
> 
> 
> container spd-lifetime-*: See earlier comments on hard/soft and acceptable actions

[Authors] OK.
> 
> grouping isakmp-proposal: I would call this ike-proposal as isakmp is kind of an IKEv1 term. See also phase1-* value suggestions earlier to
> make those ike-* or ike-sa-* values. This group is also missing pfs= which for IKEv2 only starts to matter when rekeying and whether or not
> to do a new DH.

[Authors] Agree. We will change the name. Also true about pfs. We will add this information.

> 
> grouping phase2-info: Call this ipsec-info: or ipsec-sa-info:

[Authors] Correct. As mentioned we think it would be better if this ipsec-sa-info has a lists of spd-entry-id pointing the SPD policies associated.
> 
> 	leaf-list pfs_group { type uint32;


> 
> This should really be an enum and not a uint32.

[Authors] Ok. We will change it.
> 
> typedef sadb-msg-type: where/when are these used? That is, I understand the IKE <-> IPsec API uses this, but when does this ever need to
> go over the Security Controller's communication ?

[Authors] In IKEless, the sadb_acquire notification carries this information to the Controller. Basically, what it is happenning is that the NETCONF server in the NSF captures the sadb_acquire from the kernel and builds a NETCONF notification which is sent to the Controller. Having said this, it is true that this is not required anymore since we know that this type can be only SADB_ACQUIRE since we are sending a sadb_acquire. 
> 
> typedef sadb-msg-satype: same here

[Authors] The controller needs to know what IPsec SA should be negotiated. We have realized that notificiation sabd_acquire should carry the policy that caused the acquire so that the Controller can know how to add the IPsec SA. Thus, we propose the following modification to the sadb_acquire notification:

notification sadb_acquire {
      description "A IPsec SA is required ";
      uses selector-grouping;
	  container tunnel {
	           when "../mode = 'TUNNEL'";
	           uses tunnel-grouping;
	           description "If TUNNEL mode is used the sadb_acquire notification carries the endpoints";
	        }
   }
> 
> container algorithm-supported: See earlier comments about all moderm algorithms not having a min/max but only having a limited set, for
> current ones this is only 128/192/256

[Authors] Ok.
> 
> Note the version of IKE within the ietf-ipsec namespace only has IKEv2. I agree with that now but earlier in the document it has IKEv1
> support/entries. So this needs to be made consistent here.
[Authors] Correct. We should only keep ikev2

> 
> container ike-stats: does not contain encaps entry to show if it is using UDP, TCP or TLS and on which port.

[Authors] Are you referrig to the encapsulation of the IKEv2 messages? 
> 
> list child-sas: does not contain some "state" that can be negotiated as part of an IKE session. For example, if narrowing was used, it could
> be that the current SPD is more narrow then the "connection" defined SPD. There seems to be no way to store this. Thus narrowing is not
> possible. If narrowing is not in scope here, it should probably be made more explicit. Otherwise, it needs to be better integrated.

[Authors] Basically, we are including a list of child-sas negotiated. Basically, in terms of implementation, as an example, we would provide this information with the output, for example, either ip xfrm state list or using the list-sa event in strongswan (https://www.strongswan.org/apidoc/md_src_libcharon_plugins_vici_README.html)
> 
> container number-ike-sas: See earlier note about missing COOKIES parameter here.
> 
> 
> SPD related question: Is there support for multiple TSi/TSr generating a list of spd's in a single Child SA? If so, a Child SA can have
> multiple SPDs. Otherwise, it cannot. I dont think the model is very clear on this aspect. (I speak from experience, libreswan as an
> implementation is also not very clearly specified here, and we have one known interop issue with openbsd's iked because of this)

[Authors] So openbsd's iked allows a child SA assocciated to several policies, right? 

Regarding your comment " list of spd's in a single Child SA" . Could you ellaborate a little bit more? The model is not clear in this aspect since we have not considered this case. Are you referring that a list of policies are mapped to the same child SA?. In any case, we think this would be part of the implemenation of the ike model. That is: we are doing is to inject the SPD entries that have been configured in the SPD model in the connections defined in the IKEv2 model into the particular IKEv2 implementation. For example we can use VICI in strongswan for that. If the IKEv2 implementation is able to associate a list of spd in single child SA, should the controller be able to say anything about it? Do you think we should add a flag or something for this?
> 
> 
> // Those RPCs are needed by a Security Controller in case 2 */
> 
> Please think about XFRM/netlink and/or ACQUIRE messages that are needed if there is no IKE daemon. eg when max-counter is reached
> or when a hard limit action is performed by the kernel.

[Authors] We already have modelled those messages (sadb_acquire is a notification) 

notification sadb_acquire {
      description "A IPsec SA is required ";
      uses base-grouping;
   }

   notification sadb_expire {
      description "A IPsec SA expiration (soft or hard)";
	  ....

> 
> I am missing an "SPI already in use" error, in the case it is attempted to install an SPD that already exists and thus would conflict.
> 
> 

[Authors] That will happen when the controller sends an NETCONF edit-config trying to add that an already existing SPI. From NETCONF rfc  "The <rpc-error> element is sent in <rpc-reply> messages if an error
   occurs during the processing of an <rpc> request."

Right now, , the spi value is the key element for the list of SAs in the model for this case, so the NETCONF server will detect it is already used. If it is already used the NETCONF server considers the SC wants to "update" de SA, but not to add a new one. 
   
It is true that we have NOT defined any explicit "error-message" for this rpc-error. We will add this.

Best Regards.

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