Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ship env vars as one-time secret #7923

Merged
merged 3 commits into from
Feb 4, 2022
Merged

Ship env vars as one-time secret #7923

merged 3 commits into from
Feb 4, 2022

Conversation

csweichel
Copy link
Contributor

Description

Today user environment variables become environment variables on the workspace pod. This is a clean and straight forward way to implement env vars, but it suffers from a drawback: values are stored in clear-text on the pod. If one isn't careful about logging, those values could be spread into the logs, and are visible to everyone with access to the cluster.

This PR starts shipping env vars as one-time secret, much like we ship the Git hoster token. This way, environment variable values are not visible to anyone, except within the workspace.

How to test

  1. Set up user and project level environment variables.
  2. Start a workspace and ensure those env vars are present
  3. Run kubectl get pod -o yaml ws-... and ensure the user env vars are not present.

Release Notes

Improved in-transit security of user environment variables

@roboquat roboquat added release-note team: IDE team: webapp Issue belongs to the WebApp team size/L labels Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #7923 (05dcf52) into main (c96aaee) will decrease coverage by 20.16%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #7923       +/-   ##
===========================================
- Coverage   30.45%   10.28%   -20.17%     
===========================================
  Files         115       18       -97     
  Lines       18732     1001    -17731     
===========================================
- Hits         5705      103     -5602     
+ Misses      12538      897    -11641     
+ Partials      489        1      -488     
Flag Coverage Δ
components-content-service-app ?
components-content-service-lib ?
components-ee-agent-smith-app ?
components-ee-agent-smith-lib ?
components-gitpod-cli-app 10.28% <ø> (ø)
components-gitpod-protocol-go-lib ?
components-image-builder-mk3-app ?
components-installation-telemetry-app ?
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-supervisor-app ?
components-ws-daemon-app ?
components-ws-daemon-lib ?
components-ws-manager-app ?
components-ws-proxy-app ?
installer-raw-app ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/supervisor/pkg/supervisor/config.go
components/supervisor/pkg/supervisor/ssh.go
components/supervisor/pkg/supervisor/supervisor.go
components/ws-daemon/pkg/resources/limiter.go
components/content-service/pkg/storage/gcloud.go
components/ee/agent-smith/pkg/detector/proc.go
components/ws-proxy/pkg/proxy/routes.go
installer/pkg/components/ws-manager/configmap.go
...nts/content-service/pkg/initializer/initializer.go
installer/pkg/common/objects.go
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c96aaee...05dcf52. Read the comment docs.

@csweichel
Copy link
Contributor Author

/hold

@csweichel
Copy link
Contributor Author

/hold cancel

@geropl
Copy link
Member

geropl commented Feb 3, 2022

/werft run

👍 started the job as gitpod-build-cw-envvar-ots.3

@geropl
Copy link
Member

geropl commented Feb 3, 2022

/lgtm

Code looks good, tested and works as advertised.

/hold
But IMO we should keep the served times in-sync with the ones we use for other OTS data (example): 30 minutes. Otherwise I fear we'd introduce strange (and new) failure modes.

@roboquat
Copy link
Contributor

roboquat commented Feb 3, 2022

LGTM label has been added.

Git tree hash: 1f24a3d385fdb8e9898472e4319b46676e7c6d32

@csweichel
Copy link
Contributor Author

@geropl fair point - will correct this

@csweichel csweichel requested review from a team February 4, 2022 09:04
@roboquat roboquat removed the lgtm label Feb 4, 2022
@csweichel
Copy link
Contributor Author

@geropl changed the env var OTS timeout - needs another lgtm

@geropl
Copy link
Member

geropl commented Feb 4, 2022

I hoped for a constant. 😄 But
/lgtm
anways

@roboquat roboquat added the lgtm label Feb 4, 2022
@roboquat
Copy link
Contributor

roboquat commented Feb 4, 2022

LGTM label has been added.

Git tree hash: 60750202a474f0d6177f69539a5f16bc7546bdb0

@geropl
Copy link
Member

geropl commented Feb 4, 2022

/approve no-issue

@csweichel
Copy link
Contributor Author

I hoped for a constant.

That would have been smarter. Lesson learned: first coffee, then PR

@akosyakov
Copy link
Member

I cannot really test it, it fails when i try to configure Gitpod app in order to create a new project:
Screenshot 2022-02-04 at 15 16 29

@csweichel
Copy link
Contributor Author

looking at the code you'll see that we treat the allEnvvars list just as before, i.e. if project-level env vars worked before it's very likely they'll work still.

@akosyakov
Copy link
Member

akosyakov commented Feb 4, 2022

/lgtm

I rely on @geropl testing. 😄 Scanned for compatibilities between server and supervisor looks like can be deployed in any order.

@roboquat
Copy link
Contributor

roboquat commented Feb 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, geropl

Associated issue requirement bypassed by: geropl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@csweichel
Copy link
Contributor Author

/hold cancel

@roboquat roboquat merged commit e94cb93 into main Feb 4, 2022
@roboquat roboquat deleted the cw/envvar-ots branch February 4, 2022 14:43
return ev;
});
ev.setName(e.name);
ev.setValue(e.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this work then during testing, at all? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geropl I think you tested before the bug was introduced.

return ev;
});
ev.setName(e.name);
ev.setValue(e.name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops:

Screenshot 2022-02-08 at 10 15 40

Copy link
Contributor

@jankeromnes jankeromnes Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also sometimes the value seems to be name + ' ' + value? 🤔

Screenshot 2022-02-08 at 10 17 08

EDIT: Ah, actually the JAVA_TOOL_OPTIONS normal behavior is to append values. Not part of the bug.

@AlexTugarev AlexTugarev restored the cw/envvar-ots branch February 8, 2022 09:20
@AlexTugarev AlexTugarev deleted the cw/envvar-ots branch December 8, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants