Check TLS config when creating HTTP Client (#1513)

* Check TLS config when creating HTTP Client

We do not check if TLS is set up properly. When we can't read
certificate or it was invalid we silently swtiched to insecure mode.
This PR changes this behaviour. Whenever HTTP Client is created it
checks for errors and returns one if necessary. The only exception are
`list` and `setup` commands. List will allow user to list all clusters
but call to attach will fail. `setup` uses `insecure`HttpClient` by
default – to download certs.

Fixes: https://jira.mesosphere.com/browse/DCOS-61005

api/cli.go

@@ -56,7 +56,7 @@ type Context interface {
 	Clusters() ([]*config.Cluster, error)
 
 	// HTTPClient creates an httpclient.Client for a given cluster.
-	HTTPClient(c *config.Cluster, opts ...httpclient.Option) *httpclient.Client
+	HTTPClient(c *config.Cluster, opts ...httpclient.Option) (*httpclient.Client, error)
 
 	// Prompt returns a *prompt.Prompt.
 	Prompt() *prompt.Prompt

cmd/dcos/main.go

@@ -12,13 +12,25 @@ import (
 	"github.com/dcos/dcos-cli/pkg/cli"
 	"github.com/dcos/dcos-cli/pkg/cli/version"
 	"github.com/dcos/dcos-cli/pkg/cmd"
+	"github.com/dcos/dcos-cli/pkg/config"
 	"github.com/dcos/dcos-cli/pkg/dcos"
 	"github.com/dcos/dcos-cli/pkg/httpclient"
 	"github.com/sirupsen/logrus"
 )
 
 func main() {
-	if err := run(cli.NewOsEnvironment()); err != nil {
+	env := cli.NewOsEnvironment()
+	err := run(env)
+	if err != nil {
+		if _, ok := err.(*config.SSLError); ok {
+			msg := "Error: An SSL error occurred. To configure your SSL settings, please " +
+				"run: 'dcos config set core.ssl_verify <value>'\n" +
+				"<value>: Whether to verify SSL certs for HTTPS or path to certs. " +
+				"Valid values are a path to a CA_BUNDLE, " +
+				"True (will then use CA certificates from certifi), " +
+				"or False (will then send insecure requests).\n"
+			fmt.Fprint(env.ErrOut, msg)
+		}
 		os.Exit(1)
 	}
 }
@@ -86,7 +98,11 @@ func printVersion(ctx api.Context) {
 		return
 	}
 
-	dcosClient := dcos.NewClient(ctx.HTTPClient(cluster, httpclient.Timeout(3*time.Second)))
+	httpClient, err := ctx.HTTPClient(cluster, httpclient.Timeout(3*time.Second))
+	if err != nil {
+		return
+	}
+	dcosClient := dcos.NewClient(httpClient)
 	if dcosVersion, err := dcosClient.Version(); err == nil {
 		fmt.Fprintln(ctx.Out(), "dcos.version="+dcosVersion.Version)
 		fmt.Fprintln(ctx.Out(), "dcos.variant="+dcosVersion.DCOSVariant)

pkg/cli/context.go

@@ -154,23 +154,27 @@ func (ctx *Context) Clusters() ([]*config.Cluster, error) {
 }
 
 // HTTPClient creates an httpclient.Client for a given cluster.
-func (ctx *Context) HTTPClient(c *config.Cluster, opts ...httpclient.Option) *httpclient.Client {
+func (ctx *Context) HTTPClient(c *config.Cluster, opts ...httpclient.Option) (*httpclient.Client, error) {
 	var baseOpts []httpclient.Option
 
 	if c.ACSToken() != "" {
 		baseOpts = append(baseOpts, httpclient.ACSToken(c.ACSToken()))
 	}
 	baseOpts = append(baseOpts, httpclient.Timeout(c.Timeout()))
 
+	clusterTLS, err := c.TLS()
+	if err != nil {
+		return nil, config.NewSSLError(err)
+	}
 	tlsOpt := httpclient.TLS(&tls.Config{
-		InsecureSkipVerify: c.TLS().Insecure, // nolint: gosec
-		RootCAs:            c.TLS().RootCAs,
+		InsecureSkipVerify: clusterTLS.Insecure, // nolint: gosec
+		RootCAs:            clusterTLS.RootCAs,
 	})
 
 	baseOpts = append(baseOpts, tlsOpt, httpclient.Logger(ctx.Logger()))
 	opts = append(baseOpts, opts...)
 
-	return httpclient.New(c.URL(), opts...)
+	return httpclient.New(c.URL(), opts...), nil
 }
 
 // Prompt is able to prompt for input, password or choices.

pkg/cluster/lister/lister.go

@@ -153,14 +153,23 @@ func (l *Lister) List(filters ...Filter) []*Item {
 }
 
 func (l *Lister) httpClient(cluster *config.Cluster) *httpclient.Client {
+	clusterTLS, err := cluster.TLS()
+	if err != nil {
+		// Allow listing clusters without proper tls configuration.
+		// TLS will be reconfigured after cluster setup
+		// since attach and other commands will fail.
+		clusterTLS = config.TLS{
+			Insecure: true,
+		}
+	}
 	return httpclient.New(
 		cluster.URL(),
 		httpclient.Logger(l.logger),
 		httpclient.ACSToken(cluster.ACSToken()),
 		httpclient.Timeout(3*time.Second),
 		httpclient.TLS(&tls.Config{
-			InsecureSkipVerify: cluster.TLS().Insecure, // nolint: gosec
-			RootCAs:            cluster.TLS().RootCAs,
+			InsecureSkipVerify: clusterTLS.Insecure, // nolint: gosec
+			RootCAs:            clusterTLS.RootCAs,
 		}),
 	)
 }

pkg/cmd/auth/auth_listproviders.go

@@ -24,7 +24,11 @@ func newCmdAuthListProviders(ctx api.Context) *cobra.Command {
 				if err != nil {
 					return err
 				}
-				client = login.NewClient(ctx.HTTPClient(cluster), ctx.Logger())
+				httpClient, err := ctx.HTTPClient(cluster)
+				if err != nil {
+					return err
+				}
+				client = login.NewClient(httpClient, ctx.Logger())
 			} else {
 				httpClient := httpclient.New(args[0], httpclient.Logger(ctx.Logger()))
 				client = login.NewClient(httpClient, ctx.Logger())

pkg/cmd/auth/auth_login.go

@@ -18,7 +18,11 @@ func newCmdAuthLogin(ctx api.Context) *cobra.Command {
 			if err != nil {
 				return err
 			}
-			acsToken, err := ctx.Login(flags, ctx.HTTPClient(cluster))
+			httpClient, err := ctx.HTTPClient(cluster)
+			if err != nil {
+				return err
+			}
+			acsToken, err := ctx.Login(flags, httpClient)
 			if err != nil {
 				return err
 			}

pkg/cmd/cluster/cluster_attach.go

@@ -45,7 +45,11 @@ func newCmdClusterAttach(ctx api.Context) *cobra.Command {
 				return err
 			}
 
-			clusterLinker := linker.New(ctx.HTTPClient(currentCluster), ctx.Logger())
+			httpClient, err := ctx.HTTPClient(currentCluster)
+			if err != nil {
+				return err
+			}
+			clusterLinker := linker.New(httpClient, ctx.Logger())
 			linkedClusters, err := clusterLinker.Links()
 			if err != nil {
 				ctx.Logger().Info(err)

pkg/cmd/cluster/cluster_link.go

@@ -52,7 +52,11 @@ func newCmdClusterLink(ctx api.Context) *cobra.Command {
 				return errors.New("cannot link a cluster to itself")
 			}
 
-			client := login.NewClient(ctx.HTTPClient(linkableCluster), ctx.Logger())
+			httpClient, err := ctx.HTTPClient(linkableCluster)
+			if err != nil {
+				return err
+			}
+			client := login.NewClient(httpClient, ctx.Logger())
 			rawProviders, err := client.Providers()
 			if err != nil {
 				return err
@@ -97,7 +101,11 @@ func newCmdClusterLink(ctx api.Context) *cobra.Command {
 				},
 			}
 
-			attachedClient := linker.New(ctx.HTTPClient(attachedCluster), ctx.Logger())
+			httpClient, err = ctx.HTTPClient(attachedCluster)
+			if err != nil {
+				return err
+			}
+			attachedClient := linker.New(httpClient, ctx.Logger())
 			return attachedClient.Link(linkRequest)
 		},
 	}

pkg/cmd/cluster/cluster_unlink.go

@@ -30,7 +30,11 @@ func newCmdClusterUnlink(ctx api.Context) *cobra.Command {
 			}
 			linkedCluster := config.NewCluster(linkedClusterConfig)
 
-			attachedClient := linker.New(ctx.HTTPClient(attachedCluster), ctx.Logger())
+			httpClient, err := ctx.HTTPClient(attachedCluster)
+			if err != nil {
+				return err
+			}
+			attachedClient := linker.New(httpClient, ctx.Logger())
 			return attachedClient.Unlink(linkedCluster.ID())
 		},
 	}

pkg/cmd/dcos.go

@@ -129,7 +129,11 @@ func updateCorePlugin(ctx api.Context) error {
 		return err
 	}
 
-	pluginInfo, err := cosmos.CLIPluginInfo(pkg, ctx.HTTPClient(cluster).BaseURL())
+	httpClient, err := ctx.HTTPClient(cluster)
+	if err != nil {
+		return err
+	}
+	pluginInfo, err := cosmos.CLIPluginInfo(pkg, httpClient.BaseURL())
 	if err != nil {
 		return err
 	}
@@ -165,6 +169,12 @@ func invokePlugin(ctx api.Context, cmd plugin.Command, args []string) error {
 	if err != nil && err != config.ErrNotAttached {
 		return err
 	}
+	if cluster != nil {
+		_, err := cluster.TLS()
+		if err != nil {
+			return config.NewSSLError(err)
+		}
+	}
 	execCmdEnv := pluginEnv(executablePath, cmd.Name, ctx.Logger().Level, cluster)
 	execCmd.Env = append(os.Environ(), execCmdEnv...)
 
@@ -202,13 +212,6 @@ func pluginEnv(executablePath string, cmdName string, logLevel logrus.Level, clu
 		env = append(env, "DCOS_URL="+cluster.URL())
 		env = append(env, "DCOS_ACS_TOKEN="+cluster.ACSToken())
 
-		insecure := cluster.TLS().Insecure || strings.HasPrefix(cluster.URL(), "http://")
-		if insecure {
-			env = append(env, "DCOS_TLS_INSECURE=1")
-		} else if cluster.TLS().RootCAsPath != "" {
-			env = append(env, "DCOS_TLS_CA_PATH="+cluster.TLS().RootCAsPath)
-		}
-
 		// Create env entries based on the subcommand config.
 		if cmdConfig, ok := cluster.Config().ToMap()[cmdName].(map[string]interface{}); ok {
 			for key, val := range cmdConfig {
@@ -219,6 +222,14 @@ func pluginEnv(executablePath string, cmdName string, logLevel logrus.Level, clu
 				))
 			}
 		}
+
+		clusterTLS, _ := cluster.TLS()
+		insecure := clusterTLS.Insecure || strings.HasPrefix(cluster.URL(), "http://")
+		if insecure {
+			env = append(env, "DCOS_TLS_INSECURE=1")
+		} else if clusterTLS.RootCAsPath != "" {
+			env = append(env, "DCOS_TLS_CA_PATH="+clusterTLS.RootCAsPath)
+		}
 	}
 	return env
 }

pkg/config/cluster.go

@@ -2,6 +2,7 @@ package config
 
 import (
 	"crypto/x509"
+	"fmt"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -13,6 +14,18 @@ import (
 	"github.com/spf13/cast"
 )
 
+type SSLError struct {
+	msg error
+}
+
+func (e *SSLError) Error() string {
+	return e.msg.Error()
+}
+
+func NewSSLError(e error) *SSLError {
+	return &SSLError{msg: e}
+}
+
 // Cluster is a subset representation of a DC/OS CLI configuration.
 //
 // It is a proxy struct on top of a config which provides user-friendly getters and setters for common
@@ -53,14 +66,23 @@ func (c *Cluster) SetACSToken(acsToken string) {
 	c.config.Set("core.dcos_acs_token", acsToken)
 }
 
-// TLS returns the configuration for TLS clients.
-func (c *Cluster) TLS() TLS {
+// SetTLS returns the configuration for TLS clients.
+func (c *Cluster) SetTLS(tls TLS) {
+	c.config.Set("core.ssl_verify", tls.String())
+}
+
+// CheckTLS returns error if provided certificate is invalid
+func (c *Cluster) TLS() (TLS, error) {
 	tlsVal := cast.ToString(c.config.Get("core.ssl_verify"))
 
+	if tlsVal == "" {
+		return TLS{Insecure: false}, nil
+	}
+
 	// Try to cast the value to a bool, true means we verify
 	// server certificates, false means we skip verification.
 	if verify, err := strconv.ParseBool(tlsVal); err == nil {
-		return TLS{Insecure: !verify}
+		return TLS{Insecure: !verify}, nil
 	}
 
 	// The value is not a string representing a bool thus it is a path to a root CA bundle.
@@ -69,7 +91,7 @@ func (c *Cluster) TLS() TLS {
 		return TLS{
 			Insecure:    true,
 			RootCAsPath: tlsVal,
-		}
+		}, fmt.Errorf("can't read %s: %s", tlsVal, err)
 	}
 
 	// Decode the PEM root certificate(s) into a cert pool.
@@ -78,19 +100,14 @@ func (c *Cluster) TLS() TLS {
 		return TLS{
 			Insecure:    true,
 			RootCAsPath: tlsVal,
-		}
+		}, fmt.Errorf("cannot decode the PEM root certificate(s) into a cert pool: %s", tlsVal)
 	}
 
 	// The cert pool has been successfully created, store it in the TLS config.
 	return TLS{
 		RootCAs:     certPool,
 		RootCAsPath: tlsVal,
-	}
-}
-
-// SetTLS returns the configuration for TLS clients.
-func (c *Cluster) SetTLS(tls TLS) {
-	c.config.Set("core.ssl_verify", tls.String())
+	}, nil
 }
 
 // Timeout returns the HTTP request timeout once the connection is established.