This patch provides a friendly interface with the masterzen/winrm client #249

hoenirvili · 2016-11-01T17:19:54Z

For a better integration in the juju ecosystem enabling many windows
machine interactions. This will also help us execute remote commands and scripts
across the wire.

dooferlad · 2016-11-09T11:58:45Z

Hi,

Thanks for the PR!

This is big and introduces new dependencies. Since at least https://github.com/masterzen/winrm is under a different license to juju/utils and juju/juju I would like to defer to someone who is able to work out what the implications are, if any, of incorporating them.

Please provide CI steps so the reviewer can confirm that the change works.

Please split the change up into smaller chunks of work. It seems that at least the changes in the series and shell folders could land independently of the winrm change.

The x509 certificate looks generic - is that right? If it is a specific winrm formatted certificate then I see the reason for including it in the winrm folder. Wouldn't combining this with github.com/juju/juju/cert/cert.go be useful? If so, moving code out of core into a security library is a good thing.

axw · 2016-11-10T06:55:51Z

@hoenirvili I second @dooferlad's question about using juju/cert. I'm away this week, will review this when I get back.

gabriel-samfira · 2016-11-10T09:06:00Z

@dooferlad The license for the winrm package is Apache 2.0 which should be consumable by juju/utils without any issues.

The x509 is mostly generic. The only winrm specific thing it does, is that it adds a UPN (User Principal Name) extension to the certificate. That extension is used to map the certificate to a particular system user (not unlike adding a OpenSSH key to the users ~/.ssh/authorized_keys).

Moving the x509 code to juju/cert should be fine. The reason we added it to the utils package was that we wanted to mirror the ssh package as closely as possible. Conceptually they have the same purpose (including the key generation part), so we wanted to have all the code dealing with access keys and remote commands in one place.

axw · 2016-11-10T09:10:29Z

@gabriel-samfira another option is to split out the more generic parts of juju/cert, and move them into utils/cert (say). Then we can have winrm-specific bits inside winrm, and delegate to the generic bits in utils/cert.

gabriel-samfira · 2016-11-10T09:14:51Z

@axw sounds good!

hoenirvili · 2016-11-10T11:38:49Z

@axw sounds good ! I will move the generic code from juju/cert to utils/cert.
(edit)
@axw how does the manual provisioning (linux side) is integrated in the CI?

hoenirvili · 2016-11-10T11:40:31Z

@dooferlad the juju/cert is creating CA x509 certs but the winrm cert generation creates client certs to use for authentication purposes.

axw · 2016-11-10T12:24:22Z

@axw how does the manual provisioning (linux side) is integrated in the CI?

We have a subset of CI tests which run the exact same thing as for manual machines as for others. The only difference is that they use "add-machine ssh:..." and then deploy --to. We have some machines (I think LXD instances) running all the time, and each test run reprovisions the same machine.

For this branch, I don't think there are any QA steps required, as none of this is hooked up yet. Once winrm provisioning is hooked up, then QA steps will need to be listed describing how we can test your changes. e.g. start a windows VM, , juju add-machine winrm:...

@axw

Move juju/cert pkg into utils. As discussed in here #249 @axw @dooferlad

hoenirvili · 2016-11-14T09:50:26Z

@axw Could you take now another look?

axw

Looking good overall. My main gripes are about the reliance on globals. I haven't gone into depth on the tests, as I expect they'll change significantly based on interface changes.

axw · 2016-11-16T05:44:56Z

cert/cert.go

+}
+
+// NewClientCert generates a new x509 certificates based on the CertConfig passed
+func NewClientCert(commonName, UUID string, expiry time.Time) ([]byte, []byte, error) {


can you please update the cert.NewLeaf function so that you can pass in pkix.Extension, and then rewrite this on top of that function?

Could you please add more details? Why should I add a pkix.Extensions to the NewLeaf function? And why should I rewrite this NewClientCert using the NewLeaf function?

AFAICS, the only difference we care about between NewClientCert and NewLeaf is that NewClientCert adds the UPN extension. So to avoid repeating all the generic certificate generation code here, I'd rather we used the NewLeaf to generate the cert, and provide the UPN extension as an argument to NewLeaf.

axw · 2016-11-16T06:07:56Z

shell/powershell.go

+	var err error
+	script, err = newEncodedPSScript(script)
+	if err != nil {
+		return "", errors.Annotatef(err, "Can't construct powershell command for remote execution")


please start error messages with lowercase, and prefer "cannot" over "can't"

i.e. errors.Annotate(err, "cannot construct ...")

axw · 2016-11-16T06:09:19Z

shell/powershell_test.go

@@ -77,3 +77,17 @@ func (s powershellSuite) TestMkdirAll(c *gc.C) {
 		`mkdir 'C:\some\dir'`,
 	})
 }
+
+func (s powershellSuite) TestNewPSEncodedCommand(c *gc.C) {


I think it would be more useful to use a shorter input, and check the exact output of the function.

axw · 2016-11-16T06:14:26Z

winrm/winrm.go

+	// will be used to make a new client conneciton to a endpoint
+	// TransportDecorator will enable us to switch transports
+	// this will be used for https client x509 authentication
+	winrm.DefaultParameters.TransportDecorator = func() winrm.Transporter {


This is not goroutine-safe. Please use winrm.NewClientWithParameters instead.

You could take a copy of winrm.DefaultParameters here and override the TransportDecorator field, but please don't write to the global variable.

axw · 2016-11-16T06:15:08Z

winrm/winrm.go

+		endpoint = winrm.NewEndpoint(host, httpsPort, true, true, nil, ClientCert(), ClientKey(), 30*time.Second)
+		logger.Warningf("Skipping CA server verification, using the --insecure option")
+	}
+	logger.Debugf("Creating WinRM secure clinet connection on %s %d", host, httpsPort)


s/clinet/client/

axw · 2016-11-16T07:28:50Z

winrm/winrm.go

+// Ping checks if we can make a WinRM conn use this client and runs a script
+// if the conn and script successfully runs it returns nil on call of Error()
+// If the default client of this pkg is not set this will panic.
+func (c *Client) Ping() {


as with Run, I think this should just return the error directly

axw · 2016-11-16T07:30:46Z

winrm/winrm.go

+}
+
+// UseClient set's the default client
+func UseClient(client Exec) {


What value are the default client & top-level functions providing? I don't want to see them used in Juju, and this code is all about making winrm easier for Juju... so I'm inclined to think that we should drop the default client, UseClient, and other related top-level functions.

Check the upper last comment.

axw · 2016-11-16T07:39:14Z

winrm/x509.go

+
+var (
+	mu          sync.Mutex
+	credentials x509 //winrm credentials


I'm not keen on the use of globals here. I guess this is derived from the ssh/clientkeys.go code, which frankly is a bit ugly. The forces that directed ssh/clientkeys.go do not apply to winrm. In particular, we need keys for ssh on disk because we shell out to openssh. We don't do that for winrm.

Can you please change the code to parse and return certs/keys in-memory. I'd also prefer if we separated generation from loading. The caller can intercept an errors.IsNotFound(err), and generate a cert/key pair and write it to disk. I expect we'll only need to do that in one location in Juju, like we do for ssh.LoadClientKeys.

axw · 2016-11-16T07:40:10Z

winrm/x509.go

+// if the directory in the coresponding paths key, cert
+// if the base dir dosen't exist it will create and generate the key/cert pair
+// LoadClientCert function is lock protected allowing multiple threads to call this function
+func LoadClientCert(key, cert string) error {


Please call them "keyFile", "certFile" or "keyPath", "certPath". Named as they are, I would expect them contain the key/cert values.

axw · 2016-11-16T07:43:40Z

winrm/x509.go

+// LoadCACert saves into internal singleton credentials the CACert to be used for https connections
+// if the cacert in the path is not found it will return the error rather than saing it into state
+// This behaviour will encourage that https connection will skip checking the CA.
+func LoadCACert(path string) error {


As before, please don't use globals. It seems unlikely that we would have a well-defined location for a CA cert on disk, as it's going to be different for each server. In Juju, I guess there's two places where we would need to specify a CA cert:

when manual-provisioning, using a CA cert specified by the user on the command line

when doing the equivalent of "juju ssh" ("juju winrm"?), using a CA cert published by the Juju agent on the target machine

axw

Thanks, I am happy with the interface now. Can you please update the remaining tests and delete unused code, so I can give the final review (hopefully!)

axw · 2016-11-24T03:08:44Z

cert/cert.go

+		caKey = key
+	}
+
+	certDER, err := x509.CreateCertificate(rand.Reader, template, caCert, getPublicKey(key), caKey)


Please update this function to call utils/cert.NewLeaf, instead of duplicating logic here. This can be in a follow up if you like.

This is not fixed. Did you miss a commit? Maybe I'm being unclear.

What I want to see is:

no call to x509.CreateCertificate in this function

calling NewLeaf from github.com/juju/utils/cert (https://github.com/juju/utils/blob/master/cert/cert.go#L119) to create the certificate

To do that, github.com/juju/utils/cert would need to be updated so that you can pass x509.ExtraExtensions into NewLeaf. Let's just leave a TODO for now, and tidy that up later.

This call of x509.CreateCertificate is inside of NewLeaf function. If I put the call of NewLeaf here, it will make it in a recursive loop.
And I was having some issues with the commit log.

I'm not saying to call the NewLeaf function that is defined in this package. I'm referring to the one in utils/cert. utils/winrm.NewLeaf should initialise pkix.Extensions, and then call utils/cert.NewLeaf.

Anyway, let's defer this for now, this PR has gone on long enough ;)

Wait... this is the cert package. I think something was moved and I didn't notice, I'm sorry for the confusion.

Hrm, I'm not so sure about changing the common utils/cert interface, but let's go with it for now. I may end up changing it a bit.

axw · 2016-11-24T03:10:40Z

cert/cert.go

 	keyPEMData := pem.EncodeToMemory(&pem.Block{
 		Type:  "RSA PRIVATE KEY",
 		Bytes: x509.MarshalPKCS1PrivateKey(key),
 	})
 	return string(certPEMData), string(keyPEMData), nil
 }

+//NewClientCert generates a x509 client certificate used for https authentication sessions.
+func NewClientCert(commonName, UUID string, expiry time.Time) (string, string, error) {
+	cert, key, err := NewLeaf(commonName, "", "",


Sorry, what I was asking for was for the function in this package to set up the ExtKeyUsage (any any other bits specific to winrm), and then call utils/cert.NewLeaf. There's a bunch of duplicated code in this package that isn't specific to winrm.

axw · 2016-11-24T03:22:12Z

winrm/winrm.go

+func (c ClientConfig) password() (string, error) {
+	if c.Password != nil {
+		// make it look like a dropdown shell
+		fmt.Fprintf(os.Stdout, "[Winrm] >> ")


please move the fmt.Fprintf prompts to the GetPasswd implementation. this code shouldn't assume GetPasswd is interactive.

axw · 2016-11-24T03:29:18Z

winrm/winrm_test.go

+type WinRMSuite struct{}
+
+var _ = gc.Suite(&WinRMSuite{})
+


I think these tests all need to be updated now. There's no ResetClientCert, LoadClientCert, ClientCert, ClientKey, etc. Probably you should just have pre-generated certs/keys checked in the winrm_test package, and use them in your tests. Avoid generating new certs/keys for each test (or even the suite, if possible), because it's expensive and slows down tests. Not avoidable in all cases though.

hoenirvili · 2016-11-24T09:37:52Z

Sorry for the delay @axw I will make a new commit.... somehow git doesn't' include the last fixes after rebasing with master, I don't know what happened.

axw

A few last things, and this will be good to merge. Main thing is to either fix tests for, or remove the x509.go code.

axw · 2016-11-24T23:35:50Z

winrm/x509.go

@@ -0,0 +1,198 @@
+// Copyright 2016 Canonical Ltd.
+// Copyright 2016 Canonical Ltd.


I think you want Cloudbase here.

axw · 2016-11-24T23:37:27Z

winrm/x509.go

+}
+
+// NewX509 returns a new to an empty X509
+func NewX509() *X509 {


The tests are all commented out, and this isn't used in the winrm package. Will you be using it in the juju code? If yes, please uncomment the tests and make sure they're working. If not, please delete the code.

Ohh sorry I forgot to uncomment these. Fixed !

axw · 2016-11-24T23:41:40Z

cert/cert.go

+		caKey = key
+	}
+
+	certDER, err := x509.CreateCertificate(rand.Reader, template, caCert, getPublicKey(key), caKey)


This is not fixed. Did you miss a commit? Maybe I'm being unclear.

What I want to see is:

no call to x509.CreateCertificate in this function

calling NewLeaf from github.com/juju/utils/cert (https://github.com/juju/utils/blob/master/cert/cert.go#L119) to create the certificate

To do that, github.com/juju/utils/cert would need to be updated so that you can pass x509.ExtraExtensions into NewLeaf. Let's just leave a TODO for now, and tidy that up later.

axw · 2016-11-27T23:31:17Z

winrm/x509_test.go

+)
+
+var (
+	base = "/tmp/base"


please do not use a hard-coded temporary dir

axw · 2016-11-27T23:32:13Z

winrm/x509_test.go

+)
+
+func (w *WinRMSuite) TestLoadClientCert(c *gc.C) {
+	cert := winrm.NewX509()


gocheck.C has a method called MkDir that will create a temporary dir, and return its path. Please use that instead of the hard-coded "/tmp/base".

for a better integration in the juju ecosystem enabling many windows machine interactions. This will also help us execute remote commands and scripts across the wire. Signed-off-by: Salvatore Giulitti <sgiulitti@cloudbasesolutions.com>

hoenirvili · 2016-11-28T08:17:24Z

$$merge$$

jujubot · 2016-11-28T08:18:08Z

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils

jujubot · 2016-11-28T08:19:09Z

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils/154

Signed-off-by: Giulitti Salvatore <sgiulitti@cloudbasesolutions.com>

hoenirvili · 2016-11-28T08:25:54Z

$$merge$$

jujubot · 2016-11-28T08:26:08Z

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils

jujubot · 2016-11-28T08:28:52Z

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils/155

hoenirvili · 2016-11-28T08:34:06Z

$$merge$$

jujubot · 2016-11-28T08:34:09Z

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils

Adding ExtraExtensions overwirtes or makes dns field in template to disappear. This PR #249 merged all the cert generation certificate specific for winrm with the juju/cert and a bunch of refactoring. Based on these, when I rebased the develop branch from juju/juju I noticed that in tests somehow when adding dns namefields to generate a new cert , they dissapear/or gets overwrited. Further investigations lead me to this: When the template was populated his ExtraExtensions slice, somehow the dns name fields disappear after the generation. Anyway we need that ExtraExtensions slice when we want to generate a valid winrm client certificate used for client certificate authentication.

hoenirvili mentioned this pull request Nov 1, 2016

Enable manual provisioning for windows machines. juju/juju#6523

Merged

hoenirvili mentioned this pull request Nov 10, 2016

Move juju/cert pkg into utils. #250

Merged

jujubot added a commit that referenced this pull request Nov 10, 2016

Merge pull request #250 from hoenirvili/move-cert-pkg

a84e60d

Move juju/cert pkg into utils. As discussed in here #249 @axw @dooferlad

axw suggested changes Nov 16, 2016

View reviewed changes

axw reviewed Nov 24, 2016

View reviewed changes

axw approved these changes Nov 27, 2016

View reviewed changes

Update dependencies

5765b88

Signed-off-by: Giulitti Salvatore <sgiulitti@cloudbasesolutions.com>

jujubot merged commit 7018575 into juju:master Nov 28, 2016

hoenirvili deleted the enable-winrm-support branch November 28, 2016 08:37

hoenirvili mentioned this pull request Dec 2, 2016

Adding ExtraExtensions overwirtes or makes dns field in template to disappear. #254

Merged

		@@ -0,0 +1,198 @@
		// Copyright 2016 Canonical Ltd.
		// Copyright 2016 Canonical Ltd.

This patch provides a friendly interface with the masterzen/winrm client #249

This patch provides a friendly interface with the masterzen/winrm client #249

Conversation

hoenirvili commented Nov 1, 2016

dooferlad commented Nov 9, 2016

axw commented Nov 10, 2016

gabriel-samfira commented Nov 10, 2016

axw commented Nov 10, 2016

gabriel-samfira commented Nov 10, 2016

hoenirvili commented Nov 10, 2016 • edited

hoenirvili commented Nov 10, 2016

axw commented Nov 10, 2016

hoenirvili commented Nov 14, 2016

axw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoenirvili Nov 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoenirvili commented Nov 24, 2016

axw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoenirvili commented Nov 28, 2016

jujubot commented Nov 28, 2016

jujubot commented Nov 28, 2016

hoenirvili commented Nov 28, 2016

jujubot commented Nov 28, 2016

jujubot commented Nov 28, 2016

hoenirvili commented Nov 28, 2016

jujubot commented Nov 28, 2016

hoenirvili commented Nov 10, 2016 •

edited

hoenirvili Nov 17, 2016 •

edited