Added a check to ensure clientTLS configuration contains either a cert or a key

[aantono]

Description

Added a check to ensure clientTLS configuration contains either a cert or a key
If any of the ClientTLS only sets InsecureSkipVerify property to true, the CreateTLSConfig() method fails, because it expects a Cert or a Key property to always be present.

[nmengin]

@aantono many thanks for this PR.

However, the condition you added seems to be more permissive that what you need.

I set a comment , let me know your mind about it.

[nmengin]

This confition will allow user to specify TLS configuration with no key r cert file even if the InsecureSkipVerify parameter is set to false.

It can be better to check the InsecureSkipVerify in the condition and let the current mechanism generate an error if cert file or key file are missing when InsecureSkipVerify is set to false.

if ! clientTLS.InsecureSkipVerify {
...
}

[aantono]

The problem is that CreateTLSConfig() is being used from many places, where some are using this for outbound connections, for example Marathon provider uses this to create an http client to Mesos Master like:

TLSConfig, err := p.TLS.CreateTLSConfig()
		if err != nil {
			return err
		}
		config.HTTPClient = &http.Client{
			Transport: &http.Transport{
				DialContext: (&net.Dialer{
					KeepAlive: time.Duration(p.KeepAlive),
					Timeout:   time.Duration(p.DialerTimeout),
				}).DialContext,
				TLSClientConfig: TLSConfig,
			},
		}
		client, err := marathon.NewClient(config)

If you have Mesos Master using self-signed certificate, then all you need to configure is InsecureSkipVerify=true, but you don't have either a cert or a key to provide. But there are other cases where you would have either a cert or a key, as well as InsecureSkipVerify=true. (but let me know if you think the above use-case is not accurate), I'd change the condition to just check for if ! clientTLS.InsecureSkipVerify instead.

[nmengin]

Thanks for your reply.

In fact, I suggested to check the InsecureSkipverify parameter because, into the test you did, even if the InsecureSkipVerify is set to false, the test will be OK but the configuration is not correct (there is no certificate/key file to check) and I would like to avoid this kind of behavior.

The best solution may be to check the InsecureSkipVerify and the certificate/key files length before the test you did as described above :

if ! clientTLS.InsecureSkipVerify && (len(clientTLS.Cert) == 0 || len(clientTLS.Key) == 0) {
        return nil, fmt.Errorf("Certificate and key files must be set when TLS configuration is created") // The message must be improved ;)
}
if len(clientTLS.Cert) > 0 && len(clientTLS.Key) > 0 {
...
}

With this new condition, we are sure the TLSConfiguration will be created with no certificate/key file only if the InsecureSkipVerify is set to true otherwise, an error will be returned.

@aantono What is your mind about this solution?

[nmengin]

Thanks for your reply.

In fact, I suggested to check the InsecureSkipverify parameter because, into the test you did, even if the InsecureSkipVerify is set to false, the test will be OK but the configuration is not correct (there is no certificate/key file to check) and I would like to avoid this kind of behavior.

The best solution may be to check the InsecureSkipVerify and the certificate/key files length before the test you did as described above :

if ! clientTLS.InsecureSkipVerify && (len(clientTLS.Cert) == 0 || len(clientTLS.Key) == 0) {
        return nil, fmt.Errorf("Certificate and key files must be set when TLS configuration is created") // The message must be improved ;)
}
if len(clientTLS.Cert) > 0 && len(clientTLS.Key) > 0 {
...
}

With this new condition, we are sure the TLSConfiguration will be created with no certificate/key file only if the InsecureSkipVerify is set to true otherwise, an error will be returned.

@aantono What is your mind about this solution?

[nmengin]

It can be great to add a test with the parameter InsecureSkipverify set to false and checking the error returned.

[nmengin]

LGTM 👏

[ldez]

LGTM

[dtomcej]

LGTM