[ovs-dev] [PATCH 6/6] vswitch: Add support for IPsec certificate authentication.
Justin Pettit
jpettit at nicira.com
Thu Dec 23 02:21:24 UTC 2010
On Dec 22, 2010, at 1:20 PM, Ben Pfaff wrote:
> 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).
Good catch. I fixed it by setting the umask first.
> 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.
Yes, this is a weakness. I had already checked with our controller team, and it's not a problem with how they set up ports. I have it on my to-do list to fix, so I'll try to get that cleaned up before too long.
> 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 did that because it lined up better in my eye. I really don't care, so I changed it.
> I don't know Racoon...
I'm envious.
Thanks for the review.
--Justin
More information about the dev
mailing list