yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/charms/precise/juju-gui/bug-1092515-certificates into lp:~juju-gui/charms/precise/juju-gui/trunk.

Requested reviews:
  Juju GUI Hackers (juju-gui)
Related bugs:
  Bug #1092515 in juju-gui: "Allow using an own SSL certificate and private key in the charm"
  https://bugs.launchpad.net/juju-gui/+bug/1092515

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

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/

-- 
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])