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

Justin Pettit jpettit at nicira.com
Thu Dec 23 08:08:02 UTC 2010


On Dec 22, 2010, at 5:03 PM, Reid Price wrote:

> +        # 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.

Good to know.

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

Good suggestion.  I made that change to all the private methods.

> +        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'.

Good catch.  This could actually cause the daemon to do unnecessary work, since it will keep thinking that the interfaces have changed and force a reconfigure.  I went ahead and changed it to always copy the dictionary instead.

> +        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)

That is prettier.

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

Sounds good.

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

No, we shouldn't be having interfaces with empty strings.

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

The use-case is fairly narrow, so I'm going to leave it as it is now.

Thanks, as always, for helping us work on our Python skills!

--Justin






More information about the dev mailing list