Merge lp:~rvb/maas/ssl-skip-check into lp:~maas-committers/maas/trunk

[Raphaël Badin]

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

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.

[Julian Edwards (julian-edwards)]

Looks good! Just a couple of small things:

16 + except httplib2.SSLHandshakeError:
17 + raise CommandError(
18 + "Certificate verify failed, use --disable-cert-check to disable "
19 + "the certificate check.")

s/verify/verification/

37 + parser.add_argument(
38 + '--disable-cert-check', action='store_true', help=(
39 + "Disable SSL certificate check"), default=False)

This is a long option and will get a bit annoying if someone needs to repeatedly specify it (e.g. on a dev system), can you make a short one too please, e.g. -d

Thanks!

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review!

> 16 + except httplib2.SSLHandshakeError:
> 17 + raise CommandError(
> 18 + "Certificate verify failed, use --disable-cert-check to disable "
> 19 + "the certificate check.")
>
> s/verify/verification/

I'll trust you on this one… but be aware that "Certificate verify failed" is a wording I took from the existing exception.

> 37 + parser.add_argument(
> 38 + '--disable-cert-check', action='store_true', help=(
> 39 + "Disable SSL certificate check"), default=False)
>
> This is a long option and will get a bit annoying if someone needs to
> repeatedly specify it (e.g. on a dev system), can you make a short one too
> please, e.g. -d

Good point. But '-d' is already taken (--debug) so I'll add '-di'.

[Julian Edwards (julian-edwards)]

On Monday 08 October 2012 07:28:22 you wrote:
> Thanks for the review!
>
> > 16 + except httplib2.SSLHandshakeError:
> > 17 + raise CommandError(
> > 18 + "Certificate verify failed, use --disable-cert-check to disable
> > "
> > 19 + "the certificate check.")
> >
> > s/verify/verification/
>
> I'll trust you on this one… but be aware that "Certificate verify failed" is
> a wording I took from the existing exception.

It's wrong then! Verify is a verb, and a noun is needed here. It's really
lazy English (mostly Americanised) to use a verb.

> > 37 + parser.add_argument(
> > 38 + '--disable-cert-check', action='store_true', help=(
> > 39 + "Disable SSL certificate check"), default=False)
> >
> > This is a long option and will get a bit annoying if someone needs to
> > repeatedly specify it (e.g. on a dev system), can you make a short one too
> > please, e.g. -d
>
> Good point. But '-d' is already taken (--debug) so I'll add '-di'.

Ah bugger, I was hoping a single letter opt could be used but I guess -du is
not so bad.

Cheers.

[MAAS Lander (maas-lander)]

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

text conflict in src/maascli/api.py

[MAAS Lander (maas-lander)]

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

text conflict in src/maascli/api.py

[Raphaël Badin (rvb)]

I'll follow Gavin's suggestion and use the same option names that curl uses: '-k/--insecure'. This avoids the confusing that using a double letter short option introduces: one might wonder if '-di' does not mean '-d' + '-i'.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-08 06:30:25 +0000
+++ src/maascli/api.py	2012-10-08 10:29:19 +0000
@@ -41,12 +41,24 @@
     )
 
 
-def fetch_api_description(url):
+def http_request(url, method, body=None, headers=None,
+                 insecure=False):
+    """Issue an http request."""
+    http = httplib2.Http(
+        disable_ssl_certificate_validation=insecure)
+    try:
+        return http.request(url, method, body=body, headers=headers)
+    except httplib2.SSLHandshakeError:
+        raise CommandError(
+            "Certificate verification failed, use --insecure/-k to "
+            "disable the certificate check.")
+
+
+def fetch_api_description(url, insecure=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", insecure=insecure)
     if response.status != httplib.OK:
         raise CommandError(
             "{0.status} {0.reason}:\n{1}".format(response, content))
@@ -129,6 +141,9 @@
         parser.add_argument(
             "-d", "--debug", action="store_true", default=False,
             help="Display more information about API responses.")
+        parser.add_argument(
+            '-k', '--insecure', action='store_true', help=(
+                "Disable SSL certificate check"), default=False)
 
     def __call__(self, options):
         # TODO: this is el-cheapo URI Template
@@ -148,9 +163,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)
+        insecure = options.insecure
+        response, content = http_request(
+             uri, self.method, body=body, headers=headers,
+             insecure=insecure)
 
         # Output.
         if options.debug:
 
=== modified file 'src/maascli/cli.py'
--- src/maascli/cli.py	2012-10-08 06:30:25 +0000
+++ src/maascli/cli.py	2012-10-08 10:29:19 +0000
@@ -52,6 +52,9 @@
                 "a long random-looking string composed of three parts, "
                 "separated by colons."
                 ))
+        parser.add_argument(
+            '-k', '--insecure', action='store_true', help=(
+                "Disable SSL certificate check"), default=False)
         parser.set_defaults(credentials=None)
 
     def __call__(self, options):
@@ -61,7 +64,8 @@
         # Normalise the remote service's URL.
         url = ensure_trailing_slash(options.url)
         # Get description of remote API.
-        description = fetch_api_description(url)
+        insecure = options.insecure
+        description = fetch_api_description(url, insecure)
         # Save the config.
         profile_name = options.profile_name
         with ProfileConfig.open() as config:
 
=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-10-08 06:30:25 +0000
+++ src/maascli/tests/test_api.py	2012-10-08 10:29:19 +0000
@@ -26,7 +26,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,
@@ -93,7 +96,8 @@
         self.assertEqual(
             content, api.fetch_api_description("http://example.com/api/1.0/"))
         request.assert_called_once_with(
-            b"http://example.com/api/1.0/describe/", "GET")
+            b"http://example.com/api/1.0/describe/", "GET", body=None,
+            headers=None)
 
     def test_fetch_api_description_not_okay(self):
         # If the response is not 200 OK, fetch_api_description throws toys.
@@ -159,6 +163,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 verification failed, use --insecure/-k to "
+            "disable the certificate check.")
+        self.assertEqual(error_expected, "%s" % error)
+
 
 class TestIsResponseTextual(TestCase):
     """Tests for `is_response_textual`."""