[IPsec] Review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-03

Paul Wouters <paul@nohats.ca> Sun, 18 November 2018 06:53 UTC

Return-Path: <paul@nohats.ca>
X-Original-To: ipsec@ietfa.amsl.com
Delivered-To: ipsec@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 32828130D7A; Sat, 17 Nov 2018 22:53:18 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2
X-Spam-Level:
X-Spam-Status: No, score=-2 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=nohats.ca
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 19rE9jqWX3VS; Sat, 17 Nov 2018 22:53:13 -0800 (PST)
Received: from mx.nohats.ca (mx.nohats.ca [193.110.157.68]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 2991A126CB6; Sat, 17 Nov 2018 22:53:12 -0800 (PST)
Received: from localhost (localhost [IPv6:::1]) by mx.nohats.ca (Postfix) with ESMTP id 42yN2m3d8Pz1GN; Sun, 18 Nov 2018 07:53:04 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nohats.ca; s=default; t=1542523984; bh=gFEJ33HyWhhNWfPzgwj6vn4FZu+HybZggrwyBr60Y0s=; h=Date:From:To:cc:Subject:In-Reply-To:References; b=ldoA+HI6HStI6oPN9dqz6FBWzY0Wf8jNb+jWftOQKriH6/cO1wHV/2zi5Y7TZFnAg 503yC6vt8Jv63PUPSYSccdyEQEwwip1SYW7B4WerViB+LdN5oVHlqTrxzjUAq1RE4b tiVynx75tzjUdoekPSlAuS9sT2zlUvW3RYszGIgU=
X-Virus-Scanned: amavisd-new at mx.nohats.ca
Received: from mx.nohats.ca ([IPv6:::1]) by localhost (mx.nohats.ca [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id NsObF41p8ZJR; Sun, 18 Nov 2018 07:53:00 +0100 (CET)
Received: from bofh.nohats.ca (bofh.nohats.ca [76.10.157.69]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx.nohats.ca (Postfix) with ESMTPS; Sun, 18 Nov 2018 07:52:58 +0100 (CET)
Received: by bofh.nohats.ca (Postfix, from userid 1000) id 194842EEDB5; Sun, 18 Nov 2018 01:52:58 -0500 (EST)
DKIM-Filter: OpenDKIM Filter v2.11.0 bofh.nohats.ca 194842EEDB5
Received: from localhost (localhost [127.0.0.1]) by bofh.nohats.ca (Postfix) with ESMTP id 0A7F641C3B21; Sun, 18 Nov 2018 01:52:58 -0500 (EST)
Date: Sun, 18 Nov 2018 01:52:58 -0500 (EST)
From: Paul Wouters <paul@nohats.ca>
To: Yoav Nir <ynir.ietf@gmail.com>
cc: Rafa Marin-Lopez <rafa@um.es>, i2nsf@ietf.org, "ipsec@ietf.org WG" <ipsec@ietf.org>
In-Reply-To: <DAE14995-8504-4134-B021-93D56A4994FB@gmail.com>
Message-ID: <alpine.LRH.2.21.1811180149220.25604@bofh.nohats.ca>
References: <A881C135-9BF7-4E93-BB7A-75EB3D1FF605@gmail.com> <6839D47C-4074-486F-9350-8EB7B378036C@um.es> <DAE14995-8504-4134-B021-93D56A4994FB@gmail.com>
User-Agent: Alpine 2.21 (LRH 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT
Archived-At: <https://mailarchive.ietf.org/arch/msg/ipsec/EexUBThjucnid1NOCOmpEovJlaA>
Subject: [IPsec] Review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-03
X-BeenThere: ipsec@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Discussion of IPsec protocols <ipsec.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ipsec>, <mailto:ipsec-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ipsec/>
List-Post: <mailto:ipsec@ietf.org>
List-Help: <mailto:ipsec-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ipsec>, <mailto:ipsec-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 18 Nov 2018 06:53:18 -0000

On Wed, 14 Nov 2018, Yoav Nir wrote:

> As discussed in the room, we need some reviewers for the sdn-ipsec-flow-protection draft ([1])
> 
> Thanks for these comments. Please see our response below.
> 
> While any comments on any part of the document are welcome, I would like people to concentrate on the following issues:
>  *  The YANG model in Appendix A
>      +  Some of the crypto seems obsolete (example: DES). We would get into trouble in SecDir review.  OTOH ChaCha20-Poly1305 is missing..

Based on the introduction and abstract of the draft, this document does two things:

1) Specify a yang model for use with SDWAN + IKE + IPsec
2) Define the desired modes and algorithms to use with 1)

It does not try to map the entire IKE/IPsec IANA registry into a yang model. Let me know if this is incorrect, because I use
this as an assumption for the remainder of the review.

> The TLS working group went quite far with TLS 1.3.  Only 2 ciphers remain: AES-GCM with 16-byte ICV, and ChaCha20-Poly1305. That’s it.  Specifically, they’ve deprecated everything
> that isn’t an AEAD.

I think this can work for this SDWAN use case as well. While I don't think all systems
are ready to support ChaCha20-Poly1305 for both IKE and ESP, I do believe all systems
used for SDWAN should be able to do AES-GCM with 16-byte ICV for IKE and ESP. Although
some IKE clients do not yet support AES-GCM for IKE (even while they support it for ESP)

> The IPsecME working group hasn’t gone that far yet.  But in practice pretty much nothing is used except 3DES, AES-CBC, and AES-GCM.  Perhaps ChaCha20-Poly1305 is starting to see
> some use by now. We have RFC 8221, especially sections 5 and 6.  I think (although it’s up to the working group) that we should be fine defining only the MUSTs and the SHOULDs in
> those sections.

Yes, with the exception of ENCR_NULL for encryption and AUTH_HMAC_SHA1_96 for integrity. (the latter is a MUST- only because of existing
deployment, it is not recommended for new things)

> That brings another question. What is our plan for future expansions?  Suppose there’s some hot, new algorithm that everyone is implementing. How do you update the YANG model in
> the future when you add new values to the enumerations?  Is it up to the administrator to make sure that the controller and NSFs are all on the “same page”?

I'm wondering about that too, since it is basically a snapshot of the
IANA registry SHOULD/MUST entries which changes over time.



General comments:
I'd like to see "Case 1" and "Case 2" replaced with more descriptive terms. I keep losing track of which case is which.
Perhaps call them IKE Case and IKE-less Case ?


Section 1:

    Typically, traditional IPsec VPN concentrators and, in general,
    entities (i.e. hosts or security gateways) supporting IKE/IPsec, must
    be configured directly by the administrator.  This makes the IPsec
    security association (SA) management difficult and generates a lack
    of flexibility, specially if the number of security policies and SAs
    to handle is high.

This is not entirely true. We have Opportunistic IPsec that automates
this precisely for the same reasons. We also have packet triggers in
combination with Traffic Selector narrowing to create multiple IPsec
SA's on demand. I know this is all qualified by "typically" but I think
this paragraph is not adding any real information. The idea is that we
have an SDWAN situation with SDN controllers and nodes, and we want to
automate the IKE/IPsec for that specific deployment use case.


    The analysis of the host-to-gateway (roadwarrior) scenario is TBD.

Does this sentence mean this is to be done in this document or is it out
of scope for this document (but it does allow it to be added later) ?

Section 3:

      It requires information about the
      required authentication method (i.e. preshared keys), DH groups,
      modes and algorithms for IKE SA negotiation, etc.

In the IKE world, we really try to not recommend preshared keys, because
these keys mostly based on human readable low entropy content. If this
document thinks raw RSA/ECDSA keys or X.509 certificates are also methods
that will be implemented by SDN Controllers, please change the example of
preshared keys to something else.

Section 5.1:

The diagram lists:

 	|   IKE    |      IPsec(SPD,PAD)  |

Shouldn't that be:

         | IKE(PAD) | IPsec(SPD, SAD)

IKE is using the PAD information to tell IPsec to establish SPDs and SADs) ?


Section 5.1.1:

What does this mean:

 	Moreover, an east-west interface is required to exchange IPsec-related information.

Section 5.2:

    In this case, the NSF does not deploy IKEv2 and, therefore, the
    Security Controller has to perform the management of IPsec SAs by
    populating and monitoring the SPD and the SAD.

I would write:

In this case, the NSF does not deploy IKEv2 and, therefore, the
Security Controller has to perform the IKE security functions and management
of IPsec SAs by populating and managing the SPD and the SAD.

I added "IKE security functions" to make sure some entity is responsible
for those items that are normally done by IKE (IV generation, prevent counter
resets for same key, etc).

I changed monitoring to managing, because I don't understand how it would "monitor"
the NSF or what it would monitor. I assume also if during monitoring it finds
something that needs changing (eg vpn tunnel up or down) that it will do so,
so I think managing is the better word.

For this diagram, I would say IPsec(SPD, SAD) would need to also list the IKE
security function it needs to use in an IKE-less scenario, which above I had
called "management of IPsec SAs"

Section 5.3:

As mentioned before, a better title would be: IKE vs IKEless

I would also start this section with:

Case 1 is more secure, because all security features of IKE are used to manage
the IPsec SAs.

Section 5.3.1:

    For case 1, the rekeying process is carried out by IKEv2, following
    the configuration defined in the SPD.

Technically this is the SPD and SAD, as the SPD would for example list the
maximum number of packets allowed, but the SAD lists the number of packets
that have happened.

I am a bit confused by the rekey description. I guess it assumes that the
Security Controller sending of information is blocking? Eg when it tells
the node to install an inbound IPsec SA, it is blocked from doing anything
else. If this is not the case, then the 3 step program should be clarified
that it needs to also receive confirmations. And if this is a non-blocking
process, who is responsible for "retransmits" of these confirmations ?
The same applies to the blocking or time-wait between step 2 and 3.

Also, I would say step 3 can be get rid of if the IPsec implementation can
itself detect traffic on the new IPsec SA and it can delete the old IPsec
SA itself without instruction from the Security Controller.

Section 5.3.2:

 	If one of the NSF restarts, it may lose part or all the IPsec state

This "may" suggests a very interesting case. I doubt the NSF would store on
non-volatile memory the IPsec symmetric keys for encryption, and the counter
or ICV's used. If it could store that, the document needs some text about
this in the Security Considerations because it will allow for easier theft
of session material.

Rather, I suspect this may really is not true, and all of these devices would
lose state?

 	In both cases, the Security Controller MUST be aware of the affected NSF

I think this MUST is a little misleading. It is not desired behaviour but just
an observation - it cannot not know its TCP session died ? Perhaps write:

 	In both cases, the Security Controller is aware of the affected NSF

A similar case for the next sentence MUST keyword:

 	Moreover, the Security Controller MUST have a register about all the
 	NSFs that have IPsec SAs with the affected NSF.

so write:

 	Moreover, the Security Controller has a register about all the
 	NSFs that have IPsec SAs with the affected NSF.


 	(It is TBD in the model).

This remark seems to need to be resolved before publication ?

    In Case 2, if the Security Controller detects that a NSF has lost the
    IPsec SAs (e.g. it reboots) it will follow similar steps to rekey:
    the steps 1 and 2 remain equal but the step 3 will be slightly
    different.  For example, if we assume that NSF B has lost its state,
    the Security Controller MUST only delete the old IPsec SAs from A in
    step 3.

I don't understand why this paragraph about NSF state loss seems to talk
about "step 3" which can only be referring to the "rekeying process" of
the previous section. It should only mention that it should configure new
IPsec SAs between the failed node and all the nodes the failed node talked
to and only after those are estlibhsed, it can send a delete for the old
IPsec SAs on the non-failed nodes. This prevents the non-failed nodes from
leaking plaintext.

Section 5.3.3:

I'm confused how the Security Controller can know what NAT ports to convey to
the NSF agent. Without IKE traffic, there is no port NAT mapping? So unless
the Security Controller _is_ the NAT gateway, it would never know this
information? So I am skeptical if the NAT case can ever work without IKE.
I also do not know how the Security Controller could decide between UDP,
TCP or TLS without information from an IKE negotiation.

Section 6.1:

Note that the use of start and end addresses, means that this can never work
with IKEv1, that can only negotiate CIDR networks. Perhaps this should be
explicitely stated somewhere just in case some developer thinks they can
use IKEv1 for this?

I dont understand the names in:

 	next-layer-protocol*   ipsec-next-layer-proto

It talks about ip-address and not ipsec-next-layer-ip-address? Simiarly it
talks about local-ports. So why not just call this proto or protocol ?

Is selector-priority needed? Can the ts-number not be specified to be "lower
numer is higher priority" ?


Why do we have security-protocol? If you are considered about small footprints,
clearly you would implement only ESP and use ESP-NULL if encryption is not
needed. And simply not implement AH (which is already optional not mandatory
to implement for IPsec). Also, this assumes IPCOMP, which has its own complexity,
would also not be used. I think a case for AH would be very weak, while a case
for IPCOMP could be made. I'd recommend only supporting ESP, at which point
you would not need this parameter, or you could keep it for completeness
sake even though it could only have one value "esp". This would also allow
an updated ESP or some ESPlite to be used in the future if such a thing would
be developed.

If we only go with AEAD algorithms, then the whole ah-algorithms part can be
removed too. And esp-algorithms could be changed to only contain esp-encryption
(or could be renamed aead-algorithm)

I see spd-mark but not spd-security-context  (see my Labeled IPsec draft)

I found the names in lifetimes confusing. "added" really means maxlife and
"current" really means maxidle I think? So I think the names would be
clearer as "time", "idle", "packets", "bytes". I think the time and idle
units of uint64 could be narrowed down if the yang language allows some
kind of timestamp type.

I am missing an entry for TFC (confidentiality, padding)
I also see no entry for IPCOMP algorithm. But I am fine with not allowing
IPCOMP at all :)

I am missing replay window parameters ?

I don't know what statefulfragCheck is ?

Section 6.2

See Section 6.1 comments.

Why are the encap entries in 6.2 and not 6.1 ?

What is the "state?" entry ?

Why is rpcs: only here?

I would not use minbits, maxbits. All modern ciphers, especially AEAD, are fixed key sizes. Make it a list of key sizes

I seem to miss the AEAD IV values ? Possibly the truncation values ?

Section 6.3:

I only see PSK and RSA algorithms? No ECDSA via RFC 7427 Digital Signatures?
(no PAKE, we have two IKE PAKE's although it looks like we want an CFRG new one)

Should PSK be of type "string". Some people want to hex entry? Although those are often written
as strings with a 0x prefix.

I see some CRL entries, but no OCSP entries. Is it expected those URI's are stored within the certificates,
and the received OCSP responses will never be communicated through this interface?

I see no local and remote part, meaning that only symmetric authentication methods can be used. IKEv2 supports
asymmetric authentication methods as well. Especially when using EAP and sometimes with AUTHHNULL, this method
is very asymmetric. So I believe this entry needs to be split in a local/remote auth method.

Section 6.4:

type-autostartup issues...

  typedef type-autostartup {
       type enumeration {
          enum ALWAYSON { description " ";}
          enum INITIATE-ON-DEMAND {description " ";}
          enum RESPOND-ONLY {description " ";}
       }
       description "Different types of how IKEv2 starts the IPsec SAs";

With libreswan we have similar keywords, and run into some interesting issues. We found what is really needed is to
know the current state and the connection desired state. for example, when using ondemand and the connection is up,
it is important to know it is up because of ondemand. So that when you receive a Delete request, you can go back
to ondemand state. If you dont keep this state then you cannot tell the difference between alwayson/ondemand if
the connection is "up". Similarly, we had issues where connections has a definition like above, but the administrator
performs an action. It is unclear if such an action is considered a temporary override or basically a runtime update
of the configuration. So lets say we are in respondonly, and the administrator brings the connection up. Now the other
end sends a delete. Do you assume you are in alwayson state or in ondemand state? And if you believe alwayson, be
prepared for a war with the remote endpoint that keeps sending deletes because it wants to be down.


Why is there nat-traversal and espencap. Why not just have espencap allow to have a value of 'none' ?
IKEv2 allows more then one oaddr  (eg multiple SOURCE NAT IPs) but the value is not a list?

The terms phase1-* should be called ike-* or ike-sa-* as phase1/phase2 is IKEv1 terminalogy not used for IKEv2.

Why is combined-enc-intr needed? These are stored with the encalg entries anyway.

I see:

 	+--rw local
         |   +--rw (my-identifier-type)?

 	+--rw remote
         |   +--rw (my-identifier-type)?

I think "my-identifier-type" should be "identifier-type". Talking about "my" for the remote peer is very confusing.

my-identifier-type does not support the value ID_NONE from RFC 7619

Why is there no secion for ike-lifetime-{soft|hard} ? We also have FIPS requirements that IKE SA's cannot do more
then X bytes for Y algorithm. And also bringing down idle IKE SA's etc.

I think "added", "used", "bytes" "packets" should be called lifetime, idletime, maxbytes and maxpackets

The timestamp values "added" and "used" could have a better type than uint64, eg unix epoch of unixtime or something?

the term ike-stats was confusing me. I thought at first these were the global IKE statistics, like how many tunnels
are up, how many errors received, but it turned out to be the state of a singe IKE SA. So perhaps ike-sa-state is a
better term? IKE state I expect things like SPIi and SPIr as listed here.

I am not sure why there are three nat-* bools instead of one value representing the nat-state? Possibly because we
use a number of bits to indicate which of the NAT properties are set, and you don't do this kind of bit mapping in
yang?

I dont see an entry for "latest IKE SA". We have that in libreswan. You can have multiple IKE SA's when rekeying,
and the older one is just lingering to die. Any operations such as informational requests, dpd/liveness probes
should only be done with the "latest IKE SA".  However, this is kind of a state on the IKE node, so I am not
sure if this belongs in the yang model or not.

I see a total SA's and halfopen SA's but I don't see anything related to anti-DDOS cookies. Eg the number of half
open SA's before enabling cookies. Perhaps add half-open-cookies-treshhold ?

Section 7.1

Yes I do like this diagram style with less -+-+-+-+ better. Please use it everywhere in the document :)

 	In case 2, flow-based security policies defined by the administrator are also translated into IPsec SPD entries

I don't understand the use of "also" in this sentence. Since these are not translated into IKEv2 policies like case 1.
(also reminder to use "IKE case" and "IKEless case" instead of "case 1" and "case 2")


Question: since IPsec SA parameters installed in the kernel have lifetime values, what is installed here? Is it installed
without any limits? If no limits what happens at max counter for AEAD? If there are limits, and the kernel generates
ACQUIRES for userland, and no IKe is listening, what happens? How are things signaled? Does the ikeless node listen
for netlink/pf_key messages to relay to the Security Controller?

Section 7.2:

Where does the terminology East/West come from? (and earlier South) ?

      3.  The Security Controller A realizes that protection is required
        between the NSF1 and NSF2, but the NSF2 is under the control of
        another Security Controller (Security Controller B), so it starts
        negotiations with the other controller to agree on the IPsec SPD
        policies and IKEv2 credentials for their respective NSFs.  NOTE:
        This may require extensions in the East/West interface.

The negotiation protocol for this is not specified. I would think IKE can be used here? Is there a reason why this is
not being defined here as using IKEv2? Is there another protocol ?  Especially since in the next case (with figure 6)
it does state this protocol can be IKEv2 and states for both cases an IKE implementation was used.


Section 8:

Is this section supposed to be an "Implementation Details" Section as per RFC 7942? If so, it is missing the required
note to the RFC Editor to remove the entire section before publication as RFC.

section 9.1:

In case 1, add a note to use only strong PSKs, with a minimal length and strength.

Section 9.2:

 	when ESP is used

Hoping my advise is taken to only use ESP and not AH, and to use ESP-null in the case of encryption being unwanted, please
remove this comment as ESP would always be used.

 	includes the keys for integrity and encryption

If we only allow AEAD's, maybe rewrite or leave tis out.


Appendix A:

Why is it "eipsec" and not "ipsec" ?

Why is the organization not the IETF? Is this commonly done for yang models?

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 -&gt; RFC_5996";

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

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.

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.

       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?

auth-protocol-type:  This should only contain IKEv2. The enum should remain in case we get IKEv2.1 or IKEv3 in the future.

       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?

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.

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.

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.

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)

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?

ipsec-spd-operation:

The value BYPASS probably means "let go in the clear" but I don't find the name very obvious.
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 ? 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.

ipsec-next-layer-proto:

See earlier note. I think this is a really bad name.

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 ?

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?

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)

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.

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.

grouping identity-grouping:  See earlier comments on ID_NULL / AUTH_NULL support

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?

SAD and SPD grouping : See earlier note about Security Labels. It would need to be added here. Linux/XFRM already supports these.

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.

 	container sad-lifetime-soft {
          	description "SAD lifetime hard state data";

that "hard" should be "soft".

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" ?

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.

list traffic-selector-list: Can a traffic selector have a direction of fwd ? Not in the IKE protocol.

             leaf mode { type ipsec-mode; description "transport/tunnel"; }

This "description" does not match all the available options defined before.

container ah-algorithms: See AH comments before. Also see IPCOMP comments before.

 	description "Configure ESP encryption"

Maybe say "Configure ESP/AEAD encryption"

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.


container spd-lifetime-*: See earlier comments on hard/soft and acceptable actions

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.

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

 	leaf-list pfs_group { type uint32;

This should really be an enum and not a uint32.

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 ?

typedef sadb-msg-satype: same here

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

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.

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

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.

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)


// 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.

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.






NITS:

Introduction:

What does this mean:

 	"with a reduced intervention of the network administrator."

Section 1:

IPsec architecture -> The IPsec architecture


Section 5.1:

    besides the IPsec support.

Change "besides" to "along with" ?

    With these entries, the IKEv2 implementation can operate to establish
    the IPsec SAs.

Change to:

    With this information from the SDN Controller, the NSF can use its IKEv2
    implementation to establish the IPsec SAs.


Personally I don't like this Christmas like boxes:

                 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                 |IPsec Management/Orchestration Application | Client or
                 |          I2NSF Client                     | App Gateway
                 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

and prefer:

                 +-------------------------------------------+
                 |IPsec Management/Orchestration Application | Client or
                 |          I2NSF Client                     | App Gateway
                 +-------------------------------------------+


Section 5.3:

 	Moreover hosts can install easily an IKE implementation.
 	As downside, the NSF needs more resources to hold IKEv2.
 	Moreover, the IKEv2 implementation needs to implement an
 	interface so that the I2NSF Agent can interact with them.

Change to:

 	Usually, hosts can easily install an IKE implementation,
 	although such an NSF needs more resources to run and it
 	would need an interface for the communication between IKEv2
 	and the I2NSF Agent.

Section 5.3.2:

 	It has also to send new

Change to:

 	It also has to send new


 	Nevertheless other more optimized options can be considered (e.g.
 	making iKEv2 configuration permanent between reboots).

Change to:

 	A more consistent and secure deployment would store the IKEv2 configuration
         on non-volatile storage, so all information is available after a crash or
 	reboot. IPsec SA information MUST NOT be stored on non-volatile storage to
 	prevent incorrect and insecure re-use of symmetric key material.

Section 7.1:

 	Besides, fresh SAD entries will be also generated by the Security Controller

Change to:

 	Fresh SAD entries will also be generated by the Security Controller

What are "IaaS services" ?

section 9.2:

 	SHOULD NEVER stores the keys

change to:

 	SHOULD NEVER store the keys


 	it may access to these values.

change to:

 	it may have access to these values.


 	observe the traffic

change to:

 	observe traffic


 	In any case, some escenarios

change to:

 	Some scenarios


 	Moreover, some scenarios

change to:

 	Scenarios

Appendix:

I see a few "-&gt;" manglings.