launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/ssl-skip-check into lp:maas.

Commit message:
Make the cli raise a proper CommandError if the certificate verification failed.  Add an option to disable the certificate verification check.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1059970 in MAAS: "maas-cli fails with default certificate"
  https://bugs.launchpad.net/maas/+bug/1059970

For more details, see:
https://code.launchpad.net/~rvb/maas/ssl-skip-check/+merge/128389

This branch:
- makes the cli raise a proper CommandError if the certificate verification failed.
- adds an option to disable the certificate verification check.

= Notes =

Another solution would have been, when a certificate verification fails, to offer to the user an option to permanently skip the certificate verification (and store that information in the profile).  But this solution is simpler so I decided to go with it.  This can be improved later if we choose to do it.

This change is a bit light on the testing front… but so is this whole module.  This is another reason why I choose to implement a very simple solution, especially this close to the feature freeze deadline.

Drive-by fix: fix lint.
-- 
https://code.launchpad.net/~rvb/maas/ssl-skip-check/+merge/128389
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/ssl-skip-check into lp:maas.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-05 18:44:46 +0000
+++ src/maascli/api.py	2012-10-07 17:30:29 +0000
@@ -74,12 +74,24 @@
         return None
 
 
-def fetch_api_description(url):
+def http_request(url, method, body=None, headers=None,
+                 disable_cert_check=False):
+    """Issue an http request."""
+    http = httplib2.Http(
+        disable_ssl_certificate_validation=disable_cert_check)
+    try:
+        return http.request(ascii_url(url), "GET")
+    except httplib2.SSLHandshakeError:
+        raise CommandError(
+            "Certificate verify failed, use --disable-cert-check to disable "
+            "the certificate check.")
+
+
+def fetch_api_description(url, disable_cert_check=False):
     """Obtain the description of remote API given its base URL."""
     url_describe = urljoin(url, "describe/")
-    http = httplib2.Http()
-    response, content = http.request(
-        ascii_url(url_describe), "GET")
+    response, content = http_request(
+        ascii_url(url_describe), "GET", disable_cert_check=disable_cert_check)
     if response.status != httplib.OK:
         raise CommandError(
             "{0.status} {0.reason}:\n{1}".format(response, content))
@@ -160,6 +172,9 @@
                 "a long random-looking string composed of three parts, "
                 "separated by colons."
                 ))
+        parser.add_argument(
+            '--disable-cert-check', action='store_true', help=(
+                "Disable SSL certificate check"), default=False)
         parser.set_defaults(credentials=None)
 
     def __call__(self, options):
@@ -169,7 +184,8 @@
         # Normalise the remote service's URL.
         url = ensure_trailing_slash(options.url)
         # Get description of remote API.
-        description = fetch_api_description(url)
+        disable_cert_check = options.disable_cert_check
+        description = fetch_api_description(url, disable_cert_check)
         # Save the config.
         profile_name = options.profile_name
         with ProfileConfig.open() as config:
@@ -253,6 +269,9 @@
         parser.add_argument(
             "-d", "--debug", action="store_true", default=False,
             help="Display more information about API responses.")
+        parser.add_argument(
+            '--disable-cert-check', action='store_true', help=(
+                "Disable SSL certificate check"), default=False)
 
     def __call__(self, options):
         # TODO: this is el-cheapo URI Template
@@ -272,9 +291,10 @@
         # on urllib2) so that we get full control over HTTP method. TODO:
         # create custom MAASDispatcher to use httplib2 so that MAASClient can
         # be used.
-        http = httplib2.Http()
-        response, content = http.request(
-            uri, self.method, body=body, headers=headers)
+        disable_cert_check = options.disable_cert_check
+        response, content = http_request(
+             uri, self.method, body=body, headers=headers,
+             disable_cert_check=disable_cert_check)
 
         # Output.
         if options.debug:

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-10-05 09:41:33 +0000
+++ src/maascli/tests/test_api.py	2012-10-07 17:30:29 +0000
@@ -28,7 +28,10 @@
 from maascli.config import ProfileConfig
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
-from mock import sentinel
+from mock import (
+    Mock,
+    sentinel,
+    )
 from testtools.matchers import (
     Equals,
     IsInstance,
@@ -225,6 +228,18 @@
              "  Two-Two: %(two-two)s\n") % headers,
             buf.getvalue())
 
+    def test_http_request_raises_error_if_cert_verify_fails(self):
+        self.patch(
+            httplib2.Http, "request",
+            Mock(side_effect=httplib2.SSLHandshakeError()))
+        error = self.assertRaises(
+            CommandError, api.http_request, factory.make_name('fake_url'),
+            factory.make_name('fake_method'))
+        error_expected = (
+            "Certificate verify failed, use --disable-cert-check to disable "
+            "the certificate check.")
+        self.assertEqual(error_expected, "%s" % error)
+
 
 class TestIsResponseTextual(TestCase):
     """Tests for `is_response_textual`."""

=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py	2012-10-06 14:41:34 +0000
+++ src/maasserver/urls.py	2012-10-07 17:30:29 +0000
@@ -19,10 +19,7 @@
     url,
     )
 from django.contrib.auth.decorators import user_passes_test
-from django.views.generic.simple import (
-    direct_to_template,
-    redirect_to,
-    )
+from django.views.generic.simple import direct_to_template
 from maasserver.models import Node
 from maasserver.views.account import (
     login,