NotificationsYou must be signed in to change notification settings
Fork927
Star10.1k

feat: use JWT ticket to avoid DB queries on apps#6148

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

deansheather merged 18 commits intomainfromdean/app-tickets

Mar 7, 2023

Merged

feat: use JWT ticket to avoid DB queries on apps#6148

deansheather merged 18 commits intomainfromdean/app-tickets

Mar 7, 2023

Conversation

Copy link

Member

deansheather commentedFeb 10, 2023•
edited
Loading

Issues a JWT ticket on the first request with a short expiry that contains details about which workspace/agent/app combo the ticket is valid for. This lets us avoid the ~5 DB queries on each app request, and reduces them to ~5 per minute.

Large dev server apps will often make hundreds of requests to get bundles, which strains the database. The goal of this PR is to alleviate DB strain in large deployments that heavily lean on workspace apps.

Refactors the workspace app auth logic into workspaceappsauth.go.

Disables usingme as the username in workspace app requests. For subdomains we never used this, but it's possible that it has been used for path apps, so path apps will redirect to contain the correct username.

TODO:

Generate a secret key on first start, store in DB, cache in memory
JWT signing and parsing
Path-cookies for path apps
New workspace auth internal tests
Test for path appme username redirect
Test for no port proxying allowed in paths
TODOs in the code
Fix bugs that break existing tests

Closes#6361

deansheather added6 commits

February 10, 2023 05:18

feat: use JWT ticket to avoid DB queries on apps

cd239fa

WIPIssue a JWT ticket on the first request with a short expiry thatcontains details about which workspace/agent/app combo the ticket isvalid for.Refactor the workspace app auth logic into workspaceappsauth.go.

get existing tests working

24d1112

progress

8ffd8d4

progress

a392635

working session tickets

e60c645

progress

e681f4c

Copy link

github-actionsbot commentedFeb 19, 2023

This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity.

github-actionsbot added the staleThis issue is like stale bread. label

Feb 19, 2023

deansheather removed the staleThis issue is like stale bread. label

Feb 20, 2023

Merge branch 'main' into dean/app-tickets

16cf790

github-advanced-securitybot found potential problems

Feb 28, 2023

View reviewed changes

coderd/workspaceappsauth.go FixedShow fixedHide fixed

deansheather added2 commits

February 28, 2023 17:06

chore: move workspace apps auth code to new package

22707c6

fixup! chore: move workspace apps auth code to new package

3521669

github-advanced-securitybot found potential problems

Feb 28, 2023

View reviewed changes

coderd/workspaceapps.goShow resolvedHide resolved

deansheather added2 commits

February 28, 2023 19:29

chore: tests for workspace apps auth ticket code

bffd775

chore: update RSA private key to avoid typos

b8a5526

Copy link

MemberAuthor

deansheather commentedFeb 28, 2023

I've decided to not make the ticket expiry configurable yet until a customer asks us to make it configurable.

deansheather requested review fromkylecarbs andmafredri

February 28, 2023 19:40

Copy link

MemberAuthor

deansheather commentedFeb 28, 2023

I apologize for the large line count in this PR, the majority of the code is in two filesauth.go andticket.go, and the rest of the changed lines are moved code and tests.

deansheather marked this pull request as ready for review

February 28, 2023 19:41

deansheather requested a review fromsreya

March 2, 2023 17:25

mafredri approved these changes

Mar 2, 2023

View reviewed changes

Copy link

Member

mafredri left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Given the size of this PR, another pair of eyes on the auth stuff might be good, but other than the minor things I commented on, this looks OK to me!

cli/clitest/clitest.go OutdatedShow resolvedHide resolved

coderd/database/db.go OutdatedShow resolvedHide resolved

coderd/database/dbfake/databasefake.go

		if _, ok := tx.fakeQuerier.locks[id]; ok {
		return false, nil
		}
		tx.fakeQuerier.locks[id] = struct{}{}

Copy link

Member

mafredriMar 2, 2023

Choose a reason for hiding this comment

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

Should this be protected or use sync.Map? Otherwise we may have concurrent map read/writes between goroutines.

Copy link

MemberAuthor

deansheatherMar 2, 2023

Choose a reason for hiding this comment

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

I wouldn't expect a transaction to be used concurrently from two places at once (and I don't think that even works anyways) but I can add this

Copy link

MemberAuthor

deansheatherMar 2, 2023

Choose a reason for hiding this comment

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

For example, the lock on the underlying data struct is replaced with a noop lock in a tx, so concurrent database read/writes may panic

Copy link

MemberAuthor

deansheatherMar 7, 2023

Choose a reason for hiding this comment

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

I've actually decided against this for the above reason.

coderd/workspaceapps.go OutdatedShow resolvedHide resolved

coderd/workspaceapps.go Outdated

		httpmw.RedirectToLogin(rw, r, httpmw.SignedOutErrorMessage)
		mw.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
		user := httpmw.UserParam(r)
		http.Redirect(rw, r, strings.Replace(r.URL.Path, "@me", "@"+user.Username, 1), http.StatusTemporaryRedirect)

Copy link

Member

mafredriMar 2, 2023

Choose a reason for hiding this comment

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

If we will never support@me for these apps, we could consider using permanent redirect perceived performance gain. Temporary is more future proof though.