Forbid kubeconfig exec

Users with write access to the admin kubeconfig Secret for a given
ClusterDeployment should not be able to execute arbitrary code in the
privileged environment in which we run the controllers that use those
Secrets. Funnel all code paths that load such Secrets through a
validator to ensure that the AuthInfos[].Exec path is not used.

HIVE-2485

pkg/controller/argocdregister/argocdregister_controller.go

@@ -211,7 +211,7 @@ func (r *ArgoCDRegisterController) reconcileCluster(cd *hivev1.ClusterDeployment
 		return reconcile.Result{}, nil
 	}
 
-	kubeConfigSecretName := cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name
+	kubeConfigSecretName := cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name // HIVE-2485 ✓
 	argoCDServerConfigBytes, err := r.generateArgoCDServerConfig(kubeConfigSecretName, cd.Namespace, argoCDNamespace, cdLog)
 	if err != nil {
 		return reconcile.Result{}, err
@@ -320,7 +320,7 @@ func (r *ArgoCDRegisterController) loadArgoCDServiceAccountToken(argoCDNamespace
 }
 
 func (r *ArgoCDRegisterController) generateArgoCDServerConfig(kubeconfigSecretName, kubeConfigSecretNamespace, argoCDNamespace string, cdLog log.FieldLogger) ([]byte, error) {
-	kubeconfig, err := r.loadSecretData(kubeconfigSecretName, kubeConfigSecretNamespace, adminKubeConfigKey)
+	kubeconfig, err := controllerutils.LoadSecretData(r.Client, kubeconfigSecretName, kubeConfigSecretNamespace, adminKubeConfigKey)
 	if err != nil {
 		cdLog.WithError(err).Error("unable to load cluster admin kubeconfig")
 		return nil, err
@@ -375,19 +375,6 @@ func tlsClientConfigBuilderFunc(kubeConfig clientcmd.ClientConfig, cdLog log.Fie
 	return tlsClientConfig, nil
 }
 
-func (r *ArgoCDRegisterController) loadSecretData(secretName, namespace, dataKey string) (string, error) {
-	s := &corev1.Secret{}
-	err := r.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: namespace}, s)
-	if err != nil {
-		return "", err
-	}
-	retStr, ok := s.Data[dataKey]
-	if !ok {
-		return "", fmt.Errorf("secret %s did not contain key %s", secretName, dataKey)
-	}
-	return string(retStr), nil
-}
-
 // getPredictableSecretName generates a unique secret name by hashing the server API URL,
 // which is required as all cluster secrets land in the argocd namespace.
 // This code matches what is presently done in ArgoCD (util/db/cluster.go). However the actual

pkg/controller/awsprivatelink/awsprivatelink_controller.go

@@ -25,8 +25,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"k8s.io/client-go/rest"
-	"k8s.io/client-go/tools/clientcmd"
 	"k8s.io/client-go/util/flowcontrol"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/client-go/util/workqueue"
@@ -301,7 +299,13 @@ func (r *ReconcileAWSPrivateLink) Reconcile(ctx context.Context, request reconci
 		return reconcile.Result{}, nil
 	}
 
-	return r.reconcilePrivateLink(cd, &hivev1.ClusterMetadata{InfraID: *cp.Spec.InfraID, AdminKubeconfigSecretRef: *cp.Spec.AdminKubeconfigSecretRef}, logger)
+	return r.reconcilePrivateLink(
+		cd,
+		&hivev1.ClusterMetadata{
+			InfraID:                  *cp.Spec.InfraID,
+			AdminKubeconfigSecretRef: *cp.Spec.AdminKubeconfigSecretRef, // HIVE-2585: via ClusterMetadata
+		},
+		logger)
 }
 
 // shouldSync returns if we should sync the desired ClusterDeployment. If it returns false, it also returns
@@ -533,7 +537,7 @@ func (r *ReconcileAWSPrivateLink) reconcilePrivateLink(cd *hivev1.ClusterDeploym
 
 	// Figure out the API address for cluster.
 	apiDomain, err := initialURL(r.Client,
-		client.ObjectKey{Namespace: cd.Namespace, Name: clusterMetadata.AdminKubeconfigSecretRef.Name})
+		client.ObjectKey{Namespace: cd.Namespace, Name: clusterMetadata.AdminKubeconfigSecretRef.Name}) // HIVE-2485 ✓
 	if err != nil {
 		logger.WithError(err).Error("could not get API URL from kubeconfig")
 
@@ -1303,7 +1307,7 @@ func initialURL(c client.Client, key client.ObjectKey) (string, error) {
 	); err != nil {
 		return "", err
 	}
-	cfg, err := restConfigFromSecret(kubeconfigSecret)
+	cfg, err := controllerutils.RestConfigFromSecret(kubeconfigSecret, true)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to load the kubeconfig")
 	}
@@ -1315,22 +1319,6 @@ func initialURL(c client.Client, key client.ObjectKey) (string, error) {
 	return strings.TrimSuffix(u.Hostname(), "."), nil
 }
 
-func restConfigFromSecret(kubeconfigSecret *corev1.Secret) (*rest.Config, error) {
-	kubeconfigData := kubeconfigSecret.Data[constants.RawKubeconfigSecretKey]
-	if len(kubeconfigData) == 0 {
-		kubeconfigData = kubeconfigSecret.Data[constants.KubeconfigSecretKey]
-	}
-	if len(kubeconfigData) == 0 {
-		return nil, errors.New("kubeconfig secret does not contain necessary data")
-	}
-	config, err := clientcmd.Load(kubeconfigData)
-	if err != nil {
-		return nil, err
-	}
-	kubeConfig := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{})
-	return kubeConfig.ClientConfig()
-}
-
 // awsErrCodeEquals returns true if the error matches all these conditions:
 //   - err is of type awserr.Error
 //   - Error.Code() equals code

pkg/controller/awsprivatelink/cleanup.go

@@ -61,7 +61,7 @@ func (r *ReconcileAWSPrivateLink) cleanupPreviousProvisionAttempt(cd *hivev1.Clu
 	}
 	metadata := &hivev1.ClusterMetadata{
 		InfraID:                  *cp.Spec.PrevInfraID,
-		AdminKubeconfigSecretRef: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef,
+		AdminKubeconfigSecretRef: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef, // HIVE-2485: via ClusterMetadata
 	}
 
 	if err := r.cleanupPrivateLink(cd, metadata, logger); err != nil {
@@ -138,7 +138,7 @@ func (r *ReconcileAWSPrivateLink) cleanupHostedZone(awsClient awsclient.Client,
 
 	if hzID == "" { // since we don't have the hz ID, we try to discover it to prevent leaks
 		apiDomain, err := initialURL(r.Client,
-			client.ObjectKey{Namespace: cd.Namespace, Name: metadata.AdminKubeconfigSecretRef.Name})
+			client.ObjectKey{Namespace: cd.Namespace, Name: metadata.AdminKubeconfigSecretRef.Name}) // HIVE-2485 ✓
 		if apierrors.IsNotFound(err) {
 			logger.Info("no hostedZoneID in status and admin kubeconfig does not exist, skipping hosted zone cleanup")
 			return nil

pkg/controller/clusterclaim/clusterclaim_controller.go

@@ -549,7 +549,7 @@ func (r *ReconcileClusterClaim) applyHiveClaimOwnerRole(claim *hivev1.ClusterCla
 				APIGroups: []string{corev1.GroupName},
 				Resources: []string{"secrets"},
 				ResourceNames: []string{
-					cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name,
+					cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name, // HIVE-2485: n/a
 					cd.Spec.ClusterMetadata.AdminPasswordSecretRef.Name,
 				},
 				Verbs: []string{"get"},

pkg/controller/clusterdeployment/clusterdeployment_controller.go

@@ -456,7 +456,14 @@ func (r *ReconcileClusterDeployment) addAdditionalKubeconfigCAs(cd *hivev1.Clust
 	cdLog log.FieldLogger) error {
 
 	adminKubeconfigSecret := &corev1.Secret{}
-	if err := r.Get(context.Background(), types.NamespacedName{Namespace: cd.Namespace, Name: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name}, adminKubeconfigSecret); err != nil {
+	if err := r.Get(
+		context.Background(),
+		types.NamespacedName{
+			Namespace: cd.Namespace,
+			// HIVE-2485: No need to scrub here
+			Name: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name,
+		},
+		adminKubeconfigSecret); err != nil {
 		cdLog.WithError(err).Error("failed to get admin kubeconfig secret")
 		return err
 	}
@@ -658,6 +665,7 @@ func (r *ReconcileClusterDeployment) reconcile(request reconcile.Request, cd *hi
 			}
 
 			// Add cluster deployment as additional owner reference to admin secrets
+			// HIVE-2485: No need to scrub here
 			if err := r.addOwnershipToSecret(cd, cdLog, cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name); err != nil {
 				return reconcile.Result{}, err
 			}

pkg/controller/clusterdeployment/clusterinstalls.go

@@ -54,7 +54,7 @@ func (r *ReconcileClusterDeployment) reconcileExistingInstallingClusterInstall(c
 	if met := ci.Spec.ClusterMetadata; met != nil &&
 		met.InfraID != "" &&
 		met.ClusterID != "" &&
-		met.AdminKubeconfigSecretRef.Name != "" &&
+		met.AdminKubeconfigSecretRef.Name != "" && // HIVE-2485: via ClusterMetadata
 		met.AdminPasswordSecretRef != nil &&
 		met.AdminPasswordSecretRef.Name != "" {
 		if !reflect.DeepEqual(cd.Spec.ClusterMetadata, ci.Spec.ClusterMetadata) {

pkg/controller/clusterdeployment/clusterprovisions.go

@@ -423,7 +423,7 @@ func (r *ReconcileClusterDeployment) reconcileExistingProvision(cd *hivev1.Clust
 			clusterMetadata.ClusterID = *provision.Spec.ClusterID
 		}
 		if provision.Spec.AdminKubeconfigSecretRef != nil {
-			clusterMetadata.AdminKubeconfigSecretRef = *provision.Spec.AdminKubeconfigSecretRef
+			clusterMetadata.AdminKubeconfigSecretRef = *provision.Spec.AdminKubeconfigSecretRef // HIVE-2485: via ClusterMetadata
 		}
 		if provision.Spec.AdminPasswordSecretRef != nil {
 			clusterMetadata.AdminPasswordSecretRef = provision.Spec.AdminPasswordSecretRef

pkg/controller/utils/secrets.go

@@ -2,23 +2,76 @@ package utils
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
+	"github.com/openshift/hive/pkg/constants"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/clientcmd"
+	"k8s.io/client-go/tools/clientcmd/api"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// LoadSecretData loads a given secret key and returns it's data as a string.
+// LoadSecretData loads a given secret key and returns its data as a string.
 func LoadSecretData(c client.Client, secretName, namespace, dataKey string) (string, error) {
 	s := &corev1.Secret{}
 	err := c.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: namespace}, s)
 	if err != nil {
 		return "", err
 	}
-	retStr, ok := s.Data[dataKey]
+	retBytes, ok := s.Data[dataKey]
 	if !ok {
 		return "", fmt.Errorf("secret %s did not contain key %s", secretName, dataKey)
 	}
-	return string(retStr), nil
+	switch dataKey {
+	case constants.KubeconfigSecretKey, constants.RawKubeconfigSecretKey:
+		if _, err := validateKubeconfig(retBytes); err != nil {
+			return "", err
+		}
+	}
+	return string(retBytes), nil
+}
+
+// RestConfigFromSecret accepts a Secret containing `kubeconfig` and/or `raw-kubeconfig` keys
+// and returns a rest.Config loaded therefrom.
+// If tryRaw is true, we will look for `raw-kubeconfig` first and use it if present, falling
+// back to `kubeconfig` otherwise.
+// The error return is non-nil if:
+// - The Secret's Data does not contain the [raw-]kubeconfig key(s)
+// - The kubeconfig data cannot be Load()ed
+// - The kubeconfig is insecure
+func RestConfigFromSecret(kubeconfigSecret *corev1.Secret, tryRaw bool) (*rest.Config, error) {
+	var kubeconfigData []byte
+	if tryRaw {
+		kubeconfigData = kubeconfigSecret.Data[constants.RawKubeconfigSecretKey]
+	}
+	if len(kubeconfigData) == 0 {
+		kubeconfigData = kubeconfigSecret.Data[constants.KubeconfigSecretKey]
+	}
+	if len(kubeconfigData) == 0 {
+		return nil, errors.New("kubeconfig secret does not contain necessary data")
+	}
+	config, err := validateKubeconfig(kubeconfigData)
+	if err != nil {
+		return nil, err
+	}
+	kubeConfig := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{})
+	return kubeConfig.ClientConfig()
+}
+
+// validateKubeconfig ensures the kubeconfig represented by kc does not use insecure paths.
+// HIVE-2485
+func validateKubeconfig(kc []byte) (*api.Config, error) {
+	config, err := clientcmd.Load(kc)
+	if err != nil {
+		return nil, err
+	}
+	for k, ai := range config.AuthInfos {
+		if ai.Exec != nil {
+			return nil, fmt.Errorf("insecure exec in AuthInfos[%s]", k)
+		}
+	}
+	return config, nil
 }

pkg/remoteclient/kubeconfig.go

@@ -1,6 +1,7 @@
 package remoteclient
 
 import (
+	"github.com/openshift/hive/pkg/controller/utils"
 	"github.com/openshift/hive/pkg/util/scheme"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/discovery"
@@ -81,5 +82,5 @@ func (b *kubeconfigBuilder) UseSecondaryAPIURL() Builder {
 }
 
 func (b *kubeconfigBuilder) RESTConfig() (*rest.Config, error) {
-	return restConfigFromSecret(b.secret)
+	return utils.RestConfigFromSecret(b.secret, false)
 }

pkg/remoteclient/remoteclient.go

@@ -18,11 +18,9 @@ import (
 	kubeclient "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/restmapper"
-	"k8s.io/client-go/tools/clientcmd"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	hivev1 "github.com/openshift/hive/apis/hive/v1"
-	"github.com/openshift/hive/pkg/constants"
 	"github.com/openshift/hive/pkg/controller/utils"
 	"github.com/openshift/hive/pkg/util/scheme"
 )
@@ -291,23 +289,11 @@ func unadulteratedRESTConfig(c client.Client, cd *hivev1.ClusterDeployment) (*re
 	kubeconfigSecret := &corev1.Secret{}
 	if err := c.Get(
 		context.Background(),
+		// HIVE-2485 ✓
 		client.ObjectKey{Namespace: cd.Namespace, Name: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name},
 		kubeconfigSecret,
 	); err != nil {
 		return nil, errors.Wrap(err, "could not get admin kubeconfig secret")
 	}
-	return restConfigFromSecret(kubeconfigSecret)
-}
-
-func restConfigFromSecret(kubeconfigSecret *corev1.Secret) (*rest.Config, error) {
-	kubeconfigData, ok := kubeconfigSecret.Data[constants.KubeconfigSecretKey]
-	if !ok {
-		return nil, errors.Errorf("kubeconfig secret does not contain %q data", constants.KubeconfigSecretKey)
-	}
-	config, err := clientcmd.Load(kubeconfigData)
-	if err != nil {
-		return nil, err
-	}
-	kubeConfig := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{})
-	return kubeConfig.ClientConfig()
+	return utils.RestConfigFromSecret(kubeconfigSecret, false)
 }