Glance

image import copy-image API should reflect proper authorization

Bug #1884587 reported by Dan Smith
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
Fix Released
High
Unassigned
Ussuri
Fix Committed
High
Unassigned
Victoria
Fix Released
High
Unassigned

Bug Description

In testing the image import copy-to-store mechanism from Nova, I hit an issue that seems clearly to be a bug. Scenario:

A user boots an instance from an image they have permission to see. Nova uses their credentials to start an image import copy-to-store operation, which succeeds:

"POST /v2/images/e6b1a7d0-ccd8-4be3-bef7-69c68fca4313/import HTTP/1.1" 202 211 0.481190
Task [888e97e5-496d-4b94-b530-218d633f866a] status changing from pending to processing

Note the 202 return code. My code polls for a $timeout period, waiting for the image to either arrive at the new store, or be marked as error, which never happens ($timeout=600s). The glance log shows (trace truncated):

glance-api[14039]: File "/opt/stack/glance/glance/async_/flows/api_image_import.py", line 481, in get_flow
glance-api[14039]: stores if
glance-api[14039]: File "/opt/stack/glance/glance/api/authorization.py", line 296, in forbidden_key
glance-api[14039]: raise exception.Forbidden(message % key)
glance-api[14039]: glance.common.exception.Forbidden: You are not permitted to modify 'os_glance_importing_to_stores' on this image.

So apparently Nova is unable to use the user's credentials to initiate a copy-to-store operation. That surprises me and I think it likely isn't the access control we should be enforcing. However, if we're going to reject the operation, we should reject it at the time the HTTP response is sent, not later async, since we can check authorization right then and there.

The problem in this case is that from the outside, I have no way of knowing that the task fails subsequently. I receive a 202, which means I should start polling for completion. The task fails to load/run and thus can't update any status on the image, and I'm left to wait for 600s before I give up.

So, at the very least, we're not checking the same set of permissions during the HTTP POST call, and we should be. I also would tend to argue that the user should be allowed to copy the image and not require an admin to do it, perhaps with some additional policy element to control that. However, I have to be able to determine when and when not to wait for 600s.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to glance (master)

Related fix proposed to branch: master
Review: https://review.opendev.org/737382

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on glance (master)

Change abandoned by Dan Smith (<email address hidden>) on branch: master
Review: https://review.opendev.org/737382
Reason: Sounds like we're expecting only the owner to ever be able to copy images, or admin, so this is not a solution.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to glance (master)

Reviewed: https://review.opendev.org/737548
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=c930638fcf58b43d53903cfc30e06fd6919bdad6
Submitter: Zuul
Branch: master

commit c930638fcf58b43d53903cfc30e06fd6919bdad6
Author: Dan Smith <email address hidden>
Date: Tue Jun 23 07:12:12 2020 -0700

    Check authorization before import for image

    Right now we only check to see if the user can see the image before
    we kick off an import operation. However, that will never work unless
    the user is the *owner* of the image (or an admin) which means we
    return a 202 to the API caller and then the task fails immediately.

    This change makes us check that authorization up front and return an
    appropriate error to the user so they know it failed, and avoid
    starting a task destined for failure.

    Note that there was already a check for a Forbidden result when calling
    the import API. However, that used a context.owner=None which could never
    happen in reality. A more suitable check would have been to use a context
    with a different real owner, but it turns out that the task creation
    would have succeeded in that case as well. This test is changed to use
    an alternate owner and ensure that we get the forbidden result from the
    new check immediately.

    Change-Id: I385f222c5e3b46978b40bdefdc28fcb20d9c67d3
    Closes-Bug: #1884587

Changed in glance:
status: In Progress → Fix Released
Abhishek Kekane (abhishek-kekane)
summary: - image import copy-to-store API should reflect proper authorization
+ image import copy-image API should reflect proper authorization
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to glance (stable/ussuri)

Reviewed: https://review.opendev.org/738737
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=fcba03989feed85a3301e53b252f766eabeeba3c
Submitter: Zuul
Branch: stable/ussuri

commit fcba03989feed85a3301e53b252f766eabeeba3c
Author: Dan Smith <email address hidden>
Date: Tue Jun 23 07:12:12 2020 -0700

    Check authorization before import for image

    Right now we only check to see if the user can see the image before
    we kick off an import operation. However, that will never work unless
    the user is the *owner* of the image (or an admin) which means we
    return a 202 to the API caller and then the task fails immediately.

    This change makes us check that authorization up front and return an
    appropriate error to the user so they know it failed, and avoid
    starting a task destined for failure.

    Note that there was already a check for a Forbidden result when calling
    the import API. However, that used a context.owner=None which could never
    happen in reality. A more suitable check would have been to use a context
    with a different real owner, but it turns out that the task creation
    would have succeeded in that case as well. This test is changed to use
    an alternate owner and ensure that we get the forbidden result from the
    new check immediately.

    Change-Id: I385f222c5e3b46978b40bdefdc28fcb20d9c67d3
    Closes-Bug: #1884587
    (cherry picked from commit c930638fcf58b43d53903cfc30e06fd6919bdad6)

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.