Re: [I2nsf] [IPsec] WGLC and IPR poll for draft-ietf-i2nsf-sdn-ipsec-flow-protection-04

Rafa Marin Lopez <rafa@um.es> Wed, 29 May 2019 10:55 UTC

Return-Path: <rafa@um.es>
X-Original-To: i2nsf@ietfa.amsl.com
Delivered-To: i2nsf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 5B1531200B1; Wed, 29 May 2019 03:55:13 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.2
X-Spam-Level:
X-Spam-Status: No, score=-4.2 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
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 j9_txpcBnglO; Wed, 29 May 2019 03:55:09 -0700 (PDT)
Received: from xenon44.um.es (xenon44.um.es [155.54.212.171]) by ietfa.amsl.com (Postfix) with ESMTP id 1A8B9120092; Wed, 29 May 2019 03:55:09 -0700 (PDT)
Received: from localhost (localhost [127.0.0.1]) by xenon44.um.es (Postfix) with ESMTP id DC5B320354; Wed, 29 May 2019 12:55:06 +0200 (CEST)
X-Virus-Scanned: by antispam in UMU at xenon44.um.es
Received: from xenon44.um.es ([127.0.0.1]) by localhost (xenon44.um.es [127.0.0.1]) (amavisd-new, port 10024) with LMTP id MxJ72Fx42mi2; Wed, 29 May 2019 12:55:06 +0200 (CEST)
Received: from [155.54.14.162] (unknown [155.54.14.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: rafa@um.es) by xenon44.um.es (Postfix) with ESMTPSA id 9C268202E0; Wed, 29 May 2019 12:55:03 +0200 (CEST)
Content-Type: text/plain; charset="utf-8"
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\))
From: Rafa Marin Lopez <rafa@um.es>
In-Reply-To: <alpine.LRH.2.21.1905241401320.3939@bofh.nohats.ca>
Date: Wed, 29 May 2019 12:55:02 +0200
Cc: Rafa Marin Lopez <rafa@um.es>, Fernando Pereñíguez García <fernando.pereniguez@cud.upct.es>, "i2nsf@ietf.org" <i2nsf@ietf.org>, Gabriel Lopez <gabilm@um.es>, "ipsec@ietf.org WG" <ipsec@ietf.org>, Linda Dunbar <linda.dunbar@huawei.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <718671CF-46BF-427A-A008-A9F8EB3631D0@um.es>
References: <4A95BA014132FF49AE685FAB4B9F17F66B3869DE@sjceml521-mbs.china.huawei.com> <DBBD75C3-9FB3-473F-A627-062DB3F5C32D@um.es> <alpine.LRH.2.21.1904210811200.1903@bofh.nohats.ca> <ED73306E-F807-42A4-B063-D45E133B8419@um.es> <alpine.LRH.2.21.1905241401320.3939@bofh.nohats.ca>
To: Paul Wouters <paul@nohats.ca>
X-Mailer: Apple Mail (2.3445.104.8)
Archived-At: <https://mailarchive.ietf.org/arch/msg/i2nsf/MhdRKXdWBz7AbE3aH-u5fUP_t5w>
Subject: Re: [I2nsf] [IPsec] WGLC and IPR poll for draft-ietf-i2nsf-sdn-ipsec-flow-protection-04
X-BeenThere: i2nsf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "*I2NSF: Interface to Network Security Functions mailing list*" <i2nsf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/i2nsf>, <mailto:i2nsf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/i2nsf/>
List-Post: <mailto:i2nsf@ietf.org>
List-Help: <mailto:i2nsf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/i2nsf>, <mailto:i2nsf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 29 May 2019 10:55:14 -0000

Hi Paul:

Please see some additional comments inline:

snip
> 
>>      It can also instruct the affected NSF to send IKEv2 INITIAL_CONTACT.
>> 
>>      I think this is something the IKEv2 implementation would determine on its own.
>>      I would remove the sentence or change it to say the node can determine it
>>      needs to send INITIAL_CONTACT. Also, this removes the need for the SC to
>>      have to relay this option to the IKEv2 NSF.
>> [Authors] We have observed some implementations (e.g. libreswan) has this variable initial-contact as well in the ipsec.conf. That is why we decide to
>> keep it. Is it not useful then?
> 
> We added it because sending or not sending it might change te behaviour
> of the other endpoint. libreswan as implementation ignores the payload
> completely, because it has all the known information/state needed.
> Although based on the above new finding of multiple IPsec SA's, this
> might change.
> 
> I guess my point was more to the fact that instructing the NSF to send
> an INITIAL_CONTACT seems to mixup instructions from the SC. If the SC
> says "bring tunnel X to GW Y up", and then later says "bring tunnel Z
> up to GW Y and send INITIAL_CONTACT", it is implicitly sending a message
> to the NSF to bring tunnel X to gateway Y down. Now the NSF and SC might
> be out of sync if the SC wasn't expecting this result.

[Authors] Then it is safer to remove this possibility. Done.

> 
>>        In IKE-less case, if the Security Controller detects that a NSF has
>>        lost the IPsec SAs (e.g. it reboots) it will delete the old IPsec SAs
>>        of the non-failed nodes established with the failed node (step 1).
>>        This prevents the non-failed nodes from leaking plaintext.
>> 
>>      Wouldn't doing that before installing a new IPsec SA not actually _cause_
>>      leaking plaintext? Unless the non-failed nodes, upon receiving the delete
>>      would put in an SPD policy that would catch packets and trigger an acquire.
>> [Authors] This is an interesting question. We must state that, for security reasons, any NSF should have a default DISCARD policy. A NSF restarting should
>> not allow any traffic unless SC says so. In other words, Since the NSF needs information coming from the SC, if that information is not in place yet the
>> safest option is DISCARD action.
> 
> Sure. I think adding some text that says a connection in the "configured
> but not up" state is expceted to drop or hold onto the packets, that
> would make it clear.
[Authors] We understand. In the IKE-less case we do not have this kind of "configured but not up” state. We assume that once the SC configures the NSF with the IPsec SA, the NSF will apply the configuration right way without waiting anything else.

In IKE case it is possible to define this with the type-autostartup (on-demand) by adding your text to the description. However, I was thinking that, actually, this is a general security policy that should be applied in both iKE-case and IKE-less and, even, when the NSF starts in the network and there is no contact with the NSF.

Perhaps the best place to mention this is in the Security Considerations section. Then, we can add in the model the text you mention. We could include this paragraph in Security Considerations (the general part not specific to any case):

“For security reasons, a NSF MUST NOT allow any traffic unless the Security Controller mandates so. In other words, since the NSF needs IPsec information coming from the SC, if that information is not in place yet the safest option is DISCARD (drop) packets."



> 
>>      +--rw security-protocol?          ic:ipsec-protocol
>> 
>>      What does this stand for? There is no "IPsec protocol" in the sense that we do not have
>>      an IANA protocol entry for "IPsec" (only for ESP)
>> [Authors] I understand. The intention was to say ipsec related protocol. Should we use something like ic:ipsec-related-protocol? or  ic:sec-protocol?
> 
> or maybe ipse-protocol-parameters ?

[Authors] Ok.

> 
>>        NSFs MUST NOT allow the reading of these values once they have been
>>        applied by the Security Controller (i.e. write only operations).
>> 
>>      These are not really "applied by the Security Controller". Perhaps you
>>      meant "obtained" instead of “applied"
>> [Authors] Since we were referring to IKE-less case the Security Controlles sends these keys to the NSF. In that case, the Security Controller applies the
>> keys but it MUST NOT BE obtained (read) by anyone (including the SC).
> 
> Perhaps use:
> 
> 	NSF's receiving private key material from the SC, MUST NOT allow the reading …..

[Authors] Ok
> 
>> [Authors] We are writing version -05 of these changes will be applied.
>> 
>>         typedef ipsec-upper-layer-proto {
>>                             type enumeration {
>>                                     enum TCP { description "TCP traffic"; }
>>                                     enum UDP { description "UDP traffic"; }
>>                                     enum SCTP { description "SCTP traffic";}
>>                                     enum DCCP { description "DCCP traffic";}
>>                                     enum ICMP { description "ICMP traffic";}
>>                                     enum IPv6-ICMP { description "IPv6-ICMP traffic";}
>>                                     enum GRE {description "GRE traffic";}
>>                             }
>>                             description "Next layer proto on top of IP";
>>                     }
>> 
>>      I don't understand ipsec-upper-layer-proto? If this is encapsulation protocol, then
>>      there is only TCP and UDP. If this is the protocol of the encrypted data, then there
>>      are a lot more protocols. The document also nowhere explains the term "upper layer protocol"
>>      I think you mean what I would call the "inner protocol" so that it is every number from the
>>      IANA protocol registry.
>> [Authors] Basically RFC 4301 mentions next layer protocols and we had that in previous versions but it was suggested that was not a good name. So we used
>> this one. We can use inner protocol. Thus it is not encapsulation but the protocols in the next header after IP header.
> 
> Ok, so then I think inner protocol would be more clear.

[Authors] Ok.
> 
>> "Next Layer Protocol: Obtained from the IPv4 "Protocol" or the
>>         IPv6 "Next Header" fields.”
>> So it seems  that every number from the IANA protocol registry should be there. So here we have again the same problem as crypto algorithms. However, we
>> can do as RFC 8519 : 
> 
> This one did not get answered. Do you plan to link to the protocol
> registry at IANA ?

[Authors] Our approach so far will be similar to the one we found in RFC 8519. There they use a type uint8 and give a description and preference. More specifically:

leaf protocol {
      type uint8;
      description
        "Internet Protocol number.  Refers to the protocol of the
         payload.  In IPv6, this field is known as 'next-header',
         and if extension headers are present, the protocol is
         present in the 'upper-layer' header.";
      reference
        "
RFC 791
: Internet Protocol
         
RFC 8200
: Internet Protocol, Version 6 (IPv6) Specification.";
    }

Therefore, we would like to add something a uint8 and referring to IANA registry. Is this approach acceptable for you?

We will take also the same approach for crypto-algs so far in order to see if this is valid for yang-doctors' review


> 
>>                     typedef pfs-group {
>>                             type enumeration {
>>                                     enum NONE {description "NONE";}
>>                                     enum 768-bit-MODP {description "768-bit MODP Group";}
>>                                     enum 1024-bit-MODP {description "1024-bit MODP Group";}
>>                                     enum 1536-bit-MODP {description "1536-bit MODP Group";}
>>                                     enum 2048-bit-MODP {description "2048-bit MODP Group";}
>>                                     enum 3072-bit-MODP {description "3072-bit MODP Group";}
>>                                     enum 4096-bit-MODP {description "4096-bit MODP Group";}
>>                                     enum 6144-bit-MODP {description "6144-bit MODP Group";}
>>                                     enum 8192-bit-MODP {description "8192-bit MODP Group";}
>>                             }
>>                             description "PFS group for IPsec rekey";
>>                     }
>> 
>>      This is the only entry still enumerating (partially!) an IANA registry.
>> [Authors] Right. But I am not sure if there is a final conclusion about this dilemma. Personally, I still prefer to have a uint32 and refer Group
>> Description (Value 4) in 
>> https://www.iana.org/assignments/ipsec-registry/ipsec-registry.xhtml
> 
> That's an old IKEv1 registry, you would want to link it to:
> 
> https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xhtml#ikev2-parameters-8
> 
> And it would be a uint16, not a uint32.

[Authors] Great. Here again, it is worth noting that we will specify the uint16 and link it to the information you provide to that IANA registry.

> 
>>      Use "Configuration of IPsec Security Association (SA). Section 4.4.2.1 in RFC 4301"
>>      To ensure it is clear this is not about an IKE SA. Same on the next line for
>>      "for a particular SA"
>> 
>>        leaf seq-number-overflow-flag { type boolean; description "The flag indicating whether overflow of the sequence number counter should
>>      prevent transmission of additional packets on the SA, or whether rollover is permitted."; }
>> 
>>      What is the source of this? I thought sequence numbers were never allowed to overflow?
>> [Authors] According to RFC 4301 - 4.4.2.1.  Data Items in the SAD:
>> 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 audit log entry for this event SHOULD
>>       include the SPI value, current date/time, Local Address, Remote
>>       Address, and the selectors from the relevant SAD entry.
> 
> 
> Hmm, I have never heard of this so I should look into it, but I guess it
> means you should have the option for it, but please default it to not
> allow rollover, and it would need to forbid rollover for AEAD’s.

[Authors] We have included this in the description:

"The flag indicating whether overflow of the sequence number counter should prevent
transmission of additional packets on the IPsec SA (FALSE), or whether rollover is permitted (TRUE). If Authenticated Encryption with Associated Data (AEAD) is used this flag MUST BE false.”;

> 
> Tero, do you know anything more about this feature? I don't think we
> could ever negotiate it via IKE anyway?
> 
>>        leaf anti-replay-window { type uint16 { range "0 | 32..1024"; } description "Anti replay window size"; }
>> 
>>      Why cap at 1024? That could be too low for 100gbps connections.
>> [Authors] The truth is that 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.
>> So we can replace this with uint64. On the other hand, we have observed with uint16 should be enough in real cases. We can remove the restriction (1024) .
>> what do you think?
> 
> Yes, I think the uint16 is more than enough, as long as you don't mention any cap like 1024.

[Authors] OK
> 
>> [Authors] Right.
>> 
>>        leaf replay {type uint32; default 0; description "packets detected out of the replay window and dropped because they are replay packets";}
>> 
>>      That should be: "inside the replay window”
> 
>> [Authors] Good catch. Changed.
> 
> Actually, now I am not sure anymore :)
> 
> Outside the replay window, if the number is too low, it should be
> dropped. If it is too high, I think once decrypted successfully, the
> window is updated. If the packet is within the window, we keep a list
> of received / not-received sequence numbers, and drop duplicates ?
> 
> So I think I misled you and you were correct with the original text.

[Authors] No problem. We can leave it.
> 
>> [Authors] If integrity node does not appear in the configuration sent by the controller, the integrity interpretation is none. We can comment this. 
> 
>> Our comment is if the encryption container is sent to the NSF with an AEAD algorithm the container integrity is not sent , not required since AEAD
>> algorithm provides integrity (and encryption)
> 
> Just to clarify, some (bad) implementations handle "no integrity
> transforms" differently from "the integrity transform NONE" (value 0)
> 
> I know older libreswan versions did it wrong. However, I am not sure if
> this needs to be exposed here. I think it is fine to that the model
> states to not send any integrity transform, and that the implementation
> properly handles it when it receives an ENCR transforms AES_GCM with
> an INTEG transform NONE.

[Authors] Correct. We think this can be handled by the implementation as well.

> 
>> [Authors] combined-enc-intr has been removed.
> 
> Great!
> 
>>      I see IPcomp is not used anywhere. I agree with that but I'm not sure if there is WG conensus for that.
>> [Authors] We did not include IPcomp from the beginning and until now, there has not been any comment in this regard.
> 
> Okay. I guess the model can always be extended in the future if needed.

Indeed.

Thank you very much again!.
> 
> Thanks!
> 
> Paul
> 
> _______________________________________________
> IPsec mailing list
> IPsec@ietf.org
> https://www.ietf.org/mailman/listinfo/ipsec