Merge lp:~sandy-walsh/nova/admin-only-api into lp:~hudson-openstack/nova/trunk

[Sandy Walsh]

Flag to define which operations are exposed in the OpenStack API, disabling all others.

Adds a new flag --nova_api_permitted_operations

when blank (the default) all available openstack API operations are permitted. Alternatively, you may set this to be a comma separated list of strings. Operation names are singular (not the collection names)

Currently accepts options are: server, backup_schedule, image, flavor, sharedipgroup

So if you wanted to make API not accept "flavor" commands, do:
--nova_api_permitted_operations=server, backup_schedule, image, sharedipgroup

TODO: If this list gets too long we could add a "--nova_api_disallowed_operations" flag instead.

TESTING:

1. Leave --nova_api_permitted_operation out of your /etc/nova/nova-manage.conf, start API
2. Get a flavor list from http://<ip:port>/v1.0/flavors/detail (I used a modified python-cloudserver to query API)
3. Get an image list from http://<ip:port>/v1.0/images/detail
4. Both of the prior commands should work, even if results are empty.
5. Modify --nova_api_permitted_operations=server, backup_schedule, image, sharedipgroup
(note that 'flavor' is left out)
6. Restart API and retry #2 and #3 again ... /flavors/* should report no route. As if the code was never there.
7. You can tack 'flavor' back onto your --nova_api_permitted_operations list and try again if you like.

[Soren Hansen (soren)]

I'm curious what the motivation is?

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/

[Sandy Walsh (sandy-walsh)]

There seems to be a need to stand up api servers inside the firewall which can perform privileged operations.

I had mentioned that this sort of sandboxing should be performed by the normal RBAC capabilities of the auth system. But others would rather draw hard lines in what is available to the public API in the event of a breach.

Hope it helps!
-S

________________________________________
From: <email address hidden> [<email address hidden>] on behalf of Soren Hansen [<email address hidden>]
Sent: Wednesday, November 24, 2010 4:01 AM
To: <email address hidden>
Subject: Re: [Merge] lp:~sandy-walsh/nova/admin-only-api into lp:nova

I'm curious what the motivation is?

--
Soren Hansen
Ubuntu Developer http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/
https://code.launchpad.net/~sandy-walsh/nova/admin-only-api/+merge/41666
You are the owner of lp:~sandy-walsh/nova/admin-only-api.

Confidentiality Notice: This e-mail message (including any attached or
embedded documents) is intended for the exclusive and confidential use of the
individual or entity to which this message is addressed, and unless otherwise
expressly indicated, is confidential and privileged information of Rackspace.
Any dissemination, distribution or copying of the enclosed material is prohibited.
If you receive this transmission in error, please notify us immediately by e-mail
at <email address hidden>, and delete the original message.
Your cooperation is appreciated.

[Soren Hansen (soren)]

2010/11/24 Sandy Walsh <email address hidden>:
> There seems to be a need to stand up api servers inside the firewall
> which can perform privileged operations.
>
> I had mentioned that this sort of sandboxing should be performed by
> the normal RBAC capabilities of the auth system. But others would
> rather draw hard lines in what is available to the public API in the
> event of a breach.

That makes sense. Sorry, I completely missed the fact that there was
actually a blueprint linked to the branch.

I apprecite your wanting to make this mechanism generic, but I think I
would prefer a more specific --allow_admin_api flag instead:

a) I question the usefulness of being able to pick and choose which of
e.g. flavors, images, and servers you want to expose. At least for now,
it's pretty clear which operations should be admin-only and which ones
should not.

b) It ensures that when adding new operations, you don't necessarily
have to change your config to get access to them (or, worse, deny access
to them in case of --nova_api_disallowed_operation).

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/

[Sandy Walsh (sandy-walsh)]

Changed made ... there ain't much to it anymore. Just a flag and some TODO's.

[Devin Carlen (devcamcar)]

Having a global flag is much improved. lgtm so far

review: Approve

[Soren Hansen (soren)]

2010/11/24 Sandy Walsh <email address hidden>:
> Changed made ... there ain't much to it anymore. Just a flag and some TODO's.

Heh.. Sorry about that :)

Looks good, though!

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~sandy-walsh/nova/admin-only-api into lp:nova failed. Below is the output from the failed tests.

nova/api/openstack/__init__.py:54:1: E302 expected 2 blank lines, found 1
class API(wsgi.Middleware):
^
    Separate top-level function and class definitions with two blank lines.

    Method definitions inside a class are separated by a single blank line.

    Extra blank lines may be used (sparingly) to separate groups of related
    functions. Blank lines may be omitted between a bunch of related
    one-liners (e.g. a set of dummy implementations).

    Use blank lines in functions, sparingly, to indicate logical sections.

    Okay: def a():\n pass\n\n\ndef b():\n pass
    Okay: def a():\n pass\n\n\n# Foo\n# Bar\n\ndef b():\n pass

    E301: class Foo:\n b = 0\n def bar():\n pass
    E302: def a():\n pass\n\ndef b(n):\n pass
    E303: def a():\n pass\n\n\n\ndef b(n):\n pass
    E303: def a():\n\n\n\n pass
    E304: @decorator\n\ndef a():\n pass
nova/tests/api/openstack/test_adminapi.py:29:1: E302 expected 2 blank lines, found 1
class RestrictedAPITest(unittest.TestCase):
^
    Separate top-level function and class definitions with two blank lines.

    Method definitions inside a class are separated by a single blank line.

    Extra blank lines may be used (sparingly) to separate groups of related
    functions. Blank lines may be omitted between a bunch of related
    one-liners (e.g. a set of dummy implementations).

    Use blank lines in functions, sparingly, to indicate logical sections.

    Okay: def a():\n pass\n\n\ndef b():\n pass
    Okay: def a():\n pass\n\n\n# Foo\n# Bar\n\ndef b():\n pass

    E301: class Foo:\n b = 0\n def bar():\n pass
    E302: def a():\n pass\n\ndef b(n):\n pass
    E303: def a():\n pass\n\n\n\ndef b(n):\n pass
    E303: def a():\n\n\n\n pass
    E304: @decorator\n\ndef a():\n pass
nova/tests/api/openstack/test_adminapi.py:45:63: W291 trailing whitespace
        # We should still be able to access public operations.
                                                              ^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12
nova/tests/api/openstack/test_adminapi.py:53:63: W291 trailing whitespace
        # We should still be able to access public operations.
                                                              ^
    JCR: Trailing whitespace is superflu...

[Eric Day (eday)]

Actually, I liked the previous version, since admin will mean different things to different people. As soon as we draw an arbitrary line of what is admin and what is not, we're starting to get deployment specific.

Beyond admin abilities, we may not want to expose functionality due to what is supported. For example, if one host doesn't support volumes, we want to be able to disable any volume related API methods.

My vote is to go back to the manually enable/disable service method.

[Sandy Walsh (sandy-walsh)]

Good use case Eric. Hadn't considered that usage.

If we could do macros in flags, we could easily define admin-services and public-services to keep it easy for the administrator. Soren brings up a good argument though that, currently, this would mean having two unused intermediary flags showing up in --help. :(

Configuration DSL anyone?

[Soren Hansen (soren)]

2010/11/24 Eric Day <email address hidden>:
> Actually, I liked the previous version, since admin will mean different things to different people. As soon as we draw an arbitrary line of what is admin and what is not, we're starting to get deployment specific.

I'm happy to change my mind if you can give me an example for
something that may or may not be considered admin. It seems pretty
clear cut to me.

> Beyond admin abilities, we may not want to expose functionality due to what is supported. For example, if one host doesn't support volumes, we want to be able to disable any volume related API methods.

I don't think this is how we want to handle that sort of thing. We
should have better mechanism for discovering whether we support volume
operations at all, and if not, don't offer it. It shouldn't require
explicit configuration.

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/

[Soren Hansen (soren)]

2010/11/24 Soren Hansen <email address hidden>:
> I don't think this is how we want to handle that sort of thing. We
> should have better mechanism for discovering whether we support volume
> operations at all, and if not, don't offer it. It shouldn't require
> explicit configuration.

At least not explicit configuration in terms of which URL endpoints to
expose. Something like --disable-volume would be OK, and should affect
all the API's we offer.

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/

[Eric Day (eday)]

lgtm, although it might be better off in nova.api, and not just part of nova.api.openstack. Would also be nice if there were actually something to control, but I'm guessing that will be coming in the next patch. :)

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in Authors

[lp:~sandy-walsh/nova/admin-only-api]

415. By Sandy Walsh on 2010-12-09

Fixed Authors conflict and re-merged with trunk

=== modified file 'Authors'
--- Authors	2010-12-08 23:47:25 +0000
+++ Authors	2010-12-09 14:28:18 +0000
@@ -21,6 +21,7 @@
 Paul Voccio <[e-mail]>
 Rick Clark <[e-mail]>
 Ryan Lucio <[e-mail]>
+Sandy Walsh <[e-mail]>
 Soren Hansen <[e-mail]>
 Todd Willey <[e-mail]>
 Trey Morris <[e-mail]>

=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2010-12-02 19:29:37 +0000
+++ nova/api/openstack/__init__.py	2010-12-09 14:28:18 +0000
@@ -48,6 +48,10 @@
     'nova.api.openstack.auth.BasicApiAuthManager',
     'The auth mechanism to use for the OpenStack API implemenation')
 
+flags.DEFINE_bool('allow_admin_api',
+    False,
+    'When True, this API service will accept admin operations.')
+
 
 class API(wsgi.Middleware):
     """WSGI entry point for all OpenStack API requests."""
@@ -183,6 +187,10 @@
         mapper.resource("sharedipgroup", "sharedipgroups",
                         controller=sharedipgroups.Controller())
 
+        if FLAGS.allow_admin_api:
+            logging.debug("Including admin operations in API.")
+            # TODO: Place routes for admin operations here.
+
         super(APIRouter, self).__init__(mapper)
 
 

=== added file 'nova/tests/api/openstack/test_adminapi.py'
--- nova/tests/api/openstack/test_adminapi.py	1970-01-01 00:00:00 +0000
+++ nova/tests/api/openstack/test_adminapi.py	2010-12-09 14:28:18 +0000
@@ -0,0 +1,61 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2010 OpenStack LLC.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import unittest
+
+import stubout
+import webob
+
+import nova.api
+from nova import flags
+from nova.tests.api.openstack import fakes
+
+FLAGS = flags.FLAGS
+
+
+class AdminAPITest(unittest.TestCase):
+    def setUp(self):
+        self.stubs = stubout.StubOutForTesting()
+        fakes.FakeAuthManager.auth_data = {}
+        fakes.FakeAuthDatabase.data = {}
+        fakes.stub_out_networking(self.stubs)
+        fakes.stub_out_rate_limiting(self.stubs)
+        fakes.stub_out_auth(self.stubs)
+        self.allow_admin = FLAGS.allow_admin_api
+
+    def tearDown(self):
+        self.stubs.UnsetAll()
+        FLAGS.allow_admin_api = self.allow_admin
+
+    def test_admin_enabled(self):
+        FLAGS.allow_admin_api = True
+        # We should still be able to access public operations.
+        req = webob.Request.blank('/v1.0/flavors')
+        res = req.get_response(nova.api.API('os'))
+        self.assertEqual(res.status_int, 200)
+        # TODO: Confirm admin operations are available.
+
+    def test_admin_disabled(self):
+        FLAGS.allow_admin_api = False
+        # We should still be able to access public operations.
+        req = webob.Request.blank('/v1.0/flavors')
+        res = req.get_response(nova.api.API('os'))
+        self.assertEqual(res.status_int, 200)
+        # TODO: Confirm admin operations are unavailable.
+
+if __name__ == '__main__':
+    unittest.main()