| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-22 | |||
| 18:18:51 | elbragstad | if the policy is written like "(role:admin and system_scope:all) or rule:owner" and locked_by_me is True|False, is locked_by useful? | |
| 18:18:58 | mriedem | if i'm following this unlock logic, if an admin in my project locks my server, i can unlock it. but if an admin outside my project (global god like admin) locks my server, then i can only unlock it if i pass the override policy check right? | |
| 18:19:14 | elbragstad | oh - maybe i was missing ^ that bit | |
| 18:19:37 | gmann | mriedem: right | |
| 18:19:44 | mriedem | and the override policy defaults to rule:is_admin i.e. system level god admin | |
| 18:19:47 | mriedem | in legacy terms | |
| 18:20:57 | gmann | elbragstad: we compare only project_id (not user id) for deciding it is owner or not. | |
| 18:21:00 | mriedem | so with dan's proposal, on response, we checking if you pass is_expected_locked_by and if not, do you pass the override policy, exactly like the unlock code, | |
| 18:21:16 | mriedem | are we going to want to perform that policy check when listing servers with details? ^ for each server? | |
| 18:21:55 | elbragstad | mmm - so owner == anyone with a token scoped to the project that contains the instance | |
| 18:21:57 | gmann | humm, no we should not perform that check in show. | |
| 18:22:01 | mriedem | elbragstad: yes | |
| 18:22:04 | elbragstad | and admin == anyone with the `admin` role on any project | |
| 18:22:05 | gmann | yeah | |
| 18:22:26 | openstackgerrit | Merged openstack/nova-specs master: Update Network Bandwidth resource provider spec https://review.openstack.org/644810 | |
| 18:22:38 | mriedem | gmann: "no we should not perform that check in show." is what we would have to do to return an "can i unlock this server" boolean | |
| 18:22:54 | dansmith | and it's what we do when we expose (or not) fault in instance | |
| 18:23:08 | mriedem | i don't agree on that exactly being the same | |
| 18:23:16 | dansmith | is there some api guideline that says "only reveal if someone can do something when they try" ? | |
| 18:23:19 | mriedem | with fault traceback it's just a show/hide | |
| 18:23:21 | gmann | yeah, seems like. | |
| 18:23:56 | mriedem | maybe we do a policy check on every instance when listing, for faults i mean, would have to look | |
| 18:23:57 | dansmith | you guys keep saying "we shouldn't do that" but I haven't seen any *reason* :) | |
| 18:23:58 | mriedem | anywho | |
| 18:25:21 | mriedem | well i guess we don't do a context.can check when listing servers to see if we can show the fault traceback https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/views/servers.py#L550 but it's close, just based on the hard-coded is_admin flag in the context | |
| 18:25:24 | mriedem | which elbragstad wants to remove | |
| 18:25:24 | dansmith | also, locked_by_me=True|False is something we can do today, something we can do in the future if we store the locking user, and something that doesn't require a policy check on show.. why is that not okay? | |
| 18:26:22 | mriedem | dansmith: you mean locked_by_owner yeah? | |
| 18:26:29 | dansmith | sure | |
| 18:26:33 | mriedem | where the logic would just be locked_by_owner = instance.locked_by == 'owner' | |
| 18:26:39 | dansmith | or locked_by=Owner|NotOwner | |
| 18:26:45 | dansmith | right | |
| 18:26:57 | dansmith | don't say admin, just say "not you" | |
| 18:27:03 | elbragstad | ++ | |
| 18:27:21 | mriedem | i'd be more ok with doing locked_by=owner|other | |
| 18:27:42 | mriedem | i'm trying to express this... | |
| 18:28:06 | dansmith | *simpsons | |
| 18:28:27 | mriedem | but doing a policy check when building a response based on policy for something that would be checked during a subsequent unlock request, seems very weird and capability-ish to me, which we don't do elsewhere. it's true we check policy when building a response to hide things for non-admins, like system details (hosts, fault traceback, etc), but those aren't capability things | |
| 18:28:30 | gmann | I am also ok for locked_by=owner|other if we cannot expose the UUID. | |
| 18:28:44 | mriedem | gmann: we don't store the uuid anywhere for who locked the instance | |
| 18:28:45 | elbragstad | if instances stored user information instead of project ids, we wouldn't need two lock properties, would we? | |
| 18:29:01 | gmann | mriedem: yeah, i mean if we do not want to change that as it will be complex | |
| 18:29:04 | dansmith | mriedem: so the reason is we don't do exactly this elsewhere and it "feels wrong" ? | |
| 18:29:22 | mriedem | correct | |
| 18:29:32 | dansmith | I mean, I understand that things feel wrong.. I don't understand why this one does, but that's okay | |
| 18:29:34 | dansmith | alright | |
| 18:29:36 | mriedem | i could argue it'd be nice to have a "can_be_resized" flag in the server response | |
| 18:29:41 | mriedem | that doesn't mean we're going to add that | |
| 18:29:55 | dansmith | people have asked for that :) | |
| 18:29:59 | mriedem | i'm sure they have | |
| 18:30:00 | dansmith | based on all the reasons you *can't* do that | |
| 18:30:11 | mriedem | and we've talked about that capabilities API for several years | |
| 18:30:18 | dansmith | how's that coming? :) | |
| 18:30:31 | mriedem | it's mostly rotted flesh in my storage room | |
| 18:30:32 | dansmith | because to me, | |
| 18:30:34 | mriedem | don't tell the cops please | |
| 18:31:00 | dansmith | the capaibiltiies api is the same thing, but all in one place, which IMHO lends further support to it being a good idea, but just not doable until we have enough other ones for critical mass :) | |
| 18:31:20 | dansmith | s/doable/palatable/ | |
| 18:31:24 | dansmith | and that's unfortunate | |
| 18:31:52 | dansmith | but anyway, exposing locked_by=owner|other seems like a reasonable compromise | |
| 18:32:05 | mriedem | i'd be fine with a GET /servers/{server_id}/capabilities API | |
| 18:32:14 | mriedem | and we start rolling shit into that like this | |
| 18:32:25 | dansmith | to be clear, | |
| 18:32:46 | dansmith | that means someone has to get the server, see that it's locked and a reason, then go fetch the capabilities of the server to see if they can unlock it | |
| 18:32:58 | dansmith | which is fine, but... | |
| 18:32:59 | mriedem | they could just try to unlock it | |
| 18:33:01 | mriedem | get the 403 | |
| 18:33:03 | mriedem | like always | |
| 18:33:12 | dansmith | sure | |
| 18:33:15 | mriedem | if locked_by=other | |
| 18:33:16 | elbragstad | ^ that's what people do now with other APIs (not necessarily in nova) | |
| 18:33:28 | mriedem | then there is a reasonable guess they might not be able to unlock it | |
| 18:33:32 | mriedem | which the reason should tell them why it's locked | |
| 18:33:35 | dansmith | I dunno, I'm just having trouble following all the logic spaghetti, but I don't really need to care | |
| 18:34:02 | elbragstad | does the reason include something like "this was locked by an admin, pay your bill pls" | |
| 18:34:11 | gmann | yeah lock_reason can tell, "this is locked by admin/system and ask them to unlock after xyz condition/time" | |
| 18:34:23 | gmann | elbragstad: yeah, i think it can be | |
| 18:34:32 | dansmith | elbragstad: I know they do, and I'm sure it doesn't set off any alarms to hit 403s, but it feels like trying sudo vi /etc/passwd on a system to see if you can, and if not, it's reported :) | |
| 18:35:29 | gmann | IMO with lock_reason info in response can explain the locked_by things. till now we did not have lock_reason so may be locked_by was important to know | |
| 18:35:38 | elbragstad | yeah - true... the same argument has been made to have a capabilities-like api in keystone, but we're obviously not there yet, either | |
| 18:35:49 | dansmith | lock_reason is for humans, locked_by is for computers, IMHO | |
| 18:35:54 | dansmith | so not really a substitute | |
| 18:36:20 | mriedem | i think i'm going to go lock myself in the garage with the car running | |
| 18:36:38 | dansmith | elbragstad: last time I went jiggling door handles in my neighborhood to see who has theirs locked, I was asked to not do that | |
| 18:36:48 | elbragstad | make sure you set locked_by=owner | |
| 18:36:56 | dansmith | mriedem: unfortunately if you have a catalytic converter that's going to take a really long time | |
| 18:37:16 | gmann | dansmith: ok so computer purpose still can be fulfilled by locked_by=other|owner. | |
| 18:37:22 | dansmith | gmann: yes | |
| 18:37:34 | elbragstad | dansmith yeah - not the best u-x, i agree | |
| 18:38:15 | gmann | i am thinking exposing exact UUID of locked person (admin etc) can be security leak ? | |
| 18:38:50 | gmann | so just say you can unlock it or not via locked_by=owner|other seems appropriate for me now | |
| 18:39:16 | gmann | for example exposing system admin id to end user etc | |
| 18:45:19 | openstackgerrit | melanie witt proposed openstack/nova-specs master: Move Stein implemented specs https://review.openstack.org/645749 | |
| 18:45:55 | melwitt | hm, wanted to get this other spec update done before moving specs but it's in merge conflict https://review.openstack.org/#/c/639033/1 | |
| 18:47:19 | mriedem | someone want to send this queens change through https://review.openstack.org/#/c/643219/ | |
| 18:47:23 | melwitt | er... I guess it's not. just somehow it makes the move specs patch in merge conflict | |
| 18:47:24 | mriedem | i'll start the release request | |
| 18:47:32 | mriedem | melwitt: i'll get that spec update | |
| 18:47:59 | melwitt | mriedem: thanks. I'll get the queens patch | |
| 19:01:14 | openstackgerrit | Merged openstack/nova-specs master: Update alloc-candidates-in-tree https://review.openstack.org/639033 | |