verify router, subnet/port ownership for L3 router interface calls

[dan wendlandt]

The L3 router API exposes two PUT actions "add_router_interface" and "remove_router_interface".

First, we need to make sure policy.py limits these calls to the the user owns the router or is admin.

Second, we need need to make sure we check that subnet/port passed in in the data for these calls is owned by that tenant or that user is admin.

[Nachi Ueno (nati-ueno)]

Hi Dan

May I fix floating IPs also in this patch?
Or should we have separate bug report?

[Salvatore Orlando (salvatore-orlando)]

Nachi,

What do you mean by fixing floating IPs?

Floating IPs are not directly associated to a router, but rather to an external network.
On an external network we do not want to restrict only the owner to create floating ips by default (altough other scenarios are possible, and we want therefore to make it configurable).

Also, we already have a patch under review for policies on external networks - you're welcome to review it.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/12468

[Nachi Ueno (nati-ueno)]

Hi Salvatore

I got it! The review may solve my concern.

[Akihiro Motoki (amotoki)]

Nachi and Dan,

I can not understand why this bug is filed.

In the current implementation with the patch, I confirm the following behavior and it suffices the requirement explained in the bug description.
I missed something?

> First, we need to make sure policy.py limits these calls to the the user owns the router or is admin.

add/remove_router_interface() calls self._get_router(context, router_id) at the beginning.
When a user other than owner or admin calls add/remove_router_interface(), 'router not found' will return.
It seems to suffices the requirement.

> Second, we need need to make sure we check that subnet/port passed in in the data for these calls is owned by that tenant or that user is admin.

This is similar to the first. add/remove_router_interface() calls self._get_port() or _get_subnet() with a user context.
When a user other than owner of the port/subnet or admin calls add/remove_router_interface(), 'port not found' or 'subnet not found' will return. It seems to suffices the requirement too.

[Salvatore Orlando (salvatore-orlando)]

Akihiro,

I have reviewed the patch as well and I too share your concerns.
I also have concerns about having policy logic in the plugins (policy should be plugin-independent, and increased number of calls into the db).

The proposed patch appears to add unnecessary policy checks, as the following:

"default": [["rule:admin_or_owner"]],

should also be called by default for every router operation. Also, as you pointed out in the comments, referenced policies are different from the one actually entered in policy.json, so they should never be hit.

I will do some more tests later on today.

[Nachi Ueno (nati-ueno)]

Hi Akihiro-san, Salvatore-san

I agree to current implementation defaults to admin_or_owner behavior.
However it is hard coded policy and it is not configurable.

I may misunderstand spec.
Isn't policy json configurable?
Some user wan't to limit router function for only admins.
In they he can setup policy json to "default": [["rule:admin"]].

[Salvatore Orlando (salvatore-orlando)]

I think the hardcoded behavior you are referring to is the one implemented in QuantumDBPluginv2 as far as queries on the db are concerned.

I agree that providers might want to apply more strict authZ criteria, and the policy engine is the right place to do that.
The policy engine should already provide mechanism for associating rules with action.
For instance "create_router" : [["rule:admin_only"]] should automatically allow only admins to create routers , without requiring any change to the code.
If the above is not correct, then we have an issue with the policy engine, and I'd rather fix that issue so that the policy engine works as expect.

The other point I had is that policy checks should not be performed in the plugin; as they affect they way the user interacts with Quantum, you might want to ensure a consistent behavior regardless of the plugin used.

Finally, on subnet and port ownership, Akihiro correctly observed that add_router_interface and its dual call the method for retrieving port/subnet with the user context. These method will filter the query on context.tenant_id. So I think we can consider ourselves safe as far as Folsom is concerned, unless I am missing something.

[Nachi Ueno (nati-ueno)]

Hi Salvatore

So currently rule:admin_or_owner is hard coded in QuantumDBPluginv2.
so if is safe for the user who wants rule:admin_or_owner.

I agree policy check shouldn't be done in plugin basically.

Problem is auto policy check is not applied to the custom resources.

Auto policy check for resources
https://github.com/openstack/quantum/blob/master/quantum/api/v2/base.py#L188
https://github.com/openstack/quantum/blob/master/quantum/api/v2/base.py#L259

To fix this looks big change for me, so it may not fit rc1.
This is reason, I added policy checking code on l3_db.py

[Akihiro Motoki (amotoki)]

I agree that providers might need more fine-grained authZ criteria and the policy engine should support it.
I second Salvatore's opinion that the policy engine should be fixed in case the appropriate policy check is not applied.

I also confirmed that policies in policy.json are not applied to the custom resources such as router, as Nachi says. For instance, when I defined "create_router": [["rule:admin_only"]] and restarted quantum-server, a regular user "demo" can still create a router.

On the other hand, I think Nachi's fix (that makes it configurable for operatios to the extended resources such as "router") is beyond this bug. This bug is already addressed without any fixes. My question is "Is it required for Folsom to make the policy for routers configurable?".

[Nachi Ueno (nati-ueno)]

Hi Motoki-san

IMO, this fix should be in the bug report because description says "we need to make sure policy.py limits these calls "
However I wanna confirm it to dan.

> Dan
Should I fix it in rc1?

[Salvatore Orlando (salvatore-orlando)]

Nachi, as I speak I am looking into this issue.
It looks like a policy engine flaw - personally I am more on the same line of though as Akihiro.

Will let you know more in a few minutes.

Please get in touch with me via mail/irc if you have more questions or concerns.

[Salvatore Orlando (salvatore-orlando)]

Akihiro,

I have run some tests, and the policy apparently seem to work fine for me.
Non-admin users can by default create routers and access only their own, and when restricting the policy to admin_only for create_router, I obtain the following (I've added some additional log line to see how the policy engine handles it)

2012-09-06 23:31:00 DEBUG [root] Reloading cached file /home/salvatore/git/quantum/etc/policy.json
2012-09-06 23:31:00 DEBUG [quantum.openstack.common.policy] ### and_list:('rule:create_router',)
2012-09-06 23:31:00 DEBUG [quantum.openstack.common.policy] ### and_list:[u'rule:admin_only']
2012-09-06 23:31:00 DEBUG [quantum.openstack.common.policy] ### and_list:[u'role:admin']
2012-09-06 23:31:00 DEBUG [quantum.openstack.common.policy] ### OH NO....:[u'role:admin']
2012-09-06 23:31:00 DEBUG [quantum.openstack.common.policy] ### OH NO....:[u'rule:admin_only']
2012-09-06 23:31:00 DEBUG [quantum.openstack.common.policy] ### OH NO....:('rule:create_router',)
2012-09-06 23:27:51 ERROR [quantum.api.v2.base] Create operation not authorized
Traceback (most recent call last):
  File "/home/salvatore/git/quantum/quantum/api/v2/base.py", line 285, in create
    plugin=self._plugin)
  File "/home/salvatore/git/quantum/quantum/policy.py", line 193, in enforce
    exceptions.PolicyNotAuthorized, action=action)
  File "/home/salvatore/git/quantum/quantum/openstack/common/policy.py", line 127, in enforce
    raise exc(*args, **kwargs)
PolicyNotAuthorized: Policy doesn't allow create_router to be performed.
2012-09-06 23:27:51 ERROR [quantum.api.v2.resource] create failed
Traceback (most recent call last):
  File "/home/salvatore/git/quantum/quantum/api/v2/resource.py", line 95, in resource
    result = method(request=request, **args)
  File "/home/salvatore/git/quantum/quantum/api/v2/base.py", line 302, in create
    raise webob.exc.HTTPForbidden()
HTTPForbidden: Access was denied to this resource.
2012-09-06 23:27:51 DEBUG [eventlet.wsgi.server] 192.168.36.100 - - [06/Sep/2012 23:27:51] "POST /v2.0/routers.json HTTP/1.1" 403 185 0.094674

Can you instrument the policy engine and send me a snippet of a log file to try to understand why in your case regular users are able to create routers? The method I usually instrument is HttpBrain._check in quantum.openstack.common.policy.py

[Nachi Ueno (nati-ueno)]

Aga,, Really!?

Sorry I mistook to test policy for routers.
Could you share your policy.json for this?

[Salvatore Orlando (salvatore-orlando)]

My apologies for the delay. I missed this comment.

http://paste.openstack.org/show/20778/

[Nachi Ueno (nati-ueno)]

Thanks.

OK So this is not looks bug.

[Nachi Ueno (nati-ueno)]

Ah but,
_get_port, _get_subnet still enforces admin_or_owner behavior.
This should be fixed.

[Akihiro Motoki (amotoki)]

Hi Salvatore,

I confirmed the policy check for router operation works well as we expected.

When I did not get an expected result, I did the following: (1) start devstack with unmodified policy.json (2) edit /etc/quantum/policy.json (3) visit q-svc screen and restart quantum-server.

This time I did (1) edit /opt/stack/quantum/etc/policy.json (2) start devstack.

In devstack deployment both /opt/stack/quantum/etc/policy.json and /etc/quantum/policy.json exist and utils.find_config_file() finds /opt/stack/quantum/etc/policy.json first. This is the reason I could not get an expected. result.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/12468
Committed: http://github.com/openstack/quantum/commit/c01e54bde27382bc803ef14c0a57a3fd070b1559
Submitter: Jenkins
Branch: master

commit c01e54bde27382bc803ef14c0a57a3fd070b1559
Author: Nachi Ueno <email address hidden>
Date: Wed Sep 5 20:29:27 2012 +0000

    Added policy checks for add interface and remove interface

    Fixes bug 1042037
    admin_only policy didn't works, so policy checks
    for add interface and remove interface needed.
    Also updated policy.json

    Change-Id: Ifec281250ccbe1680a3e634f4efdb7ba7ef3ec94