Merge lp:~teknico/charms/precise/juju-gui/encrypt-api-env-connection into lp:~juju-gui/charms/precise/juju-gui/trunk

[Nicola Larosa]

Setup encrypted conn. to the API environment

Pass the same certificate and private key used by nginx to the
API environment, so that the websocket connection can use WSS.

https://codereview.appspot.com/7007045/

[Nicola Larosa (teknico)]

Reviewers: mp+141107_code.launchpad.net,

Message:
Please take a look.

Description:
Setup encrypted conn. to the API environment

Pass the same certificate and private key used by nginx to the
API environment, so that the websocket connection can use WSS.

This sets the code up, but HTTPS is still disabled, and WSS too.
To test this you need to enable HTTPS in config/nginx.conf.template,
WSS in config/config.js.template, and expose the 443 port in
hooks/start.

Also, this is not yet working while deploying manually, and needs
further testing. Do not land without checking first.

https://code.launchpad.net/~teknico/charms/precise/juju-gui/encrypt-api-env-connection/+merge/141107

(do not edit description out of merge proposal)

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

Affected files:
   M HACKING.md
   A [revision details]
   M config.yaml
   M config/juju-api-agent.conf.template
   M config/juju-api-improv.conf.template
   M config/nginx.conf.template
   M hooks/start
   M hooks/utils.py
   M revision
   M tests/test_utils.py

Index: HACKING.md
=== modified file 'HACKING.md'
--- HACKING.md 2012-12-19 15:27:53 +0000
+++ HACKING.md 2012-12-21 18:06:41 +0000
@@ -114,7 +114,7 @@
  this (again, assuming you have set up your repo the way the functional
tests
  need them, as described above).

- juju deploy --repository=/path/to/charm/repo local:precise/juju-gui
+ juju deploy --repository=/path/to/charm/repo --upgrade
local:precise/juju-gui
      juju expose juju-gui

  Now you are working with a test run, as described in

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: config.yaml
=== modified file 'config.yaml'
--- config.yaml 2012-12-20 14:56:29 +0000
+++ config.yaml 2012-12-21 18:06:41 +0000
@@ -50,4 +50,4 @@
      description: |
        The path to the directory where the SSL certificates are stored.
      type: string
- default: /etc/ssl/private/juju-gui
+ default: /etc/ssl/private/juju-gui/

Index: config/juju-api-agent.conf.template
=== modified file 'config/juju-api-agent.conf.template'
--- config/juju-api-agent.conf.template 2012-11-29 13:23:28 +0000
+++ config/juju-api-agent.conf.template 2012-12-21 15:12:03 +0000
@@ -10,4 +10,5 @@
  # Use --nodaemon so that upstart can correctly retrieve the process ID.
  exec /usr/bin/python -m juju.agents.api --nodaemon --port %(port)s \
      --logfile /var/log/juju/api-agent.log \
- --session-file /var/run/juju/api-agent.zksession
+ --session-file /var/run/juju/api-agent.zksession \
+ --keys %(keys)s

Index: config/juju-api-improv.conf.template
=== modified file 'config/juju-api-improv.conf.template'
--- config/juju-api-improv.conf.template 2012-12-03 10:02:45 +0000
+++ config/juju-api-improv.conf.template 2012-12-21 15:12:03 +0000
@@ -8,4 +8,5 @@
  env PYTHONPATH=%(juju_dir)s:$PYTHONPATH

  exec /usr/bin/python %(juju_dir)s/improv.py --port %(port)s \
- -f %...

[Nicola Larosa (teknico)]

*** Submitted:

Setup encrypted conn. to the API environment

Pass the same certificate and private key used by nginx to the
API environment, so that the websocket connection can use WSS.

R=
CC=
https://codereview.appspot.com/7007045

https://codereview.appspot.com/7007045/

[Nicola Larosa (teknico)]

Please take a look.

https://codereview.appspot.com/7007045/

[Francesco Banconi (frankban)]

Land with changes.

This branch looks good Nicola: it's nice to see everything's working
with TLS enabled. Tests pass.
Please take a look at my comments below: especially the config-changed
issue needs to be addressed IMHO.

https://codereview.appspot.com/7007045/diff/6002/HACKING.md
File HACKING.md (right):

https://codereview.appspot.com/7007045/diff/6002/HACKING.md#newcode128
HACKING.md:128: /var/lib/juju/units/[NAME OF UNIT]. There is a
charm.log file to investigate,
Good catch, thank you.

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed
File hooks/config-changed (right):

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed#newcode88
hooks/config-changed:88: start_improv(
The improv and the api agent services now depend on the certificates
path, and need to be restarted if the ssl-cert-path is changed. AFAICT,
currently this is not handled here. E.g., if the user run ``juju set
juju-gui ssl-cert-path=/tmp/juju-gui``, the services are not restarted
and the GUI ends up in a broken state.

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed#newcode106
hooks/config-changed:106: start_agent(
See above.

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py#newcode59
hooks/utils.py:59: SSL_CERT_PATH = '/etc/ssl/juju-gui'
Here the config default is redefined, and this value is used as default
for the functions below needing the tls keys path.
Is this really required? I believe that the default value is never used,
i.e. each time we call start_improv, start_agent etc., we always pass
the value taken from config. Moreover, it seems to me that if, in the
future, we want to change the default path, we will need to change the
same value in two different places.

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py#newcode339
hooks/utils.py:339: crt_path = os.path.join(ssl_cert_path, 'juju.crt')
I like how you renamed the keys.

https://codereview.appspot.com/7007045/

[Nicola Larosa (teknico)]

Thanks for the review, very useful.

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed
File hooks/config-changed (right):

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed#newcode88
hooks/config-changed:88: start_improv(
frankban wrote:
> The improv and the api agent services now depend on the certificates
> path, and need to be restarted if the ssl-cert-path is changed.
> AFAICT, currently this is not handled here.

You are right, I will add ssl-cert-path to the checked properties.

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed#newcode106
hooks/config-changed:106: start_agent(
frankban wrote:
> See above.

Ditto.

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py#newcode59
hooks/utils.py:59: SSL_CERT_PATH = '/etc/ssl/juju-gui'
frankban wrote:
> I believe that the default value is never used, i.e. each time we call
> start_improv, start_agent etc., we always pass the value taken from
config.

Indeed. I foolishly unified the defaults into the constant, forgetting
that the value comes from the config anyway, and the defaults do not
really make sense. I'll remove them.

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py#newcode339
hooks/utils.py:339: crt_path = os.path.join(ssl_cert_path, 'juju.crt')
frankban wrote:
> I like how you renamed the keys.

Well, I had to. :-) The --keys Juju option only passes the certificates
directory, not the filenames: they are hardwired.

https://codereview.appspot.com/7007045/

[Nicola Larosa (teknico)]

Please take a look.

https://codereview.appspot.com/7007045/

[Nicola Larosa (teknico)]

Please take a look.

https://codereview.appspot.com/7007045/

[Gary Poster (gary)]

Land as is.

Thanks, Nicola. Looks very good to me.

Gary

https://codereview.appspot.com/7007045/

[Nicola Larosa (teknico)]

*** Submitted:

Setup encrypted conn. to the API environment

Pass the same certificate and private key used by nginx to the
API environment, so that the websocket connection can use WSS.

R=frankban, gary.poster
CC=
https://codereview.appspot.com/7007045

https://codereview.appspot.com/7007045/

=== modified file 'HACKING.md'
--- HACKING.md	2012-12-19 15:27:53 +0000
+++ HACKING.md	2013-01-04 14:51:21 +0000
@@ -114,7 +114,7 @@
 this (again, assuming you have set up your repo the way the functional tests
 need them, as described above).
 
-    juju deploy --repository=/path/to/charm/repo local:precise/juju-gui
+    juju deploy --repository=/path/to/charm/repo --upgrade local:precise/juju-gui
     juju expose juju-gui
 
 Now you are working with a test run, as described in
@@ -125,7 +125,7 @@
 When something goes wrong, on your local machine run
 `juju debug-hooks juju-gui/0` or similar.  This will initially put you on the
 unit that has the problem.  You can look at what is going on in
-/var/log/juju/units/[NAME OF UNIT].  There is a charm.log file to investigate,
+/var/lib/juju/units/[NAME OF UNIT].  There is a charm.log file to investigate,
 and a charm directory which contains the charm.  The charm directory contains
 the juju-gui and juju directories, so everything you need is there.
 

=== modified file 'config.yaml'
--- config.yaml	2012-12-21 15:05:06 +0000
+++ config.yaml	2013-01-04 14:51:21 +0000
@@ -27,7 +27,7 @@
       Connect the Juju GUI to the staging backend
       (i.e. a simulated Juju environment).
     type: boolean
-    default: False
+    default: false
   staging-environment:
     description: |
       The environment JSON export used by the staging server. This option can
@@ -50,7 +50,7 @@
     description: |
       The path to the directory where the SSL certificates are stored.
     type: string
-    default: /etc/ssl/private/juju-gui
+    default: /etc/ssl/juju-gui
   ssl-cert-contents:
     description: |
       The contents of the certificate file to be used in SSL connections to

=== modified file 'config/config.js.template'
--- config/config.js.template	2013-01-02 13:31:03 +0000
+++ config/config.js.template	2013-01-04 14:51:21 +0000
@@ -6,5 +6,5 @@
   viewContainer: '#main',
   transitions: false,
   charm_store_url: 'https://jujucharms.com/',
-  socket_url: 'ws://%(address)s:%(port)s/ws'
+  socket_url: 'wss://%(address)s:%(port)s/ws'
 };

=== modified file 'config/juju-api-agent.conf.template'
--- config/juju-api-agent.conf.template	2012-11-29 13:23:28 +0000
+++ config/juju-api-agent.conf.template	2013-01-04 14:51:21 +0000
@@ -10,4 +10,5 @@
 # Use --nodaemon so that upstart can correctly retrieve the process ID.
 exec /usr/bin/python -m juju.agents.api --nodaemon --port %(port)s \
     --logfile /var/log/juju/api-agent.log \
-    --session-file /var/run/juju/api-agent.zksession
+    --session-file /var/run/juju/api-agent.zksession \
+    --secure --keys %(keys)s

=== modified file 'config/juju-api-improv.conf.template'
--- config/juju-api-improv.conf.template	2012-12-03 10:02:45 +0000
+++ config/juju-api-improv.conf.template	2013-01-04 14:51:21 +0000
@@ -8,4 +8,5 @@
 env PYTHONPATH=%(juju_dir)s:$PYTHONPATH
 
 exec /usr/bin/python %(juju_dir)s/improv.py --port %(port)s \
-    -f %(juju_dir)s/%(staging_env)s.json
+    -f %(juju_dir)s/%(staging_env)s.json \
+    --secure --keys %(keys)s

=== modified file 'config/nginx.conf.template'
--- config/nginx.conf.template	2013-01-02 13:31:03 +0000
+++ config/nginx.conf.template	2013-01-04 14:51:21 +0000
@@ -9,8 +9,8 @@
     server_name _;
     root %(server_root)s;
     index index.html;
-    ssl_certificate %(ssl_cert_path)s/server.pem;
-    ssl_certificate_key %(ssl_cert_path)s/server.key;
+    ssl_certificate %(ssl_cert_path)s/juju.crt;
+    ssl_certificate_key %(ssl_cert_path)s/juju.key;
 
     # Serve static assets.
     location ^~ /juju-ui/ {

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-12-21 16:32:53 +0000
+++ hooks/config-changed	2013-01-04 14:51:21 +0000
@@ -41,7 +41,7 @@
 
     added_or_changed = diff.added_or_changed
     juju_api_port = config.get('juju-api-port')
-    staging = config.get('staging')
+    in_staging = config.get('staging')
 
     # The juju_gui_source_changed and juju_api_branch_changed variables
     # control whether we restart the GUI and the API, respectively, at the
@@ -62,17 +62,18 @@
     # Handle changes to SSL certificates.
     ssl_properties = set(
         ['ssl-cert-path', 'ssl-cert-contents', 'ssl-key-contents'])
-    if added_or_changed & ssl_properties:
+    ssl_changed = added_or_changed & ssl_properties
+    if ssl_changed:
         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:
+    if in_staging:
         staging_properties = set(
             ['staging', 'staging-environment', 'juju-api-port'])
         staging_changed = added_or_changed & staging_properties
-        if staging_changed or juju_api_branch_changed:
+        if staging_changed or ssl_changed or juju_api_branch_changed:
             if 'staging' in added_or_changed:
                 # 'staging' went from False to True, so the agent server is
                 # running and must be stopped.
@@ -85,11 +86,12 @@
             service_control(current_api, STOP)
             # Now the improv server can be cleanly started.
             log('Starting or restarting staging.')
-            start_improv(juju_api_port, config.get('staging-environment'))
+            start_improv(juju_api_port, config.get('staging-environment'),
+                         config['ssl-cert-path'])
     else:
         agent_properties = set(['juju-api-port', 'staging'])
         agent_changed = added_or_changed & agent_properties
-        if agent_changed or juju_api_branch_changed:
+        if agent_changed or ssl_changed or juju_api_branch_changed:
             if 'staging' in added_or_changed:
                 # If 'staging' transitions to False we need to stop the backend
                 # and start the agent.
@@ -100,19 +102,18 @@
                 current_api = AGENT
             service_control(current_api, STOP)
             log('Starting or restarting Juju API agent.')
-            start_agent(juju_api_port)
+            start_agent(juju_api_port, config['ssl-cert-path'])
 
     # Handle changes to the juju-gui configuration.
     gui_properties = set(
         ['juju-gui-console-enabled', 'juju-api-port', 'staging'])
     gui_changed = added_or_changed & gui_properties
-    ssl_cert_path_changed = 'ssl-cert-path' in added_or_changed
-    if gui_changed or juju_gui_source_changed or ssl_cert_path_changed:
+    if gui_changed or ssl_changed or juju_gui_source_changed:
         with su('root'):
             service_control(GUI, STOP)
         console_enabled = config.get('juju-gui-console-enabled')
         ssl_cert_path = config['ssl-cert-path']
-        start_gui(juju_api_port, console_enabled, staging, ssl_cert_path)
+        start_gui(juju_api_port, console_enabled, in_staging, ssl_cert_path)
 
 
 def main():

=== modified file 'hooks/start'
--- hooks/start	2013-01-02 13:31:03 +0000
+++ hooks/start	2013-01-04 14:51:21 +0000
@@ -34,9 +34,10 @@
         juju_api_port, config['juju-gui-console-enabled'], staging,
         config['ssl-cert-path'])
     if staging:
-        start_improv(juju_api_port, config['staging-environment'])
+        start_improv(juju_api_port, config['staging-environment'],
+                     config['ssl-cert-path'])
     else:
-        start_agent(juju_api_port)
+        start_agent(juju_api_port, config['ssl-cert-path'])
     open_ports(juju_api_port)
 
 

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2012-12-21 15:22:50 +0000
+++ hooks/utils.py	2013-01-04 14:51:21 +0000
@@ -178,7 +178,7 @@
     results_log.info('\n' + results)
 
 
-def start_improv(juju_api_port, staging_env,
+def start_improv(juju_api_port, staging_env, ssl_cert_path,
                  config_path='/etc/init/juju-api-improv.conf'):
     """Start a simulated juju environment using ``improv.py``."""
     log('Setting up staging start up script.')
@@ -186,6 +186,7 @@
         'juju_dir': JUJU_DIR,
         'port': juju_api_port,
         'staging_env': staging_env,
+        'keys': ssl_cert_path,
     }
     render_to_file('juju-api-improv.conf.template', context, config_path)
     log('Starting the staging backend.')
@@ -193,7 +194,8 @@
         service_control(IMPROV, START)
 
 
-def start_agent(juju_api_port, config_path='/etc/init/juju-api-agent.conf'):
+def start_agent(juju_api_port, ssl_cert_path,
+                config_path='/etc/init/juju-api-agent.conf'):
     """Start the Juju agent and connect to the current environment."""
     # Retrieve the Zookeeper address from the start up script.
     unit_dir = os.path.realpath(os.path.join(CURRENT_DIR, '..'))
@@ -204,6 +206,7 @@
         'juju_dir': JUJU_DIR,
         'port': juju_api_port,
         'zookeeper': zookeeper,
+        'keys': ssl_cert_path,
     }
     render_to_file('juju-api-agent.conf.template', context, config_path)
     log('Starting API agent.')
@@ -211,7 +214,7 @@
         service_control(AGENT, START)
 
 
-def start_gui(juju_api_port, console_enabled, staging, ssl_cert_path,
+def start_gui(juju_api_port, console_enabled, in_staging, ssl_cert_path,
               config_path='/etc/init/juju-gui.conf',
               nginx_path=JUJU_GUI_SITE,
               config_js_path=None):
@@ -219,7 +222,7 @@
     with su('root'):
         run('chown', '-R', 'ubuntu:', JUJU_GUI_DIR)
     build_dir = JUJU_GUI_DIR + '/build-'
-    build_dir += 'debug' if staging else 'prod'
+    build_dir += 'debug' if in_staging else 'prod'
     log('Setting up Juju GUI start up script.')
     render_to_file('juju-gui.conf.template', {}, config_path)
     log('Generating the Juju GUI configuration file.')
@@ -244,12 +247,12 @@
         service_control(GUI, START)
 
 
-def stop(staging):
+def stop(in_staging):
     """Stop the Juju API agent."""
     with su('root'):
         log('Stopping Juju GUI.')
         service_control(GUI, STOP)
-        if staging:
+        if in_staging:
             log('Stopping the staging backend.')
             service_control(IMPROV, STOP)
         else:
@@ -330,13 +333,13 @@
     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')
+    crt_path = os.path.join(ssl_cert_path, 'juju.crt')
+    key_path = os.path.join(ssl_cert_path, 'juju.key')
     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:
+        with open(crt_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)
@@ -348,4 +351,4 @@
             '-days', '365', '-nodes', '-x509', '-subj',
             # These are arbitrary test values for the certificate.
             '/C=GB/ST=Juju/L=GUI/O=Ubuntu/CN=juju.ubuntu.com',
-            '-keyout', key_path, '-out', pem_path))
+            '-keyout', key_path, '-out', crt_path))

=== modified file 'revision'
--- revision	2012-12-20 10:52:39 +0000
+++ revision	2013-01-04 14:51:21 +0000
@@ -1,1 +1,1 @@
-17
+18

=== modified file 'tests/deploy.test'
--- tests/deploy.test	2013-01-02 13:31:03 +0000
+++ tests/deploy.test	2013-01-04 14:51:21 +0000
@@ -56,7 +56,7 @@
         url = 'https://{0}:{1}'.format(hostname, self.port)
         response = open_url(url)
         self.assertEqual(200, response.getcode())
-        ws_url = 'http://{0}:{1}/ws'.format(hostname, ws_port)
+        ws_url = 'https://{0}:{1}/ws'.format(hostname, ws_port)
         # A bad request status code here means the websocket resource is found.
         # It would take an actual websocket client to properly interact with
         # the websocket server.
@@ -104,7 +104,7 @@
 
     def test_staging(self):
         # Ensure the Juju GUI and improv services are correctly set up.
-        config = {self.charm: {'staging': 'True'}}
+        config = {self.charm: {'staging': 'true'}}
         config_file = make_charm_config_file(config)
         hostname = self.deploy(config_path=config_file.name)
         # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2012-12-21 16:46:20 +0000
+++ tests/test_utils.py	2013-01-04 14:51:21 +0000
@@ -295,8 +295,8 @@
         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')
+        self.cert_file = os.path.join(self.cert_path, 'juju.crt')
+        self.key_file = os.path.join(self.cert_path, 'juju.key')
 
     def test_generation(self):
         """Ensure certificates are correctly generated."""
@@ -372,6 +372,7 @@
 
         self.destination_file = tempfile.NamedTemporaryFile()
         self.addCleanup(self.destination_file.close)
+        self.ssl_cert_path = 'ssl/cert/path'
 
     def tearDown(self):
         # Undo all of the monkey patching.
@@ -382,20 +383,23 @@
     def test_start_improv(self):
         port = '1234'
         staging_env = 'large'
-        start_improv(port, staging_env, self.destination_file.name)
+        start_improv(port, staging_env, self.ssl_cert_path,
+            self.destination_file.name)
         conf = self.destination_file.read()
         self.assertTrue('--port %s' % port in conf)
         self.assertTrue(staging_env + '.json' in conf)
+        self.assertTrue(self.ssl_cert_path in conf)
         self.assertEqual(self.svc_ctl_call_count, 1)
         self.assertEqual(self.service_names, ['juju-api-improv'])
         self.assertEqual(self.actions, [charmhelpers.START])
 
     def test_start_agent(self):
         port = '1234'
-        start_agent(port, self.destination_file.name)
+        start_agent(port, self.ssl_cert_path, self.destination_file.name)
         conf = self.destination_file.read()
         self.assertTrue('--port %s' % port in conf)
         self.assertTrue('JUJU_ZOOKEEPER=%s' % self.fake_zk_address in conf)
+        self.assertTrue(self.ssl_cert_path in conf)
         self.assertEqual(self.svc_ctl_call_count, 1)
         self.assertEqual(self.service_names, ['juju-api-agent'])
         self.assertEqual(self.actions, [charmhelpers.START])
@@ -413,8 +417,8 @@
         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.assertIn('/tmp/certificates/juju.crt', nginx_conf)
+        self.assertIn('/tmp/certificates/juju.key', nginx_conf)
         self.assertEqual(self.svc_ctl_call_count, 1)
         self.assertEqual(self.service_names, ['juju-gui'])
         self.assertEqual(self.actions, [charmhelpers.START])