Merge lp:~jacekn/charm-helpers/charm-helpers-ssl into lp:charm-helpers

Proposed by Jacek Nykis
Status: Merged
Merged at revision: 76
Proposed branch: lp:~jacekn/charm-helpers/charm-helpers-ssl
Merge into: lp:charm-helpers
Diff against target: 151 lines (+139/-0)
2 files modified
charmhelpers/contrib/ssl/__init__.py (+79/-0)
tests/contrib/ssl/test_ssl.py (+60/-0)
To merge this branch: bzr merge lp:~jacekn/charm-helpers/charm-helpers-ssl
Reviewer Review Type Date Requested Status
Matthew Wedgwood (community) Approve
Review via email: mp+182365@code.launchpad.net

Description of the change

Added contrib.ssl module which can create selfsigned SSL certificates.

To post a comment you must log in.
Revision history for this message
Matthew Wedgwood (mew) wrote :

This looks quite helpful. Thanks.

I see a few issues here.

Running this as generate_selfsigned("mykey", "mycert") fails:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "charmhelpers/contrib/ssl/__init__.py", line 19, in generate_selfsigned
    subprocess.check_call(cmd)
UnboundLocalError: local variable 'cmd' referenced before assignment

There should be a test that catches this.

Furthermore, the "subject" argument is a little opaque. I somewhat expected to be able to pass it a hostname, but that's not at all what's needed. I'd recommend any combination of:

1. Adding a docstring with a little explanation
2. Exposing the subject components as function args (rather than as one dict)

As a bonus, it might be nice to allow users to specify the number of bits the for the cert as 1024 will inevitably fall out of vogue.
3. Building a simple "CertificateSubject" class to help users construct a valid subject
4. Providing reasonable defaults.

review: Needs Fixing
75. By Jacek Nykis

Added docstring to the generate_selfsigned function. Improved the function to accept single cn argument and partial subject dict

Revision history for this message
Matthew Wedgwood (mew) wrote :

I'm able to get the new function succeed using the "cn" parameter, but it explodes[1] when I try to use "subject." I've tried that one with various permutations on the keys included in the dict. I haven't tested the "config" parameter.

Also, I notice you're returning False for failures, but when the function actually succeeds, it returns None. You might want an explicit "return True" at the end.

[1] http://pastebin.ubuntu.com/6034287/

review: Needs Fixing
76. By Jacek Nykis

Added more error checking to contrib.ssl.generate_selfsigned function

Revision history for this message
Jacek Nykis (jacekn) wrote :

I added more error checking and return True statement.

Your test is failing as expected, country code should be 2 letter code. With my changes this will be picked up by the try/except block I added but there is no way I can return exact reason for openssl command failure.

Revision history for this message
Matthew Wedgwood (mew) wrote :

Ah, that makes sense. With these changes, LGTM.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'charmhelpers/contrib/ssl'
2=== added file 'charmhelpers/contrib/ssl/__init__.py'
3--- charmhelpers/contrib/ssl/__init__.py 1970-01-01 00:00:00 +0000
4+++ charmhelpers/contrib/ssl/__init__.py 2013-08-28 09:12:15 +0000
5@@ -0,0 +1,79 @@
6+import subprocess
7+from charmhelpers.core import hookenv
8+
9+
10+def generate_selfsigned(keyfile, certfile, keysize="1024", config=None, subject=None, cn=None):
11+ """Generate selfsigned SSL keypair
12+
13+ You must provide one of the 3 optional arguments:
14+ config, subject or cn
15+ If more than one is provided the leftmost will be used
16+
17+ Arguments:
18+ keyfile -- (required) full path to the keyfile to be created
19+ certfile -- (required) full path to the certfile to be created
20+ keysize -- (optional) SSL key length
21+ config -- (optional) openssl configuration file
22+ subject -- (optional) dictionary with SSL subject variables
23+ cn -- (optional) cerfificate common name
24+
25+ Required keys in subject dict:
26+ cn -- Common name (eq. FQDN)
27+
28+ Optional keys in subject dict
29+ country -- Country Name (2 letter code)
30+ state -- State or Province Name (full name)
31+ locality -- Locality Name (eg, city)
32+ organization -- Organization Name (eg, company)
33+ organizational_unit -- Organizational Unit Name (eg, section)
34+ email -- Email Address
35+ """
36+
37+ cmd = []
38+ if config:
39+ cmd = ["/usr/bin/openssl", "req", "-new", "-newkey",
40+ "rsa:{}".format(keysize), "-days", "365", "-nodes", "-x509",
41+ "-keyout", keyfile,
42+ "-out", certfile, "-config", config]
43+ elif subject:
44+ ssl_subject = ""
45+ if "country" in subject:
46+ ssl_subject = ssl_subject + "/C={}".format(subject["country"])
47+ if "state" in subject:
48+ ssl_subject = ssl_subject + "/ST={}".format(subject["state"])
49+ if "locality" in subject:
50+ ssl_subject = ssl_subject + "/L={}".format(subject["locality"])
51+ if "organization" in subject:
52+ ssl_subject = ssl_subject + "/O={}".format(subject["organization"])
53+ if "organizational_unit" in subject:
54+ ssl_subject = ssl_subject + "/OU={}".format(subject["organizational_unit"])
55+ if "cn" in subject:
56+ ssl_subject = ssl_subject + "/CN={}".format(subject["cn"])
57+ else:
58+ hookenv.log("When using \"subject\" argument you must " \
59+ "provide \"cn\" field at very least")
60+ return False
61+ if "email" in subject:
62+ ssl_subject = ssl_subject + "/emailAddress={}".format(subject["email"])
63+
64+ cmd = ["/usr/bin/openssl", "req", "-new", "-newkey",
65+ "rsa:{}".format(keysize), "-days", "365", "-nodes", "-x509",
66+ "-keyout", keyfile,
67+ "-out", certfile, "-subj", ssl_subject]
68+ elif cn:
69+ cmd = ["/usr/bin/openssl", "req", "-new", "-newkey",
70+ "rsa:{}".format(keysize), "-days", "365", "-nodes", "-x509",
71+ "-keyout", keyfile,
72+ "-out", certfile, "-subj", "/CN={}".format(cn)]
73+
74+ if not cmd:
75+ hookenv.log("No config, subject or cn provided," \
76+ "unable to generate self signed SSL certificates")
77+ return False
78+ try:
79+ subprocess.check_call(cmd)
80+ return True
81+ except Exception as e:
82+ print "Execution of openssl command failed:\n{}".format(e)
83+ return False
84+
85
86=== added directory 'tests/contrib/ssl'
87=== added file 'tests/contrib/ssl/__init__.py'
88=== added file 'tests/contrib/ssl/test_ssl.py'
89--- tests/contrib/ssl/test_ssl.py 1970-01-01 00:00:00 +0000
90+++ tests/contrib/ssl/test_ssl.py 2013-08-28 09:12:15 +0000
91@@ -0,0 +1,60 @@
92+import subprocess
93+
94+from mock import patch, call, MagicMock
95+from testtools import TestCase
96+
97+from charmhelpers.contrib import ssl
98+
99+
100+class HelpersTest(TestCase):
101+ @patch('subprocess.check_call')
102+ def test_generate_selfsigned_dict(self, mock_call):
103+ subject = {"country": "UK",
104+ "locality": "my_locality",
105+ "state": "my_state",
106+ "organization": "my_organization",
107+ "organizational_unit": "my_unit",
108+ "cn": "mysite.example.com",
109+ "email": "me@example.com"
110+ }
111+
112+ ssl.generate_selfsigned("mykey.key", "mycert.crt", subject=subject)
113+ mock_call.assert_called_with(['/usr/bin/openssl', 'req', '-new',
114+ '-newkey', 'rsa:1024', '-days', '365',
115+ '-nodes', '-x509', '-keyout',
116+ 'mykey.key', '-out', 'mycert.crt',
117+ '-subj',
118+ '/C=UK/ST=my_state/L=my_locality'
119+ '/O=my_organization/OU=my_unit'
120+ '/CN=mysite.example.com'
121+ '/emailAddress=me@example.com']
122+ )
123+
124+ @patch('charmhelpers.core.hookenv.log')
125+ def test_generate_selfsigned_failure(self, mock_log):
126+ # This is NOT enough, functino requires cn key
127+ subject = {"country": "UK",
128+ "locality": "my_locality"}
129+
130+ result = ssl.generate_selfsigned("mykey.key", "mycert.crt", subject=subject)
131+ self.assertFalse(result)
132+
133+
134+ @patch('subprocess.check_call')
135+ def test_generate_selfsigned_file(self, mock_call):
136+ ssl.generate_selfsigned("mykey.key", "mycert.crt", config="test.cnf")
137+ mock_call.assert_called_with(['/usr/bin/openssl', 'req', '-new',
138+ '-newkey', 'rsa:1024', '-days', '365',
139+ '-nodes', '-x509', '-keyout',
140+ 'mykey.key', '-out', 'mycert.crt',
141+ '-config', 'test.cnf'])
142+
143+
144+ @patch('subprocess.check_call')
145+ def test_generate_selfsigned_cn_key(self, mock_call):
146+ ssl.generate_selfsigned("mykey.key", "mycert.crt", keysize="2048", cn="mysite.example.com")
147+ mock_call.assert_called_with(['/usr/bin/openssl', 'req', '-new',
148+ '-newkey', 'rsa:2048', '-days', '365',
149+ '-nodes', '-x509', '-keyout',
150+ 'mykey.key', '-out', 'mycert.crt',
151+ '-subj', '/CN=mysite.example.com'])

Subscribers

People subscribed via source and target branches