Subject: [I2nsf] Some comments / questions on draft-abad-i2nsf-sdn-ipsec-flow-protection-03

From: Paul Wouters <[email-masked]>
Date: Sun, 17 Sep 2017 13:03:46 -0400 (EDT)

I had another look at the draft.

I am not in favour or against adoption of this draft.

However, I am wondering if the two use cases are not better split into
two separate documents, as the requirements, deployment and security
considerations are very different. It would also allow implementors to
more clearly indicate supporting one or both use cases by referencing
RFC numbers.

Specific comments on text follow below.

In section 6.2, I see a difference in ah-sa and esp-sa. The document
has:

+--rw ah-sa
    +--rw integrity-algorithm?
    +--rw key?
    +--rw esp-sa
    |     |  +--rw encryption
    |     |  |  +--rw encryption-algorithm?   encryption-algorithm-t
    |     |  |  +--rw key?                    string
    |     |  |  +--rw iv?                     string
    |     |  +--rw integrity
    |     |  |  +--rw integrity-algorithm?   integrity-algorithm-t
    |     |  |  +--rw key?                   string
    |     |  +--rw combined
    |     |     +--rw combined-algorithm?

This seems inconsistent between ah-sa and esp-sa. More consistent would
be:

+--rw ah-sa
    +--rw-integrity
    |  +--rw integrity-algorithm?
    |  +--rw key?
    +--rw esp-sa
    |     |  +--rw encryption
    |     |  |  +--rw encryption-algorithm?   encryption-algorithm-t
    |     |  |  +--rw key?                    string
    |     |  |  +--rw iv?                     string
    |     |  +--rw integrity
    |     |  |  +--rw integrity-algorithm?   integrity-algorithm-t
    |     |  |  +--rw key?                   string

That is two changes:

1) Give rw ah-sa a subsection rw-integrity, even if it is the only subsection.
2) Don't define rw combined on its own. It is an rw encryption-algorithm.
    (also, otherwise rw combined needs rw key and rw iv)
    (alternatively rw combined could be a bool)
    (also see below)

The rw encap entry should add an entry for rw espintcp to indicate
support for RFC 8229.

I'm not sure why the rpcs ro authentication and ro encryption entries
have "max-bits" and "min bits" entries. The idea that we would have
algorithms with variable key length was only ever valid for the
(basically abandoned) CAST and (never adopted) blowfish/twofish
algorithms. These days we have a discrete set of bits, at the moment
128/192/256 only. (libreswan IKE has removed some scar tissue based
on this, where it had concepts of min/default/max that were removed)

I'm confused by the SAD entries have sa-lifetime's of "soft" and "hard".
The counters in the SPD sure need to have these, but the counters in
the SAD are representing the actual counters themselves, so there is
no hard or soft.

The notifications could use an entry for sadb_bad-spi, as it would
be useful to somehow notify that packets with unknown SPI's are
received, which indicated one of the two endpoints rebooted but the
other end isn't aware and still sending encrypted packets that
can no longer be decrypted.

It seemds IPCOMP has been left out everywhere. I'm not sure if that
was by design or not. (I think IPCOMP should die, so I'm fine with it)

Section 6.4

the rw autostartup is a boolean, but we tend to have three states here,
alwayson, initiate-on-demand, or respond-only.

nat-traversal should probably be expanded from a boolean, due to the
ESPinUDP and ESPinTCP and ESPinTLs options. It should also have a port
value since TCP/TLS could run on different ports.

It seems for IKE, combined algorithms were not seperated from enc+integ
algorithms (see previous comment) as was done for IPsec.

If I read the syntax correctly, there seems to only be one PFS group
possible? Often IKE proposals allow more then one.

traffic selectors seem also to be limited to only single IP-to-IP without
port or protocol selectors? That seems to conflict with an earlier value
in Section 6.2 indicating support for Populate From Packet (pfp) which
would create multiple IPsec SA's limited by protocols and ports.

phase2-lifetime seems to only have a uint32 value. I don't understand
what unit this is in? And what happend to the policies for maxtime,
maxpackets and maxbytes?

Section 7.1

In use case 2 it states "In this case the execution of IKE is not necessary
in the controller". I would say that you MUST NOT run an IKE daemon in
that case because otherwise the Security Controller and the IKE daemon
are going to fight each other for the SAD/SPD states.

I'm still a little confused about what is meant by "gateway-to-gateway"
since I don't see any policies being able to be defined for "site to
site" IPsec policies. Perhaps the context of gateway here does not mean
"IPsec gateway" ? Although it then goes on to talk about VPN connections
between two branch offices. Is this document only targeting that one
host in one branch talks to another host in another branch using
host-to-host and that the two subnets are connected by gateways that
are not in scope for this document? If site-to-site is in scope, then
the entries in Section 6.2 and 6.4 need updating to allow such policies
(SPD) and states (SADs)

Section 8

I'm not sure if PF_KEYv2 should be recommended for new things. It seems
an old interface that people have moved away from.

Paul