-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[server] Set JWT cookie on sign-in WEB-100 #17200
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
Changes from all commits
ea00bb7
24ee06b
155f067
8e7b818
f9bec51
463e007
15ab233
895c652
3cfb2eb
6f8ae73
99a5403
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/** | ||
* Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
* Licensed under the GNU Affero General Public License (AGPL). | ||
* See License.AGPL.txt in the project root for license information. | ||
*/ | ||
|
||
import { suite, test } from "mocha-typescript"; | ||
import { AuthJWT, sign, verify } from "./jwt"; | ||
import { Container } from "inversify"; | ||
import { Config } from "../config"; | ||
import * as crypto from "crypto"; | ||
import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url"; | ||
import * as chai from "chai"; | ||
|
||
const expect = chai.expect; | ||
|
||
@suite() | ||
class TestAuthJWT { | ||
private container: Container; | ||
|
||
private signingKeyPair = crypto.generateKeyPairSync("rsa", { modulusLength: 2048 }); | ||
private validatingKeyPair1 = crypto.generateKeyPairSync("rsa", { modulusLength: 2048 }); | ||
private validatingKeyPair2 = crypto.generateKeyPairSync("rsa", { modulusLength: 2048 }); | ||
|
||
private config: Config = { | ||
hostUrl: new GitpodHostUrl("https://mp-server-d7650ec945.preview.gitpod-dev.com"), | ||
auth: { | ||
pki: { | ||
signing: toKeyPair(this.signingKeyPair), | ||
validating: [toKeyPair(this.validatingKeyPair1), toKeyPair(this.validatingKeyPair2)], | ||
}, | ||
}, | ||
} as Config; | ||
|
||
async before() { | ||
this.container = new Container(); | ||
this.container.bind(Config).toConstantValue(this.config); | ||
this.container.bind(AuthJWT).toSelf().inSingletonScope(); | ||
} | ||
|
||
@test | ||
async test_sign() { | ||
const sut = this.container.get<AuthJWT>(AuthJWT); | ||
|
||
const subject = "user-id"; | ||
const encoded = await sut.sign(subject, {}); | ||
|
||
const decoded = await verify(encoded, this.config.auth.pki.signing.publicKey, { | ||
algorithms: ["RS512"], | ||
}); | ||
|
||
expect(decoded["sub"]).to.equal(subject); | ||
expect(decoded["iss"]).to.equal("https://mp-server-d7650ec945.preview.gitpod-dev.com"); | ||
} | ||
|
||
@test | ||
async test_verify_uses_primary_first() { | ||
const sut = this.container.get<AuthJWT>(AuthJWT); | ||
|
||
const subject = "user-id"; | ||
const encoded = await sut.sign(subject, {}); | ||
|
||
const decoded = await sut.verify(encoded); | ||
|
||
expect(decoded["sub"]).to.equal(subject); | ||
expect(decoded["iss"]).to.equal("https://mp-server-d7650ec945.preview.gitpod-dev.com"); | ||
} | ||
|
||
@test | ||
async test_verify_validates_older_keys() { | ||
const sut = this.container.get<AuthJWT>(AuthJWT); | ||
|
||
const subject = "user-id"; | ||
const encoded = await sign({}, this.config.auth.pki.validating[1].privateKey, { | ||
algorithm: "RS512", | ||
expiresIn: "1d", | ||
issuer: this.config.hostUrl.toStringWoRootSlash(), | ||
subject, | ||
}); | ||
|
||
// should use the second validating key and succesfully verify | ||
const decoded = await sut.verify(encoded); | ||
|
||
expect(decoded["sub"]).to.equal(subject); | ||
expect(decoded["iss"]).to.equal("https://mp-server-d7650ec945.preview.gitpod-dev.com"); | ||
} | ||
} | ||
|
||
function toKeyPair(kp: crypto.KeyPairKeyObjectResult): { | ||
privateKey: string; | ||
publicKey: string; | ||
} { | ||
return { | ||
privateKey: kp.privateKey | ||
.export({ | ||
type: "pkcs1", | ||
format: "pem", | ||
}) | ||
.toString(), | ||
publicKey: kp.publicKey | ||
.export({ | ||
type: "pkcs1", | ||
format: "pem", | ||
}) | ||
.toString(), | ||
}; | ||
} | ||
|
||
module.exports = new TestAuthJWT(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
/** | ||
* Copyright (c) 2023 Gitpod GmbH. All rights reserved. | ||
* Licensed under the GNU Affero General Public License (AGPL). | ||
* See License.AGPL.txt in the project root for license information. | ||
*/ | ||
|
||
import * as jsonwebtoken from "jsonwebtoken"; | ||
import { Config } from "../config"; | ||
import { inject, injectable } from "inversify"; | ||
import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; | ||
|
||
const algorithm: jsonwebtoken.Algorithm = "RS512"; | ||
|
||
@injectable() | ||
export class AuthJWT { | ||
@inject(Config) protected config: Config; | ||
|
||
async sign(subject: string, payload: object | Buffer, expiresIn: string = `${24 * 7}h`): Promise<string> { | ||
const opts: jsonwebtoken.SignOptions = { | ||
algorithm, | ||
expiresIn, | ||
issuer: this.config.hostUrl.toStringWoRootSlash(), | ||
subject, | ||
}; | ||
|
||
return sign(payload, this.config.auth.pki.signing.privateKey, opts); | ||
} | ||
|
||
async verify(encoded: string): Promise<jsonwebtoken.JwtPayload> { | ||
// When we verify an encoded token, we verify it using all available public keys | ||
// That is, we check the following: | ||
// * The current signing public key | ||
// * All other validating public keys, in order | ||
// | ||
// We do this to allow for key-rotation. But tokens already issued would fail to validate | ||
// if the signing key was changed. To accept older sessions, which are still valid | ||
// we need to check for older keys also. | ||
const validatingPublicKeys = this.config.auth.pki.validating.map((keypair) => keypair.publicKey); | ||
const publicKeys = [ | ||
this.config.auth.pki.signing.publicKey, // signing key is checked first | ||
...validatingPublicKeys, | ||
]; | ||
|
||
let lastErr; | ||
for (let publicKey of publicKeys) { | ||
try { | ||
const decoded = await verify(encoded, publicKey, { | ||
algorithms: [algorithm], | ||
}); | ||
return decoded; | ||
} catch (err) { | ||
log.debug(`Failed to verify JWT token using public key.`, err); | ||
lastErr = err; | ||
} | ||
} | ||
|
||
log.error(`Failed to verify JWT using any available public key.`, lastErr, { | ||
publicKeyCount: publicKeys.length, | ||
}); | ||
throw lastErr; | ||
} | ||
} | ||
|
||
export async function sign( | ||
payload: string | object | Buffer, | ||
secret: jsonwebtoken.Secret, | ||
opts: jsonwebtoken.SignOptions, | ||
): Promise<string> { | ||
return new Promise((resolve, reject) => { | ||
jsonwebtoken.sign(payload, secret, opts, (err, encoded) => { | ||
if (err || !encoded) { | ||
return reject(err); | ||
} | ||
return resolve(encoded); | ||
}); | ||
}); | ||
} | ||
|
||
export async function verify( | ||
encoded: string, | ||
publicKey: string, | ||
opts: jsonwebtoken.VerifyOptions, | ||
): Promise<jsonwebtoken.JwtPayload> { | ||
return new Promise((resolve, reject) => { | ||
jsonwebtoken.verify(encoded, publicKey, opts, (err, decoded) => { | ||
if (err || !decoded) { | ||
return reject(err); | ||
} | ||
resolve(decoded); | ||
}); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,11 +13,14 @@ import { AuthFlow } from "./auth-provider"; | |
import { HostContextProvider } from "./host-context-provider"; | ||
import { AuthProviderService } from "./auth-provider-service"; | ||
import { TosFlow } from "../terms/tos-flow"; | ||
import { increaseLoginCounter } from "../prometheus-metrics"; | ||
import { increaseLoginCounter, reportJWTCookieIssued } from "../prometheus-metrics"; | ||
import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics"; | ||
import { trackLogin } from "../analytics"; | ||
import { UserService } from "../user/user-service"; | ||
import { SubscriptionService } from "@gitpod/gitpod-payment-endpoint/lib/accounting"; | ||
import { getExperimentsClientForBackend } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server"; | ||
import { SessionHandlerProvider } from "../session-handler"; | ||
import { AuthJWT } from "./jwt"; | ||
|
||
/** | ||
* The login completion handler pulls the strings between the OAuth2 flow, the ToS flow, and the session management. | ||
|
@@ -30,6 +33,7 @@ export class LoginCompletionHandler { | |
@inject(AuthProviderService) protected readonly authProviderService: AuthProviderService; | ||
@inject(UserService) protected readonly userService: UserService; | ||
@inject(SubscriptionService) protected readonly subscriptionService: SubscriptionService; | ||
@inject(AuthJWT) protected readonly authJWT: AuthJWT; | ||
|
||
async complete( | ||
request: express.Request, | ||
|
@@ -93,6 +97,26 @@ export class LoginCompletionHandler { | |
); | ||
} | ||
|
||
const isJWTCookieExperimentEnabled = await getExperimentsClientForBackend().getValueAsync( | ||
"jwtSessionCookieEnabled", | ||
false, | ||
{ | ||
user: user, | ||
}, | ||
); | ||
if (isJWTCookieExperimentEnabled) { | ||
const token = await this.authJWT.sign(user.id, {}); | ||
|
||
response.cookie(SessionHandlerProvider.getJWTCookieName(this.config.hostUrl), token, { | ||
maxAge: this.config.session.maxAgeMs, | ||
httpOnly: true, | ||
sameSite: "lax", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches the current cookie setting which we issue for session cookie. (it actually also marks the cookie as "secure" which we don't do currently) Do you know the exact implications of changing this to strict? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A strict cookie means, that if the browser (already received the cookie) is redirected to a 3rd party, and then back to Gitpod, the cookie will not be sent. This is a typical scenario for redirect-based flows, where the 3rd party knows some sort of "returnTo" URL to redirect back, e.g. after asking user for consent. Hmm, now having that said, it sounds like we'd break Git Integrations with that. There are (at least) two solutions available:
That's getting to complicating here, I guess. So, not raising the bar here seems ok, but keep in mind that we need to make it strict eventually to improve the security posture. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
secure: true, | ||
}); | ||
|
||
reportJWTCookieIssued(); | ||
} | ||
|
||
response.redirect(returnTo); | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.