Open Stack

On Mon, 18 Feb 2019 13:16:38 +0800, He Jie Xu <soulxu at gmail.com> wrote:
> Add the maillist back, I missed from the previous reply...
> 
> melanie witt <melwittt at gmail.com <mailto:melwittt at gmail.com>> 于2019年2 
> 月16日周六 上午12:22写道：
> 
>     Thanks for the reply, Alex. Response is inline.
> 
>     On Fri, 15 Feb 2019 15:49:19 +0800, He Jie Xu <soulxu at gmail.com
>     <mailto:soulxu at gmail.com>> wrote:
>      > We need to ensure all the APIs check the policy with real instance's
>      > project and user id, like this
>      >
>     https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/evacuate.py#L85-L88
>      > Otherwise, any user can get any other tenant's instance.
>      >
>      > Some of APIs doesn't check like this, for example:
>      >
>     https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/attach_interfaces.py#L55
> 
>     While I agree with this (policy check instance project/user instead of
>     request context project/user), I'm not sure it's related to the
>     project_only=True at the database layer. The project_only=True at the
>     database layer only enforces request context project, which is what the
>     second example here will also do, if policy contains
>     'project_id:%(project_id)s'. It seems to me that if we remove
>     project_only=True from the database layer, we will get proper
>     enforcement of request context project/user by the existing policy
>     checks, if the policy is configured as such. And we default policy to
>     either rule:admin_api or rule:admin_or_owner:
> 
>     https://docs.openstack.org/nova/latest/configuration/sample-policy.html
> 
>     So, it seems to me that changing policy enforcement from request
>     context
>     project/user => instance project/user would be a separate change.
>     Please
>     let me know if I'm misunderstanding you.
> 
>     One thing I do notice though, in your first example, is that the
>     get_instance is done _before_ the policy check, which would need to be
>     moved after the policy check, in the same change that would remove
>     project_only=True. So I'm glad you pointed that out.
> 
> 
> Emm...no, the first example is the right example.
> 
> project_only=True will ensure db call return the instance belong to the 
> project
> in the request context. If project_only=False, the db call may return 
> other project's
> instance.
> 
> Then when the rule is 'project_id:%(project_id)s' and the target is 
> instance's project_id,
> the policy enforcement will ensure the request context's project id 
> match the instance's project_id,
> then the user won't get other project's instance.

Right, that is the proposal in this email. That we should remove 
project_only=True and let the API policy check handle whether or not the 
user from a different project is allowed to get the instance. Otherwise, 
users are not able to use policy to control the behavior because it is 
hard-coded in the database layer.

I was trying to say that adding the instance.project_id target seems 
unrelated to the issue about removing project_only=True.

>      > I'm trying to memory why we didn't do that in the beginning,
>     sounds like
>      > we refused to support user-id based policy. But
>      > in the end for the backward-campatible for the user like CERN, we
>      > support user-id based policy for few APIs.
>      >
>     https://review.openstack.org/#/q/topic:bp/user_id_based_policy_enforcement+(status:open+OR+status:merged)
>      >
>      > This is probably why some APIs checks policy with real instance's
>      > project id and user id and some APIs not.
>      >
>      > melanie witt <melwittt at gmail.com <mailto:melwittt at gmail.com>
>     <mailto:melwittt at gmail.com <mailto:melwittt at gmail.com>>> 于2019年2
>      > 月15日周五 上午1:23写道：
>      >
>      >     Hey all,
>      >
>      >     Recently, we had a customer try the following command as a
>     non-admin
>      >     with a policy role granted in policy.json to allow live migrate:
>      >
>      >         "os_compute_api:os-migrate-server:migrate_live":
>     "rule:admin_api or
>      >     role:Operator"
>      >
>      >     The scenario is that they have a server in project A and a
>     user in
>      >     project B with role:Operator and the user makes a call to
>     live migrate
>      >     the server.
>      >
>      >     But when they call the API, they get the following error
>     response:
>      >
>      >         {"itemNotFound": {"message": "Instance <uuid server 1>
>     could not be
>      >     found.", "code": 404}}
>      >
>      >     A superficial look through the code shows that the live
>     migrate should
>      >     work, because we have appropriate policy checks in the API,
>     and the
>      >     request makes it past those checks because the policy.json
>     has been set
>      >     correctly.
>      >
>      >     A common pattern in our APIs is that we first
>     compute_api.get() the
>      >     instance object and then we call the server action (live
>     migrate, stop,
>      >     start, etc) with it after we retrieve it. In this scenario, the
>      >     compute_api.get() fails with NotFound.
>      >
>      >     And the reason it fails with NotFound is because, much lower
>     level, at
>      >     the DB layer, we have a keyword arg called 'project_only'
>     which, when
>      >     True, will scope a database query to the
>     RequestContext.project_id
>      >     only.
>      >     We have hard-coded 'project_only=True' for the instance get
>     query.
>      >
>      >     So, when the user in project B with role:Operator tries to
>     retrieve the
>      >     instance record in project A, with appropriate policy rules
>     set, it
>      >     will
>      >     fail because 'project_only=True' and the request context is
>     project B,
>      >     while the instance is in project A.
>      >
>      >     My question is: can we get rid of the hard-coded
>     'project_only=True' at
>      >     the database layer? This seems like something that should be
>      >     enforced at
>      >     the API layer and not at the database layer. It reminded me of an
>      >     effort
>      >     we had a few years ago where we removed other hard-coded policy
>      >     enforcement from the database layer [1][2]. I've uploaded a WIP
>      >     patch to
>      >     demonstrate the proposed change [3].
>      >
>      >     Can anyone think of any potential problems with doing this?
>     I'd like to
>      >     be able to remove it so that operators are able use policy to
>     allow
>      >     non-admin users with appropriately configured roles to run server
>      >     actions.
>      >
>      >     Cheers,
>      >     -melanie
>      >
>      >     [1]
>      >
>     https://blueprints.launchpad.net/nova/+spec/nova-api-policy-final-part
>      >     [2]
>      >
>     https://review.openstack.org/#/q/topic:bp/nova-api-policy-final-part+(status:open+OR+status:merged)
>      >     [3] https://review.openstack.org/637010
>      >
> 
> 
> 
>