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

[Installer]: change authProviders from raw data to a secret #7177

Merged
merged 3 commits into from
Dec 21, 2021

Conversation

mrsimonemms
Copy link
Contributor

@mrsimonemms mrsimonemms commented Dec 10, 2021

Description

Allows Installer users to set their auth providers as a secret rather than putting sensitive data in plain text.

Technically this is a breaking change on the config so should discuss how we publicise it - personally, as we're only in the early days of the Installer, I would favour putting it out there and updating the Discord with an announcement. cc @csweichel @kylos101 @corneliusludmann

Related Issue(s)

Fixes #6867

How to test

Deploy via the Installer with the auth provider

Set your config:

authProviders:
  - kind: secret
    name: public-github

Create a secret file:

# Save to public-github.yaml

id: Public-GitHub
host: github.com
type: GitHub
oauth:
  clientId: xxx
  clientSecret: xxx
  callBackUrl: https://$DOMAIN/auth/github.com/callback
  settingsUrl: https://github.com/$ORG_ID/openapiary/settings/applications/$APP_ID

Create a secret:

kubectl create secret generic --from-file=provider=./public-github.yaml public-github

Go to your Gitpod instance and you should be prompted to login and not give OAuth provider details

Release Notes

Allow auth provider secrets to be passed in via a secret

Documentation

@codecov
Copy link

codecov bot commented Dec 10, 2021

Codecov Report

Merging #7177 (9d41659) into main (1316b8b) will decrease coverage by 30.49%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #7177       +/-   ##
==========================================
- Coverage   36.25%   5.76%   -30.50%     
==========================================
  Files          21      13        -8     
  Lines        4763    1162     -3601     
==========================================
- Hits         1727      67     -1660     
+ Misses       2899    1094     -1805     
+ Partials      137       1      -136     
Flag Coverage Δ
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 ?
installer-raw-app 5.76% <ø> (?)

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

Impacted Files Coverage Δ
components/supervisor/pkg/supervisor/supervisor.go
components/supervisor/pkg/ports/ports-config.go
components/supervisor/pkg/ports/ports.go
components/supervisor/pkg/terminal/ring-buffer.go
components/supervisor/pkg/supervisor/ssh.go
components/supervisor/pkg/supervisor/tasks.go
components/local-app/pkg/auth/auth.go
components/supervisor/pkg/ports/slirp4netns.go
components/supervisor/pkg/supervisor/config.go
components/supervisor/pkg/dropwriter/dropwriter.go
... and 24 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 1316b8b...9d41659. Read the comment docs.

@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch from 31a4cda to 3eae49c Compare December 10, 2021 17:22
@mrsimonemms mrsimonemms changed the title Sje/installer auth provider secrets [Installer]: change authProviders from raw data to a secret Dec 10, 2021
@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch 3 times, most recently from e4e0970 to 6f7e405 Compare December 13, 2021 10:51
@mrsimonemms mrsimonemms added the team: delivery Issue belongs to the self-hosted team label Dec 13, 2021
@mrsimonemms mrsimonemms marked this pull request as ready for review December 13, 2021 11:00
@mrsimonemms mrsimonemms requested review from a team December 13, 2021 11:01
@corneliusludmann
Copy link
Contributor

personally, as we're only in the early days of the Installer, I would favour putting it out there and updating the Discord with an announcement

Makes sense to me. 👍

@kylos101
Copy link
Contributor

Hey @mrsimonemms my preference would be to hold on this, as we're trying to get the Installer into core-dev (ideally for Tuesday). Is that okay?

@mrsimonemms
Copy link
Contributor Author

@kylos101 yes, I think that's very pragmatic. Your core-dev changes are both bigger and more useful

/hold

@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch from 6f7e405 to 0487277 Compare December 14, 2021 09:27
@mrsimonemms
Copy link
Contributor Author

@kylos101 I've resolved the conflict. Let's chat when you're online later today and I'll get the required updates in for the auth provider secrets to work with your core-dev changes

@mrsimonemms mrsimonemms requested a review from a team December 14, 2021 09:32
@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch 2 times, most recently from 5a935f0 to e92b27e Compare December 14, 2021 16:17
@roboquat roboquat added size/XL and removed size/L labels Dec 14, 2021
@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch from e92b27e to 31c5cab Compare December 14, 2021 16:26
@mrsimonemms
Copy link
Contributor Author

Ready for review @gitpod-io/engineering-self-hosted @gitpod-io/engineering-meta @gitpod-io/engineering-workspace

Copy link
Contributor

@kylos101 kylos101 left a comment

Choose a reason for hiding this comment

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

/approve

.werft/build.ts Show resolved Hide resolved
installer/pkg/components/server/types.go Show resolved Hide resolved
@mrsimonemms
Copy link
Contributor Author

@jankeromnes can you added your thoughts/approval please?

/hold
/assign @jankeromnes

@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch from 768e4b9 to 49aa909 Compare December 15, 2021 13:18
Simon Emms added 3 commits December 17, 2021 11:01
@mrsimonemms mrsimonemms force-pushed the sje/installer-auth-provider-secrets branch from 49aa909 to 9d41659 Compare December 17, 2021 11:01
@mrsimonemms mrsimonemms requested a review from kylos101 December 17, 2021 11:02
@kylos101
Copy link
Contributor

/approve

@csweichel
Copy link
Contributor

csweichel commented Dec 20, 2021

personally, as we're only in the early days of the Installer, I would favour putting it out there and updating the Discord with an announcement

Makes sense to me. 👍

Word of caution: this is a slippery slope. For one, we already have folks out there who use the installer (at the very very least we don't know, so it's safe to assume they exist). But also, it's exactly this kind of argument that we've resorted to over the past year(s) when making incompatible changes to the config surface.

Instead, we must flex that muscle of maintaining a compatible surface. The sooner we start with a new config version, auto-migration, and so forth, the sooner we get used to this, the less effort it becomes, the more likely we are to make better changes.

@mrsimonemms
Copy link
Contributor Author

Instead, we must flex that muscle of maintaining a compatible surface.

I 100% agree with you @csweichel. As far as I'm concerned, this is the only incompatible change that will be made whilst retaining v1 and done because 1) there are validation tests around this that will error if the config is the old one and 2) I've got the community ready for this. As it's removing sensitive data from the config (the last piece), I'm keen to get this out there so we can ask for users to provide their config.yaml when submitting support requests.

With regards to a v2, if we make any other changes then this would trigger that. This has a dependency on the config migration command #7308

@mrsimonemms
Copy link
Contributor Author

/approve

@laushinka
Copy link
Contributor

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: ef7d1ecb488950ab79a0add7900aee96664a1c00

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kylos101, laushinka, MrSimonEmms

Associated issue: #6867

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

@mrsimonemms mrsimonemms added the installer: needs interface change Change required to input, output or configuration file(s) label Dec 21, 2021
@mrsimonemms
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit be64564 into main Dec 21, 2021
@roboquat roboquat deleted the sje/installer-auth-provider-secrets branch December 21, 2021 16:17
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Dec 22, 2021
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production installer: needs interface change Change required to input, output or configuration file(s) release-note size/L team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Installer]: change authProviders from raw data to a secret
7 participants