[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