| openstackgerrit | Lance Bragstad proposed openstack/glance master: Cleanup remaining tenant terminology in glance API docs https://review.opendev.org/c/openstack/glance/+/775299 | 00:07 |
|---|---|---|
| *** zzzeek has quit IRC | 00:08 | |
| *** zzzeek has joined #openstack-glance | 00:10 | |
| *** rcernin has quit IRC | 02:19 | |
| *** rcernin has joined #openstack-glance | 02:33 | |
| *** rcernin has quit IRC | 02:42 | |
| *** rcernin has joined #openstack-glance | 02:42 | |
| *** udesale has joined #openstack-glance | 03:25 | |
| *** zzzeek has quit IRC | 03:57 | |
| *** zzzeek has joined #openstack-glance | 03:58 | |
| *** k_mouza has joined #openstack-glance | 04:06 | |
| *** k_mouza has quit IRC | 04:11 | |
| *** gyee has quit IRC | 04:30 | |
| *** zzzeek has quit IRC | 04:43 | |
| openstackgerrit | Abhishek Kekane proposed openstack/glance_store stable/victoria: Adjust requirements and lower-constraints https://review.opendev.org/c/openstack/glance_store/+/775190 | 04:44 |
| *** zzzeek has joined #openstack-glance | 04:45 | |
| *** k_mouza has joined #openstack-glance | 04:54 | |
| openstackgerrit | Merged openstack/glance master: Uncap PrettyTable https://review.opendev.org/c/openstack/glance/+/775141 | 04:56 |
| *** k_mouza has quit IRC | 04:58 | |
| *** udesale_ has joined #openstack-glance | 05:09 | |
| *** udesale__ has joined #openstack-glance | 05:10 | |
| *** udesale has quit IRC | 05:11 | |
| *** udesale_ has quit IRC | 05:14 | |
| *** zzzeek has quit IRC | 05:15 | |
| *** zzzeek has joined #openstack-glance | 05:16 | |
| *** zzzeek has quit IRC | 06:03 | |
| *** zzzeek has joined #openstack-glance | 06:04 | |
| *** k_mouza has joined #openstack-glance | 06:07 | |
| *** k_mouza has quit IRC | 06:11 | |
| *** zzzeek has quit IRC | 06:13 | |
| *** zzzeek has joined #openstack-glance | 06:14 | |
| *** nikparasyr has joined #openstack-glance | 06:21 | |
| *** rchurch has quit IRC | 06:24 | |
| *** zzzeek has quit IRC | 06:24 | |
| *** zzzeek has joined #openstack-glance | 06:30 | |
| *** zzzeek has quit IRC | 06:37 | |
| *** zzzeek has joined #openstack-glance | 06:37 | |
| openstackgerrit | Merged openstack/glance master: Remove unused option "owner_is_tenant" https://review.opendev.org/c/openstack/glance/+/763920 | 06:43 |
| *** zzzeek has quit IRC | 07:01 | |
| *** zzzeek has joined #openstack-glance | 07:02 | |
| *** rcernin has quit IRC | 07:37 | |
| *** zzzeek has quit IRC | 07:44 | |
| *** zzzeek has joined #openstack-glance | 07:46 | |
| *** ralonsoh has joined #openstack-glance | 07:50 | |
| *** zzzeek has quit IRC | 08:05 | |
| *** zzzeek has joined #openstack-glance | 08:07 | |
| *** rcernin has joined #openstack-glance | 08:18 | |
| *** zzzeek has quit IRC | 08:43 | |
| *** zzzeek has joined #openstack-glance | 08:46 | |
| *** zzzeek has quit IRC | 08:55 | |
| *** zzzeek has joined #openstack-glance | 08:56 | |
| *** zzzeek has quit IRC | 09:05 | |
| *** zzzeek has joined #openstack-glance | 09:07 | |
| *** zzzeek has quit IRC | 09:19 | |
| *** zzzeek has joined #openstack-glance | 09:21 | |
| *** rcernin has quit IRC | 09:55 | |
| *** k_mouza has joined #openstack-glance | 10:02 | |
| *** rcernin has joined #openstack-glance | 10:09 | |
| *** rajivmucheli has joined #openstack-glance | 10:11 | |
| *** rcernin has quit IRC | 10:23 | |
| *** k_mouza has quit IRC | 10:56 | |
| *** k_mouza has joined #openstack-glance | 10:56 | |
| *** k_mouza has quit IRC | 11:02 | |
| *** zzzeek has quit IRC | 11:42 | |
| *** zzzeek has joined #openstack-glance | 11:45 | |
| *** rcernin has joined #openstack-glance | 11:48 | |
| *** k_mouza has joined #openstack-glance | 11:53 | |
| *** udesale_ has joined #openstack-glance | 12:02 | |
| *** udesale__ has quit IRC | 12:04 | |
| *** rcernin has quit IRC | 12:08 | |
| *** rcernin has joined #openstack-glance | 12:22 | |
| *** udesale__ has joined #openstack-glance | 12:44 | |
| *** udesale_ has quit IRC | 12:46 | |
| *** rcernin has quit IRC | 12:53 | |
| *** k_mouza has quit IRC | 13:04 | |
| *** k_mouza has joined #openstack-glance | 13:21 | |
| *** rcernin has joined #openstack-glance | 13:27 | |
| openstackgerrit | Abhishek Kekane proposed openstack/glance master: Expand tasks database table to add more columns https://review.opendev.org/c/openstack/glance/+/763739 | 13:43 |
| openstackgerrit | Abhishek Kekane proposed openstack/glance master: Utilize newly added tasks database fields https://review.opendev.org/c/openstack/glance/+/774615 | 13:43 |
| openstackgerrit | Abhishek Kekane proposed openstack/glance master: WIP: New API /v2/images/{id}/tasks to show tasks associated with image https://review.opendev.org/c/openstack/glance/+/774830 | 13:43 |
| *** zzzeek has quit IRC | 13:48 | |
| *** zzzeek has joined #openstack-glance | 13:49 | |
| *** zzzeek has quit IRC | 13:59 | |
| *** zzzeek has joined #openstack-glance | 14:04 | |
| *** sangeet has quit IRC | 14:04 | |
| *** sangeet has joined #openstack-glance | 14:10 | |
| sangeet | I have glance (Train) deployed as wsgi with Nginx. I am not able to upload images bigger than 5GB via cli (--file option). I get "Command error: HTTP 504 Gateway Time-out: nginx". But in glance log I see traceback with error " ERROR glance.common.wsgi Got error from Swift: put_object('glance', 'e7b58c3e-3894-46bb-b8a8-a3798ca6446c-00023', ...) failure and no ability to reset contents for reupload." Please help | 14:10 |
| *** rajivmucheli has quit IRC | 14:17 | |
| *** rcernin has quit IRC | 14:30 | |
| dansmith | gmann: what's the preferred way to signal that a user doesn't have access to a subresource? Meaning: | 15:28 |
| dansmith | GET /v2/images/$image_id/tasks | 15:28 |
| dansmith | if the user doesn't have access to $image_id, shouldn't we return 404 for this ^ ? | 15:28 |
| gmann | dansmith: yes, 404 is recommended as 403 can tell them it exist and they do not have access but 404 hide that info | 15:29 |
| dansmith | yeah, okay thanks | 15:30 |
| openstackgerrit | Merged openstack/glance-specs master: New Image API /v2/images/{id}/tasks for task information https://review.opendev.org/c/openstack/glance-specs/+/763740 | 15:31 |
| *** lbragstad_ has joined #openstack-glance | 15:46 | |
| *** lbragstad has quit IRC | 15:50 | |
| *** nikparasyr has left #openstack-glance | 15:53 | |
| openstackgerrit | Merged openstack/glance master: trivial: Fix a typo in devstack plugin.sh https://review.opendev.org/c/openstack/glance/+/775276 | 16:04 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Implement API protection testing for images https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568 | 16:17 |
| lbragstad_ | dansmith gmann fwiw - i think we took a different approach in keystone | 16:25 |
| *** lbragstad_ is now known as lbragstad | 16:25 | |
| *** k_mouza has quit IRC | 16:26 | |
| lbragstad | i think we check explicitly if a user has access to something, if they don't we return a 403, regardless of the resource existing | 16:26 |
| lbragstad | we return a 404 if the user is authorized to access something, but it doesn't exist | 16:26 |
| lbragstad | https://review.opendev.org/c/openstack/keystone-tempest-plugin/+/686305/48/keystone_tempest_plugin/tests/rbac/v3/test_domain.py#187 is an example of the former | 16:27 |
| lbragstad | https://review.opendev.org/c/openstack/keystone-tempest-plugin/+/686305/48/keystone_tempest_plugin/tests/rbac/v3/test_domain.py#125 is an example of the later | 16:27 |
| dansmith | so they get a 403 if they ask for something that doesn't exist, or something they don't have access to right? | 16:27 |
| lbragstad | latter* | 16:27 |
| dansmith | how do you know if something doesn't exist that they _do_ have access to? :) | 16:27 |
| dansmith | or rather, how can someone have access to something that doesn't exist | 16:29 |
| lbragstad | backing up a bit | 16:32 |
| lbragstad | https://github.com/openstack/keystone/blob/master/keystone/api/domains.py#L37 is where we populate the target data | 16:32 |
| lbragstad | and if the domain doesn't exist the target is empty - which fails with the domain check in the policy https://github.com/openstack/keystone/blob/master/keystone/common/policies/domain.py#L44 | 16:32 |
| dansmith | that means if the domain doesn't exist you can't prove they have access, so 403 right? | 16:33 |
| lbragstad | yes | 16:34 |
| lbragstad | we have some of this captured in an old bug report because we were bleeding existence checks depending on if a 404 or a 403 was returned | 16:35 |
| lbragstad | and it allowed someone to fish for resources they didn't have access to | 16:35 |
| dansmith | lbragstad: for context, the code I was reviewing was returning an empty list of sub resources in both cases of "the base resource does not exist" and "the base resource is not allowed to the current user" | 16:35 |
| lbragstad | but - the way you phrase "how can someone have access to something that doesn't exist" is a good point | 16:36 |
| dansmith | so 403 or 404 would be better, but it seems like 404 is the preferred thing elsewhere.. I didn't know keystone did that differently though | 16:36 |
| dansmith | lbragstad: right, I get the 403-if-not-exists, but I don't see how you can return 404 if they have access to the base resource, but it doesn't exist | 16:36 |
| lbragstad | i'm having a hard time understanding the last part of ^ | 16:38 |
| lbragstad | shouldn't we return a 404 when someone is authorized to do something, but the resource doesn't exist? | 16:39 |
| dansmith | well, you said "we return a 404 if the user is authorized to access something, but it doesn't exist" | 16:39 |
| lbragstad | oh | 16:39 |
| lbragstad | ok - so here | 16:40 |
| lbragstad | https://review.opendev.org/c/openstack/keystone-tempest-plugin/+/686305/48/keystone_tempest_plugin/tests/rbac/v3/test_domain.py#103 | 16:40 |
| lbragstad | technical system-administrators represent god-mode=True, right? | 16:40 |
| lbragstad | in that case we return a 404 because we know they're technically authorized to view any domain in the deployment, but the one they are looking for doesn't exist | 16:41 |
| dansmith | well, I dunno how you factor that in.. super admins are a bit of a different thing | 16:41 |
| dansmith | sure, that's fine | 16:41 |
| dansmith | lbragstad: here's the code I was reviewing, for context: https://review.opendev.org/c/openstack/glance/+/774830/2/glance/api/v2/images.py | 16:43 |
| dansmith | this is /v2/images/$image_id/tasks | 16:43 |
| dansmith | I don't think they have anything that will have short-circuited if $image_id doesn't exist, and if they did, he wouldn't have written that code I think | 16:44 |
| lbragstad | ok - so iiuc that would be calling https://github.com/openstack/glance/blob/master/glance/api/policy.py#L120 | 16:47 |
| lbragstad | from a policy perspective | 16:47 |
| dansmith | I don't even know that means.. checking to see if image get is possible ever for that user? | 16:48 |
| dansmith | but I guess that's the answer to "is authorized to see something that doesn't exist" :) | 16:50 |
| dansmith | like x perms on the directory or something | 16:50 |
| lbragstad | if i'm reading this right, which might not be the case, the policy will enforce an empty target if the database returns a NotFound | 16:52 |
| dansmith | right, but what is the point of that? | 16:52 |
| dansmith | just so you can override the result of a nonexistent image with 403 instead of 404? | 16:52 |
| dansmith | because that seems like a bad thing if one cloud gives you a 404 for missing images and another 403 | 16:53 |
| lbragstad | because their policies are different you mean | 16:53 |
| lbragstad | or their policies could potentially be different because of override behavior | 16:54 |
| dansmith | right, one cloud decides that show on a nonexistent image should be 403 for everyone, where another cloud decides it should be 404 | 16:55 |
| lbragstad | i see what you mean | 16:56 |
| dansmith | you write your client to poll for deletion until you get 404, and that breaks when you move to another cloud where that never happens | 16:57 |
| dansmith | or you report "you don't have access to this thing, but it definitely exists, because I didn't get a 404" to the user | 16:57 |
| gmann | yeah that is ^^ main issue and info leak. | 16:58 |
| dansmith | gmann: but because of that override, it might *not* exist | 16:58 |
| dansmith | can be different on two different glances | 16:58 |
| dansmith | because that policy override, AFAICT | 16:58 |
| dansmith | the converse is the info leak, where we return 403 for a thing that *does* exist, which tells the user it's there | 16:59 |
| gmann | so if image exist then check the policy right | 16:59 |
| dansmith | gmann: but also if the image doesn't exist, check the policy, which could return 403 instead of 404 | 16:59 |
| lbragstad | the second case you just mentioned dansmith is what we try and prevent in keystone | 16:59 |
| lbragstad | (specifically, we don't want to leak information) | 17:00 |
| dansmith | lbragstad: well, it's what returning 404 for unauthorized does, | 17:00 |
| dansmith | but if you return 403 for "does not exist" and "unauthorized" that's the same difference, it's just upside down, IMHO | 17:00 |
| *** udesale__ has quit IRC | 17:01 | |
| lbragstad | i found a separate example in keystone that doesn't use the domain API | 17:02 |
| lbragstad | https://github.com/openstack/keystone/blob/master/keystone/tests/protection/v3/test_projects.py#L696-L710 | 17:02 |
| dansmith | I think that's in line with your always-403 strategy | 17:03 |
| lbragstad | that test is written running as a domain administrator | 17:03 |
| dansmith | I think it's okay to pick "default to 404" or "default to 403" -- both avoid the leak, but IMHO 404 is more correct | 17:04 |
| lbragstad | ok | 17:04 |
| dansmith | but you probably also have more complex permission structures and relations than we do | 17:04 |
| gmann | well 404 can be non-exist or url does not exist. but 403 is clear that you do not have access to things which might or might not exist | 17:04 |
| gmann | yeah i feel 403 in all cases is more info leak then 404 | 17:04 |
| dansmith | agreed, but if you're consistent about always 403 then it's probably not exposing anything too badly | 17:05 |
| dansmith | but from a client design perspective, | 17:05 |
| dansmith | I think 403 means "you need to do something to gain access" which is confusing if the thing really doesn't exist | 17:05 |
| dansmith | whereas 404 promises less to the user | 17:05 |
| dansmith | "this thing doesn't exist, but it might just not exist FOR YOU" | 17:05 |
| lbragstad | yeah... | 17:06 |
| lbragstad | gdi | 17:06 |
| gmann | true | 17:06 |
| dansmith | but what seems to be the case here for glance is... "ask your cloud administrator how they have configured their policy to know what this really means" | 17:06 |
| dansmith | I think that's a 6xx error code: "601: Call your operator" | 17:07 |
| lbragstad | i've seen that statement sprinkled through the docs | 17:07 |
| dansmith | or maybe 602: Software engineer failed you here | 17:07 |
| lbragstad | ok - so for glance, can we pursue the 404-always strategy? | 17:09 |
| dansmith | I would think we should be headed in that direction, yeah | 17:09 |
| dansmith | but glance does also have some finer-grained permissions which may now depend on people being able to see something after it's shared or something, I dunno | 17:10 |
| dansmith | I bet rosmaita knows :) | 17:10 |
| dansmith | but definitely for anything new, the goal should be default to 404, IMHO | 17:10 |
| lbragstad | thinking back on the keystone example again | 17:12 |
| lbragstad | we didn't want to expose information outside of the domain (for domain users) because we didn't want to violate tenancy at that level | 17:12 |
| lbragstad | and that drove us to the 403-always strategy | 17:13 |
| dansmith | it really comes down to what you do for nonexistent things.. if you report 403 for nonexistent things you should have access to, then you can't deduce whether something exists or not from the 403 | 17:13 |
| dansmith | if you do return 404 in those cases, but 403 for unauthorized, then I think you're exposing existence | 17:14 |
| dansmith | that's why I said consistency and what you promise to the user matters | 17:14 |
| lbragstad | but - i suppose we could have implemented the 404-always strategy if we 1.) check if the resource existed - if not 404 (this is normal) 2.) check if the resource exists, it does, return 404 if policy enforcement returns a 403 | 17:14 |
| dansmith | I think that exposes more than you want, | 17:15 |
| dansmith | unless you know that this user is granted enough permission to know if something exists within their domain or whatever | 17:15 |
| dansmith | that doesn't really exist for nova at least, and I kinda doubt for glance too | 17:15 |
| dansmith | but that's why I said keystone may have situations where you're an admin for your group, so it's okay to know if something exists that you can't access *within* that group | 17:15 |
| lbragstad | yeah - so #1 would pull the resource, check if the user has access, if they don't policy would return a 403, if they don't it would return a 404(?) | 17:15 |
| dansmith | but you don't want to expose that info across pepsi/coke | 17:16 |
| dansmith | I think it's 200 if authorized and exists, else 404 | 17:16 |
| lbragstad | yea | 17:16 |
| dansmith | like email, the internet was a much nicer place when they came up with 401, 403, etc | 17:17 |
| dansmith | and like I said, | 17:17 |
| dansmith | if you're in the same tenant, but your *user* doesn't have access, 403 is probably fine | 17:17 |
| dansmith | you're telling the pepsi marketing team that they can't access pepsi HR files | 17:18 |
| dansmith | but you don't want to tell coke anything about that | 17:18 |
| lbragstad | in which case coke should see 404s, is what you're saying | 17:21 |
| dansmith | right total lack of acknowledging anything "huh? images? what are images? I dispense kitkat bars here" | 17:22 |
| * lbragstad nods | 17:22 | |
| lbragstad | makes sense | 17:22 |
| dansmith | the easier thing though, is to just play dumb with everyone, | 17:22 |
| dansmith | and not introduce the middle layer of 403-if-you're-friendly | 17:22 |
| lbragstad | we should have taken that approach in keystone | 17:24 |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Validate volume type during volume create https://review.opendev.org/c/openstack/glance_store/+/774703 | 17:28 |
| gmann | nice example | 17:28 |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Validate volume type during volume create https://review.opendev.org/c/openstack/glance_store/+/774703 | 17:33 |
| lbragstad | dansmith gmann ok - i tried to distill things in a bug report https://bugs.launchpad.net/keystone/+bug/1915540 so we at least document it | 17:36 |
| openstack | Launchpad bug 1915540 in OpenStack Identity (keystone) "HTTP 403s are more confusing than HTTP 404s when evaluating authorization of a non-existent resource" [Undecided,New] | 17:36 |
| gmann | lbragstad: +1, thanks. may be we can document it in api-wg repo guidelines | 17:37 |
| dansmith | yeah | 17:37 |
| *** k_mouza has joined #openstack-glance | 17:38 | |
| dansmith | I guess I thought we had the default-to-404 approach codified as the goal already, openstack-wide | 17:38 |
| dansmith | I was asking gmann instead of looking, because I rudely depend on him instead of doing my own research :) | 17:38 |
| gmann | we might have there. i just remember as I had same question/discussion form neutron (amotoki) when I was in Tokyo :) | 17:39 |
| *** gmann is now known as gmann_afk | 17:40 | |
| *** k_mouza has quit IRC | 17:41 | |
| lbragstad | are these still the guidelines? https://specs.openstack.org/openstack/api-wg/guidelines/http/response-codes.html | 17:45 |
| lbragstad | or have they moved somewhere else? | 17:45 |
| dansmith | seems legit, but yeah, surprised to not see anything in there about this, | 17:49 |
| dansmith | especially since I thought this had been discussed a lot | 17:50 |
| lbragstad | yeah - same here | 17:54 |
| jokke | dansmith: lbragstad: Could you point me to where this 403/404 confusion happens? It should always be 404 for the very reasons dansmith laid down and that has been the approach we've been living on. So if we're returning 403 for non-existing resource, that's a bug breaking our API and we should fix it | 18:10 |
| dansmith | jokke: it's not that we _are_ returning 403, I think it's that an operator could configure it that way, because we run enforce on a NotFound | 18:11 |
| dansmith | lbragstad linked above, let me find it | 18:11 |
| dansmith | jokke: https://github.com/openstack/glance/blob/master/glance/api/policy.py#L120 | 18:12 |
| dansmith | unless that's doing something other than what we expect, | 18:12 |
| dansmith | it seems like that offers the opportunity for a policy rule that would return something like 401 or 403 instead of 404 in the case where we fail to look up an instance | 18:12 |
| dansmith | jokke: and most of the discussion above was about keystone, FWIW because lbragstad is lonely and hanging out in the glance channel :) | 18:13 |
| jokke | yeah, I got that much ... I was just wondering how is that a case, but I had no idea we would do that. So the enforce is actually raising Forbidden in that case? | 18:22 |
| jokke | as from that code we are handling the NotFound exception, but the raise would push forward what ever exception the enforce() raised rather than the NotFound we caught, right? | 18:23 |
| dansmith | jokke: I think it would if the operator wrote a rule that denied access to a non-existent target object | 18:23 |
| dansmith | right | 18:23 |
| dansmith | I don't know why else that enforce would be there, other than to allow the operator to write such a rule, which I expect is just legacy from someone wanting to be able to override that behavior | 18:24 |
| jokke | Me neither, and I'm not sure why do we allow that in the first place | 18:25 |
| dansmith | because some cloud vendor wanted it at some point I'm sure :) | 18:25 |
| jokke | but, I can't recall at least writing nor reviewing that code so I have no idea of the intentions of it | 18:25 |
| dansmith | but yeah, I dunno how you feel about just removing it, vs deprecating it | 18:25 |
| dansmith | we could try..except around it and then just warn that this isn't supported anymore, or you could just remove it and reno it | 18:26 |
| jokke | dansmith: that's probably it "No but our security guy, yeah that on left with folio hat, told us that this is the right way we need to do it" :P | 18:26 |
| dansmith | jokke: right, obviously 403 is more "secure" than 404 :P | 18:26 |
| dansmith | I can remove it and see if any tests fail | 18:27 |
| jokke | sounds like a good start. | 18:27 |
| jokke | Maybe we should shoot mail to the list and check if we'd be breaking anyone removing it. If no-one yells hard, get rid of it | 18:28 |
| dansmith | glance.tests.unit.v2.test_images_resource.TestImagesControllerPolicies.test_show_unauthorized | 18:28 |
| dansmith | https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/tests/unit/v2/test_images_resource.py#L3480-L3485 | 18:29 |
| jokke | uuh so we even test that it does the stuupid thing so we've definitely been deliberate about it | 18:29 |
| dansmith | yeah | 18:29 |
| jokke | gr8 | 18:29 |
| dansmith | been there a looooong time: https://review.opendev.org/c/openstack/glance/+/11412 | 18:30 |
| jokke | thought so | 18:31 |
| jokke | almost from the nova times | 18:31 |
| dansmith | I'll remove the test too and push it up for comment I guess | 18:31 |
| jokke | That almost looks like test that was written because it was the behaviour out of the test case rather than intentionally make sure we do forbidden on notFound | 18:36 |
| dansmith | yeah could be, it doesn't have a comment that says "this is definitely how it should be" | 18:37 |
| jokke | As it's not mentioned anywhere that it's the desired behaviour, it's all about "This is mess, all over the place and not consistent" | 18:37 |
| dansmith | yeah, a common mistake when adding tests after code.. testing the code and not the desired behavior | 18:37 |
| jokke | happens _a_lot_ over here | 18:38 |
| jokke | People adding "test coverage" not reading docs or checking what the intended behaviour is, just testing that " True == True" | 18:38 |
| dansmith | this one too: https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/tests/functional/v2/test_images.py#L1943-L1943 | 18:38 |
| dansmith | atually, | 18:39 |
| dansmith | looking at that test, | 18:39 |
| dansmith | I think we probably are *always* returning 403, | 18:39 |
| dansmith | for any get_image rule that looks at the image properties, we'd always fail that enforce since we don't pass an object | 18:40 |
| dansmith | that test doesn't intend to test this behavior, but fails because it can't check {}['owner'] | 18:40 |
| dansmith | lbragstad: right? | 18:40 |
| jokke | https://docs.openstack.org/api-ref/image/v2/index.html?expanded=list-image-members-detail#list-image-members we should be returning 404 | 18:47 |
| jokke | Very clear in the API docs if tenant that is not a member tries to list the members, they will get 404 | 18:47 |
| dansmith | that's good | 18:48 |
| dansmith | if we're always returning 403 right now, then less good in that it's quite likely that people will notice | 18:49 |
| jokke | yeah, well it looks like we're returning forbidden at least for the image that exists, not sure if the image does not exists | 18:50 |
| dansmith | oh duh, right I see | 18:51 |
| dansmith | we're probably getting notfound from the db layer because of permissions, | 18:51 |
| dansmith | but that's getting converted to 403 in the middle so that the http user never sees it | 18:51 |
| jokke | So it might be that the idea has been right, to homogenize those two, _but_ the implementation has been left half way and instead of returning Fobidden to the client, we'd supposed to translate that to NotFound in the API layer | 18:52 |
| *** gmann_afk is now known as gmann | 18:52 | |
| dansmith | I think it's the other way, | 18:52 |
| dansmith | we're raising notfound from the lower layer, | 18:52 |
| jokke | ohh | 18:52 |
| jokke | crap | 18:52 |
| dansmith | but we're translating that to 403 when we shouldn't | 18:52 |
| jokke | that's stupid | 18:53 |
| jokke | Like I'd be ok with that if it was the actual documented API behaviour, like fine. But the fact that we're translating the response to something which really doesn't make sense, test it and then document it in the opposite way ... ffs | 18:54 |
| dansmith | sounds like keystone has intentionally gone that direction, which if you're consistent, isn't terrible, but I think it's less desirable from the client perspective because you end up doing things like "delete and poll until 403" instead of 404 | 18:58 |
| jokke | Ohh we're even better | 18:58 |
| jokke | IIRC we just happily tell you the delete request was successful, even if you do delete on resource that was already deleted | 18:59 |
| openstackgerrit | Dan Smith proposed openstack/glance master: RFC: Stop raising 403 when image is not found https://review.opendev.org/c/openstack/glance/+/775435 | 18:59 |
| dansmith | jokke: ^ for comments and discussion | 18:59 |
| dansmith | bug probably needs more words and link to the api docs | 18:59 |
| dansmith | surely we have a test somewhere that makes sure we get a 404 for a non-existent image though | 19:09 |
| dansmith | so there must be something else going on here | 19:09 |
| dansmith | maybe only if we fetch the image as part of a call other than a direct show or something | 19:10 |
| jokke | I'm wondering that too | 19:11 |
| jokke | have no idea tbh | 19:11 |
| dansmith | I get a 404 from devstack with a nonexistent image id, so... it's clearly not *always* a 403 | 19:11 |
| *** k_mouza has joined #openstack-glance | 19:13 | |
| jokke | ;) | 19:13 |
| jokke | This is getting better | 19:13 |
| jokke | Yeah, I'm pretty sure you should most of the times get 404 | 19:13 |
| jokke | Just worried that it's clearly not always | 19:14 |
| dansmith | yeah, so I can make my policy such that I get a 403 for a non-existent image: | 19:14 |
| dansmith | dan@guaranine:~$ cat /etc/glance/policy.json | 19:15 |
| dansmith | {"get_image": "false"} | 19:15 |
| dansmith | dan@guaranine:~$ glance image-show 39398e72-ff56-40ca-878f-43c58c437634 | 19:15 |
| dansmith | HTTP 403 Forbidden: You are not authorized to complete get_image action. | 19:15 |
| dansmith | that image-id should be not found | 19:15 |
| jokke | yup | 19:15 |
| *** k_mouza has quit IRC | 19:17 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: Stop raising 403 when image is not found https://review.opendev.org/c/openstack/glance/+/775435 | 19:27 |
| dansmith | added back the test, but changed and commented to validate the *desired* behavior | 19:27 |
| * lbragstad reads up | 19:36 | |
| lbragstad | about the lower layer thing... | 19:36 |
| lbragstad | glance's db layer definitely has some code that tacks SQL queries together depending on the project in the request - which results in a 404 when you're looking for an image outside your project | 19:37 |
| lbragstad | and that behavior is a little different for people with the admin role | 19:38 |
| lbragstad | because of the _is_visible logic - which special cases the admin role in the database layer before returning the image | 19:38 |
| lbragstad | https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/db/sqlalchemy/api.py#L286 | 19:41 |
| lbragstad | https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/db/utils.py#L44 | 19:41 |
| jokke | yeah, the voodoo going on around the db is bit out of my comfort zone | 19:41 |
| lbragstad | you and me both :) | 19:42 |
| lbragstad | ideally - it would be nice to write a check string that makes these obsolete https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/db/utils.py#L45-L74 | 19:50 |
| lbragstad | and then just rely on https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/api/policy.py#L123-L124 after removing https://github.com/openstack/glance/blob/2c893fbd80d0241fad2515221b61266ced12f92d/glance/api/policy.py#L120 (hypothetically) | 19:51 |
| lbragstad | and i think there is a discrepancy with existing images | 19:55 |
| lbragstad | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568/15/glance_tempest_plugin/tests/rbac/v2/test_images.py#562 (project-member case) | 19:55 |
| lbragstad | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568/15/glance_tempest_plugin/tests/rbac/v2/test_images.py#993 (project-admin case) | 19:56 |
| lbragstad | in both of those cases - we should be returning a 404, right? | 20:08 |
| *** rcernin has joined #openstack-glance | 20:35 | |
| *** ralonsoh has quit IRC | 20:37 | |
| *** k_mouza has joined #openstack-glance | 20:40 | |
| *** k_mouza has quit IRC | 20:41 | |
| *** k_mouza has joined #openstack-glance | 20:43 | |
| sangeet | I have glance (Train) deployed as wsgi with Nginx. I am not able to upload images bigger than 5GB via cli (--file option). I get "Command error: HTTP 504 Gateway Time-out: nginx". But in glance log I see traceback with error " ERROR glance.common.wsgi Got error from Swift: put_object('glance', 'e7b58c3e-3894-46bb-b8a8-a3798ca6446c-00023', ...) failure and no ability to reset contents for reupload." Please help | 20:45 |
| *** k_mouza has quit IRC | 20:48 | |
| *** rcernin has quit IRC | 21:22 | |
| lbragstad | dansmith jokke ok - i'm working on converting this conversation into glance_tempest_plugin tests - to be clear, regardless if an image exists in a project, a user from another project should always see a 404 when they attempt to fetch that image, right? | 21:28 |
| dansmith | lbragstad: I would think so yeah.. only hedging because I'm not super familiar with the sharing model in glance | 21:29 |
| dansmith | meaning, if you're not granted permission to see an image, you should get 404, yes | 21:29 |
| dansmith | however in some permission models, you can deny someone access they would have otherwise inherited from group membership | 21:30 |
| dansmith | in which case you might want them to see a 403 in that case since you're explicitly denying them | 21:30 |
| dansmith | I doubt there's anything like that in glance, but I'm hedging because I don't know | 21:30 |
| lbragstad | yeah - that makes sense | 21:30 |
| lbragstad | so - anything raising a Forbidden from glance's database layer is going to result in a 404 ultimately from the API | 21:31 |
| lbragstad | (assuming we fix the bug you opened) | 21:31 |
| dansmith | well, the bug was actually the opposite, that anything resulting in a NotFound could result in a 403 | 21:32 |
| dansmith | anything that is Forbidden from the lower layers would be a different path | 21:32 |
| lbragstad | yeah - different path, same outcome | 21:33 |
| lbragstad | if a project-admin tries to get an image in a project that isn't their own, they get a 403 | 21:33 |
| dansmith | they do? or you're asking if they should? | 21:34 |
| lbragstad | they do | 21:34 |
| lbragstad | project members get a 404 in the same situation | 21:34 |
| dansmith | okay that seems wrong too, because that's like exposing pepsi secrets to the sysadmin for coke right? | 21:34 |
| lbragstad | yes | 21:34 |
| lbragstad | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568/15/glance_tempest_plugin/tests/rbac/v2/test_images.py#993 | 21:35 |
| dansmith | just because they are a coke sysadmin doesn't mean they should know any more about pepsi than coke regulars | 21:35 |
| lbragstad | right - i agree | 21:35 |
| dansmith | yeah, seems broken | 21:35 |
| lbragstad | but i think it's because of a mismatch in the assumptions of what an admin is between the database and new policy defaults we're working on | 21:36 |
| dansmith | well, this is why I was saying earlier that it's easier if you don't try to expose a little more rich information to admins just because they're admins, because you can get unintentional leaks like this | 21:36 |
| dansmith | easier to just say "either 200 or 404" | 21:36 |
| lbragstad | yeah - that makes sense, but i'm struggling to find a case where we would use a 403 now with image_get | 21:37 |
| dansmith | can't really think of one | 21:38 |
| lbragstad | ok - so... good | 21:38 |
| dansmith | if glance has a "transfer ownership" option, then maybe after you've given the image away, 403 for you until they accept or something | 21:38 |
| dansmith | but yeah, otherwise I think not | 21:38 |
| lbragstad | ok - and that might just be a side-effect of get_image, too? | 21:38 |
| lbragstad | because that might be different is someone is a project-reader and they're trying to create private images | 21:39 |
| lbragstad | in that case, 403 makes sense i think | 21:39 |
| dansmith | yeah, I dunno, and like I say I don't know if glance has that | 21:39 |
| dansmith | so I'm really speaking from my posterior | 21:39 |
| dansmith | probably best to get an answer from the elders | 21:39 |
| lbragstad | agreed - i'll go with that for now, translate it to tests, and then post a patch | 21:40 |
| dansmith | yeah, asking for reviews on tests is very concrete, so a good plan :) | 21:41 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!