[ovs-dev] [PATCH 2/2] debian: Add support for GRE-over-IPsec

[Ben Pfaff]

On Tue, Sep 21, 2010 at 06:25:25PM -0700, Justin Pettit wrote:
> The ovs-monitor-ipsec daemon monitors the Interface table for GRE
> entries.  If an entry specifies other-config parameters "ipsec_local_ip"
> and ("ipsec_psk" or "ipsec_cert"), it will create the appropriate

The commit message uses underscores ("ipsec_psk"), the code uses
underscores, but vswitch.xml uses hyphens ("ipsec-psk").  Not good.

> security associations so that all GRE traffic to the remote host will be
> encrypted.  In order for the two GRE tunnels to communicate, both sides
> need to be configured for IPsec with appropriate authentication.
> 
> Currently, ovs-monitor-ipsec does not support certificate authentication
> or ensure that an interface is actually attached to a bridge.  Both of
> these issues will be addressed in a forthcoming patch.
> 
> NB: While GRE-over-IPsec should work on any system with a relatively
> recent racoon and setkey, it has only be tested on Debian.  As such,
> only Debian packaging has been provided.

"only be" => "only been"

Is there a reason to give this a separate init script?  Does it make
sense to start or stop this independent of the rest of the OVS stuff?
If not, then we could probably integrate it into the main OVS init
script: the script would just check whether the ipsec package was
installed and if so then start/stop it at the same time.

Simon recently pointed out that Debhelper's dh_installinit adds the
proper magic to the postinst, so that there was no need for us to do so
too.  Can you check whether this is true for this package too?  If we
can just omit the postinst, that's great.

The 2009 copyright year in debian/ovs-monitor-ispec is odd.  Is that
script derived from something not written recently?

ovs-monitor-ipsec imports signal and time but doesn't use them.

All of the writes to s_log include "ovs-monitor-ipsec" literally in the
output.  Isn't there some way to make that part of the logging template
so it doesn't have to be repeated each time?

There is a race in commit_psk between writing the file and restricting
its permissions.  I would move the chmod call just after the open.

IPsec.call_setkey() seems ripe for deadlock, if setkey blocks waiting
for its output to stdout to finish, while ovs-monitor-ipsec blocks
waiting to feed input to its stdin.  I assume that's why communicate()
takes an input string as argument: so that it can interleave reads and
writes, resolving deadlock.  So I would change
        p.stdin.write(cmds)
        out = p.communicate()[0]
to
        out = p.communicate(cmds)[0]
to avoid the problem.

I would have expected call_setkey() to do something to make sure that
the child dies.  (Will you get zombies otherwise?)

I personally find the "..." % (args) easier to read than " ".join(...),
but it's not a big deal either way.

> +      <column name="other_config">
> +        Key-value pairs for configuring IPsec-over-GRE.  The currently
> +        defined key-value pairs are:
> +        <dl>
> +          <dt><code>ipsec-local-ip</code></dt>
> +          <dd>Required key for GRE-over-IPsec interfaces.  Additionally,
> +            the <code>type</code> field of the <code>option</code>
> +            column must be <code>GRE</code> and the <code>ipsec-psk</code>

Don't you mean that the type must be gre?  No one has defined a type key
for the options column as far as I know.

The type is case-sensitive so it should be gre not GRE.

A reference to a column name should be written as e.g. <ref
column="options"/>, not using <code>.

How do we describe, for users, the difference between options and
other_config?  I cannot see a clear difference.  Is there any reason
these shouldn't be part of the options?  ovs-vswitchd might complain
about them, but we can fix that.

> +            key must be set.  The <code>key</code> options for GRE must
> +            not be set.</dd>

By that you mean that in_key, out_key, and key must all be unset?

> +          <dt><code>ipsec-psk</code></dt>
> +          <dd>Required key for GRE-over-IPsec interfaces.  Specifies a
> +            pre-shared key for authentication that must be identical on 
> +            both sides of the tunnel.  Additionally, the
> +            <code>ipsec-local-ip</code> key must also be set.</dd>
> +        </dl>
> +      </column>