Only admin should update device_id

Bug #1031473 reported by Nachi Ueno on 2012-07-31
This bug affects 1 person
Affects Status Importance Assigned to Milestone

Bug Description

For current implementation, device_id can be updated by non-admin user.
Policy check for device_id is also needed.

Tags: api Edit Tag help
Nachi Ueno (nati-ueno) on 2012-08-06
summary: - device_id should not be updated twice
+ Only admin should update device_id
description: updated
Jiajun Liu (ljjjustin) on 2012-08-27
Changed in quantum:
assignee: nobody → ljjjustin (ljjjustin)
dan wendlandt (danwent) wrote :

what is meant by 'policy change for device_id is also needed'? You mean to enforce some format (e.g., that its a UUID)? I think DHCP may violate that right now.

Changed in quantum:
status: New → Confirmed
importance: Undecided → Low

Please note that device_id is set by the nova integration API, in, method allocate_for_instane.

The corresponding port create operation is then executed with the same context of the nova request. Is that context guaranteed to be always an admin context?

dan wendlandt (danwent) wrote :

salvatore, that's a very good point. It makes me wonder if it is correct for nova to always be using the user context here, or if it should be an admin user acting on behalf of the tenant. Would be good to check with Yong around his original reasoning of using the existing tenant credentials.

dan wendlandt (danwent) wrote :

yong, can you comment here? thanks.

dan wendlandt (danwent) on 2012-09-09
Changed in quantum:
assignee: ljjjustin (ljjjustin) → nobody
importance: Low → Critical
milestone: none → folsom-rc1
dan wendlandt (danwent) wrote :

we're trying to sort out whether this is a blocker or not. Working through some use cases. Will update soon.

Changed in quantum:
importance: Critical → High
yong sheng gong (gongysh) wrote :

First, nova quantum API does it (attach instance) in two ways:
1. list nets, subnets, create new port with device_id. This is in the same tenant context as nova
2.attach instance to port. In this case, we also need to make sure the port belongs to the same nova tenant

our current behaviour is: tenant can only operate his own net and port.
If this is not enough for port, we can make sure port's device_id cannot be changed after first being assigned a value.

if we need admin context to do port's device id assignment, nova quantum api has the quantum user which must be a admin account, since it will list the network information for all instances hosted by that host, so we can use this account.

all in all, we can limit the only once assignment of device_id for tenant user, but allow admin user for change it without limit. This way, we don't need to modify nova code, at the same time, keep the flexibility.

dan wendlandt (danwent) wrote :

I believe the is a case in the existing nova code were we update the device_id on a port using a non-admin context: . This is the case where the tenant passing in an existing port-id to Nova, rather than a network-id.

dan wendlandt (danwent) wrote :

I believe that a user that changed with device_ids on its own ports could cause Nova to be confused about what VIFs are on a VM, reporting incorrect IPs, and potentially incorrectly deallocating ports when a VM is torn down. As long as the tenant_id of a port always matches the owner of a Nova instance though, I think this behavior is limited to a tenant confusing itself, but never being able to mess with another tenant.

yong sheng gong (gongysh) wrote :

So do we need to fix it?
if we need to fix it, how about my advice at floor #6?

dan wendlandt (danwent) on 2012-09-11
Changed in quantum:
milestone: folsom-rc1 → none
importance: High → Medium
tags: added: api

We need to look back at nova and see now if we can do this operation with an admin context without adding extra chattiness on the nova-quantum interface.

This is not enough in terms of importance however IMHO to target this bug to grizzly.

Fix proposed to branch: master

Changed in quantum:
assignee: nobody → Anton Frolov (anton0)
status: Confirmed → In Progress
Anton Frolov (anton0) on 2013-06-11
Changed in quantum:
assignee: Anton Frolov (anton0) → nobody
status: In Progress → Confirmed

Re-triaging to verify whether this can be safely and simply fixed for Havana.

Changed in neutron:
assignee: nobody → Salvatore Orlando (salvatore-orlando)

re-triage analysis.

This could be sorted out in two ways.
1) Make the fields admin_only and fix in order to perform the port_update operation needed when a neutron port is passed to create server with admin context.

This is simple, but will look weird and violate the principle of performing port create operations within tenant context

2) Enforce a policy where device_id and device_owner can be updated only if they're empty. Once set, they can be updated only with admin context. This is easy to enforce by hardcoding in db_plugin, but harder to perform in an orthogonal way using the policy engine. I have tried using the field check to authorize only when tenant_id is the owner of the port and device_id is empty, but the policy is enforced on the resource with the updated attributes. the solution would involve the policy engine retrieving a context and performing a query, which sounds expensive.

If we feel we need to fix this, I can probably extend the policy engine to pass both the original and the modified target.
Otherwise let's leave this bug untargeted.

shihanzhang (shihanzhang) wrote :

I think the 'device_owner' also shouldn't be updated!

Eugene Nikanorov (enikanorov) wrote :

Lowering as this has not gotten enough attention during last 2 years.

Changed in neutron:
importance: Medium → Low
goocher (farmerworking) on 2015-03-26
Changed in neutron:
status: Confirmed → In Progress
goocher (farmerworking) on 2015-03-26
Changed in neutron:
status: In Progress → Confirmed
Changed in neutron:
assignee: Salvatore Orlando (salvatore-orlando) → nobody

This actually recently came up again:

Not really about device_id, more about device_owner. But the problem is the same.
These two attributes should really become read only, but as they're widely used any change in the logic here requires more thought.

Changed in neutron:
status: Confirmed → Incomplete
Launchpad Janitor (janitor) wrote :

[Expired for neutron because there has been no activity for 60 days.]

Changed in neutron:
status: Incomplete → Expired
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers