[ovs-dev] [PATCH 6/6] vswitch: Add support for IPsec certificate authentication.

Reid Price reid at nicira.com
Thu Dec 23 01:03:51 UTC 2010


Hi there, a few notes below.  It seems pretty reasonable overall.  Please
forgive any general ignorance of racoon conf files or specific ignorance of
where this is used =)

+        # Rewrite the Racoon configuration file
+        conf_file = open(Racoon.conf_file, 'w')
+        conf_file.write(Racoon.conf_header % (Racoon.psk_file,
Racoon.cert_dir))
You can use 'self' instead of 'Racoon' here (and elsewhere).  Ususally you
don't have to use the class name unless you are referencing it externally,
are planning to inherit this class and perform trickery with overloaded
functions/variables, or you have different values for the instance than the
class.

+        if "certificate" not in vals:
Is there a reason that certificate missing is a specific error.Error, while
peer_cert/peer_cert_file would just trigger normal KeyError exceptions if
missing?  I don't think you should have to check every key before you use it
in general, but it seems that the update_ipsec function would catch the
exception for the cert, but not for those keys missing.
+        except error.Error, msg:
+            s_log.warning("skipping ipsec config for %s: %s" % (name, msg))
Update:  Ah nevermind this, I see that this is never invoked except through
add_entry, which checks for the key before using.  Perhaps prefix add_cert
with an underscore to note that it is not used externally?

+        elif "private_key" not in vals:
+            # Assume the private key is stored in the same PEM file as
+            # the certificate
+            vals["private_key"] = vals["certificate"]
This will change the object you pass in, which doesn't seem to be a problem
in this case.  If you had invoked this with something that looked like:

cert_template = {'peer_cert':  'pcert 1'}
for cert in ['cert 1', 'cert 2]:
    cert_template['certificate'] = cert
    self.add_cert('host 1', cert_template)

You would end up adding the first with a cert/private key of ''cert1'/'cert
1' and the second with 'cert 2'/'cert 1'.

+        peer_cert_file = Racoon.cert_dir + "/ovs-" + host + ".pem"
It is slightly more "standard" to write these as:

peer_cert_file = "%s/ovs-%s.pem" % (self.cert_dir, host)

+    for name, vals in interfaces.items():
+        if name not in new_interfaces.keys():
Nit, could write this using iteritems and dropping the keys

for name, vals in interfaces.iteritems():
    if name not in new_interfaces:

+        orig_vals = interfaces.get(name)
+        if orig_vals:
+            # Configuration for this host already exists.  If
+            # it has changed, this is an error.
Are interfaces allowed to have the empty string as a name?  I don't think
they are, but if so, probably want to check for None, or do 'if name in
interfaces' explicitly.
I don't know the usage cases for this script, but you might want to consider
some flag to indicate whether you have any of these errors, in case you
decide to add a returncode to the main function.  Otherwise you would have
to parse the log messages.

  -Reid


On Wed, Dec 22, 2010 at 1:20 PM, Ben Pfaff <blp at nicira.com> wrote:

> [Reid, I am a blind man wandering in this Pythonic wilderness, so your
> feedback on the original patch would be nice too.]
>
> On Wed, Dec 22, 2010 at 12:04:34AM -0800, Justin Pettit wrote:
> > Previously, it was possible to fake configuring the use of certificate
> > authentication for IPsec, but it really just used a static pre-shared key
> > behind the scenes.  This commit publicly mentions certificate
> > authentication and finally does the real work behind the scenes.
>
> I noticed that there was some code in here that creates a file and
> chmods it afterward to make it readable only by root.  That is insecure,
> because it leaves a race window in which an unprivileged process could
> open the file (Unix checks permissions only at open time).
>
> It seems to me undesirable that this daemon prevents modifying a host
> in-place ("configuration changed for %s, need to delete interface
> first").  In contrast, ovs-vswitchd is designed so that it can it can
> transition from any current state to any new state.  Otherwise, it makes
> life difficult for the database client: it means that, to change
> something, the client must commit a deletion, then commit an insertion,
> in two separate commits.  And if ovs-monitor-ipsec's connection to the
> database goes down in the middle, so that it only sees the initial and
> final states without observing the deletion itself, then it will never
> update the interface at all, at least not until the next time it is
> killed and restarted.  In short, this doesn't look very robust.
>
> In spd_add() and spd_del() (which you didn't change here) it seemed odd
> to use a completely separate statement to add a newline.
>
> I don't know Racoon, but I didn't see any other obvious problems.
>
> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20101222/12a14f6d/attachment-0003.html>


More information about the dev mailing list