environs: split bootstrap certificate/key

environs: split bootstrap certificate/key

It's more consistent with the way we treat cert/key
pairs elsewhere.

https://code.launchpad.net/~rogpeppe/juju-core/163-bootstrap-cert-and-key/+merge/135785

Requires: https://code.launchpad.net/~rogpeppe/juju-core/161-move-bootstrap/+merge/135784

(do not edit description out of merge proposal)

[rog, 2012-11-22 22:09:40]

Please take a look.

[gz, 2012-11-23 17:38:32]

I wonder if a better convention for passing around the PEM wouldn't be a custom
struct/interface that included methods for getting one or other or both when
needed, but this seems to be pretty much what the cert functions in bootstrap.go
have already and you've decided passing around just the serialised form is more
appropriate.

https://codereview.appspot.com/6782103/diff/6001/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/6782103/diff/6001/environs/bootstrap_test.go#n...
environs/bootstrap_test.go:283: e.stateServerPEM = []byte(string(certPEM) +
string(keyPEM))
This is the best convention for concating two byte arrays?

[jameinel, 2012-11-25 11:24:17]

I would second the idea of having a struct that has '.Cert' and '.Key' methods
or attributes. However, this seems a logical one-step-forward sort of change to
me.

 LGTM

https://codereview.appspot.com/6782103/diff/6001/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/6782103/diff/6001/environs/bootstrap_test.go#n...
environs/bootstrap_test.go:283: e.stateServerPEM = []byte(string(certPEM) +
string(keyPEM))
On 2012/11/23 17:38:32, gz wrote:
> This is the best convention for concating two byte arrays?

I *think* the issue is that doing just append() would add one to the other one.
And the alternatives are a bit verbose. I think that:

e.stateServerPEM = make([]byte, 0, len(certPEM) + len(keyPEM))
e.stateServerPEM = append(e.stateServerPEM, certPEM..., keyPEM...)

or something like that would work. (You might not have to supply ..., or you
might have to supply it but only one at a time, etc.)

I don't think go has 'extend', I think the foo = append(foo, bar...) is
equivalent to Python's foo.extend(bar)

[rog, 2012-11-25 12:19:42]

On 23 November 2012 17:38,  <martin.packman@canonical.com> wrote:
> I wonder if a better convention for passing around the PEM wouldn't be a
> custom struct/interface that included methods for getting one or other
> or both when needed, but this seems to be pretty much what the cert
> functions in bootstrap.go have already and you've decided passing around
> just the serialised form is more appropriate.

I've wondered about that too. There are three possibilities for passing
around a certificate: *x509.Certificate, the DER-encoded raw bytes
and the PEM data. I've gone for the PEM form because it's less tied
to our implementation - we should be able to change the underlying
model (for example to move away from x509, or to allow a chain of
more than one certificate) without changing most of the logic that
deals with it.

It's less efficient, and not as type safe, but to be honest many of the
places that we use it are reading it or writing it, so keeping
it in serialised format makes reasonable sense I think.
That said, we could easily define our own type if we thought that
was better.

In a upcoming branch, I've moved the low level certificate
handling code into a new package, launchpad.net/juju-core/cert.
This has the particular advantage that we can use the same logic
for generating the testing cert/key as we use when actually bootstrapping,
rather that storing a pre-generated key in testing/cert.go.

I've currently managed to get all internal tests now passing using TLS.
Not *quite* there on the live tests though...

> https://codereview.appspot.com/6782103/diff/6001/environs/bootstrap_test.go
> File environs/bootstrap_test.go (right):
>
>
https://codereview.appspot.com/6782103/diff/6001/environs/bootstrap_test.go#n...
> environs/bootstrap_test.go:283: e.stateServerPEM =
> []byte(string(certPEM) + string(keyPEM))
> This is the best convention for concating two byte arrays?

It depends. If you know that len(certPEM) == cap(certPEM)
or you don't mind side-effecting certPEM, then append(certPEM, keyPEM...)
works. In this case, we just go for simplest brute force solution
that avoids side-effects - this line is going away very soon,
as we're moving to splitting cert from key throughout.

[rog, 2012-11-25 12:33:53]

On 25 November 2012 11:24,  <john.meinel@canonical.com> wrote:
> I would second the idea of having a struct that has '.Cert' and '.Key'
> methods or attributes. However, this seems a logical one-step-forward
> sort of change to me.

I'm not sure how much this would simplify things. We we most often pass
the CA certificate around without its private key, and the config can
return either cert or cert and key. It feels like I've been thrashing around
quite a bit in the config domain for a while, so perhaps we should
let it settle for a bit and see how it works out. In the end, we end up
with a situation where not much code actually needs to know about
the certificates. (see lp:~rogpeppe/juju-core/170-use-tls)

If we did pass decide to go this way, we'd probably standardise on
just passing around a *tls.Certificate, which has both cert and key.

> I *think* the issue is that doing just append() would add one to the
> other one. And the alternatives are a bit verbose. I think that:
>
> e.stateServerPEM = make([]byte, 0, len(certPEM) + len(keyPEM))
> e.stateServerPEM = append(e.stateServerPEM, certPEM..., keyPEM...)

that doesn't quite work. (you can only pass a single "..." slice to a function)
you'd need two appends.

or, in one line (and slightly less efficient):

e.stateServerPEM = append(append([]byte(nil), certPEM...), keyPEM...)

> I don't think go has 'extend', I think the foo = append(foo, bar...) is
> equivalent to Python's foo.extend(bar)

yup.

[TheMue, 2012-11-26 14:59:29]

LGTM, only one small note.

> I'm not sure how much this would simplify things. We we most often pass
> the CA certificate around without its private key, and the config can
> return either cert or cert and key. It feels like I've been thrashing around
> quite a bit in the config domain for a while, so perhaps we should
> let it settle for a bit and see how it works out. In the end, we end up
> with a situation where not much code actually needs to know about
> the certificates. (see lp:~rogpeppe/juju-core/170-use-tls)

+1

I'm not sure how append() works in Detail, but AFAIK 

e.stateServerPEM = append(certPEM, keyPEM...)

does not touch certPEM, so it should be enough.

[rog, 2012-11-26 16:38:31]

*** Submitted:

environs: split bootstrap certificate/key

It's more consistent with the way we treat cert/key
pairs elsewhere.

R=gz, john.meinel, TheMue
CC=
https://codereview.appspot.com/6782103