Serve the GUI assets over HTTPS

Serve the GUI assets over HTTPS

Generate and install a passphrase-less SSL certificate and private
key, and configure nginx to use it to serve the GUI via HTTPS.

https://code.launchpad.net/~teknico/charms/precise/juju-gui/serve-via-https/+merge/140511

(do not edit description out of merge proposal)

[teknico, 2012-12-18 18:04:55]

Please take a look.

[gary.poster, 2012-12-18 18:16:40]

Hey Nicola.  I'm going to try out the charm now, but I had a few preliminary
comments from the code alone.

Beyond what I mention in-line, and as I mentioned on IRC, I think you need to
expose port 443 now, rather than 80.

I'll give the charm a try now and report back.

https://codereview.appspot.com/6940084/diff/1/README.md
File README.md (right):

https://codereview.appspot.com/6940084/diff/1/README.md#newcode60
README.md:60: - 80/tcp
I assume this is 443 now?

https://codereview.appspot.com/6940084/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/6940084/diff/1/hooks/utils.py#newcode227
hooks/utils.py:227: if not (os.path.exists(pem_path) and
os.path.exists(pem_path)):
I suspect you meant key_path for one of those.

If only one of the files is there, is it still the right behavior to create both
files, overwriting them?  I don't know the answer, just asking.

[teknico, 2012-12-18 18:19:05]

Please take a look.

[teknico, 2012-12-18 18:23:41]

Also fixed the expose port, thanks.

https://codereview.appspot.com/6940084/diff/1/README.md
File README.md (right):

https://codereview.appspot.com/6940084/diff/1/README.md#newcode60
README.md:60: - 80/tcp
gary.poster wrote:
> I assume this is 443 now?

Indeed, fixed.

https://codereview.appspot.com/6940084/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/6940084/diff/1/hooks/utils.py#newcode227
hooks/utils.py:227: if not (os.path.exists(pem_path) and
os.path.exists(pem_path)):
gary.poster wrote:
> I suspect you meant key_path for one of those.

Indeed, good catch.

> If only one of the files is there, is it still the right behavior
> to create both files, overwriting them?  I don't know the answer,
> just asking.

I don't really know. I guess better safe than sorry.

[teknico, 2012-12-18 18:27:32]

Please take a look.

[teknico, 2012-12-18 19:05:47]

Please take a look.

[gary.poster, 2012-12-19 03:55:52]

Land with changes.

Hi Nicola.  Thanks for the fixes.  This still wasn't working, but I got it to
work--I visited an https URL for the ec2 site and it worked fine.

I hope what I did is a help.  I'd like all the changes I request to be made in
order for this to land, with the possible exception of the NO_BZR change if that
really annoys you.  The NO_BZR change is not exactly related to the other work
you are doing, but we want it so I slipped it in.  Francesco will want it for
running "make distfile" in his branch.

The most important changes are the semicolons in the nginx template, and the
key/cert generation bits that must not require user interaction.  If you don't
want to use the approach I found for key/cert generation, here are instructions
on getting creating a key without a user prompt:
http://stackoverflow.com/questions/4294689/how-to-generate-a-key-with-passphr...
.  You can then use the -subj option as I show it in the pastebin to make the
openssl req command not require user interaction.  However, I can confirm that
what I have works.

Please verify somehow that the tests work now, also.  I was going to do that for
you, but I'm about ready to go to bed now. :-)

Thanks again,

Gary

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template#n...
config/nginx.conf.template:1: server {
As we discussed, please add a redirect from port 80 to port 443, and then
re-expose port 80 in the docs and code (in addition to port 443).

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template#n...
config/nginx.conf.template:6: ssl_certificate
/etc/ssl/private/juju-gui/server.pem
You need to terminate this with a semicolon.

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template#n...
config/nginx.conf.template:7: ssl_certificate_key
/etc/ssl/private/juju-gui/server.key
This one needs a semicolon too.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode25
hooks/utils.py:25: command,
Please also import environ...

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode208
hooks/utils.py:208: with cd('juju-gui'):
Please precede or follow this with this line:

with environ(NO_BZR='1'):

This is a speed optimization in our Makefile from Matt that will be nice to
have.

See http://pastebin.ubuntu.com/1449174/ for an example.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode228
hooks/utils.py:228: os.makedirs(ssl_cert_path)
To make debugging easier, I suggest only creating the directory if it does not
exist.  See http://pastebin.ubuntu.com/1449174/ for example.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode230
hooks/utils.py:230: cmd_log(run('openssl', 'genrsa', '-des3', '-out', key_path,
'1024'))
This is interactive, and fails.

See http://pastebin.ubuntu.com/1449174/ for an approach that eliminates this
problem and seems simpler.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode233
hooks/utils.py:233: cmd_log(run('openssl', 'req', '-new', '-key', key_path,
'-out',
This is interactive, and fails.

See http://pastebin.ubuntu.com/1449174/ for an approach that eliminates this
problem and seems simpler.

[teknico, 2012-12-19 11:50:10]

Please take a look.

[teknico, 2012-12-19 11:54:38]

Uhm. It looks like the OpenSSl commands in the nginx are suboptimal, or
obsolete, or both. I like the conciseness in the superuser.com solution: I
applied that one, and the other changes you suggested.

I will test this all better manually before landing. Thanks.

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template#n...
config/nginx.conf.template:1: server {
On 2012/12/19 03:55:52, gary.poster wrote:
> As we discussed, please add a redirect from port 80 to port 443, and then
> re-expose port 80 in the docs and code (in addition to port 443).

Done.

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template#n...
config/nginx.conf.template:6: ssl_certificate
/etc/ssl/private/juju-gui/server.pem
On 2012/12/19 03:55:52, gary.poster wrote:
> You need to terminate this with a semicolon.

Done.

https://codereview.appspot.com/6940084/diff/6009/config/nginx.conf.template#n...
config/nginx.conf.template:7: ssl_certificate_key
/etc/ssl/private/juju-gui/server.key
On 2012/12/19 03:55:52, gary.poster wrote:
> This one needs a semicolon too.

Done.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode25
hooks/utils.py:25: command,
On 2012/12/19 03:55:52, gary.poster wrote:
> Please also import environ...

Done.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode228
hooks/utils.py:228: os.makedirs(ssl_cert_path)
On 2012/12/19 03:55:52, gary.poster wrote:
> To make debugging easier, I suggest only creating the directory if it does not
> exist.  See http://pastebin.ubuntu.com/1449174/ for example.

Done.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode230
hooks/utils.py:230: cmd_log(run('openssl', 'genrsa', '-des3', '-out', key_path,
'1024'))
On 2012/12/19 03:55:52, gary.poster wrote:
> This is interactive, and fails.
> 
> See http://pastebin.ubuntu.com/1449174/ for an approach that eliminates this
> problem and seems simpler.

Done.

https://codereview.appspot.com/6940084/diff/6009/hooks/utils.py#newcode233
hooks/utils.py:233: cmd_log(run('openssl', 'req', '-new', '-key', key_path,
'-out',
On 2012/12/19 03:55:52, gary.poster wrote:
> This is interactive, and fails.
> 
> See http://pastebin.ubuntu.com/1449174/ for an approach that eliminates this
> problem and seems simpler.

Done.

[gary.poster, 2012-12-19 13:25:53]

Land as is.

Thanks Nicola.  Looks good.

I have one important thought for a future branch that I ramble on about below.

Gary

https://codereview.appspot.com/6940084/diff/13001/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/6940084/diff/13001/hooks/utils.py#newcode227
hooks/utils.py:227: pem_path = os.path.join(ssl_cert_path, 'server.pem')
It strikes me that we need the ability to specify the contents of these files
(server.pem and server.key) from the charm.  From the experience with the
buildbot charms, the only way to do this in a charm is to specify the entire
file contents in a string (doing this in a YAML configuration file is the
easiest way to do that).  This means that using the feature will be a bit
annoying, but on the bright side, it should be easy to implement.  Simply accept
two strings which are the contents of server.pem and server.key, respectively. 
If these are provided, put the content in the expected file locations and move
on.  Otherwise (if neither or only one are provided), do what we have here.

If only one of the two is provided, I think it would be appropriate to log that
we are ignoring it because we need both values.  This will mean that both values
must be provided at once, but all three mechanisms to set values support this
(juju deploy --config, juju set, and juju set --config).

I'm fine with this being done in a separate card.  While it takes so long to
spin up a charm to test in manually and automatically, maybe it makes sense to
combine this change with the one that turns on the secure websocket (which needs
to be able to be turned off, btw, for as long as uistage is served on http!).

[benji, 2012-12-19 14:10:24]

Looks good.  I had one small question that was mostly for my own edification.

https://codereview.appspot.com/6940084/diff/13001/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6940084/diff/13001/config/nginx.conf.template#...
config/nginx.conf.template:4: return 301 https://$host$request_uri;
I assume $request_uri has a leading slash and that is why we do not need one
between $host and $request_uri.  Is that right?

[teknico, 2012-12-19 16:32:46]

https://codereview.appspot.com/6940084/diff/13001/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6940084/diff/13001/config/nginx.conf.template#...
config/nginx.conf.template:4: return 301 https://$host$request_uri;
benji wrote:
> I assume $request_uri has a leading slash and that is why we do
> not need one between $host and $request_uri.  Is that right?

That's the suggested incantation, I guess so.

[gary.poster, 2012-12-20 00:00:09]

Land with changes.

I got the tests to pass, with the one change described below. Please make that
change. 

Also, please make a card to add the feature to support supplying a pem and key
string, as we discussed and as I mentioned in the previous review; and make a
kanban card for that.

Thanks!

Gary

https://codereview.appspot.com/6940084/diff/13001/tests/deploy.test
File tests/deploy.test (right):

https://codereview.appspot.com/6940084/diff/13001/tests/deploy.test#newcode56
tests/deploy.test:56: url = 'http://{0}:{1}'.format(hostname, self.port)
This needs to be https in order for the tests to pass.

[gary.poster, 2012-12-20 03:16:24]

On 2012/12/20 00:00:09, gary.poster wrote:
> Land with changes.
> 
> I got the tests to pass, with the one change described below. Please make that
> change. 
> 
> Also, please make a card to add the feature to support supplying a pem and key
> string, as we discussed and as I mentioned in the previous review; and make a
> kanban card for that.
> 
> Thanks!
> 
> Gary
> 
> https://codereview.appspot.com/6940084/diff/13001/tests/deploy.test
> File tests/deploy.test (right):
> 
> https://codereview.appspot.com/6940084/diff/13001/tests/deploy.test#newcode56
> tests/deploy.test:56: url = 'http://{0}:{1}'.format(hostname, self.port)
> This needs to be https in order for the tests to pass.

One more thing: please don't forget to increment the value in the version file

[teknico, 2012-12-20 11:26:30]

Please take a look.

[teknico, 2012-12-20 11:27:50]

gary.poster wrote:
> Please make a card to add the feature to support supplying a pem
> and key string, as we discussed and as I mentioned in the previous
> review; and make a kanban card for that.

Done.

> https://codereview.appspot.com/6940084/diff/13001/tests/deploy.test#newcode56
> tests/deploy.test:56: url = 'http://{0}:{1}'.format(hostname, self.port)
> This needs to be https in order for the tests to pass.

Done.

> One more thing: please don't forget to increment the value in the version file

Done.

[teknico, 2012-12-20 11:52:25]

*** Submitted:

Serve the GUI assets over HTTPS

Generate and install a passphrase-less SSL certificate and private
key, and configure nginx to use it to serve the GUI via HTTPS.

R=gary.poster, benji
CC=
https://codereview.appspot.com/6940084