Merge lp:~frankban/charms/precise/juju-gui/bug-1092515-certificates into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

Allow using an own SSL cert and private key.

Added two new config options, one for the certificate, one
for the private key. If they are both provided, they are used
by nginx, otherwise, a new certificate is automatically
generated.

Fixed a pre-existent bug: even if you can specify the
directory where to store the certificates, this path
was not used by nginx, because an hardcoded one was
present in the configuration file.

Improved how ssl options are handled in config-changes.
If the SSL path is changed using 'juju set', now that
change is reflected in the nginx config file, and the
service correctly restarted.

Added tests for the process of saving or generating SSL
certificates.

Some code clean up.

Please note that all the SSL stuff is still
disabled/commented.

https://codereview.appspot.com/6976046/

[Francesco Banconi (frankban)]

Reviewers: mp+141105_code.launchpad.net,

Message:
Please take a look.

Description:
Allow using an own SSL cert and private key.

Added two new config options, one for the certificate, one
for the private key. If they are both provided, they are used
by nginx, otherwise, a new certificate is automatically
generated.

Fixed a pre-existent bug: even if you can specify the
directory where to store the certificates, this path
was not used by nginx, because an hardcoded one was
present in the configuration file.

Improved how ssl options are handled in config-changes.
If the SSL path is changed using 'juju set', now that
change is reflected in the nginx config file, and the
service correctly restarted.

Added tests for the process of saving or generating SSL
certificates.

Some code clean up.

Please note that all the SSL stuff is still
disabled/commented.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/bug-1092515-certificates/+merge/141105

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6976046/

Affected files:
   [revision details]
   config.yaml
   config/nginx.conf.template
   hooks/config-changed
   hooks/install
   hooks/start
   hooks/utils.py
   tests/test_utils.py

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/6976046/

[Nicola Larosa (teknico)]

Land as is.

Great job.

https://codereview.appspot.com/6976046/

[Madison Scott-Clary (makyo)]

Looks good to me, land as is.

Thanks for the changes.

https://codereview.appspot.com/6976046/

[Gary Poster (gary)]

I'm distracted that I can't run the tests successfully--part of the
problem was that a new d3 release broke our build, and I asked bac to
fix that with a better package spec--but this looks good and should
land. Thank you!

Gary

https://codereview.appspot.com/6976046/

[Francesco Banconi (frankban)]

*** Submitted:

Allow using an own SSL cert and private key.

Added two new config options, one for the certificate, one
for the private key. If they are both provided, they are used
by nginx, otherwise, a new certificate is automatically
generated.

Fixed a pre-existent bug: even if you can specify the
directory where to store the certificates, this path
was not used by nginx, because an hardcoded one was
present in the configuration file.

Improved how ssl options are handled in config-changes.
If the SSL path is changed using 'juju set', now that
change is reflected in the nginx config file, and the
service correctly restarted.

Added tests for the process of saving or generating SSL
certificates.

Some code clean up.

Please note that all the SSL stuff is still
disabled/commented.

R=teknico, matthew.scott, gary.poster
CC=
https://codereview.appspot.com/6976046

https://codereview.appspot.com/6976046/

[Kapil Thangavelu (hazmat)]

fwiw, jujucharms.com supports ssl

On Sat, Dec 22, 2012 at 4:55 AM, Francesco Banconi <
<email address hidden>> wrote:

> *** Submitted:
>
> Allow using an own SSL cert and private key.
>
> Added two new config options, one for the certificate, one
> for the private key. If they are both provided, they are used
> by nginx, otherwise, a new certificate is automatically
> generated.
>
> Fixed a pre-existent bug: even if you can specify the
> directory where to store the certificates, this path
> was not used by nginx, because an hardcoded one was
> present in the configuration file.
>
> Improved how ssl options are handled in config-changes.
> If the SSL path is changed using 'juju set', now that
> change is reflected in the nginx config file, and the
> service correctly restarted.
>
> Added tests for the process of saving or generating SSL
> certificates.
>
> Some code clean up.
>
> Please note that all the SSL stuff is still
> disabled/commented.
>
> R=teknico, matthew.scott, gary.poster
> CC=
> https://codereview.appspot.com/6976046
>
>
> https://codereview.appspot.com/6976046/
>
> --
>
> https://code.launchpad.net/~frankban/charms/precise/juju-gui/bug-1092515-certificates/+merge/141105
> Your team Juju GUI Hackers is requested to review the proposed merge of
> lp:~frankban/charms/precise/juju-gui/bug-1092515-certificates into
> lp:~juju-gui/charms/precise/juju-gui/trunk.
>

=== modified file 'config.yaml'
--- config.yaml	2012-12-20 14:56:29 +0000
+++ config.yaml	2012-12-21 17:47:23 +0000
@@ -51,3 +51,15 @@
       The path to the directory where the SSL certificates are stored.
     type: string
     default: /etc/ssl/private/juju-gui
+  ssl-cert-contents:
+    description: |
+      The contents of the certificate file to be used in SSL connections to
+      the GUI. Both ssl-cert-contents and ssl-key-contents must be provided.
+      If not, cetificates will be automatically generated.
+    type: string
+  ssl-key-contents:
+    description: |
+      The contents of the private key file to be used in SSL connections to
+      the GUI. Both ssl-cert-contents and ssl-key-contents must be provided.
+      If not, cetificates will be automatically generated.
+    type: string
 
=== modified file 'config/nginx.conf.template'
--- config/nginx.conf.template	2012-12-20 18:02:44 +0000
+++ config/nginx.conf.template	2012-12-21 17:47:23 +0000
@@ -13,8 +13,8 @@
     root %(server_root)s;
     index index.html;
     # Uncomment to switch back to TLS connections.
-    # ssl_certificate /etc/ssl/private/juju-gui/server.pem;
-    # ssl_certificate_key /etc/ssl/private/juju-gui/server.key;
+    # ssl_certificate %(ssl_cert_path)s/server.pem;
+    # ssl_certificate_key %(ssl_cert_path)s/server.key;
 
     # Serve static assets.
     location ^~ /juju-ui/ {
 
=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-12-20 14:56:29 +0000
+++ hooks/config-changed	2012-12-21 17:47:23 +0000
@@ -27,8 +27,8 @@
     fetch_gui,
     GUI,
     IMPROV,
+    save_or_create_certificates,
     setup_gui,
-    setup_nginx,
     start_agent,
     start_gui,
     start_improv,
@@ -55,11 +55,18 @@
         release_tarball = fetch_gui(
             config['juju-gui-source'], config['command-log-file'])
         setup_gui(release_tarball)
-        setup_nginx(config['ssl-cert-path'])
     if 'juju-api-branch' in added_or_changed:
         juju_api_branch_changed = True
         fetch_api(config['juju-api-branch'])
 
+    # Handle changes to SSL certificates.
+    ssl_properties = set(
+        ['ssl-cert-path', 'ssl-cert-contents', 'ssl-key-contents'])
+    if added_or_changed & ssl_properties:
+        save_or_create_certificates(
+            config['ssl-cert-path'], config.get('ssl-cert-contents'),
+            config.get('ssl-key-contents'))
+
     # Handle changes to the improv server configuration.
     if staging:
         staging_properties = set(
@@ -99,11 +106,13 @@
     gui_properties = set(
         ['juju-gui-console-enabled', 'juju-api-port', 'staging'])
     gui_changed = added_or_changed & gui_properties
-    if gui_changed or juju_gui_source_changed:
+    ssl_cert_path_changed = 'ssl-cert-path' in added_or_changed
+    if gui_changed or juju_gui_source_changed or ssl_cert_path_changed:
         with su('root'):
             service_control(GUI, STOP)
         console_enabled = config.get('juju-gui-console-enabled')
-        start_gui(juju_api_port, console_enabled, staging)
+        ssl_cert_path = config['ssl-cert-path']
+        start_gui(juju_api_port, console_enabled, staging, ssl_cert_path)
 
 
 def main():
 
=== modified file 'hooks/install'
--- hooks/install	2012-12-20 14:56:29 +0000
+++ hooks/install	2012-12-21 17:47:23 +0000
@@ -28,6 +28,7 @@
     config_json,
     fetch_api,
     fetch_gui,
+    save_or_create_certificates,
     setup_gui,
     setup_nginx,
 )
@@ -51,7 +52,10 @@
     release_tarball = fetch_gui(
         config['juju-gui-source'], config['command-log-file'])
     setup_gui(release_tarball)
-    setup_nginx(config['ssl-cert-path'])
+    setup_nginx()
+    save_or_create_certificates(
+        config['ssl-cert-path'], config.get('ssl-cert-contents'),
+        config.get('ssl-key-contents'))
     fetch_api(config['juju-api-branch'])
     config_json.set(config)
 
 
=== modified file 'hooks/start'
--- hooks/start	2012-12-20 18:02:44 +0000
+++ hooks/start	2012-12-21 17:47:23 +0000
@@ -31,7 +31,9 @@
     config = get_config()
     juju_api_port = config['juju-api-port']
     staging = config.get('staging')
-    start_gui(juju_api_port, config['juju-gui-console-enabled'], staging)
+    start_gui(
+        juju_api_port, config['juju-gui-console-enabled'], staging,
+        config['ssl-cert-path'])
     if staging:
         start_improv(juju_api_port, config['staging-environment'])
     else:
 
=== modified file 'hooks/utils.py'
--- hooks/utils.py	2012-12-20 14:56:29 +0000
+++ hooks/utils.py	2012-12-21 17:47:23 +0000
@@ -16,6 +16,7 @@
     'JUJU_GUI_DIR',
     'parse_source',
     'render_to_file',
+    'save_or_create_certificates',
     'setup_gui',
     'setup_nginx',
     'start_agent',
@@ -27,7 +28,6 @@
 import json
 import os
 import logging
-import shutil
 import tempfile
 
 from launchpadlib.launchpad import Launchpad
@@ -55,6 +55,7 @@
 CURRENT_DIR = os.getcwd()
 JUJU_DIR = os.path.join(CURRENT_DIR, 'juju')
 JUJU_GUI_DIR = os.path.join(CURRENT_DIR, 'juju-gui')
+JUJU_GUI_SITE = '/etc/nginx/sites-available/juju-gui'
 
 # Store the configuration from on invocation to the next.
 config_json = Serializer('/tmp/config.json')
@@ -210,9 +211,9 @@
         service_control(AGENT, START)
 
 
-def start_gui(juju_api_port, console_enabled, staging,
+def start_gui(juju_api_port, console_enabled, staging, ssl_cert_path,
               config_path='/etc/init/juju-gui.conf',
-              nginx_path='/etc/nginx/sites-available/juju-gui',
+              nginx_path=JUJU_GUI_SITE,
               config_js_path=None):
     """Set up and start the Juju GUI server."""
     with su('root'):
@@ -233,7 +234,8 @@
     render_to_file('config.js.template', context, config_js_path)
     log('Generating the nginx site configuration file.')
     context = {
-        'server_root': build_dir
+        'server_root': build_dir,
+        'ssl_cert_path': ssl_cert_path.rstrip('/'),
     }
     render_to_file('nginx.conf.template', context, nginx_path)
     log('Starting Juju GUI.')
@@ -307,25 +309,39 @@
     cmd_log(run('ln', '-sf', first_path_in_dir(release_dir), JUJU_GUI_DIR))
 
 
-def setup_nginx(ssl_cert_path):
+def setup_nginx():
     """Set up nginx."""
     log('Setting up nginx.')
     nginx_default_site = '/etc/nginx/sites-enabled/default'
-    juju_gui_site = '/etc/nginx/sites-available/juju-gui'
     if os.path.exists(nginx_default_site):
         os.remove(nginx_default_site)
-    if not os.path.exists(juju_gui_site):
-        cmd_log(run('touch', juju_gui_site))
-        cmd_log(run('chown', 'ubuntu:', juju_gui_site))
+    if not os.path.exists(JUJU_GUI_SITE):
+        cmd_log(run('touch', JUJU_GUI_SITE))
+        cmd_log(run('chown', 'ubuntu:', JUJU_GUI_SITE))
         cmd_log(
-            run('ln', '-s', juju_gui_site,
+            run('ln', '-s', JUJU_GUI_SITE,
                 '/etc/nginx/sites-enabled/juju-gui'))
-    # Generate the nginx SSL certificates, if needed.
+
+
+def save_or_create_certificates(
+        ssl_cert_path, ssl_cert_contents, ssl_key_contents):
+    """Generate the SSL certificates.
+
+    If both *ssl_cert_contents* and *ssl_key_contents* are provided, use them
+    as certificates; otherwise, generate them.
+    """
     pem_path = os.path.join(ssl_cert_path, 'server.pem')
     key_path = os.path.join(ssl_cert_path, 'server.key')
-    if not (os.path.exists(pem_path) and os.path.exists(key_path)):
-        if not os.path.exists(ssl_cert_path):
-            os.makedirs(ssl_cert_path)
+    if not os.path.exists(ssl_cert_path):
+        os.makedirs(ssl_cert_path)
+    if ssl_cert_contents and ssl_key_contents:
+        # Save the provided certificates.
+        with open(pem_path, 'w') as cert_file:
+            cert_file.write(ssl_cert_contents)
+        with open(key_path, 'w') as key_file:
+            key_file.write(ssl_key_contents)
+    else:
+        # Generate certificates.
         # See http://superuser.com/questions/226192/openssl-without-prompt
         cmd_log(run(
             'openssl', 'req', '-new', '-newkey', 'rsa:4096',
 
=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2012-12-20 13:27:30 +0000
+++ tests/test_utils.py	2012-12-21 17:47:23 +0000
@@ -16,6 +16,7 @@
     get_zookeeper_address,
     parse_source,
     render_to_file,
+    save_or_create_certificates,
     start_agent,
     start_gui,
     start_improv,
@@ -288,6 +289,29 @@
         self.assertEqual(expected, self.destination_file.read())
 
 
+class SaveOrCreateCertificatesTest(unittest.TestCase):
+
+    def setUp(self):
+        base_dir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, base_dir)
+        self.cert_path = os.path.join(base_dir, 'certificates')
+        self.cert_file = os.path.join(self.cert_path, 'server.pem')
+        self.key_file = os.path.join(self.cert_path, 'server.key')
+
+    def test_generation(self):
+        """Ensure certificates are correctly generated."""
+        save_or_create_certificates(
+            self.cert_path, 'some ignored contents', None)
+        self.assertIn('CERTIFICATE', open(self.cert_file).read())
+        self.assertIn('PRIVATE KEY', open(self.key_file).read())
+
+    def test_provided_certificates(self):
+        # Ensure files are correctly saved if their contents are provided.
+        save_or_create_certificates(self.cert_path, 'mycert', 'mykey')
+        self.assertIn('mycert', open(self.cert_file).read())
+        self.assertIn('mykey', open(self.key_file).read())
+
+
 class CmdLogTest(unittest.TestCase):
     def setUp(self):
         # Patch the charmhelpers 'command', which powers get_config.  The
@@ -382,12 +406,15 @@
         self.addCleanup(nginx_file.close)
         config_js_file = tempfile.NamedTemporaryFile()
         self.addCleanup(config_js_file.close)
-        start_gui(port, False, True, self.destination_file.name,
-                  nginx_file.name, config_js_file.name)
+        start_gui(
+            port, False, True, '/tmp/certificates/',
+            self.destination_file.name, nginx_file.name, config_js_file.name)
         conf = self.destination_file.read()
         self.assertTrue('/usr/sbin/nginx' in conf)
         nginx_conf = nginx_file.read()
         self.assertTrue('juju-gui/build-debug' in nginx_conf)
+        self.assertIn('/tmp/certificates/server.pem', nginx_conf)
+        self.assertIn('/tmp/certificates/server.key', nginx_conf)
         self.assertEqual(self.svc_ctl_call_count, 1)
         self.assertEqual(self.service_names, ['juju-gui'])
         self.assertEqual(self.actions, [charmhelpers.START])