Provide configuration option to disallow omitting known_hosts

This commit closes #2981

Prior to this commit when a Git SSH secret does not include a known_hosts field, Tekton will accept any public key that an SSH/Git server returns. This could be a security concern and may not be an organization's desired behavior.

This commit introduces a feature flag `require-git-ssh-secret-known-hosts`: the default value is false. When the flag is true, `known_host` field must be included in Git SSH Secret.

`checkGitSSHSecret()` will check the type of every secrets. If the type of the secret is `kubernetes.io/ssh-auth` and `require-git-ssh-secret-known-hosts` is true, `checkGitSSHSecret` will validate whether `known_hosts` field is in the secret data. If `known_hosts` is not found, it will emit an error which ceases the creation of the pod.

config/config-feature-flags.yaml

@@ -58,3 +58,9 @@ data:
   #
   # See https://github.com/tektoncd/pipeline/issues/2080 for more info.
   running-in-environment-with-injected-sidecars: "true"
+  # Setting this flag to "true" will require that any Git SSH Secret
+  # offered to Tekton must have known_hosts included.
+  #
+  # See https://github.com/tektoncd/pipeline/issues/2981 for more
+  # info.
+  require-git-ssh-secret-known-hosts: "false"

pkg/apis/config/feature_flags.go

@@ -29,10 +29,12 @@ const (
 	disableWorkingDirOverwriteKey           = "disable-working-directory-overwrite"
 	disableAffinityAssistantKey             = "disable-affinity-assistant"
 	runningInEnvWithInjectedSidecarsKey     = "running-in-environment-with-injected-sidecars"
+	requireGitSSHSecretKnownHostsKey        = "require-git-ssh-secret-known-hosts" // nolint: gosec
 	DefaultDisableHomeEnvOverwrite          = false
 	DefaultDisableWorkingDirOverwrite       = false
 	DefaultDisableAffinityAssistant         = false
 	DefaultRunningInEnvWithInjectedSidecars = true
+	DefaultRequireGitSSHSecretKnownHosts    = false
 )
 
 // FeatureFlags holds the features configurations
@@ -42,6 +44,7 @@ type FeatureFlags struct {
 	DisableWorkingDirOverwrite       bool
 	DisableAffinityAssistant         bool
 	RunningInEnvWithInjectedSidecars bool
+	RequireGitSSHSecretKnownHosts    bool
 }
 
 // GetFeatureFlagsConfigName returns the name of the configmap containing all
@@ -81,6 +84,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) {
 	if err := setFeature(runningInEnvWithInjectedSidecarsKey, DefaultRunningInEnvWithInjectedSidecars, &tc.RunningInEnvWithInjectedSidecars); err != nil {
 		return nil, err
 	}
+	if err := setFeature(requireGitSSHSecretKnownHostsKey, DefaultRequireGitSSHSecretKnownHosts, &tc.RequireGitSSHSecretKnownHosts); err != nil {
+		return nil, err
+	}
 	return &tc, nil
 }
 

pkg/apis/config/feature_flags_test.go

@@ -45,6 +45,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
 				DisableWorkingDirOverwrite:       true,
 				DisableAffinityAssistant:         true,
 				RunningInEnvWithInjectedSidecars: false,
+				RequireGitSSHSecretKnownHosts:    true,
 			},
 			fileName: "feature-flags-all-flags-set",
 		},

pkg/apis/config/testdata/feature-flags-all-flags-set.yaml

@@ -22,3 +22,4 @@ data:
   disable-working-directory-overwrite: "true"
   disable-affinity-assistant: "true"
   running-in-environment-with-injected-sidecars: "false"
+  require-git-ssh-secret-known-hosts: "true"

pkg/apis/config/testdata/feature-flags.yaml

@@ -22,3 +22,4 @@ data:
   disable-working-directory-overwrite: "false"
   disable-affinity-assistant: "false"
   running-in-environment-with-injected-sidecars: "true"
+  require-git-ssh-secret-known-hosts: "false"

pkg/pod/creds_init.go

@@ -17,6 +17,7 @@ limitations under the License.
 package pod
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
@@ -30,7 +31,10 @@ import (
 	"k8s.io/client-go/kubernetes"
 )
 
-const credsInitHomeMountPrefix = "tekton-creds-init-home"
+const (
+	credsInitHomeMountPrefix = "tekton-creds-init-home"
+	sshKnownHosts            = "known_hosts"
+)
 
 // credsInit reads secrets available to the given service account and
 // searches for annotations matching a specific format (documented in
@@ -43,7 +47,7 @@ const credsInitHomeMountPrefix = "tekton-creds-init-home"
 // Any errors encountered during this process are returned to the
 // caller. If no matching annotated secrets are found, nil lists with a
 // nil error are returned.
-func credsInit(serviceAccountName, namespace string, kubeclient kubernetes.Interface) ([]string, []corev1.Volume, []corev1.VolumeMount, error) {
+func credsInit(ctx context.Context, serviceAccountName, namespace string, kubeclient kubernetes.Interface) ([]string, []corev1.Volume, []corev1.VolumeMount, error) {
 	// service account if not specified in pipeline/task spec, read it from the ConfigMap
 	// and defaults to `default` if its missing from the ConfigMap as well
 	if serviceAccountName == "" {
@@ -66,6 +70,10 @@ func credsInit(serviceAccountName, namespace string, kubeclient kubernetes.Inter
 			return nil, nil, nil, err
 		}
 
+		if err := checkGitSSHSecret(ctx, secret); err != nil {
+			return nil, nil, nil, err
+		}
+
 		matched := false
 		for _, b := range builders {
 			if sa := b.MatchingAnnotations(secret); len(sa) > 0 {
@@ -115,3 +123,17 @@ func getCredsInitVolume() (corev1.Volume, corev1.VolumeMount) {
 	}
 	return v, vm
 }
+
+// checkGitSSHSecret requires `known_host` field must be included in Git SSH Secret when feature flag
+// `require-git-ssh-secret-known-hosts` is true.
+func checkGitSSHSecret(ctx context.Context, secret *corev1.Secret) error {
+	cfg := config.FromContextOrDefaults(ctx)
+
+	if secret.Type == corev1.SecretTypeSSHAuth && cfg.FeatureFlags.RequireGitSSHSecretKnownHosts {
+		if _, ok := secret.Data[sshKnownHosts]; !ok {
+			return fmt.Errorf("TaskRun validation failed. Git SSH Secret must have \"known_hosts\" included " +
+				"when feature flag \"require-git-ssh-secret-known-hosts\" is set to true")
+		}
+	}
+	return nil
+}

pkg/pod/creds_init_test.go

@@ -17,20 +17,25 @@ limitations under the License.
 package pod
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/tektoncd/pipeline/pkg/apis/config"
+	"github.com/tektoncd/pipeline/pkg/system"
 	"github.com/tektoncd/pipeline/test/diff"
 	"github.com/tektoncd/pipeline/test/names"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	fakek8s "k8s.io/client-go/kubernetes/fake"
+	logtesting "knative.dev/pkg/logging/testing"
 )
 
 const (
-	serviceAccountName = "my-service-account"
-	namespace          = "namespacey-mcnamespace"
+	serviceAccountName           = "my-service-account"
+	namespace                    = "namespacey-mcnamespace"
+	featureFlagRequireKnownHosts = "require-git-ssh-secret-known-hosts"
 )
 
 func TestCredsInit(t *testing.T) {
@@ -153,7 +158,7 @@ func TestCredsInit(t *testing.T) {
 		t.Run(c.desc, func(t *testing.T) {
 			names.TestingSeed()
 			kubeclient := fakek8s.NewSimpleClientset(c.objs...)
-			args, volumes, volumeMounts, err := credsInit(serviceAccountName, namespace, kubeclient)
+			args, volumes, volumeMounts, err := credsInit(context.Background(), serviceAccountName, namespace, kubeclient)
 			if err != nil {
 				t.Fatalf("credsInit: %v", err)
 			}
@@ -169,3 +174,94 @@ func TestCredsInit(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckGitSSHSecret(t *testing.T) {
+	for _, tc := range []struct {
+		desc         string
+		configMap    *corev1.ConfigMap
+		secret       *corev1.Secret
+		wantErrorMsg string
+	}{{
+		desc: "require known_hosts but secret does not include known_hosts",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagRequireKnownHosts: "true",
+			},
+		},
+		secret: &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-creds",
+				Namespace: namespace,
+				Annotations: map[string]string{
+					"tekton.dev/git-0": "github.com",
+				},
+			},
+			Type: "kubernetes.io/ssh-auth",
+			Data: map[string][]byte{
+				"ssh-privatekey": []byte("Hello World!"),
+			},
+		},
+		wantErrorMsg: "TaskRun validation failed. Git SSH Secret must have \"known_hosts\" included " +
+			"when feature flag \"require-git-ssh-secret-known-hosts\" is set to true",
+	}, {
+		desc: "require known_hosts and secret includes known_hosts",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagRequireKnownHosts: "true",
+			},
+		},
+		secret: &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-creds",
+				Namespace: namespace,
+				Annotations: map[string]string{
+					"tekton.dev/git-0": "github.com",
+				},
+			},
+			Type: "kubernetes.io/ssh-auth",
+			Data: map[string][]byte{
+				"ssh-privatekey": []byte("Hello World!"),
+				"known_hosts":    []byte("Hello World!"),
+			},
+		},
+	}, {
+		desc: "not require known_hosts",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagRequireKnownHosts: "false",
+			},
+		},
+		secret: &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-creds",
+				Namespace: namespace,
+				Annotations: map[string]string{
+					"tekton.dev/git-0": "github.com",
+				},
+			},
+			Type: "kubernetes.io/ssh-auth",
+			Data: map[string][]byte{
+				"ssh-privatekey": []byte("Hello World!"),
+			},
+		},
+	}} {
+		t.Run(tc.desc, func(t *testing.T) {
+			store := config.NewStore(logtesting.TestLogger(t))
+			store.OnConfigChanged(tc.configMap)
+			err := checkGitSSHSecret(store.ToContext(context.Background()), tc.secret)
+
+			if wantError := tc.wantErrorMsg != ""; wantError {
+				if err == nil {
+					t.Errorf("expected error %q, got nil", tc.wantErrorMsg)
+				} else if diff := cmp.Diff(tc.wantErrorMsg, err.Error()); diff != "" {
+					t.Errorf("unexpected (-want, +got) = %v", diff)
+				}
+			} else if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+		})
+	}
+}

pkg/pod/pod.go

@@ -106,7 +106,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
 	// Create Volumes and VolumeMounts for any credentials found in annotated
 	// Secrets, along with any arguments needed by Step entrypoints to process
 	// those secrets.
-	credEntrypointArgs, credVolumes, credVolumeMounts, err := credsInit(taskRun.Spec.ServiceAccountName, taskRun.Namespace, b.KubeClient)
+	credEntrypointArgs, credVolumes, credVolumeMounts, err := credsInit(ctx, taskRun.Spec.ServiceAccountName, taskRun.Namespace, b.KubeClient)
 	if err != nil {
 		return nil, err
 	}

pkg/reconciler/taskrun/taskrun.go

@@ -464,34 +464,35 @@ func (c *Reconciler) updateLabelsAndAnnotations(tr *v1beta1.TaskRun) (*v1beta1.T
 }
 
 func (c *Reconciler) handlePodCreationError(ctx context.Context, tr *v1beta1.TaskRun, err error) error {
-	var msg string
-	if isExceededResourceQuotaError(err) {
+	switch {
+	case isExceededResourceQuotaError(err):
 		backoff, currentlyBackingOff := c.timeoutHandler.GetBackoff(tr.GetNamespacedName(), *tr.Status.StartTime, *tr.Spec.Timeout)
 		if !currentlyBackingOff {
 			go c.timeoutHandler.SetTimer(tr.GetNamespacedName(), time.Until(backoff.NextAttempt))
 		}
-		msg = fmt.Sprintf("TaskRun Pod exceeded available resources, reattempted %d times", backoff.NumAttempts)
+		msg := fmt.Sprintf("TaskRun Pod exceeded available resources, reattempted %d times", backoff.NumAttempts)
 		tr.Status.SetCondition(&apis.Condition{
 			Type:    apis.ConditionSucceeded,
 			Status:  corev1.ConditionUnknown,
 			Reason:  podconvert.ReasonExceededResourceQuota,
 			Message: fmt.Sprintf("%s: %v", msg, err),
 		})
-		// return a transient error, so that the key is requeued
-		return err
-	}
-	// The pod creation failed, not because of quota issues. The most likely
-	// reason is that something is wrong with the spec of the Task, that we could
-	// not check with validation before - i.e. pod template fields
-	msg = fmt.Sprintf("failed to create task run pod %q: %v. Maybe ", tr.Name, err)
-	if tr.Spec.TaskRef != nil {
-		msg += fmt.Sprintf("missing or invalid Task %s/%s", tr.Namespace, tr.Spec.TaskRef.Name)
-	} else {
-		msg += "invalid TaskSpec"
+	case isTaskRunValidationFailed(err):
+		tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
+	default:
+		// The pod creation failed with unknown reason. The most likely
+		// reason is that something is wrong with the spec of the Task, that we could
+		// not check with validation before - i.e. pod template fields
+		msg := fmt.Sprintf("failed to create task run pod %q: %v. Maybe ", tr.Name, err)
+		if tr.Spec.TaskRef != nil {
+			msg += fmt.Sprintf("missing or invalid Task %s/%s", tr.Namespace, tr.Spec.TaskRef.Name)
+		} else {
+			msg += "invalid TaskSpec"
+		}
+		err = controller.NewPermanentError(errors.New(msg))
+		tr.Status.MarkResourceFailed(podconvert.ReasonCouldntGetTask, err)
 	}
-	newErr := controller.NewPermanentError(errors.New(msg))
-	tr.Status.MarkResourceFailed(podconvert.ReasonCouldntGetTask, newErr)
-	return newErr
+	return err
 }
 
 // failTaskRun stops a TaskRun with the provided Reason
@@ -638,6 +639,10 @@ func isExceededResourceQuotaError(err error) bool {
 	return err != nil && k8serrors.IsForbidden(err) && strings.Contains(err.Error(), "exceeded quota")
 }
 
+func isTaskRunValidationFailed(err error) bool {
+	return err != nil && strings.Contains(err.Error(), "TaskRun validation failed")
+}
+
 // resourceImplBinding maps pipeline resource names to the actual resource type implementations
 func resourceImplBinding(resources map[string]*resourcev1alpha1.PipelineResource, images pipeline.Images) (map[string]v1beta1.PipelineResourceInterface, error) {
 	p := make(map[string]v1beta1.PipelineResourceInterface)

pkg/reconciler/taskrun/taskrun_test.go

@@ -2037,6 +2037,12 @@ func TestHandlePodCreationError(t *testing.T) {
 		expectedType:   apis.ConditionSucceeded,
 		expectedStatus: corev1.ConditionUnknown,
 		expectedReason: podconvert.ReasonExceededResourceQuota,
+	}, {
+		description:    "taskrun validation failed",
+		err:            errors.New("TaskRun validation failed"),
+		expectedType:   apis.ConditionSucceeded,
+		expectedStatus: corev1.ConditionFalse,
+		expectedReason: podconvert.ReasonFailedValidation,
 	}, {
 		description:    "errors other than exceeded quota fail the taskrun",
 		err:            errors.New("this is a fatal error"),