diff --git a/components/content-service-api/go/initializer.go b/components/content-service-api/go/initializer.go index 6fe6dffda48abf..fd2b98ba1d6145 100644 --- a/components/content-service-api/go/initializer.go +++ b/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 }) diff --git a/components/content-service-api/go/initializer_test.go b/components/content-service-api/go/initializer_test.go index 369efbdfb4c136..9ae86b74f00870 100644 --- a/components/content-service-api/go/initializer_test.go +++ b/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) } diff --git a/components/ws-manager/pkg/manager/create.go b/components/ws-manager/pkg/manager/create.go index 8427d4109ce1d8..b8c362a056e699 100644 --- a/components/ws-manager/pkg/manager/create.go +++ b/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 } diff --git a/components/ws-manager/pkg/manager/create_test.go b/components/ws-manager/pkg/manager/create_test.go index 260692b9ec7e4d..93e2db26ea1986 100644 --- a/components/ws-manager/pkg/manager/create_test.go +++ b/components/ws-manager/pkg/manager/create_test.go @@ -17,10 +17,8 @@ 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 ( @@ -28,56 +26,6 @@ var ( 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"` diff --git a/components/ws-manager/pkg/manager/manager.go b/components/ws-manager/pkg/manager/manager.go index 78a292dfdce004..a4760667972463 100644 --- a/components/ws-manager/pkg/manager/manager.go +++ b/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)) diff --git a/components/ws-manager/pkg/manager/manager_test.go b/components/ws-manager/pkg/manager/manager_test.go index 2eed80ebe8b95e..7e63b7fd5e7c3a 100644 --- a/components/ws-manager/pkg/manager/manager_test.go +++ b/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()} diff --git a/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.golden b/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.golden index 003b9df246d2f1..18f62cb3fa5991 100644 --- a/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.golden +++ b/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.golden @@ -1,3 +1,3 @@ { - "error": "invalid request: workspace_image: cannot be blank; workspace_location: cannot be blank." -} \ No newline at end of file + "error": "invalid request: type: value 200 is out of range." +} diff --git a/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.json b/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.json index b74c5edc864c33..bae7280a199e8f 100644 --- a/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.json +++ b/components/ws-manager/pkg/manager/testdata/validateStartReq_invalidType.json @@ -7,14 +7,14 @@ "owner": "tester", "metaId": "foobar" }, - "servicePrefix": "foobarservice", + "service_prefix": "foobarservice", "spec": { "ideImage": { "webRef": "eu.gcr.io/gitpod-core-dev/buid/theia-ide:someversion" }, - "workspaceImage": "eu.gcr.io/gitpod-dev/workspace-images/ac1c0755007966e4d6e090ea821729ac747d22ac/eu.gcr.io/gitpod-dev/workspace-base-images/github.com/typefox/gitpod:80a7d427a1fcd346d420603d80a31d57cf75a7af", - "checkoutLocation": "/", - "workspaceLocation": "/", + "workspace_image": "eu.gcr.io/gitpod-dev/workspace-images/ac1c0755007966e4d6e090ea821729ac747d22ac/eu.gcr.io/gitpod-dev/workspace-base-images/github.com/typefox/gitpod:80a7d427a1fcd346d420603d80a31d57cf75a7af", + "checkout_location": "/", + "workspace_location": "/", "initializer": { "snapshot": { "snapshot": "workspaces/cryptic-id-goes-herg/fd62804b-4cab-11e9-843a-4e645373048e.tar@gitpod-dev-user-christesting" @@ -37,4 +37,4 @@ } } } -} \ No newline at end of file +} diff --git a/components/ws-manager/pkg/manager/testdata/validateStartReq_oversizedSecret.golden b/components/ws-manager/pkg/manager/testdata/validateStartReq_oversizedSecret.golden new file mode 100644 index 00000000000000..3b98c7ec6815f8 --- /dev/null +++ b/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" +} diff --git a/components/ws-manager/pkg/manager/testdata/validateStartReq_oversizedSecret.json b/components/ws-manager/pkg/manager/testdata/validateStartReq_oversizedSecret.json new file mode 100644 index 00000000000000..b4f591aa028688 --- /dev/null +++ b/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" + } + } + } +} diff --git a/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.golden b/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.golden index 003b9df246d2f1..0967ef424bce67 100644 --- a/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.golden +++ b/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.golden @@ -1,3 +1 @@ -{ - "error": "invalid request: workspace_image: cannot be blank; workspace_location: cannot be blank." -} \ No newline at end of file +{} diff --git a/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.json b/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.json index 549e032a02453b..82f2438aa2cac5 100644 --- a/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.json +++ b/components/ws-manager/pkg/manager/testdata/validateStartReq_valid.json @@ -7,13 +7,13 @@ "owner": "tester", "metaId": "foobar" }, - "servicePrefix": "foobarservice", + "service_prefix": "foobarservice", "spec": { "ideImage": { "webRef": "eu.gcr.io/gitpod-core-dev/buid/theia-ide:someversion" }, - "workspaceImage": "eu.gcr.io/gitpod-dev/workspace-images/ac1c0755007966e4d6e090ea821729ac747d22ac/eu.gcr.io/gitpod-dev/workspace-base-images/github.com/typefox/gitpod:80a7d427a1fcd346d420603d80a31d57cf75a7af", - "workspaceLocation": "/", + "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" @@ -36,4 +36,4 @@ } } } -} \ No newline at end of file +}