Certificate and CA management is broken

[prydin]

It doesn't seem to be possible to set certificates and custom CAs on a Client, since they require an existing client and vim25.NewClient, as well as govmomi.NewClient send traffic through the client.

Here's the smoking gun:

govmomi/vim25/client.go

Line 79 in 5bb443a

c.ServiceContent, err = methods.GetServiceContent(ctx, rt)

GetServiceContent is called on the connection, which causes traffic to be sent across the transport. If the CA chain is not set up and the server presents a certificate from a private CA, the call will fail and the client never gets connected.

Suggested solution: Remove SetRootCAs et al and allow callers to NewClient to pass a TLSConfig instead. To preserve backward compatibility, we may want to introduce a NewClientWithTLSConfig instead.

[dougm]

SetRootCAs is a soap.Client method. You just need to configure the soap.Client before calling vim25.NewClient. Example:

	soapClient := soap.NewClient(u, false)
	err = soapClient.SetRootCAs("/path/to/file")
	if err != nil {
		log.Fatal(err)
	}

	vim25Client, err := vim25.NewClient(ctx, soapClient)
	if err != nil {
		log.Fatal(err)
	}

[prydin]

So I can't use govmomi.NewClient() if I want to specify a custom CA chain? I'd like to avoid having to use the lower level functions if possible. govmomi.NewClient() creates both the vim25 client and the soap client without any chance of intervening.

What do you think of the idea of letting callers pass a TLSConfiguration? It would work as a catch-all for any TLS customization a caller would like to make. I can issue a PR if there's interest.

[prydin]

From govmomi.NewClient. After line 80, it's too late to set up any CA chain, since the connection would already have failed.

govmomi/client.go

Lines 80 to 85 in 5bb443a

	vimClient, err := vim25.NewClient(ctx, soapClient)
	if err != nil {
	return nil, err
	}
	c := &Client{

[dougm]

I'd prefer to deprecate govmomi.Client than to change it, as it only provides a handful of shortcuts. We probably should have a long time ago. If you look at the git history you'll see how it was pruned back and we moved to using vim25.Client instead in govc. There are a number of other config options, authentication methods, etc., that govmomi.NewClient doesn't support.

[prydin]

Not that you asked for my opinion, but I'll give it anyway.... :) I think removing govmomi.client would be a mistake. It's better to hide the implementation details from the caller. But that's just my opinion...

Could we still consider adding a soap.NewClientWithTLSConfig()? There are tons of configurable things, like ciphers, on a TLS connection and adding this function would provide a simple way of configuring the connection without adding tons methods on the client. Just a thought.

[prydin]

This behavior seems to be by design and a functioning workaround to my issue has been provided. I'm closing this.

[dougm]

Meant to follow up on this.. we could improve the doc at least. I hesitate to add more methods to govmomi.Client as there are a number of other config options, auth methods, RoundTrip wrappers (retry, keepalive), etc., that need to happen in between soap.NewClient and vim25.NewClient.