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

[local-app] gracefully handle invalid token #5369

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Aug 25, 2021

What it does

I had to move bearer auth into verify callback to return a proper error code during web socket handshake, instead of failing after web socket is established. It allows clients to analyse the bad hahdshake request for unauthorised access.

How to test

# mac
curl -OL https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com/static/bin/gitpod-local-companion-darwin
chmod +x ./gitpod-local-companion-darwin
GITPOD_HOST=https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com ./gitpod-local-companion-darwin --log-level=debug

# linux
# curl -OL https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com/static/bin/gitpod-local-companion-linux
chmod +x ./gitpod-local-companion-linux
GITPOD_HOST=https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com ./gitpod-local-companion-linux --log-level=debug

# windows
# curl -OL https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com/static/bin/gitpod-local-companion-windows.exe
GITPOD_HOST=https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com ./gitpod-local-companion-windows.exe --log-level=debug
  • check that you went through the auth flow and output looks fine
  • restart the app, checks that you don't need to go through the auth flow this time
  • go and corrupt the token for https://akosyakov-local-app-is-crashing-5368.staging.gitpod-dev.com in keychain access
  • restart the app, you should see the error that token is invalid and go through the auth flow again
  • now corrupt tokens in the DB: update d_b_gitpod_token set scopes = "";
  • restart the app, you should see the error that token is invalid and go through the auth flow again

@akosyakov akosyakov marked this pull request as draft August 25, 2021 10:55
@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch from 36c0f54 to 66ed329 Compare August 25, 2021 11:59
@roboquat roboquat added size/L and removed size/M labels Aug 25, 2021
@akosyakov akosyakov changed the title [local-app] gracefully handle invalid token on GetWorkspace [local-app] gracefully handle invalid token Aug 25, 2021
@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch 5 times, most recently from 45b6e0d to 4210b0f Compare August 26, 2021 07:13
@roboquat roboquat added size/XL and removed size/L labels Aug 26, 2021
@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch from 4210b0f to cbaa18c Compare August 26, 2021 07:16
@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #5369 (018e57f) into main (0e270bc) will decrease coverage by 44.37%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #5369       +/-   ##
===========================================
- Coverage   79.52%   35.15%   -44.38%     
===========================================
  Files           2       23       +21     
  Lines         127     5063     +4936     
===========================================
+ Hits          101     1780     +1679     
- Misses         17     3135     +3118     
- Partials        9      148      +139     
Flag Coverage Δ
components-ee-agent-smith-app 25.82% <ø> (?)
components-licensor-lib ?
components-local-app-app-darwin ∅ <ø> (∅)
components-local-app-app-linux 19.39% <66.66%> (∅)
components-local-app-app-windows ∅ <ø> (∅)
components-supervisor-app 37.58% <ø> (?)

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

Impacted Files Coverage Δ
components/local-app/pkg/auth/auth.go 16.54% <66.66%> (ø)
components/licensor/ee/pkg/licensor/keys.go
components/licensor/ee/pkg/licensor/licensor.go
components/supervisor/pkg/supervisor/config.go 4.51% <0.00%> (ø)
components/supervisor/pkg/ports/exposed-ports.go 0.00% <0.00%> (ø)
components/supervisor/pkg/ports/ports-config.go 79.85% <0.00%> (ø)
components/ee/agent-smith/pkg/agent/actions.go 0.00% <0.00%> (ø)
components/supervisor/pkg/ports/tunnel.go 63.63% <0.00%> (ø)
components/ee/agent-smith/pkg/agent/egress.go 0.00% <0.00%> (ø)
components/supervisor/pkg/ports/served-ports.go 76.00% <0.00%> (ø)
... and 16 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 0e270bc...018e57f. Read the comment docs.

@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch from cbaa18c to bf375c7 Compare August 26, 2021 08:13
@akosyakov akosyakov marked this pull request as ready for review August 26, 2021 08:13
@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch 5 times, most recently from 9818ee3 to bbb5b5e Compare August 30, 2021 13:00
@akosyakov
Copy link
Member Author

@csweichel Could you check it again? I rewrote getGitpodTokenScopes to accept the token hash.

@akosyakov
Copy link
Member Author

akosyakov commented Aug 31, 2021

/werft run

👍 started the job as gitpod-build-akosyakov-local-app-is-crashing-5368.14

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Other than a couple of questions and a nit this looks good to me

components/local-app/main.go Outdated Show resolved Hide resolved
components/local-app/main.go Outdated Show resolved Hide resolved
components/server/src/server.ts Show resolved Hide resolved
@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch from bbb5b5e to 0336a71 Compare August 31, 2021 07:11
@akosyakov
Copy link
Member Author

@csweichel Could you have a look again please? All comments should be addressed.

@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch 6 times, most recently from f56b033 to 4f89718 Compare August 31, 2021 08:00
@akosyakov akosyakov force-pushed the akosyakov/local-app-is-crashing-5368 branch from 4f89718 to 018e57f Compare August 31, 2021 08:25
@csweichel
Copy link
Contributor

/lgtm

@roboquat roboquat added the lgtm label Aug 31, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 99039cd043fb7f4830c96d1bb45e23c2fa7d53eb

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel

Associated issue: #5368

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

@roboquat roboquat merged commit f66598c into main Aug 31, 2021
@roboquat roboquat deleted the akosyakov/local-app-is-crashing-5368 branch August 31, 2021 08:33
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.

local-app is crashing with old token
4 participants