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

Proposed by Raphaël Badin
Status: Merged
Approved by: Raphaël Badin
Approved revision: no longer in the source branch.
Merged at revision: 1213
Proposed branch: lp:~rvb/maas/ssl-skip-check
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 125 lines (+46/-10)
3 files modified
src/maascli/api.py (+23/-7)
src/maascli/cli.py (+5/-1)
src/maascli/tests/test_api.py (+18/-2)
To merge this branch: bzr merge lp:~rvb/maas/ssl-skip-check
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+128389@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

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
Revision history for this message
Raphaël Badin (rvb) 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.

> 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'.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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.

Revision history for this message
MAAS Lander (maas-lander) wrote :

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

text conflict in src/maascli/api.py

Revision history for this message
MAAS Lander (maas-lander) wrote :

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

text conflict in src/maascli/api.py

Revision history for this message
Raphaël Badin (rvb) wrote :

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'.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maascli/api.py'
2--- src/maascli/api.py 2012-10-08 06:30:25 +0000
3+++ src/maascli/api.py 2012-10-08 10:29:19 +0000
4@@ -41,12 +41,24 @@
5 )
6
7
8-def fetch_api_description(url):
9+def http_request(url, method, body=None, headers=None,
10+ insecure=False):
11+ """Issue an http request."""
12+ http = httplib2.Http(
13+ disable_ssl_certificate_validation=insecure)
14+ try:
15+ return http.request(url, method, body=body, headers=headers)
16+ except httplib2.SSLHandshakeError:
17+ raise CommandError(
18+ "Certificate verification failed, use --insecure/-k to "
19+ "disable the certificate check.")
20+
21+
22+def fetch_api_description(url, insecure=False):
23 """Obtain the description of remote API given its base URL."""
24 url_describe = urljoin(url, "describe/")
25- http = httplib2.Http()
26- response, content = http.request(
27- ascii_url(url_describe), "GET")
28+ response, content = http_request(
29+ ascii_url(url_describe), "GET", insecure=insecure)
30 if response.status != httplib.OK:
31 raise CommandError(
32 "{0.status} {0.reason}:\n{1}".format(response, content))
33@@ -129,6 +141,9 @@
34 parser.add_argument(
35 "-d", "--debug", action="store_true", default=False,
36 help="Display more information about API responses.")
37+ parser.add_argument(
38+ '-k', '--insecure', action='store_true', help=(
39+ "Disable SSL certificate check"), default=False)
40
41 def __call__(self, options):
42 # TODO: this is el-cheapo URI Template
43@@ -148,9 +163,10 @@
44 # on urllib2) so that we get full control over HTTP method. TODO:
45 # create custom MAASDispatcher to use httplib2 so that MAASClient can
46 # be used.
47- http = httplib2.Http()
48- response, content = http.request(
49- uri, self.method, body=body, headers=headers)
50+ insecure = options.insecure
51+ response, content = http_request(
52+ uri, self.method, body=body, headers=headers,
53+ insecure=insecure)
54
55 # Output.
56 if options.debug:
57
58=== modified file 'src/maascli/cli.py'
59--- src/maascli/cli.py 2012-10-08 06:30:25 +0000
60+++ src/maascli/cli.py 2012-10-08 10:29:19 +0000
61@@ -52,6 +52,9 @@
62 "a long random-looking string composed of three parts, "
63 "separated by colons."
64 ))
65+ parser.add_argument(
66+ '-k', '--insecure', action='store_true', help=(
67+ "Disable SSL certificate check"), default=False)
68 parser.set_defaults(credentials=None)
69
70 def __call__(self, options):
71@@ -61,7 +64,8 @@
72 # Normalise the remote service's URL.
73 url = ensure_trailing_slash(options.url)
74 # Get description of remote API.
75- description = fetch_api_description(url)
76+ insecure = options.insecure
77+ description = fetch_api_description(url, insecure)
78 # Save the config.
79 profile_name = options.profile_name
80 with ProfileConfig.open() as config:
81
82=== modified file 'src/maascli/tests/test_api.py'
83--- src/maascli/tests/test_api.py 2012-10-08 06:30:25 +0000
84+++ src/maascli/tests/test_api.py 2012-10-08 10:29:19 +0000
85@@ -26,7 +26,10 @@
86 from maascli.config import ProfileConfig
87 from maastesting.factory import factory
88 from maastesting.testcase import TestCase
89-from mock import sentinel
90+from mock import (
91+ Mock,
92+ sentinel,
93+ )
94 from testtools.matchers import (
95 Equals,
96 IsInstance,
97@@ -93,7 +96,8 @@
98 self.assertEqual(
99 content, api.fetch_api_description("http://example.com/api/1.0/"))
100 request.assert_called_once_with(
101- b"http://example.com/api/1.0/describe/", "GET")
102+ b"http://example.com/api/1.0/describe/", "GET", body=None,
103+ headers=None)
104
105 def test_fetch_api_description_not_okay(self):
106 # If the response is not 200 OK, fetch_api_description throws toys.
107@@ -159,6 +163,18 @@
108 " Two-Two: %(two-two)s\n") % headers,
109 buf.getvalue())
110
111+ def test_http_request_raises_error_if_cert_verify_fails(self):
112+ self.patch(
113+ httplib2.Http, "request",
114+ Mock(side_effect=httplib2.SSLHandshakeError()))
115+ error = self.assertRaises(
116+ CommandError, api.http_request, factory.make_name('fake_url'),
117+ factory.make_name('fake_method'))
118+ error_expected = (
119+ "Certificate verification failed, use --insecure/-k to "
120+ "disable the certificate check.")
121+ self.assertEqual(error_expected, "%s" % error)
122+
123
124 class TestIsResponseTextual(TestCase):
125 """Tests for `is_response_textual`."""