Merge lp:~blake-rouse/maas/fix-1583670 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Allow get-config to work as a standard user.

[Gavin Panella (allenap)]

There are a few config items that should not be exposed to ordinary
users:

- omapi_key
- rpc_region_certificate (unused right now)
- rpc_shared_secret

TBH, I'm not sure we want to share these with admins either.

It might be prudent for MaasHandler to use a whitelist to limit read and
write access to a subset of all config keys, i.e. configuration is not
readable or writable by default.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Uhm... did you test the branch?

That is already handled and yes administrators cannot access those either.

[Gavin Panella (allenap)]

> Uhm... did you test the branch?

There are no tests for this. I've written some: lp:~allenap/maas/blake-rouse--fix-1583670

>
> That is already handled and yes administrators cannot access those either.

You're correct, but without tests it doesn't count :)

In writing the tests I discovered that "commissioning_osystem" is not available to configure via the Web API but it ought to be.

[Blake Rouse (blake-rouse)]

I merged your branch.

[Gavin Panella (allenap)]

Looks good then :)

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/fix-1583670 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [94.5 kB]
Get:3 http://security.ubuntu.com/ubuntu xenial-security InRelease [94.5 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Fetched 189 kB in 0s (442 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+dfsg-11...

=== modified file 'src/maasserver/api/maas.py'
--- src/maasserver/api/maas.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/maas.py	2016-06-14 18:49:36 +0000
@@ -12,6 +12,7 @@
 from django.http import HttpResponse
 from formencode import validators
 from maasserver.api.support import (
+    admin_method,
     operation,
     OperationsHandler,
 )
@@ -31,6 +32,7 @@
     api_doc_section_name = "MAAS server"
     create = read = update = delete = None
 
+    @admin_method
     @operation(idempotent=False)
     def set_config(self, request):
         """Set a config value.

=== modified file 'src/maasserver/api/tests/test_api.py'
--- src/maasserver/api/tests/test_api.py	2016-05-24 22:05:45 +0000
+++ src/maasserver/api/tests/test_api.py	2016-06-14 18:49:36 +0000
@@ -298,19 +298,19 @@
 class MAASAPIAnonTest(APITestCase.ForAnonymous):
     """The MAAS' handler is not accessible to anon users."""
 
-    def test_anon_get_config_forbidden(self):
+    def test_anon_get_config_unauthorized(self):
         response = self.client.get(
             reverse('maas_handler'),
             {'op': 'get_config'})
 
-        self.assertEqual(http.client.FORBIDDEN, response.status_code)
+        self.assertEqual(http.client.UNAUTHORIZED, response.status_code)
 
-    def test_anon_set_config_forbidden(self):
+    def test_anon_set_config_unauthorized(self):
         response = self.client.post(
             reverse('maas_handler'),
             {'op': 'set_config'})
 
-        self.assertEqual(http.client.FORBIDDEN, response.status_code)
+        self.assertEqual(http.client.UNAUTHORIZED, response.status_code)
 
 
 class MAASAPIVersioningTest(APITestCase.ForAnonymousAndUserAndAdmin):
@@ -343,13 +343,6 @@
         self.assertEqual(
             '/api/2.0/maas/', reverse('maas_handler'))
 
-    def test_simple_user_get_config_forbidden(self):
-        response = self.client.get(
-            reverse('maas_handler'),
-            {'op': 'get_config'})
-
-        self.assertEqual(http.client.FORBIDDEN, response.status_code)
-
     def test_simple_user_set_config_forbidden(self):
         response = self.client.post(
             reverse('maas_handler'),
@@ -358,7 +351,6 @@
         self.assertEqual(http.client.FORBIDDEN, response.status_code)
 
     def test_get_config_requires_name_param(self):
-        self.become_admin()
         response = self.client.get(
             reverse('maas_handler'),
             {
@@ -369,7 +361,6 @@
         self.assertEqual(b"No provided name!", response.content)
 
     def test_get_config_returns_config(self):
-        self.become_admin()
         name = 'maas_name'
         value = factory.make_string()
         Config.objects.set_config(name, value)
@@ -386,7 +377,6 @@
         self.assertEqual(value, parsed_result)
 
     def test_get_config_rejects_unknown_config_item(self):
-        self.become_admin()
         name = factory.make_string()
         value = factory.make_string()
         Config.objects.set_config(name, value)

=== modified file 'src/maasserver/api/tests/test_maas.py'
--- src/maasserver/api/tests/test_maas.py	2016-05-24 21:29:53 +0000
+++ src/maasserver/api/tests/test_maas.py	2016-06-14 18:49:36 +0000
@@ -6,10 +6,16 @@
 __all__ = []
 
 import http.client
+import json
+from operator import itemgetter
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
-from maasserver.models.config import Config
+from maasserver.forms_settings import CONFIG_ITEMS_KEYS
+from maasserver.models.config import (
+    Config,
+    DEFAULT_CONFIG,
+)
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from maasserver.testing.osystems import (
@@ -17,12 +23,38 @@
     make_usable_osystem,
     patch_usable_osystems,
 )
+from maastesting.matchers import DocTestMatches
+from maastesting.testcase import MAASTestCase
+from testtools.content import text_content
+from testtools.matchers import (
+    AfterPreprocessing,
+    Equals,
+    MatchesAll,
+    MatchesDict,
+    MatchesListwise,
+    MatchesStructure,
+)
+
+# Names forbidden for use via the Web API.
+FORBIDDEN_NAMES = {
+    "omapi_key", "rpc_region_certificate",
+    "rpc_shared_secret", "commissioning_osystem",
+}
+
+
+class TestForbiddenNames(MAASTestCase):
+
+    def test_forbidden_names(self):
+        # The difference between the set of possible configuration keys and
+        # those permitted via the Web API is small but important to security.
+        self.assertThat(
+            set(DEFAULT_CONFIG).difference(CONFIG_ITEMS_KEYS),
+            Equals(FORBIDDEN_NAMES))
 
 
 class MAASHandlerAPITest(APITestCase.ForUser):
 
     def test_get_config_default_distro_series(self):
-        self.become_admin()
         default_distro_series = factory.make_name("distro_series")
         Config.objects.set_config(
             "default_distro_series", default_distro_series)
@@ -69,3 +101,48 @@
             })
         self.assertEqual(
             http.client.BAD_REQUEST, response.status_code, response.content)
+
+    def assertInvalidConfigurationSetting(self, name, response):
+        self.addDetail(
+            "Response for op={get,set}_config&name=%s" % name, text_content(
+                response.serialize().decode(settings.DEFAULT_CHARSET)))
+        self.expectThat(
+            response, MatchesAll(
+                # An HTTP 400 response,
+                MatchesStructure(
+                    status_code=Equals(http.client.BAD_REQUEST)),
+                # with a JSON body,
+                AfterPreprocessing(
+                    itemgetter("Content-Type"),
+                    Equals("application/json")),
+                # containing a serialised ValidationError.
+                AfterPreprocessing(
+                    lambda response: json.loads(
+                        response.content.decode(settings.DEFAULT_CHARSET)),
+                    MatchesDict({
+                        name: MatchesListwise([
+                            DocTestMatches(
+                                name + " is not a valid config setting "
+                                "(valid settings are: ...)."),
+                        ]),
+                    })),
+                first_only=True,
+            ))
+
+    def test_get_config_forbidden_config_items(self):
+        for name in FORBIDDEN_NAMES:
+            response = self.client.get(
+                reverse('maas_handler'), {
+                    "op": "get_config", "name": name,
+                })
+            self.assertInvalidConfigurationSetting(name, response)
+
+    def test_set_config_forbidden_config_items(self):
+        self.become_admin()
+        for name in FORBIDDEN_NAMES:
+            response = self.client.post(
+                reverse('maas_handler'), {
+                    "op": "set_config", "name": name,
+                    "value": factory.make_name("nonsense"),
+                })
+            self.assertInvalidConfigurationSetting(name, response)

=== modified file 'src/maasserver/tests/test_forms_config.py'
--- src/maasserver/tests/test_forms_config.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/tests/test_forms_config.py	2016-06-14 18:49:36 +0000
@@ -70,6 +70,9 @@
     def test_form_loads_initial_values_from_default_value(self):
         value = factory.make_string()
         DEFAULT_CONFIG['field1'] = value
+        # Remove the added config from the DEFAULT_CONFIG or it will
+        # break other tests.
+        self.addCleanup(lambda: DEFAULT_CONFIG.pop('field1'))
         form = TestOptionForm()
 
         self.assertItemsEqual(['field1'], form.initial)

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2016-05-23 09:16:07 +0000
+++ src/maasserver/urls_api.py	2016-06-14 18:49:36 +0000
@@ -166,6 +166,7 @@
 )
 
 
+maas_handler = RestrictedResource(MaasHandler, authentication=api_auth)
 account_handler = RestrictedResource(AccountHandler, authentication=api_auth)
 boot_resource_handler = RestrictedResource(
     BootResourceHandler, authentication=api_auth)
@@ -266,7 +267,6 @@
 
 
 # Admin handlers.
-maas_handler = AdminRestrictedResource(MaasHandler, authentication=api_auth)
 commissioning_script_handler = AdminRestrictedResource(
     CommissioningScriptHandler, authentication=api_auth)
 commissioning_scripts_handler = AdminRestrictedResource(
@@ -296,6 +296,7 @@
 # API URLs for logged-in users.
 urlpatterns += patterns(
     '',
+    url(r'^maas/$', maas_handler, name='maas_handler'),
     url(r'^nodes/(?P<system_id>[^/]+)/blockdevices/$',
         blockdevices_handler, name='blockdevices_handler'),
     url(r'^nodes/(?P<system_id>[^/]+)/blockdevices/(?P<device_id>[^/]+)/$',
@@ -458,7 +459,6 @@
 # API URLs for admin users.
 urlpatterns += patterns(
     '',
-    url(r'^maas/$', maas_handler, name='maas_handler'),
     url(
         r'^commissioning-scripts/$', commissioning_scripts_handler,
         name='commissioning_scripts_handler'),