Comment 44 for bug 1243327

Revision history for this message
Akihiro Motoki (amotoki) wrote : Re: Routers can be cross plugged by other tenants (CVE-2014-0056)

@Aaron, thanks for the patch. The patch itself works.

One question: The patch checks there is a router with specified device_id and we can set non-existing UUID.
Even though the possibility is very very low, if other tenant creates a router with that UUID, the port is connected to the router. The possibility completely depends on randomness of UUID. It is different from the original case. The original case reports in this bug is if someone can know UUID of other tenant routers, he/she can connect a port to other tenant's router. I am not sure we should cover the case I mentioned here in this bug report, but it is worth considered once.

The below is minor suggestions to improve the codes.
----
+class DeviceIDNotOwnedByTenant(Conflict):
+ message = _("The following device_id %(device_id)s is not owned by your "
+ "tenant or matches another tenants router.")

I think this exception is generic and not specific to "router".
How about passing "router" as a parameter.

  message = _("...... matches another tenants %(resource)s.")

-----
update_port()
- "new_device_owner" looks better than "current_device_owner" because a device_owner is not updated yet. IMO new and prev/old prefix looks better in update operation. "current_" is a bit ambiguous.
- changed_device_id is set to True when 'device_id' is not specified in update_port. This means if device_id is not specified in update_port changed_device_id would be set to True.

----
_enforce_device_owner_not_router_intf_or_device_id
- get_service_plugins() handles a case where core plugin supports L3. We can write it as follows:

if device_id:
    l3_plugin = (
        manager.NeturonManager.get_service_plugins().get(
            service_constants.L3_ROUTER_NAT))
    if l3_plugin:
        try:
            ctx_admin = ctx.get_admin_context()
            router = l3plugin.get_router(ctx_admin, device_id)
        except l3.RouterNotFound:
            return
    else:
        # raise as extension doesn't support L3 anyways.
        raise n_exc.DeviceIDNotOwnedByTenant(....)