Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Initialize default x509 certificate and allow https host strings. #6831
Conversation
axw
reviewed
Jan 19, 2017
Looks fine in general, just a few things to tidy up. Thanks for working on this.
| @@ -23,5 +28,22 @@ func InitJujuXDGDataHome() error { | ||
| if err := ssh.LoadClientKeys(osenv.JujuXDGDataHomePath("ssh")); err != nil { | ||
| return errors.Annotate(err, "cannot load ssh client keys") | ||
| } | ||
| + expiry := time.Now().UTC().AddDate(10, 0, 0) |
axw
Jan 19, 2017
Member
Would you please move this code to a new function, and call it from this one? Also, please add a comment explaining the purpose of the certificate. i.e. that it can be used for client authentication, e.g. for LXD.
Pekkari
Jan 19, 2017
Contributor
I guess this comment is asking the whole lines in this file, not only this one, so assuming it, I'll do the change.
| - } | ||
| - } | ||
| - logger.Debugf("connecting to LXD remote %q: %q", remote.ID(), host) | ||
| + remote_id := remote.ID() |
| @@ -251,6 +242,46 @@ func newRawClient(remote Remote) (*lxd.Client, error) { | ||
| return client, nil | ||
| } | ||
| +// Generate host string from remote.ID and remote.Host | ||
| +func generateHostString(id string, host string) (string) { |
| +// Generate host string from remote.ID and remote.Host | ||
| +func generateHostString(id string, host string) (string) { | ||
| + if host != "" { | ||
| + is_localhost, _ := isLocalhost(host) |
axw
Jan 19, 2017
Member
I don't think there's a compelling reason to swallow the error. If InterfaceAddrs returns an error, something's probably very wrong. Can we please just bubble it up?
| + return host | ||
| +} | ||
| + | ||
| +// Check if the remote host is pointing to a local ip address configured in |
| + | ||
| +// Check if the remote host is pointing to a local ip address configured in | ||
| +// our interfaces. | ||
| +func isLocalhost(host string) (bool, error) { |
axw
Jan 19, 2017
Member
should this function be resolving host if it's a hostname, and not an IP address?
Pekkari
Jan 19, 2017
Contributor
if we plan to put the hostname instead of the ip address in the configuration of the cloud yes, but I guess we can live with this for now, and add the name resolution later.
| @@ -15,6 +15,7 @@ import ( | ||
| jujuos "github.com/juju/utils/os" | ||
| "github.com/lxc/lxd" | ||
| gc "gopkg.in/check.v1" | ||
| + "strings" |
axw
Jan 19, 2017
Member
please move this to the group at the top of the import list. the groupings should be:
- stdlib
- third-party (incl. github.com/juju/... not covered by the third grouping below)
- github.com/juju/juju/...
Pekkari
Jan 19, 2017
Contributor
Ooops, it might be the ide pulling it automatically, I'll change it, thanks!
| + if certdir != "" { | ||
| + err := os.MkdirAll(certdir, 0700) | ||
| + if err == nil { | ||
| + pem, key, err := cert.NewCA("client", uuid.String(), expiry, 2048) |
howbazaar
Jan 19, 2017
Owner
This is very problematic during tests. On -race builds, and arm64, this is very slow 4-10s.
Why do we need this?
Pekkari
Jan 19, 2017
Contributor
as axw pointed, this plans to be a default certificate to be able to tackle the LXD api on a remote endpoint. This can easily be used in any other provider that requires x509 certificates to encrypt communications. The idea in the code is to generate it once, when generating a new config, if it does make the delay in further executions this might be a problem to check now. Let me know if that is the case. Thanks!
howbazaar
Jan 23, 2017
Owner
Yes... but every single test that uses a JujuHomeSuite of some form, which is many of them, will have to call this for every test. That is very expensive and time consuming for test runs. How do you suggest we patch it out for testing?
Pekkari
Jan 23, 2017
Contributor
Well, though I don't think this is misplaced, we can consider to move it back to some place more lxd dependent(let's say the configuration of a new lxd cloud) and, if other providers starts using it, and the number grows, move it back here when the juju team decides. Does it sound good to you howbazaar?
Pekkari
Jan 23, 2017
Contributor
I noticed the code was called every time, so I moved the call to when we actually don't have any config, as stated in: e5d6a46
If that doesn't ease your testing, I can consider to offload the computation of the certificates to a separate thread(may ease, but not heal all the delay) or apply the suggestion before.
Pekkari
added some commits
Jan 13, 2017
|
This is missing: juju/utils#261. Retry it when pull request there is merged. |
|
@Pekkari I'm going to have to make some changes to the LXD provider that will impact you. I'll be changing the way we inject LXD credentials into the controllers. Once I'm done, users will be able to add certificate credentials to ~/.local/share/juju/credentials.yaml. I think that makes the certificate part of what you've done redundant. |
|
Sorry to strongly disagree, if you think about the ssh keys case, you have a default stored, and certainly, there might be a way(or if it happened to be), to use different ssh keys on demand. On that case you don't drop the code to generate a default one, you bypass it to use the selected one. Your proposal seems to me a good complement in case anyone prefer to use a different certificate for any reason, though it opens the door to allow to create any other type of certificate and pipe it to the code. Sound logic @axw ? |
We can consider doing this still, but it will make the LXD provider different to every other one in terms of credential selection. I'd prefer to keep them consistent, unless there's a compelling reason to do otherwise. Let's leave that debate until my changes are made please, so we can see how they would interact. |
|
I'd rather merge something ready, than making it wait for something to come, but hey, that is your decision, so no problem, go ahead, and come back when you are done. Thanks @axw! |
|
@Pekkari Please take a look at the latest provider/lxd code in the 2.1 branch: https://github.com/juju/juju/tree/2.1/provider/lxd. In particular, the code in credentials.yaml: https://github.com/juju/juju/blob/2.1/provider/lxd/credentials.go I haven't tested this yet, but we should either be close to, or it should be possible to bootstrap juju into a remote LXD:
What I'd like to see eventually is an "interactive" auth-type like in the azure provider, which takes the user through the same flow as in "lxc remote add", verifying the fingerprint and specifying the trust password. |
|
@axw So what is the outcome concerning to this pull request, do you want me to rebase it and link it in another point as you suggested before, or simply trash it? |
I think this particular PR should be closed, since there's a way of injecting certificates specifically for LXD now. If we need certificates for other things later, then we can assess at that time how best to do it for those use cases. It would be good to support user-specified port in the endpoint string, so if you wouldn't mind sending a PR for that, that would be great. |
|
Juju 2.1 supports host:port endpoints for LXD. I believe everything here is covered. Please send a new PR if there's still something we're missing. |
Pekkari commentedJan 18, 2017
•
Edited 1 time
-
Pekkari
Jan 18, 2017
These commits open the way to create https host strings in LXD provider as well as creating
a default x509 certificate to use against the https remotes.
This depends on the following merge in juju/utils: juju/utils#261