Skip to content

Commit da1053e

Browse files
filiptronicekgeropl
filiptronicek
and
geropl
authored
Make auth cookie stricter (#19973)
* Use `__Host-` prefix for cookie * Fix tests * Remove domain from cookie * Fix logout * remove unused fn * fix user logout properly * [server] Make domain-only cookie work for GitHub oauth login ... by adding additional step so we can set the cookie for the base domain only * test: fix by redirecting before callbacl/authorize * [server] SessionHandler: Allow to login with both primary and secondary cookies * [server] Clear 2ndary cookie on logout * Fix filtering cookie values when primary cookie is empty * Fix logouts * Fix tests --------- Co-authored-by: Gero Posmyk-Leinemann <[email protected]>
1 parent 3bf7b41 commit da1053e

File tree

14 files changed

+145
-31
lines changed

14 files changed

+145
-31
lines changed

‎components/gitpod-protocol/src/util/gitpod-host-url.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,4 +216,12 @@ export class GitpodHostUrl {
216216
}
217217
return newUrl.with((url) => ({ pathname: "/metrics-api" }));
218218
}
219+
220+
asLoginWithOTS(userId: string, key: string, returnToUrl?: string) {
221+
const result = this.withApi({ pathname: `/login/ots/${userId}/${key}` });
222+
if (returnToUrl) {
223+
return result.with({ search: `returnTo=${encodeURIComponent(returnToUrl)}` });
224+
}
225+
return result;
226+
}
219227
}

‎components/server/go/pkg/lib/cookie.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,5 +9,5 @@ import "regexp"
99
func CookieNameFromDomain(domain string) string {
1010
// replace all non-word characters with underscores
1111
derived := regexp.MustCompile(`[\W_]+`).ReplaceAllString(domain, "_")
12-
return "_" + derived + "_jwt2_"
12+
return "__Host-_" + derived + "_jwt2_"
1313
}

‎components/server/src/auth/generic-auth-provider.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,14 @@ export abstract class GenericAuthProvider implements AuthProvider {
302302
return;
303303
}
304304

305+
if (!this.loginCompletionHandler.isBaseDomain(request)) {
306+
// For auth requests that are not targetting the base domain, we redirect to the base domain, so they come with our cookie.
307+
log.info(`(${strategyName}) Auth request on subdomain, redirecting to base domain`, { clientInfo });
308+
const target = new URL(request.url, this.config.hostUrl.url.toString()).toString();
309+
response.redirect(target);
310+
return;
311+
}
312+
305313
if (isAlreadyLoggedIn) {
306314
if (!authFlow) {
307315
log.warn(

‎components/server/src/auth/login-completion-handler.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import { inject, injectable } from "inversify";
88
import express from "express";
9+
import * as crypto from "crypto";
910
import { User } from "@gitpod/gitpod-protocol";
1011
import { log, LogContext } from "@gitpod/gitpod-protocol/lib/util/logging";
1112
import { Config } from "../config";
@@ -16,6 +17,7 @@ import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
1617
import { trackLogin } from "../analytics";
1718
import { SessionHandler } from "../session-handler";
1819
import { AuthJWT } from "./jwt";
20+
import { OneTimeSecretServer } from "../one-time-secret-server";
1921

2022
/**
2123
* The login completion handler pulls the strings between the OAuth2 flow, the ToS flow, and the session management.
@@ -28,6 +30,7 @@ export class LoginCompletionHandler {
2830
@inject(AuthProviderService) protected readonly authProviderService: AuthProviderService;
2931
@inject(AuthJWT) protected readonly authJWT: AuthJWT;
3032
@inject(SessionHandler) protected readonly session: SessionHandler;
33+
@inject(OneTimeSecretServer) private readonly otsServer: OneTimeSecretServer;
3134

3235
async complete(
3336
request: express.Request,
@@ -78,6 +81,26 @@ export class LoginCompletionHandler {
7881
);
7982
}
8083

84+
if (!this.isBaseDomain(request)) {
85+
// (GitHub edge case) If we got redirected here onto a sub-domain (e.g. api.gitpod.io), we need to redirect to the base domain in order to Set-Cookie properly.
86+
const secret = crypto
87+
.createHash("sha256")
88+
.update(user.id + this.config.session.secret)
89+
.digest("hex");
90+
const expirationDate = new Date(Date.now() + 1000 * 60); // 1 minutes
91+
const token = await this.otsServer.serveToken({}, secret, expirationDate);
92+
93+
reportLoginCompleted("succeeded_via_ots", "git");
94+
log.info(
95+
logContext,
96+
`User will be logged in via OTS on the base domain. (Indirect) redirect to: ${returnTo}`,
97+
);
98+
const baseDomainRedirect = this.config.hostUrl.asLoginWithOTS(user.id, token.token, returnTo).toString();
99+
response.redirect(baseDomainRedirect);
100+
return;
101+
}
102+
103+
// (default case) If we got redirected here onto the base domain of the Gitpod installation, we can just issue the cookie right away.
81104
const cookie = await this.session.createJWTSessionCookie(user.id);
82105
response.cookie(cookie.name, cookie.value, cookie.opts);
83106
reportJWTCookieIssued();
@@ -87,6 +110,10 @@ export class LoginCompletionHandler {
87110
response.redirect(returnTo);
88111
}
89112

113+
public isBaseDomain(req: express.Request): boolean {
114+
return req.hostname === this.config.hostUrl.url.hostname;
115+
}
116+
90117
public async updateAuthProviderAsVerified(hostname: string, user: User) {
91118
const hostCtx = this.hostContextProvider.get(hostname);
92119
log.info("Updating auth provider as verified", { hostname });

‎components/server/src/prometheus-metrics.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ type LoginCounterStatus =
8585
| "failed"
8686
// The login attempt succeeded
8787
| "succeeded"
88+
// The login was successful, but we need to defer cookie creation via an OTS
89+
| "succeeded_via_ots"
8890
// The login attempt failed, because the client failed to provide complete session information, for instance.
8991
| "failed_client";
9092

‎components/server/src/session-handler.spec.db.ts

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,9 +163,9 @@ describe("SessionHandler", () => {
163163
expect(opts.httpOnly).to.equal(true);
164164
expect(opts.secure).to.equal(true);
165165
expect(opts.maxAge).to.equal(maxAge * 1000);
166-
expect(opts.sameSite).to.equal("strict");
166+
expect(opts.sameSite).to.equal("lax");
167167

168-
expect(name, "Check cookie name").to.equal("_gitpod_dev_jwt_");
168+
expect(name, "Check cookie name").to.equal("__Host-_gitpod_dev_jwt_");
169169
});
170170
});
171171
describe("jwtSessionConvertor", () => {
@@ -212,5 +212,35 @@ describe("SessionHandler", () => {
212212
expect(res.value).to.equal("JWT Session is invalid");
213213
expect(res.cookie).to.be.undefined;
214214
});
215+
216+
it("old JWT cookie is present, is accepted (!), and we get a new one", async () => {
217+
const oldExpiredCookie = await sessionHandler.createJWTSessionCookie(existingUser.id, {
218+
issuedAtMs: Date.now() - SessionHandler.JWT_REFRESH_THRESHOLD - 1,
219+
});
220+
oldExpiredCookie.name = "_gitpod_dev_jwt_";
221+
const newCookie = await sessionHandler.createJWTSessionCookie(existingUser.id);
222+
223+
const res = await handle(existingUser, `${oldExpiredCookie.name}=${oldExpiredCookie.value}`);
224+
expect(res.status).to.equal(200);
225+
expect(res.value).to.equal("Refreshed JWT cookie issued.");
226+
expect(res.cookie).to.not.be.undefined;
227+
expect(res.cookie?.split("=")[0]).to.equal(newCookie.name);
228+
});
229+
230+
it("old expired AND new one JWT cookies are present, new one is accepted", async () => {
231+
const oldExpiredCookie = await sessionHandler.createJWTSessionCookie(existingUser.id, {
232+
issuedAtMs: Date.now() - SessionHandler.JWT_REFRESH_THRESHOLD - 1,
233+
});
234+
oldExpiredCookie.name = "_gitpod_dev_jwt_";
235+
const newCookie = await sessionHandler.createJWTSessionCookie(existingUser.id);
236+
237+
const res = await handle(
238+
existingUser,
239+
`${oldExpiredCookie.name}=${oldExpiredCookie.value}; ${newCookie.name}=${newCookie.value}`,
240+
);
241+
expect(res.status).to.equal(200);
242+
expect(res.value).to.equal("User session already has a valid JWT session.");
243+
expect(res.cookie).to.be.undefined;
244+
});
215245
});
216246
});

‎components/server/src/session-handler.ts

Lines changed: 41 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,15 @@ export class SessionHandler {
2727

2828
public jwtSessionConvertor(): express.Handler {
2929
return async (req, res) => {
30-
const user = req.user;
30+
const { user } = req;
3131
if (!user) {
3232
res.status(401);
3333
res.send("User has no valid session.");
3434
return;
3535
}
3636

3737
const cookies = parseCookieHeader(req.headers.cookie || "");
38-
const jwtTokens = cookies[getJWTCookieName(this.config)];
38+
const jwtTokens = this.filterCookieValues(cookies);
3939

4040
let decoded: { payload: JwtPayload; keyId: string } | undefined = undefined;
4141
try {
@@ -146,12 +146,29 @@ export class SessionHandler {
146146
*/
147147
async verifyJWTCookie(cookie: string): Promise<JwtPayload | undefined> {
148148
const cookies = parseCookieHeader(cookie);
149-
const cookieValues = cookies[getJWTCookieName(this.config)];
149+
const cookieValues = this.filterCookieValues(cookies);
150150

151151
const token = await this.verifyFirstValidJwt(cookieValues);
152152
return token?.payload;
153153
}
154154

155+
/**
156+
* @param cookies
157+
* @returns Primary (the cookie name we set) AND secondary cookie (old accepted cookie name) values (in that order).
158+
*/
159+
private filterCookieValues(cookies: { [key: string]: string[] }): string[] {
160+
const cookieValues = cookies[getPrimaryJWTCookieName(this.config)] ?? [];
161+
162+
const secondaryCookieName = getSecondaryJWTCookieName(this.config);
163+
if (secondaryCookieName) {
164+
const secondaryCookieValues = cookies[secondaryCookieName];
165+
if (secondaryCookieValues) {
166+
cookieValues.push(...secondaryCookieValues);
167+
}
168+
}
169+
return cookieValues;
170+
}
171+
155172
/**
156173
* Returns the first valid session token it finds.
157174
* Edge cases:
@@ -204,10 +221,9 @@ export class SessionHandler {
204221
const token = await this.authJWT.sign(userID, payload, options?.expirySeconds);
205222

206223
return {
207-
name: getJWTCookieName(this.config),
224+
name: getPrimaryJWTCookieName(this.config),
208225
value: token,
209226
opts: {
210-
domain: getJWTCookieDomain(this.config),
211227
maxAge: this.config.auth.session.cookie.maxAge * 1000, // express does not match the HTTP spec and uses milliseconds
212228
httpOnly: this.config.auth.session.cookie.httpOnly,
213229
sameSite: this.config.auth.session.cookie.sameSite,
@@ -216,19 +232,33 @@ export class SessionHandler {
216232
};
217233
}
218234

219-
public clearSessionCookie(res: express.Response, config: Config): void {
220-
res.clearCookie(getJWTCookieName(this.config), {
221-
domain: getJWTCookieDomain(config),
235+
public clearSessionCookie(res: express.Response): void {
236+
const { secure, sameSite, httpOnly } = this.config.auth.session.cookie;
237+
res.clearCookie(getPrimaryJWTCookieName(this.config), {
238+
httpOnly,
239+
sameSite,
240+
secure,
222241
});
242+
243+
const secondaryCookieName = getSecondaryJWTCookieName(this.config);
244+
if (secondaryCookieName) {
245+
res.clearCookie(secondaryCookieName, {
246+
domain: this.config.hostUrl.url.hostname,
247+
});
248+
}
223249
}
224250
}
225251

226-
function getJWTCookieName(config: Config) {
252+
function getPrimaryJWTCookieName(config: Config) {
227253
return config.auth.session.cookie.name;
228254
}
229255

230-
function getJWTCookieDomain(config: Config): string {
231-
return config.hostUrl.url.hostname;
256+
function getSecondaryJWTCookieName(config: Config) {
257+
const PREFIX = "__Host-";
258+
if (!config.auth.session.cookie.name.startsWith(PREFIX)) {
259+
return undefined;
260+
}
261+
return config.auth.session.cookie.name.slice(PREFIX.length);
232262
}
233263

234264
function parseCookieHeader(c: string): { [key: string]: string[] } {

‎components/server/src/test/service-testing-container-module.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,11 @@ export const mockAuthConfig: AuthConfig = {
5555
issuer: "https://mp-server-d7650ec945.preview.gitpod-dev.com",
5656
lifetimeSeconds: 7 * 24 * 60 * 60,
5757
cookie: {
58-
name: "_gitpod_dev_jwt_",
58+
name: "__Host-_gitpod_dev_jwt_",
5959
secure: true,
6060
httpOnly: true,
6161
maxAge: 7 * 24 * 60 * 60,
62-
sameSite: "strict",
62+
sameSite: "lax",
6363
},
6464
},
6565
};

‎components/server/src/user/user-controller.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import { UserService } from "./user-service";
3737
import { WorkspaceService } from "../workspace/workspace-service";
3838
import { runWithSubjectId } from "../util/request-context";
3939
import { SubjectId } from "../auth/subject-id";
40+
import { TrustedValue } from "@gitpod/gitpod-protocol/lib/util/scrubbing";
4041

4142
export const ServerFactory = Symbol("ServerFactory");
4243
export type ServerFactory = () => GitpodServerImpl;
@@ -213,6 +214,14 @@ export class UserController {
213214
res.cookie(cookie.name, cookie.value, cookie.opts);
214215
reportJWTCookieIssued();
215216

217+
// If returnTo was passed and it's safe, redirect to it
218+
const returnTo = this.getSafeReturnToParam(req);
219+
if (returnTo) {
220+
log.info(`Redirecting after OTS login ${returnTo}`);
221+
res.redirect(returnTo);
222+
return;
223+
}
224+
216225
res.sendStatus(200);
217226
}),
218227
);
@@ -269,7 +278,7 @@ export class UserController {
269278
}
270279

271280
// clear cookies
272-
this.sessionHandler.clearSessionCookie(res, this.config);
281+
this.sessionHandler.clearSessionCookie(res);
273282

274283
// then redirect
275284
log.info(logContext, "(Logout) Redirecting...", { redirectToUrl, ...logPayload });
@@ -618,7 +627,7 @@ export class UserController {
618627
return returnToURL;
619628
}
620629

621-
log.debug("The redirect URL does not match", { query: req.query });
630+
log.debug("The redirect URL does not match", { query: new TrustedValue(req.query).value });
622631
return;
623632
}
624633

‎components/ws-proxy/pkg/proxy/routes.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -822,8 +822,8 @@ func removeSensitiveCookies(cookies []*http.Cookie, domain string) []*http.Cooki
822822

823823
n := 0
824824
for _, c := range cookies {
825-
if strings.HasPrefix(c.Name, hostnamePrefix) {
826-
// skip session cookie
825+
if strings.HasPrefix(c.Name, hostnamePrefix) || strings.HasPrefix(c.Name, "__Host-"+hostnamePrefix) {
826+
// skip session cookies
827827
continue
828828
}
829829
log.WithField("hostnamePrefix", hostnamePrefix).WithField("name", c.Name).Debug("keeping cookie")

0 commit comments

Comments
 (0)