Merge pull request #11145 from fabianofranz/issues_11122

Merged by openshift-bot

Author: OpenShift Bot

pkg/cmd/cli/cmd/login/helpers.go

@@ -2,6 +2,7 @@ package login
 
 import (
 	"bytes"
+	"crypto/x509"
 	"fmt"
 	"io"
 	"net"
@@ -82,10 +83,23 @@ func dialToServer(clientConfig restclient.Config) error {
 	return nil
 }
 
-func promptForInsecureTLS(reader io.Reader, out io.Writer) bool {
+func promptForInsecureTLS(reader io.Reader, out io.Writer, reason error) bool {
+	var insecureTLSRequestReason string
+	if reason != nil {
+		switch reason.(type) {
+		case x509.UnknownAuthorityError:
+			insecureTLSRequestReason = "The server uses a certificate signed by an unknown authority."
+		case x509.HostnameError:
+			insecureTLSRequestReason = fmt.Sprintf("The server is using a certificate that does not match its hostname: %s", reason.Error())
+		case x509.CertificateInvalidError:
+			insecureTLSRequestReason = fmt.Sprintf("The server is using an invalid certificate: %s", reason.Error())
+		}
+	}
 	var input bool
 	if term.IsTerminal(reader) {
-		fmt.Fprintln(out, "The server uses a certificate signed by an unknown authority.")
+		if len(insecureTLSRequestReason) > 0 {
+			fmt.Fprintln(out, insecureTLSRequestReason)
+		}
 		fmt.Fprintln(out, "You can bypass the certificate check, but any data you send to the server could be intercepted by others.")
 		input = cmdutil.PromptForBool(os.Stdin, out, "Use insecure connections? (y/n): ")
 		fmt.Fprintln(out)

pkg/cmd/cli/cmd/login/loginoptions.go

@@ -121,10 +121,10 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {
 		// certificate authority unknown, check or prompt if we want an insecure
 		// connection or if we already have a cluster stanza that tells us to
 		// connect to this particular server insecurely
-		case x509.UnknownAuthorityError:
+		case x509.UnknownAuthorityError, x509.HostnameError, x509.CertificateInvalidError:
 			if o.InsecureTLS ||
 				hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig) ||
-				promptForInsecureTLS(o.Reader, o.Out) {
+				promptForInsecureTLS(o.Reader, o.Out, err) {
 				clientConfig.Insecure = true
 				clientConfig.CAFile = ""
 				clientConfig.CAData = nil

pkg/cmd/cli/cmd/login/loginoptions_test.go

@@ -1,15 +1,21 @@
 package login
 
 import (
+	"crypto/tls"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"regexp"
 	"strings"
 	"testing"
 
+	"github.com/MakeNowJust/heredoc"
+
 	"github.com/openshift/origin/pkg/cmd/cli/config"
 	"github.com/openshift/origin/pkg/cmd/util/clientcmd"
 
 	"k8s.io/kubernetes/pkg/client/restclient"
+	kclientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
 )
 
 func TestNormalizeServerURL(t *testing.T) {
@@ -55,6 +61,163 @@ func TestNormalizeServerURL(t *testing.T) {
 	}
 }
 
+func TestTLSWithCertificateNotMatchingHostname(t *testing.T) {
+	// generated by 'go run src/crypto/tls/generate_cert.go --rsa-bits 1024 --host invalidhost.com,8.8.8.8 --ca --start-date "Jan 1 00:00:00 1970" --duration=1000000h'
+	invalidHostCert := heredoc.Doc(`
+		-----BEGIN CERTIFICATE-----
+		MIICBjCCAW+gAwIBAgIRALOIWXyeLzunaiVkP2itHAEwDQYJKoZIhvcNAQELBQAw
+		EjEQMA4GA1UEChMHQWNtZSBDbzAgFw03MDAxMDEwMDAwMDBaGA8yMDg0MDEyOTE2
+		MDAwMFowEjEQMA4GA1UEChMHQWNtZSBDbzCBnzANBgkqhkiG9w0BAQEFAAOBjQAw
+		gYkCgYEAuKDlC4aMBbHaXgS+MFud5h3zeE4boSqKgFI6HceySF/a+qg0v+ID6EwQ
+		DpJ2W5AdJGEBfixo+tym6q3oKWHJUX0hInkJ6dXIdUbVOeO5dIsGG0fZmRD7DDDx
+		snkXrDB/E0JglHNckRbIh/jvznbDfbddIcdgZ7JVIfnNpigtHZECAwEAAaNaMFgw
+		DgYDVR0PAQH/BAQDAgKkMBMGA1UdJQQMMAoGCCsGAQUFBwMBMA8GA1UdEwEB/wQF
+		MAMBAf8wIAYDVR0RBBkwF4IPaW52YWxpZGhvc3QuY29thwQICAgIMA0GCSqGSIb3
+		DQEBCwUAA4GBAAkPU044aFkBl4f/muwSh/oPGinnA4fp8ei0KMnLk+0/CjNb3Waa
+		GtuRVIudRTK2M/RzdpUrwfWlVmkezV4BR1K/aOH9a29zqDTkEjnkIbWwe+piAs+w
+		VxIxrTqM8rqq8qxeWS54AyF/OaLJgXzDpCFnCb7kY3iyHv6lcmCjluLW
+		-----END CERTIFICATE-----`)
+	invalidHostKey := heredoc.Doc(`
+		-----BEGIN RSA PRIVATE KEY-----
+		MIICXQIBAAKBgQC4oOULhowFsdpeBL4wW53mHfN4ThuhKoqAUjodx7JIX9r6qDS/
+		4gPoTBAOknZbkB0kYQF+LGj63KbqregpYclRfSEieQnp1ch1RtU547l0iwYbR9mZ
+		EPsMMPGyeResMH8TQmCUc1yRFsiH+O/OdsN9t10hx2BnslUh+c2mKC0dkQIDAQAB
+		AoGAZ0ZAuNC7NFhHEL5QcJZe3aC1Vv9B/0XfkWXtckkJFejggcNjNk5D50Xc2Xnd
+		0NvtITNN9Xj8BA83IyDCM5uqUwDbOLIc6qYgAGWzxZZSDAQg1iOAAZoXmMTNS6Zf
+		hQhNUIwB68ELGvbcq7cxQL7L9n4GfISz7PKOOUKTZp0Q8G0CQQD07K7NES340c3I
+		QVkCW5/ygNK0GuQ8nTcG5yC8R5SDS47N8YzPp17Pajah8+wawYiemY1fUmD7P/bq
+		Cjl2RtIHAkEAwPo1GzJubN7PSYgPir3TxUGtMJoyc3jfdjblXyGJHwTu2YxeRjd2
+		YUPVRpu9JvNjZc+GONvTbTZeNWCvy0JNpwJBAKEsi49JCd6eefOZBTDnCKd1nLKG
+		q8Ezl/2D5WfhFtsbwrrFhOs1cc++Tnte3/VvfC8aTwz2UfmkyyCSX+P0kMsCQCIL
+		glb7/LNEU7mbQXKurq+8OHu8mG36wyGt6aVw2yoXyrOiqfclTcM3HmdIjoRSqBSM
+		Ghfp4FECKHiuSBVJ6z0CQQDF37CRpdQRDPnAedhyApLcIxSbYo1oUm7FxBLyVb7V
+		HQjFvsOylsSCABXz0FyC7zXQxkEo6CiSahVI/PHz6Zta
+		-----END RSA PRIVATE KEY-----`)
+
+	server, err := newTLSServer(invalidHostCert, invalidHostKey)
+	if err != nil {
+		t.Errorf(err.Error())
+	}
+	server.StartTLS()
+	defer server.Close()
+
+	testCases := map[string]struct {
+		serverURL      string
+		skipTLSVerify  bool
+		expectedErrMsg *regexp.Regexp
+	}{
+		"succeed skipping tls": {
+			serverURL:     server.URL,
+			skipTLSVerify: true,
+		},
+		"certificate hostname doesn't match": {
+			serverURL:      server.URL,
+			expectedErrMsg: regexp.MustCompile(`The server is using a certificate that does not match its hostname(.*)is valid for 8\.8\.8\.8`),
+		},
+	}
+
+	for name, test := range testCases {
+		t.Logf("evaluating test: %s", name)
+		options := &LoginOptions{
+			Server:             test.serverURL,
+			InsecureTLS:        test.skipTLSVerify,
+			StartingKubeConfig: &kclientcmdapi.Config{},
+		}
+
+		if _, err = options.getClientConfig(); err != nil {
+			if !test.expectedErrMsg.MatchString(err.Error()) {
+				t.Errorf("%s: expected error %q but got %q", name, test.expectedErrMsg, err)
+			}
+			if test.expectedErrMsg == nil {
+				t.Errorf("%s: unexpected error: %v", name, err)
+			}
+		} else {
+			if test.expectedErrMsg != nil {
+				t.Errorf("%s: expected error but got nothing", name)
+			}
+		}
+	}
+}
+
+func TestTLSWithExpiredCertificate(t *testing.T) {
+	// generated by 'go run src/crypto/tls/generate_cert.go --rsa-bits 1024 --host 127.0.0.1,::1,example.com --ca --start-date "Jan 1 00:00:00 1970" --duration=1h'
+	expiredCert := heredoc.Doc(`
+		-----BEGIN CERTIFICATE-----
+		MIICEjCCAXugAwIBAgIRALf82bYpro/jQS8fP74dG5EwDQYJKoZIhvcNAQELBQAw
+		EjEQMA4GA1UEChMHQWNtZSBDbzAeFw03MDAxMDEwMDAwMDBaFw03MDAxMDEwMTAw
+		MDBaMBIxEDAOBgNVBAoTB0FjbWUgQ28wgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJ
+		AoGBAONNgDXBk2Q1i/aJjTwt03KpQ3nQblMS3IX/H9JWw6ta6UublKBOaD/2o5Xt
+		FM+Q7XDEnzYw88CK5KHdyejkJo5IBpUjQYJZFzUJ1BC8Lw7yy6dXWYBJboRR1S+1
+		JhkMJOtpPecv+4cTaynplYj0WMBjcQthg2RM7tdpyUYpsp2rAgMBAAGjaDBmMA4G
+		A1UdDwEB/wQEAwICpDATBgNVHSUEDDAKBggrBgEFBQcDATAPBgNVHRMBAf8EBTAD
+		AQH/MC4GA1UdEQQnMCWCC2V4YW1wbGUuY29thwR/AAABhxAAAAAAAAAAAAAAAAAA
+		AAABMA0GCSqGSIb3DQEBCwUAA4GBAFpdiiM5YAQQN0H5ZMNuHWGlprjp7qVilO8/
+		WFePZRWY2vQF8g7/c1cX4bPqG+qFJd+9j2UZNjhadNfMCxvu6BY7NCupOHVHmnRQ
+		ocvkPoSqobE7qDPfiUuU1J+61Libu6b2IjV3/K9pvZkLiBrqn0YhoXXa0PG+rG1L
+		9X7+mb5z
+		-----END CERTIFICATE-----`)
+	expiredKey := heredoc.Doc(`
+		-----BEGIN RSA PRIVATE KEY-----
+		MIICXQIBAAKBgQDjTYA1wZNkNYv2iY08LdNyqUN50G5TEtyF/x/SVsOrWulLm5Sg
+		Tmg/9qOV7RTPkO1wxJ82MPPAiuSh3cno5CaOSAaVI0GCWRc1CdQQvC8O8sunV1mA
+		SW6EUdUvtSYZDCTraT3nL/uHE2sp6ZWI9FjAY3ELYYNkTO7XaclGKbKdqwIDAQAB
+		AoGBAJPFWKqZ9CZboWhfuE/9Qs/yNonE9VRQmMkMOTXXblHCQpUCyjcFgkTDJUpc
+		3QCsKZD8Yr0qSe1M3qJUu+UKHf18LqwiL/ynnalYggxIFS5/SidWCngKvIuEfkLK
+		VsnCK3jt5qx21iljGHU6bQZHnHB9IGEiBYcnQlvvw/WdvRDBAkEA8/KMpJVwnI1W
+		7fzcZ1+mbMeSJoAVIa9u7MgI+LIRZMokDRYeAMvEjm3GYpZDqA5l1dp7KochMep/
+		0vSSTHt7ewJBAO6IbcUIDhXuh2qdxR/Xk5DdDCoxaD1o4ivyj9JsSlGa9JWD7kKN
+		6ZFFrn8i7uQuniC1Rwc/4yHhs6OqbiF695ECQQCBwVKzvFUwwDEr1yK4zXStSZ3g
+		YqJaz4CV63RyK+z6ilaQq2H8FGaRR6yNBdYozre1/0ciAMxUS6H/6Fzk141/AkBe
+		SguqIP8AaGObH3Z2mc65KsfOPe2IqNcOrDlx4mCWVXxtRdN+933mcPcDRpnMFSlo
+		oH/NO9Ha6M8L2SjjjyohAkBJHU61+OWz/TAy1nxsMbFsISLn/JrdEZIf2uFORlDN
+		Z3/XIQ+yeg4Jk1VbTMZ0/fHf9xMFR8acC/7n7jxnzQau
+		-----END RSA PRIVATE KEY-----`)
+
+	server, err := newTLSServer(expiredCert, expiredKey)
+	if err != nil {
+		t.Errorf(err.Error())
+	}
+	server.StartTLS()
+	defer server.Close()
+
+	testCases := map[string]struct {
+		serverURL      string
+		skipTLSVerify  bool
+		expectedErrMsg *regexp.Regexp
+	}{
+		"succeed skipping tls": {
+			serverURL:     server.URL,
+			skipTLSVerify: true,
+		},
+		"certificate expired": {
+			serverURL:      server.URL,
+			expectedErrMsg: regexp.MustCompile(`The server is using an invalid certificate(.*)has expired`),
+		},
+	}
+
+	for name, test := range testCases {
+		t.Logf("evaluating test: %s", name)
+		options := &LoginOptions{
+			Server:             test.serverURL,
+			InsecureTLS:        test.skipTLSVerify,
+			StartingKubeConfig: &kclientcmdapi.Config{},
+		}
+
+		if _, err = options.getClientConfig(); err != nil {
+			if !test.expectedErrMsg.MatchString(err.Error()) {
+				t.Errorf("%s: expected error %q but got %q", name, test.expectedErrMsg, err)
+			}
+			if test.expectedErrMsg == nil {
+				t.Errorf("%s: unexpected error: %v", name, err)
+			}
+		} else {
+			if test.expectedErrMsg != nil {
+				t.Errorf("%s: expected error but got nothing", name)
+			}
+		}
+	}
+}
+
 func TestDialToHTTPServer(t *testing.T) {
 	invoked := make(chan struct{}, 1)
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -92,3 +255,60 @@ func TestDialToHTTPServer(t *testing.T) {
 		}
 	}
 }
+
+func TestDialToHTTPSServer(t *testing.T) {
+	invoked := make(chan struct{}, 1)
+	server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		invoked <- struct{}{}
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	testCases := map[string]struct {
+		serverURL       string
+		skipTLSVerify   bool
+		evalExpectedErr func(error) bool
+	}{
+		"succeed dialing": {
+			serverURL:     server.URL,
+			skipTLSVerify: true,
+		},
+		"certificate unknown": {
+			serverURL:       server.URL,
+			evalExpectedErr: clientcmd.IsCertificateAuthorityUnknown,
+		},
+	}
+
+	for name, test := range testCases {
+		t.Logf("evaluating test: %s", name)
+		clientConfig := &restclient.Config{
+			Host:     test.serverURL,
+			Insecure: test.skipTLSVerify,
+		}
+		if err := dialToServer(*clientConfig); err != nil {
+			if test.evalExpectedErr == nil || !test.evalExpectedErr(err) {
+				t.Errorf("%s: unexpected error: %v", name, err)
+			}
+		} else {
+			if test.evalExpectedErr != nil {
+				t.Errorf("%s: expected error but got nothing", name)
+			}
+		}
+	}
+}
+
+func newTLSServer(certString, keyString string) (*httptest.Server, error) {
+	invoked := make(chan struct{}, 1)
+	server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		invoked <- struct{}{}
+		w.WriteHeader(http.StatusOK)
+	}))
+	cert, err := tls.X509KeyPair([]byte(certString), []byte(keyString))
+	if err != nil {
+		return nil, fmt.Errorf("error configuring server cert: %s", err)
+	}
+	server.TLS = &tls.Config{
+		Certificates: []tls.Certificate{cert},
+	}
+	return server, nil
+}

pkg/cmd/util/clientcmd/errors.go

@@ -1,6 +1,7 @@
 package clientcmd
 
 import (
+	"crypto/x509"
 	"errors"
 	"fmt"
 	"strings"
@@ -13,6 +14,8 @@ const (
 	unknownReason = iota
 	noServerFoundReason
 	certificateAuthorityUnknownReason
+	certificateHostnameErrorReason
+	certificateInvalidReason
 	configurationInvalidReason
 	tlsOversizedRecordReason
 
@@ -50,6 +53,12 @@ func GetPrettyMessageForServer(err error, serverName string) string {
 			serverName = "server"
 		}
 		return fmt.Sprintf(tlsOversizedRecordMsg, err, serverName)
+
+	case certificateHostnameErrorReason:
+		return fmt.Sprintf("The server is using a certificate that does not match its hostname: %s", err)
+
+	case certificateInvalidReason:
+		return fmt.Sprintf("The server is using an invalid certificate: %s", err)
 	}
 
 	return err.Error()
@@ -91,6 +100,17 @@ func IsTLSOversizedRecord(err error) bool {
 	return detectReason(err) == tlsOversizedRecordReason
 }
 
+// IsCertificateHostnameError checks whether the set of authorized names doesn't match the requested name
+func IsCertificateHostnameError(err error) bool {
+	return detectReason(err) == certificateHostnameErrorReason
+}
+
+// IsCertificateInvalid checks whether the certificate is invalid for reasons like expired,	CA not authorized
+// to sign, there are too many cert intermediates, or the cert usage is not valid for the wanted purpose.
+func IsCertificateInvalid(err error) bool {
+	return detectReason(err) == certificateInvalidReason
+}
+
 func detectReason(err error) int {
 	if err != nil {
 		switch {
@@ -103,6 +123,14 @@ func detectReason(err error) int {
 		case strings.Contains(err.Error(), "tls: oversized record received"):
 			return tlsOversizedRecordReason
 		}
+		switch err.(type) {
+		case x509.UnknownAuthorityError:
+			return certificateAuthorityUnknownReason
+		case x509.HostnameError:
+			return certificateHostnameErrorReason
+		case x509.CertificateInvalidError:
+			return certificateInvalidReason
+		}
 	}
 	return unknownReason
 }