oc login: Respect insecure flag from kubeconfig

When running oc login, the insecure flag from kubeconfig is not
consulted properly when calling getClientConfig(). This is now fixed.

Assisted-by: Claude Code

Author: tchap

pkg/cli/login/helpers.go

@@ -30,20 +30,21 @@ func getMatchingClusters(clientConfig restclient.Config, kubeconfig clientcmdapi
 	return ret
 }
 
-// findExistingClientCA returns *either* the existing client CA file name as a string,
-// *or* data in a []byte for a given host, and true if it exists in the given config
-func findExistingClientCA(host string, kubeconfig clientcmdapi.Config) (string, []byte, bool) {
+// findCluster returns the cluster matching the given host and optionally the cond function, which can be nil.
+func findCluster(host string, kubeconfig clientcmdapi.Config, cond func(*clientcmdapi.Cluster) bool) *clientcmdapi.Cluster {
 	for _, cluster := range kubeconfig.Clusters {
-		if cluster.Server == host {
-			if len(cluster.CertificateAuthority) > 0 {
-				return cluster.CertificateAuthority, nil, true
-			}
-			if len(cluster.CertificateAuthorityData) > 0 {
-				return "", cluster.CertificateAuthorityData, true
-			}
+		if cluster.Server == host && (cond == nil || cond(cluster)) {
+			return cluster
 		}
 	}
-	return "", nil, false
+	return nil
+}
+
+// findClusterWithCA returns the cluster matching the given host and having CertificateAuthority or CertificateAuthorityData set.
+func findClusterWithCA(host string, kubeconfig clientcmdapi.Config) *clientcmdapi.Cluster {
+	return findCluster(host, kubeconfig, func(c *clientcmdapi.Cluster) bool {
+		return len(c.CertificateAuthorityData) > 0 || len(c.CertificateAuthority) > 0
+	})
 }
 
 // dialToServer takes the Server URL from the given clientConfig and dials to

pkg/cli/login/loginoptions.go

@@ -162,16 +162,26 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {
 	clientConfig.Host = o.Server
 	clientConfig.Insecure = o.InsecureTLS
 
-	if !o.InsecureTLS {
-		// use specified CA or find existing CA
+	if !clientConfig.Insecure {
+		// Try to use the specified CA. Then fall back into searching kubeconfig.
+		// In case no CA is found, still check the kubeconfig for insecure-skip-tls-verify.
 		if len(o.CAFile) > 0 {
 			clientConfig.CAFile = o.CAFile
 			clientConfig.CAData = nil
-		} else if caFile, caData, ok := findExistingClientCA(clientConfig.Host, *o.StartingKubeConfig); ok {
-			clientConfig.CAFile = caFile
-			clientConfig.CAData = caData
+		} else if cluster := findClusterWithCA(clientConfig.Host, *o.StartingKubeConfig); cluster != nil {
+			clientConfig.Insecure = cluster.InsecureSkipTLSVerify
+			clientConfig.CAFile = cluster.CertificateAuthority
+			clientConfig.CAData = cluster.CertificateAuthorityData
+		} else if cluster := findCluster(clientConfig.Host, *o.StartingKubeConfig, nil); cluster != nil {
+			clientConfig.Insecure = cluster.InsecureSkipTLSVerify
 		}
 	}
+	// In case the kubeconfig contained insecure-skip-tls-verify, we must unset the CA settings,
+	// otherwise an error is returned from k8s transport init machinery.
+	if clientConfig.Insecure {
+		clientConfig.CAFile = ""
+		clientConfig.CAData = nil
+	}
 
 	// try to TCP connect to the server to make sure it's reachable, and discover
 	// about the need of certificates or insecure TLS
@@ -188,7 +198,7 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {
 		// connection or if we already have a cluster stanza that tells us to
 		// connect to this particular server insecurely
 		case x509.UnknownAuthorityError, x509.HostnameError, x509.CertificateInvalidError:
-			if o.InsecureTLS ||
+			if clientConfig.Insecure ||
 				hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig) ||
 				promptForInsecureTLS(o.In, o.Out, err) {
 				clientConfig.Insecure = true

pkg/cli/login/loginoptions_test.go

@@ -1,14 +1,17 @@
 package login
 
 import (
+	"bytes"
 	"crypto/tls"
+	"encoding/base64"
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"regexp"
 	"testing"
+	"time"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/google/go-cmp/cmp"
@@ -20,6 +23,7 @@ import (
 	kclientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 
 	userv1 "github.com/openshift/api/user/v1"
+	"github.com/openshift/library-go/pkg/crypto"
 	"github.com/openshift/library-go/pkg/oauth/oauthdiscovery"
 	cliconfig "github.com/openshift/oc/pkg/helpers/kubeconfig"
 )
@@ -377,6 +381,87 @@ func TestDialToHTTPSServer(t *testing.T) {
 	}
 }
 
+func TestGetClientConfig_InsecureSkipTLSVerify(t *testing.T) {
+	// Test that insecure-skip-tls-verify setting from kubeconfig is respected
+	// when logging in without the --insecure-skip-tls-verify flag.
+
+	// Generate a foo CA cert to use in the kubeconfig. This doesn't match the testing TLS server.
+	cert, _ := generateCA(t, "test", 1*time.Hour)
+	certBase64 := base64.StdEncoding.EncodeToString(cert)
+
+	server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	testCases := map[string]struct {
+		insecureFlag                 bool
+		insecureKubeconfig           bool
+		expectedInsecureClientConfig bool
+	}{
+		"command flag set": {
+			insecureFlag:                 true,
+			expectedInsecureClientConfig: true,
+		},
+		"kubeconfig flag set": {
+			insecureKubeconfig:           true,
+			expectedInsecureClientConfig: true,
+		},
+		"no flag set": {
+			insecureFlag:                 false,
+			insecureKubeconfig:           false,
+			expectedInsecureClientConfig: false,
+		},
+		"both command and kubeconfig flag set": {
+			insecureFlag:                 true,
+			insecureKubeconfig:           true,
+			expectedInsecureClientConfig: true,
+		},
+	}
+
+	for name, test := range testCases {
+		t.Run(name, func(t *testing.T) {
+			startingConfig := &kclientcmdapi.Config{
+				Clusters: map[string]*kclientcmdapi.Cluster{},
+			}
+			if test.insecureKubeconfig {
+				startingConfig.Clusters["test-cluster"] = &kclientcmdapi.Cluster{
+					Server: server.URL,
+					// CertificateAuthority is only set so that we can see getClientConfig works fine when it is set.
+					// It's actually dropped by getClientConfig since CA cert cannot be set together with insecure.
+					// That's enforces by the k8s client transport init machinery.
+					CertificateAuthority:  certBase64,
+					InsecureSkipTLSVerify: true,
+				}
+			}
+
+			options := &LoginOptions{
+				Server:             server.URL,
+				InsecureTLS:        test.insecureFlag,
+				StartingKubeConfig: startingConfig,
+			}
+
+			clientConfig, err := options.getClientConfig()
+			if err != nil {
+				if test.expectedInsecureClientConfig {
+					t.Fatalf("Expected to succeed with insecure connection, but got error: %v", err)
+				} else {
+					// If we expect secure connection and get a TLS error, that's expected
+					// since we're using a test server with a self-signed cert.
+					if err.Error() != certificateAuthorityUnknownMsg {
+						t.Fatalf("Expected to fail with insecure connection, but got another error: %v", err)
+					}
+					return
+				}
+			}
+
+			if clientConfig.Insecure != test.expectedInsecureClientConfig {
+				t.Errorf("expected Insecure=%v, got %v", test.expectedInsecureClientConfig, clientConfig.Insecure)
+			}
+		})
+	}
+}
+
 func TestPreserveExecProviderOnUsernameLogin(t *testing.T) {
 	// Test that when using -u flag with existing OIDC credentials,
 	// the ExecProvider configuration is preserved
@@ -584,3 +669,17 @@ func newTLSServer(certString, keyString string) (*httptest.Server, error) {
 	}
 	return server, nil
 }
+
+func generateCA(t testing.TB, name string, expiration time.Duration) (cert, key []byte) {
+	ca, err := crypto.MakeSelfSignedCAConfigForDuration(name, expiration)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var certBytes, keyBytes bytes.Buffer
+	if err = ca.WriteCertConfig(&certBytes, &keyBytes); err != nil {
+		t.Fatal(err)
+	}
+
+	return certBytes.Bytes(), keyBytes.Bytes()
+}