Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-11-26
16:22:58 tssurya gmann: sure but the problem with the current implementation is that it treats everything as "right". Even if the project is invalid, we end up giving the user the right to feed anything into the flavor_projects table
16:28:14 gmann tssurya: but it is 403, in case of invalid it will hit 404 condition. not 404 means project is there
16:34:49 openstackgerrit Merged openstack/nova master: api-guide: remove empty sections about inter-service interactions https://review.opendev.org/695774
16:35:19 openstackgerrit Eric Fried proposed openstack/nova master: DNM: Test openstacksdk weakrefs https://review.opendev.org/695934
16:35:32 tssurya gmann: not really there can be a 403 on a case where the project doesn't exist right ?
16:35:52 tssurya because tenant doesn't have permission to verify if the project exists ?
16:39:19 gmann tssurya: not sure keystone does that and return 404 instead of 403. I know neutron return otherway- any PUT/DELETE request first call the GET and return 404 even resource exist and no permissions to GET(they do not return 403 because that is somehow telling resource exit).
16:40:23 tssurya gmann: yea I guess keystone is returning 403 which nova is silently ignoring
16:40:28 gmann i think keystone return 403 for permissions denied on GET project not 404. lbragstad ?
16:40:44 gmann lbragstad can confirm this for us.
16:40:47 lbragstad gmann it depends
16:41:09 lbragstad if you're technically authorized to get a project (e.g., you're a system administrator) you'll get a 404
16:41:30 lbragstad if you don't have the right authorization - then you get a 403
16:41:58 gmann yeah that is what nova assume- https://github.com/openstack/nova/blob/fd67f69cfdaf04620f2e8a5f1fbf5737096965d8/nova/api/openstack/identity.py#L26
16:42:22 lbragstad gmann ok - yeah, that looks right
16:42:24 gmann 403 means project is valid and exist and its only permission issue
16:42:35 gmann lbragstad: thanks for quick confirmation.
16:42:47 lbragstad well - the project may exist, but your first problem is that you're hitting a permission issue
16:43:00 tssurya gmann: I don't think 403 ensures project exists
16:43:19 lbragstad if you hit a 403, you should fix the permission issue and eventually retry
16:43:37 tssurya because we have a couple of non-existing entries in our flavor_projects
16:43:55 tssurya non-existing project entries*
16:44:14 lbragstad a 403 isn't conclusive that the project exists
16:44:20 gmann tssurya: but in those cases, we will get 404 right
16:44:26 tssurya lbragstad: yea that makes sense
16:44:31 tssurya gmann: nope we don't get the 404
16:45:01 tssurya I had a user inject "ABCD" project into the database :D
16:45:09 gmann tssurya: yeah it is 'may' exist
16:45:11 tssurya which doesn't exist
16:45:14 tssurya hehe
16:46:08 tssurya I've opened a bug here: https://bugs.launchpad.net/nova/+bug/1854053
16:46:08 openstack Launchpad bug 1854053 in OpenStack Compute (nova) "_add_tenant_access silently ignores 403" [Undecided,New]
16:46:42 tssurya probably nova should raise for a 403 or prompt user to retry or something
16:49:29 gmann tssurya: with default policy which is admin only should work but if overridden then yes it can make non-exist project entry in flavor access
16:50:11 tssurya gmann: yea that's true, in this case it was done from a service project by a user having admin role
16:50:51 mriedem tssurya: without reading all of the scrollback, "sure but the problem with the current implementation is that it treats everything as "right". Even if the project is invalid, we end up giving the user the right to feed anything into the flavor_projects table" is exactly how that api worked before that change to validate the project id provided
16:50:56 tssurya nevertheless allowing "ok" and then feeding whatever value is coming from the user is not ok
16:51:12 mriedem the project id validation is best effort
16:51:47 tssurya mriedem: aha, I didn't know the history
16:52:17 gmann tssurya: mriedem i think we should only proceed the request for 2xx case and rest all return 400 with error message of what we get from keystone
16:52:24 mriedem so in this case we're using the token from the request context or the configurable keystone auth in nova?
16:52:44 mriedem gmann: which is an api change
16:53:33 mriedem by configurable keystone auth in nova i mean this https://docs.openstack.org/nova/latest/configuration/config.html#keystone
16:53:37 gmann yeah
16:53:39 mriedem service user configured in nova to talk to keystone
16:53:44 mriedem like we do for neutron and cinder
16:54:21 mriedem so doesn't that just mean the service user configured in nova to talk to keystone doesn't have enough permissions to know if the given project_id exists?
16:54:40 tssurya mriedem: right
16:54:45 tssurya exactly
16:55:07 tssurya https://github.com/openstack/nova/blob/fd67f69cfdaf04620f2e8a5f1fbf5737096965d8/nova/api/openstack/identity.py#L62
16:55:39 mriedem yeah, so my point is, it sounds like this is a config issue in your deployment
16:55:54 mriedem i.e. why can't you grant access to the service user that is doing this check on behalf of nova?
16:56:51 tssurya the service user already has an admin role, I think this happens because the project is not admin project ?
16:57:19 mriedem idk
16:57:31 mriedem isn't that also configurable in keystone?
16:57:59 mriedem i.e. the policy to GET a project by id - it's not hard-coded to admin project only is it?
16:57:59 gmann mriedem: but point is users do not know it was permission issue for them or they are adding wrong project
16:58:20 tssurya probably, I'll see if it can be tweaked on the config side for our particular scenario,
16:58:49 tssurya but in general its strange nova allows entries from users that don't have permission
16:58:53 mriedem gmann: if the service user nova is configured with has permissions to ask the question "does this project exist" that's all this utility code is used for
16:59:07 gmann policy can be changed to make it work properly and with default policy it should work at long as keystone system scope is enabled
16:59:36 mriedem and again, this was added as best effort to close a leak where the flavor access and quotas apis would just blindly take whatever project id and assume it existed and shove it into the nova db
16:59:50 gmann mriedem: from API usage they do not know nova does this check internally ?
17:00:41 tssurya mriedem: yea if its best effort, I'll probably just tweak it downstream then and write it in our documentation
17:00:56 mriedem i don't understand the question. if configured properly and nova determines the project doesn't exist (404 from keystone) the compute API users will also get a 404
17:00:58 gmann mriedem: yeah agree on best effort but if API return the permissions issue to user and then they ask provider to modify the permission or ask other right user to add the tenant in flavor access
17:01:38 mriedem i see 3 options:
17:02:02 mriedem 1. close the bug as invalid - tweak your config (we could add docs, idk if that would be found or help) to do what you need to avoid the 403 from keystone
17:02:13 gmann mriedem: no. if permissions issue then keystone return 403 (which means project may exist or not) and nova proceed the request with 200. so user does not know if that is correct or not.
17:02:26 mriedem 2. change the 403 case as an error and raise it back to the compute api caller - maybe enough time has passed to not worry about backward compat with the old non-validating behavior
17:03:11 mriedem 3. add a config option to toggle what the behavior of the api should be in the 403 case, default to backward compat, but allow making it raise for those that want that, but that means you need to know to twiddle that config which if you know that, you likely know you can just fix the permissions for your service user
17:03:17 tssurya I vote for 2
17:03:29 mriedem of course you do :)
17:03:36 tssurya :D
17:05:22 mriedem one thing to point out for #2 is that the quota set and flavor access add/remove style write APIs are admin-only by default, so if we do change the behavior of that 403 it would only be on admin apis, not end-user apis
17:06:03 gmann 3rd is not good option. config driven API not good.
17:06:08 tssurya ack
17:06:12 gmann even admin only API it still API change in option 2
17:06:46 mriedem option 1 is the only non-api change
17:06:54 mriedem we're not going to microversion this imo
17:07:00 mriedem if that's what you're thinking
17:07:15 gmann with option 1 yes no microversion but option 2 need microverion
17:07:19 openstackgerrit Dan Smith proposed openstack/nova master: Add a way to exit early from a wait_for_instance_event() https://review.opendev.org/695985
17:07:25 mriedem not really
17:07:48 mriedem i don't necessarily think we need a microversion to prevent you from shooting yourself in the foot
17:07:57 mriedem we have a few cases where we've made the api error on cases that are wrong
17:08:08 tssurya dansmith: when you have time would you lightly review the idea in https://review.opendev.org/#/c/689422/ ? thanks in advance :)
17:08:18 mriedem and would just fail later, e.g. attaching sriov ports (not supported), rebuilding volume-backed instances with a new image (not supported)
17:08:56 gmann mriedem: yes but here we are changing the behavior. user does not permissions to GET project and project exist is right request and nvoa is 200 currently which will be 400.
17:09:40 mriedem gmann: the "user" in this case is an admin
17:09:46 gmann yes
17:10:10 dansmith tssurya: -1.9
17:10:11 mriedem i'm agreeing that re-raising the 403 from keystone would be a behavior change
17:10:17 tssurya I would prefer not to have a microversion to fix this, but I don't know if its "okay" to do it
17:10:24 mriedem but i'm not agreeing it requires a microversion
17:10:37 mriedem we didn't add the project id validation in a new microversion either, because it was stopping a leaky bug
17:10:54 tssurya dansmith: bad idea ?
17:11:02 mriedem iow, i shouldn't have to request a microversion to tell nova to properly do validation
17:11:05 dansmith tssurya: can't get past the typo in the commit message ;P
17:11:15 dansmith tssurya: I added two people that should look at it

Earlier   Later