| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-10-05 | |||
| 17:56:29 | dansmith | no | |
| 17:56:40 | artom | Though currently only Neutron does, IIRC | |
| 17:56:47 | dansmith | it doesn't need to know that neutron only sends network events, but... that's kinda the point | |
| 17:56:49 | artom | And it's obvious that neutron does from the ebent name | |
| 17:57:03 | sean-k-mooney | artom: cinder and cyborg also call it | |
| 17:57:07 | dansmith | it's waiting for a thing, doesn't really need to know that neutron sent it, it just only really works if that's the case :) | |
| 17:57:27 | sean-k-mooney | artom: cinder for swap volume i think and cybrog for the arq binding completion | |
| 17:57:44 | artom | sean-k-mooney, I thought cinder swap volume was another "proper" API? | |
| 17:57:49 | artom | volume-update... | |
| 17:58:01 | sean-k-mooney | perhaps but there are cinder volume event | |
| 17:58:19 | sean-k-mooney | dansmith: so ya neutron is using cinder client https://github.com/openstack/neutron/blob/master/neutron/notifiers/nova.py#L84-L91 | |
| 17:58:26 | sean-k-mooney | *novaclient | |
| 17:58:27 | artom | Yeah https://docs.openstack.org/api-ref/compute/?expanded=update-a-volume-attachment-detail#update-a-volume-attachment | |
| 17:59:25 | sean-k-mooney | artom: it uses for volume extetnions not swap sorry | |
| 17:59:42 | artom | Anyways, we're off point here | |
| 18:00:02 | sean-k-mooney | artom: this is the list of events currently https://github.com/openstack/nova/blob/50fdbc752a9ca9c31488140ef2997ed59d861a41/nova/objects/external_event.py#L18-L36 | |
| 18:00:11 | dansmith | currently neutron is using a project token to send these, | |
| 18:00:30 | artom | "power-update" is weird | |
| 18:00:35 | artom | When/how does that happen? | |
| 18:01:02 | dansmith | is that because that's the token and client that neutron uses to interact with nova or other things in a sensible way, or just because it has no knowledge that a system scoped token would be right? | |
| 18:01:07 | dansmith | artom: ironic, IIRC | |
| 18:01:17 | dansmith | artom: it's not just neutron :) | |
| 18:01:59 | lbragstad | yeah - i think it's using the nova service user, which has the admin role | |
| 18:02:15 | lbragstad | and we haven't really gone through that migration yet, from a policy perspective | |
| 18:03:47 | dansmith | lbragstad: meaning it's just naive and not like that user/client is project-scoped for other stuff? | |
| 18:05:06 | dansmith | even still, if neutron is sending an event for an instance or group of ports or something, having it be project-scoped in that call to the event thing also helps to prevent any missteps in sending events to other instances | |
| 18:05:23 | dansmith | like if there was some port change thing going on, during ownership transfer, or anything else | |
| 18:05:43 | dansmith | this goes back to my "the interface is specific to project-scoped resources, so it feels like it too should be project-scoped" | |
| 18:07:24 | lbragstad | iiuc - it's using the nova service user, which just happens to have the admin role (probably for legacy reasons) and that's why it works | |
| 18:07:52 | lbragstad | so - because the nova user has the 'admin' role on the service project, it can update external server events for instance 'foo' in project bar... | |
| 18:07:53 | dansmith | yeah, well the default policy makes the api admin-only anyway | |
| 18:08:57 | sean-k-mooney | yes so its relyin gon haveing a user with admin rights | |
| 18:09:13 | sean-k-mooney | that can be the nova user or a common service user | |
| 18:09:14 | lbragstad | correct - but it's not enforcing tenancy in any way - from what i can tell | |
| 18:09:33 | sean-k-mooney | i dont think it is either today | |
| 18:09:59 | sean-k-mooney | since this is mainly a service to service api | |
| 18:10:04 | dansmith | the api? | |
| 18:11:44 | dansmith | it uses the context to look up the instances it creates the events for, so it should be scoped to the project of the token | |
| 18:11:48 | sean-k-mooney | the external events api does not assert that the user that created teh event is a has any relationship with the project reosuce that is being modified | |
| 18:12:00 | dansmith | or I guess if you're admin then it lets you see them all, but it should still be tight if you're not admin | |
| 18:12:32 | dansmith | you won't be able to look up instances you don't have access to, and you'll fail with a 404 I think | |
| 18:12:58 | sean-k-mooney | well this has been admin only for as long as i can rememebr it exsiting | |
| 18:13:02 | dansmith | or just not send those events, I guess, I'm not sure, but.. | |
| 18:13:22 | dansmith | sean-k-mooney: by default you mean :) | |
| 18:13:31 | sean-k-mooney | yes by default | |
| 18:13:36 | dansmith | I'm just saying I don't think that the api is not tenant-safe | |
| 18:14:08 | sean-k-mooney | well while you can get a responce form tha tapi when you do a post we dont supprot get request on it correct | |
| 18:14:21 | dansmith | there's nothing to get | |
| 18:14:24 | sean-k-mooney | and the info you get back is pretty limited in general | |
| 18:14:24 | dansmith | there's no persistence | |
| 18:14:40 | dansmith | I'm saying you can't POST events for instances you don't own | |
| 18:14:41 | sean-k-mooney | right so since it most a write only api | |
| 18:14:50 | dansmith | it is entirely write-only | |
| 18:14:50 | sean-k-mooney | sure you can | |
| 18:15:10 | gmann | dansmith: sean-k-mooney I remember the discussion of need of service specific role for such cross service API. system scope in this API was not concluded/discussed permission for this at the time of moving to new defaults. | |
| 18:15:12 | sean-k-mooney | well ok you saying if you change the policy to allow anyone call it | |
| 18:15:15 | sean-k-mooney | then it wont work | |
| 18:15:16 | dansmith | objects.InstanceList.get_by_filters( | |
| 18:15:16 | dansmith | cctxt, {'uuid': instance_uuids_by_cell[cell_uuid]}, | |
| 18:15:32 | dansmith | sean-k-mooney: this ^ will not work for instances you don't own if you are not admin | |
| 18:15:42 | gmann | either project scope or both or need service user specific role for such operation | |
| 18:15:52 | sean-k-mooney | do we have a db level check to enforce that | |
| 18:16:22 | dansmith | sean-k-mooney: that's how that interface works right? | |
| 18:16:35 | dansmith | I mean, it certainly was when the code was written :) | |
| 18:17:25 | sean-k-mooney | in the port bindign case the event are all submited as tyepically the nova user regradesss of who owns the port | |
| 18:17:33 | sean-k-mooney | dansmith: the user token is never used | |
| 18:17:57 | dansmith | ...right, as admin yes? | |
| 18:18:03 | sean-k-mooney | also even if it was the even it decupled form a user action | |
| 18:18:07 | sean-k-mooney | yes as admin | |
| 18:18:17 | dansmith | what are we arguing about again? | |
| 18:19:04 | sean-k-mooney | wether the resouce that is being evented on is own by the user/project assocated with teh keyton token used to send the event to nova i think | |
| 18:19:43 | sean-k-mooney | i kind of got confused by what you were saying would and would not work and im not sure you were following me either | |
| 18:20:18 | dansmith | https://github.com/openstack/nova/blob/7967ad78649a1f8d7ffc34ae28274ee89b0011cf/nova/db/main/api.py#L1413-L1416 | |
| 18:20:32 | dansmith | this ^ is where we enforce you can only see your own instances in the DB layer, if not admin | |
| 18:20:55 | dansmith | which makes that API not allow non-admins (if so granted) able to send events to instances they do not own | |
| 18:21:14 | sean-k-mooney | i see | |
| 18:21:19 | dansmith | if the nova user neutron uses has admin, that's why it can send events to any instance, as expected | |
| 18:21:43 | dansmith | I'm talking about if someone were to drop the admin requirement from the policy, or if we later add a human-initiated event, the API is not insecure | |
| 18:22:09 | sean-k-mooney | right i tought we were trying to remove those low level db enfrocements form the code | |
| 18:22:14 | dansmith | in response to this: | |
| 18:22:14 | dansmith | [11:09:14] <lbragstad> correct - but it's not enforcing tenancy in any way - from what i can tell | |
| 18:22:14 | dansmith | [11:09:33] <sean-k-mooney> i dont think it is either today | |
| 18:22:14 | gmann | dansmith: so your proposal is to allow only project scoped (owner or admin) not system right? | |
| 18:22:47 | dansmith | sean-k-mooney: not those, AFAIK.. tons of stuff would need to change at the top, and always provide a project-level filter down or we'd be querying tons of information all the time that we need to filter in python | |
| 18:22:48 | sean-k-mooney | dansmith: right so its the db laywer not the api that enforcing that | |
| 18:23:16 | gmann | sean-k-mooney: we have that as TODO to move admin-checks from DB to API layer side | |
| 18:23:16 | dansmith | gmann: no I'm saying we allow either and enforce scope type based on the event_type in the event, which are probably all system right now (but need to look) | |
| 18:24:03 | dansmith | gmann: you're not talking about removing the project filtering from the DB queries entirely right? | |
| 18:24:10 | gmann | yeah it is all system-admin for now | |
| 18:24:59 | dansmith | (not that it matters for this conversation, because this API behaves the same as all our other ones at the moment, which rely on db filtering) | |
| 18:25:16 | gmann | dansmith: not filtering but like if admin then we check admin policy and call separate DB interface itself at API layer and do not check is_admin in DB | |
| 18:25:27 | dansmith | gmann: right ++ | |
| 18:27:37 | gmann | on event type: keeping system scope is for human-initiated event ? | |
| 18:28:15 | dansmith | no | |
| 18:28:24 | gmann | making them system-only was not right thing at initial implementation | |
| 18:28:48 | dansmith | honestly, I don't even know what to say anymore: I think neutron declaring a project id when making the call is not bad, | |
| 18:28:50 | sean-k-mooney | i think we just converted admin_api to system_admin_api as an itital step | |
| 18:28:53 | dansmith | which makes it a project-scoped interface It hink | |
| 18:29:11 | gmann | yeah | |
| 18:29:12 | dansmith | I'm just saying I don't want this interface to just be forever tagged as system-only | |