[ws-manager] Only extract secrets when FF is set

Author: csweichel

components/content-service-api/go/initializer.go

@@ -33,9 +33,20 @@ func GetCheckoutLocationsFromInitializer(init *WorkspaceInitializer) []string {
 
 const extractedSecretPrefix = "extracted-secret/"
 
-// ExtractSecretsFromInitializer removes secrets to the initializer.
+// GatherSecretsFromInitializer collects all from an initializer. This function does not
+// alter the initializer in any way.
+func GatherSecretsFromInitializer(init *WorkspaceInitializer) map[string]string {
+	return extractSecretsFromInitializer(init, false)
+}
+
+// ExtractAndReplaceSecretsFromInitializer removes secrets to the initializer.
+// This function alters the initializer, which only becomes useful calling InjectSecretsToInitializer.
 // This is the counterpart of InjectSecretsToInitializer.
-func ExtractSecretsFromInitializer(init *WorkspaceInitializer) map[string]string {
+func ExtractAndReplaceSecretsFromInitializer(init *WorkspaceInitializer) map[string]string {
+	return extractSecretsFromInitializer(init, true)
+}
+
+func extractSecretsFromInitializer(init *WorkspaceInitializer, replaceValue bool) map[string]string {
 	res := make(map[string]string)
 
 	_ = WalkInitializer([]string{"initializer"}, init, func(path []string, init *WorkspaceInitializer) error {
@@ -51,7 +62,10 @@ func ExtractSecretsFromInitializer(init *WorkspaceInitializer) map[string]string
 
 		name := strings.Join(path, ".")
 		res[name] = pwd
-		git.Git.Config.AuthPassword = extractedSecretPrefix + name
+
+		if replaceValue {
+			git.Git.Config.AuthPassword = extractedSecretPrefix + name
+		}
 
 		return nil
 	})

components/content-service-api/go/initializer_test.go

@@ -11,6 +11,8 @@ import (
 
 	"github.com/gitpod-io/gitpod/content-service/api"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"google.golang.org/protobuf/proto"
 )
 
 func TestGetCheckoutLocationsFromInitializer(t *testing.T) {
@@ -176,7 +178,24 @@ func TestExtractInjectSecretsFromInitializer(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.Name, func(t *testing.T) {
-			act := api.ExtractSecretsFromInitializer(test.Input)
+			original := proto.Clone(test.Input)
+			act := api.GatherSecretsFromInitializer(test.Input)
+			if diff := cmp.Diff(test.Expectation, act); diff != "" {
+				t.Errorf("unexpected GatherSecretsFromInitializer (-want +got):\n%s", diff)
+			}
+
+			ignoreUnexported := []interface{}{
+				api.WorkspaceInitializer{},
+				api.WorkspaceInitializer_Git{},
+				api.GitInitializer{},
+				api.GitConfig{},
+				api.PrebuildInitializer{},
+			}
+			if diff := cmp.Diff(original, test.Input, cmpopts.IgnoreUnexported(ignoreUnexported...)); diff != "" {
+				t.Errorf("unexpected alteration from GatherSecretsFromInitializer (-want +got):\n%s", diff)
+			}
+
+			act = api.ExtractAndReplaceSecretsFromInitializer(test.Input)
 			if diff := cmp.Diff(test.Expectation, act); diff != "" {
 				t.Errorf("unexpected ExtractSecretsFromInitializer (-want +got):\n%s", diff)
 			}

components/ws-manager/pkg/manager/create.go

@@ -969,30 +969,6 @@ func (m *Manager) newStartWorkspaceContext(ctx context.Context, req *api.StartWo
 		}
 	}
 
-	var (
-		secrets    = make(map[string]string)
-		secretsLen int
-	)
-	for _, env := range req.Spec.Envvars {
-		if env.Secret != nil {
-			continue
-		}
-		if !isProtectedEnvVar(env.Name) {
-			continue
-		}
-
-		name := fmt.Sprintf("%x", sha256.Sum256([]byte(env.Name)))
-		secrets[name] = env.Value
-		secretsLen += len(env.Value)
-	}
-	for k, v := range csapi.ExtractSecretsFromInitializer(req.Spec.Initializer) {
-		secrets[k] = v
-		secretsLen += len(v)
-	}
-	if secretsLen > maxSecretsLength {
-		return nil, xerrors.Errorf("secrets exceed maximum permitted length (%d > %d bytes): please reduce the numer or length of environment variables", secretsLen, maxSecretsLength)
-	}
-
 	return &startWorkspaceContext{
 		Labels:         labels,
 		CLIAPIKey:      cliAPIKey,
@@ -1004,7 +980,6 @@ func (m *Manager) newStartWorkspaceContext(ctx context.Context, req *api.StartWo
 		Headless:       headless,
 		Class:          class,
 		VolumeSnapshot: volumeSnapshot,
-		Secrets:        secrets,
 	}, nil
 }
 

components/ws-manager/pkg/manager/create_test.go

@@ -17,67 +17,15 @@ import (
 	"sigs.k8s.io/yaml"
 
 	ctesting "github.com/gitpod-io/gitpod/common-go/testing"
-	csapi "github.com/gitpod-io/gitpod/content-service/api"
 	"github.com/gitpod-io/gitpod/ws-manager/api"
 	config "github.com/gitpod-io/gitpod/ws-manager/api/config"
-	"github.com/google/go-cmp/cmp"
 )
 
 var (
 	team    = "awesome"
 	project = "gitpod"
 )
 
-func TestNewStartWorkspaceContext(t *testing.T) {
-	type Expectation struct {
-		Context *startWorkspaceContext
-		Error   string
-	}
-	tests := []struct {
-		Name        string
-		Req         *api.StartWorkspaceRequest
-		Expectation Expectation
-	}{
-		{
-			Name: "oversized secrets",
-			Req: &api.StartWorkspaceRequest{
-				Metadata: &api.WorkspaceMetadata{Owner: "foo"},
-				Spec: &api.StartWorkspaceSpec{
-					Initializer: &csapi.WorkspaceInitializer{
-						Spec: &csapi.WorkspaceInitializer_Empty{},
-					},
-					Envvars: []*api.EnvironmentVariable{
-						{Name: "too_large", Value: string(func() []byte { return make([]byte, 2*maxSecretsLength) }())},
-					},
-				},
-			},
-			Expectation: Expectation{
-				Error: "secrets exceed maximum permitted length (1610612736 > 805306368 bytes): please reduce the numer or length of environment variables",
-			},
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.Name, func(t *testing.T) {
-			mgmtCfg := forTestingOnlyManagerConfig()
-			manager := &Manager{Config: mgmtCfg}
-
-			sctx, err := manager.newStartWorkspaceContext(context.Background(), test.Req)
-
-			act := Expectation{
-				Context: sctx,
-			}
-			if err != nil {
-				act.Error = err.Error()
-			}
-
-			if diff := cmp.Diff(test.Expectation, act); diff != "" {
-				t.Errorf("unexpected newStartWorkspaceContext (-want +got):\n%s", diff)
-			}
-		})
-	}
-}
-
 func TestCreateDefiniteWorkspacePod(t *testing.T) {
 	type WorkspaceClass struct {
 		DefaultTemplate    *corev1.Pod                   `json:"defaultTemplate,omitempty"`

components/ws-manager/pkg/manager/manager.go

@@ -6,6 +6,7 @@ package manager
 
 import (
 	"context"
+	"crypto/sha256"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -39,6 +40,7 @@ import (
 	wsk8s "github.com/gitpod-io/gitpod/common-go/kubernetes"
 	"github.com/gitpod-io/gitpod/common-go/log"
 	"github.com/gitpod-io/gitpod/common-go/tracing"
+	csapi "github.com/gitpod-io/gitpod/content-service/api"
 	"github.com/gitpod-io/gitpod/content-service/pkg/layer"
 	regapi "github.com/gitpod-io/gitpod/registry-facade/api"
 	wsdaemon "github.com/gitpod-io/gitpod/ws-daemon/api"
@@ -82,7 +84,6 @@ type startWorkspaceContext struct {
 	Headless       bool                           `json:"headless"`
 	Class          *config.WorkspaceClass         `json:"class"`
 	VolumeSnapshot *workspaceVolumeSnapshotStatus `json:"volumeSnapshot"`
-	Secrets        map[string]string              `json:"secrets"`
 }
 
 func (swctx *startWorkspaceContext) ContainerConfiguration() config.ContainerConfiguration {
@@ -267,13 +268,21 @@ func (m *Manager) StartWorkspace(ctx context.Context, req *api.StartWorkspaceReq
 		}
 	}
 	if createSecret {
+		secrets, _ := buildWorkspaceSecrets(startContext.Request.Spec)
+
+		// This call actually modifies the initializer and removes the secrets.
+		// Prior to the `InitWorkspace` call, we inject the secrets back into the initializer.
+		// We do this so that no Git token is stored as annotation on the pod, but solely
+		// remains within the Kubernetes secret.
+		_ = csapi.ExtractAndReplaceSecretsFromInitializer(startContext.Request.Spec.Initializer)
+
 		secret := &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      podName(startContext.Request),
 				Namespace: m.Config.Namespace,
 				Labels:    startContext.Labels,
 			},
-			StringData: startContext.Secrets,
+			StringData: secrets,
 		}
 		err = m.Clientset.Create(ctx, secret)
 		if err != nil && !k8serr.IsAlreadyExists(err) {
@@ -411,6 +420,27 @@ func (m *Manager) StartWorkspace(ctx context.Context, req *api.StartWorkspaceReq
 	return okResponse, nil
 }
 
+func buildWorkspaceSecrets(spec *api.StartWorkspaceSpec) (secrets map[string]string, secretsLen int) {
+	secrets = make(map[string]string)
+	for _, env := range spec.Envvars {
+		if env.Secret != nil {
+			continue
+		}
+		if !isProtectedEnvVar(env.Name) {
+			continue
+		}
+
+		name := fmt.Sprintf("%x", sha256.Sum256([]byte(env.Name)))
+		secrets[name] = env.Value
+		secretsLen += len(env.Value)
+	}
+	for k, v := range csapi.GatherSecretsFromInitializer(spec.Initializer) {
+		secrets[k] = v
+		secretsLen += len(v)
+	}
+	return secrets, secretsLen
+}
+
 func (m *Manager) restoreVolumeSnapshotFromHandle(ctx context.Context, id, handle string) (err error) {
 	span, ctx := tracing.FromContext(ctx, "restoreVolumeSnapshotFromHandle")
 	defer tracing.FinishSpan(span, &err)
@@ -586,6 +616,10 @@ func validateStartWorkspaceRequest(req *api.StartWorkspaceRequest) error {
 		return xerrors.Errorf("invalid request: %w", err)
 	}
 
+	if _, secretsLen := buildWorkspaceSecrets(req.Spec); secretsLen > maxSecretsLength {
+		return xerrors.Errorf("secrets exceed maximum permitted length (%d > %d bytes): please reduce the numer or length of environment variables", secretsLen, maxSecretsLength)
+	}
+
 	rules := make([]*validation.FieldRules, 0)
 	rules = append(rules, validation.Field(&req.Id, validation.Required))
 	rules = append(rules, validation.Field(&req.Spec, validation.Required))

components/ws-manager/pkg/manager/manager_test.go

@@ -23,7 +23,8 @@ import (
 
 func TestValidateStartWorkspaceRequest(t *testing.T) {
 	type fixture struct {
-		Req *api.StartWorkspaceRequest `json:"request"`
+		Req      *api.StartWorkspaceRequest `json:"request"`
+		BloatEnv bool                       `json:"bloatEnv"`
 	}
 	type gold struct {
 		Error string `json:"error,omitempty"`
@@ -41,6 +42,13 @@ func TestValidateStartWorkspaceRequest(t *testing.T) {
 				return nil
 			}
 
+			if fixture.BloatEnv {
+				fixture.Req.Spec.Envvars = append(fixture.Req.Spec.Envvars, &api.EnvironmentVariable{
+					Name:  "BLOAT",
+					Value: string(make([]byte, 2*maxSecretsLength)),
+				})
+			}
+
 			err := validateStartWorkspaceRequest(fixture.Req)
 			if err != nil {
 				return &gold{Error: err.Error()}

components/ws-manager/pkg/manager/testdata/validateStartReq_oversizedSecret.golden

@@ -0,0 +1,3 @@
+{
+    "error": "secrets exceed maximum permitted length (1610612739 \u003e 805306368 bytes): please reduce the numer or length of environment variables"
+}

components/ws-manager/pkg/manager/testdata/validateStartReq_oversizedSecret.json

@@ -0,0 +1,39 @@
+{
+    "bloatEnv": true,
+    "request": {
+        "id": "foobar",
+        "type": 0,
+        "metadata": {
+            "owner": "tester",
+            "metaId": "foobar"
+        },
+        "servicePrefix": "foobarservice",
+        "spec": {
+            "ideImage": {
+                "webRef": "eu.gcr.io/gitpod-core-dev/buid/theia-ide:someversion"
+            },
+            "workspace_image": "eu.gcr.io/gitpod-dev/workspace-images/ac1c0755007966e4d6e090ea821729ac747d22ac/eu.gcr.io/gitpod-dev/workspace-base-images/github.com/typefox/gitpod:80a7d427a1fcd346d420603d80a31d57cf75a7af",
+            "workspace_location": "/",
+            "initializer": {
+                "snapshot": {
+                    "snapshot": "workspaces/cryptic-id-goes-herg/fd62804b-4cab-11e9-843a-4e645373048e.tar@gitpod-dev-user-christesting"
+                }
+            },
+            "ports": [
+                {
+                    "port": 8080
+                }
+            ],
+            "envvars": [
+                {
+                    "name": "foo",
+                    "value": "bar"
+                }
+            ],
+            "git": {
+                "username": "usernameGoesHere",
+                "email": "some@user.com"
+            }
+        }
+    }
+}