Merge lp://staging/~dpb/charms/precise/apache2/avoid-regen-cert into lp://staging/charms/apache2

Hi David,

I'd like to review this submission but your branch doesn't merge cleanly with lp:charms/apache2. Would you mind pushing an update?

Thanks!
Tim

On Mon, Jun 16, 2014 at 04:02:42PM -0000, Tim Van Steenburgh wrote:
> Hi David,
>
> I'd like to review this submission but your branch doesn't merge
> cleanly with lp:charms/apache2. Would you mind pushing an update?
>

Hi Tim --

I resolved that conflict, it's ready for a re-review. (the merge
conflict was trivial, tests all still pass).

--
David Britton <email address hidden>

Thanks David!

+1 LGTM. And you get +1000 bonus points for adding unit tests. :D

A member of ~charmers will be along after me for a follow-up review and to push this change to the charm store.

Hello David,

Thanks for your submission. I understand your motivation, but i am unsure if we should force to keep the same certificate without having an configuration option to force the certificate re-generation.

My suggestions for your changes:

- Implement a configuration directive ( ssl_cert_regenerate ) to allow users to force the self-signed certificate generation. Which would be the first check to do.

- On the function is_selfsigned_cert_stale() Please check first if the private key has changed (available on config.get('ssl_keylocation') that will allow us to determine quicker is the certificate is stale.

Please Let me know your observations.

On Fri, Jun 20, 2014 at 4:25 PM, Jorge Niedbalski <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> Hello David,
>
> Thanks for your submission. I understand your motivation, but i am unsure
> if we should force to keep the same certificate without having an
> configuration option to force the certificate re-generation.
>
>
Hi Jorge --

Thanks for looking this over...

> My suggestions for your changes:
>
> - Implement a configuration directive ( ssl_cert_regenerate ) to allow
> users to force the self-signed certificate generation. Which would be the
> first check to do.
>
>
I'm not sure that would be a good investment here. This would be covered
in the existing charm (and in my change), by simply doing:

juju set ssl_certlocation="a_new_cert.cert"

SELFSIGNED is already mostly a testing/debugging feature. Having another
way to regenerate it with a config key seems a bit too much? What do you
think? If you still feel strongly, I think it's probably best left for an
add-on feature if someone feels the need to implement it.

> - On the function is_selfsigned_cert_stale() Please check first if the
> private key has changed (available on config.get('ssl_keylocation') that
> will allow us to determine quicker is the certificate is stale.
>

In the case of SELFSIGNED, the key is written by the charm to the
ssl_keylocation. Are you suggesting that the location of the key would
change by the admin through juju set? If so, I do check that. That case
will cause the self-signed cert to be regenerated.

Could you please explain further if I have misunderstood you?

>
>
> Diff comments:
>
> > +def is_selfsigned_cert_stale(config, cert_file, key_file):
> > + """
> > + Do we need to generate a new self-signed cert?
> > +
> > + @param config: charm data from config-get
> > + @param cert_file: destination path of generated certificate
> > + @param key_file: destination path of generated private key
> > + """
> > + # Basic Existence Checks
> > + if not os.path.exists(cert_file):
> > + return True
> > + if not os.path.exists(key_file):
> > + return True
> > +
> > + # Common Name
> > + from OpenSSL import crypto
> > + cert = crypto.load_certificate(
>
> If for whatever reason the load_certificate function fails, We should mark
> the certificate as stale, right? I think so.
>
>
If it failed -- threw an exception? returned None? -- it will actually
cause this method to error, and the hook, in turn, to error. There are a
couple of reasons this could fail, the ones I can think of seem appropriate
to allow the hook to error, instead of masking the problem, removing the
file (who knows what was there?), and generating another cert over the top
of it.

Presumably we generated the cert that we just failed to read, so I don't
think it would be helpful to continue generating certs like that.

Are you thinking of a particular use case or failure mode here? It would
help if I had something to test. I admit I may not be thinking of all the
options.

--
David Britton <email address hidden>

Thanks for your quick reply.

> I'm not sure that would be a good investment here. This would be covered
> in the existing charm (and in my change), by simply doing:
>
> juju set ssl_certlocation="a_new_cert.cert"
>
> SELFSIGNED is already mostly a testing/debugging feature. Having another
> way to regenerate it with a config key seems a bit too much? What do you
> think? If you still feel strongly, I think it's probably best left for an
> add-on feature if someone feels the need to implement it.
>

Charm's features must be made for being used in production, never for debugging/testing purposes, so i assumed by default that everything should be configurable and reversible.

Looking at the ssl_certlocation will cover the suggested case and having
something more specific would be a 'nice to have' feature. But for now, is OK to me to
use the `certlocation` directive.

>
> > - On the function is_selfsigned_cert_stale() Please check first if the
> > private key has changed (available on config.get('ssl_keylocation') that
> > will allow us to determine quicker is the certificate is stale.
> >
>
> In the case of SELFSIGNED, the key is written by the charm to the
> ssl_keylocation. Are you suggesting that the location of the key would
> change by the admin through juju set? If so, I do check that. That case
> will cause the self-signed cert to be regenerated.

Yes, that variable can be changed via `juju set`, so we must cover that
case on your re-generation constraints.

> If it failed -- threw an exception? returned None? -- it will actually
> cause this method to error, and the hook, in turn, to error. There are a
> couple of reasons this could fail, the ones I can think of seem appropriate
> to allow the hook to error, instead of masking the problem, removing the
> file (who knows what was there?), and generating another cert over the top
> of it.

I am certainly not aware of all the possible failures here, just making sure
that you know the potential errors that should be caught here. If you
think is OK to error the hook and not will not be necessary to flag the certificate as stale, i am OK with that.

On Mon, Jun 23, 2014 at 02:56:58PM -0000, Jorge Niedbalski wrote:
>
> Yes, that variable can be changed via `juju set`, so we must cover that
> case on your re-generation constraints.

I think we are good then:

    if not os.path.exists(cert_file):
        return True

That should catch the case you are talking about.

>
> I am certainly not aware of all the possible failures here, just making sure
> that you know the potential errors that should be caught here. If you
> think is OK to error the hook and not will not be necessary to flag the certificate as stale, i am OK with that.

Great -- Yes, I'm fine with that. These class of errors work best
making the charm error, IMO. I don't know why they would happen, and
it's dangerous to make assumptions about that.

Thanks!

--
David Britton <email address hidden>

LGTM