From 09de76bd438bc84a30772314469a0de4ac1f014a Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 4 Jul 2023 09:12:15 +1200 Subject: [PATCH] OIDC: validate id token (#3531) * validate id token * comments * tidy comments --- package.json | 1 + spec/unit/oidc/authorize.spec.ts | 62 +++++++++++++++++- spec/unit/oidc/validate.spec.ts | 104 ++++++++++++++++++++++++++++++- src/oidc/authorize.ts | 6 +- src/oidc/error.ts | 3 +- src/oidc/validate.ts | 85 ++++++++++++++++++++++++- yarn.lock | 5 ++ 7 files changed, 259 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 4cfe6b38090..1dc1b531892 100644 --- a/package.json +++ b/package.json @@ -59,6 +59,7 @@ "another-json": "^0.2.0", "bs58": "^5.0.0", "content-type": "^1.0.4", + "jwt-decode": "^3.1.2", "loglevel": "^1.7.1", "matrix-events-sdk": "0.0.1", "matrix-widget-api": "^1.3.1", diff --git a/spec/unit/oidc/authorize.spec.ts b/spec/unit/oidc/authorize.spec.ts index 51f46aadf8d..d773954889a 100644 --- a/spec/unit/oidc/authorize.spec.ts +++ b/spec/unit/oidc/authorize.spec.ts @@ -15,6 +15,8 @@ limitations under the License. */ import fetchMock from "fetch-mock-jest"; +import { mocked } from "jest-mock"; +import jwtDecode from "jwt-decode"; import { Method } from "../../../src"; import * as crypto from "../../../src/crypto/crypto"; @@ -26,6 +28,8 @@ import { } from "../../../src/oidc/authorize"; import { OidcError } from "../../../src/oidc/error"; +jest.mock("jwt-decode"); + // save for resetting mocks const realSubtleCrypto = crypto.subtleCrypto; @@ -112,15 +116,28 @@ describe("oidc authorization", () => { describe("completeAuthorizationCodeGrant", () => { const codeVerifier = "abc123"; + const nonce = "test-nonce"; const redirectUri = baseUrl; const code = "auth_code_xyz"; const validBearerTokenResponse = { token_type: "Bearer", access_token: "test_access_token", refresh_token: "test_refresh_token", + id_token: "valid.id.token", expires_in: 12345, }; + const validDecodedIdToken = { + // nonce matches + nonce, + // not expired + exp: Date.now() / 1000 + 100000, + // audience is this client + aud: clientId, + // issuer matches + iss: delegatedAuthConfig.issuer, + }; + beforeEach(() => { fetchMock.mockClear(); fetchMock.resetBehavior(); @@ -129,10 +146,18 @@ describe("oidc authorization", () => { status: 200, body: JSON.stringify(validBearerTokenResponse), }); + + mocked(jwtDecode).mockReturnValue(validDecodedIdToken); }); it("should make correct request to the token endpoint", async () => { - await completeAuthorizationCodeGrant(code, { clientId, codeVerifier, redirectUri, delegatedAuthConfig }); + await completeAuthorizationCodeGrant(code, { + clientId, + codeVerifier, + redirectUri, + delegatedAuthConfig, + nonce, + }); expect(fetchMock).toHaveBeenCalledWith(tokenEndpoint, { method: Method.Post, @@ -147,6 +172,7 @@ describe("oidc authorization", () => { codeVerifier, redirectUri, delegatedAuthConfig, + nonce, }); expect(result).toEqual(validBearerTokenResponse); @@ -171,6 +197,7 @@ describe("oidc authorization", () => { codeVerifier, redirectUri, delegatedAuthConfig, + nonce, }); // results in token that uses 'Bearer' token type @@ -187,7 +214,13 @@ describe("oidc authorization", () => { { overwriteRoutes: true }, ); await expect(() => - completeAuthorizationCodeGrant(code, { clientId, codeVerifier, redirectUri, delegatedAuthConfig }), + completeAuthorizationCodeGrant(code, { + clientId, + codeVerifier, + redirectUri, + delegatedAuthConfig, + nonce, + }), ).rejects.toThrow(new Error(OidcError.CodeExchangeFailed)); }); @@ -202,8 +235,31 @@ describe("oidc authorization", () => { { overwriteRoutes: true }, ); await expect(() => - completeAuthorizationCodeGrant(code, { clientId, codeVerifier, redirectUri, delegatedAuthConfig }), + completeAuthorizationCodeGrant(code, { + clientId, + codeVerifier, + redirectUri, + delegatedAuthConfig, + nonce, + }), ).rejects.toThrow(new Error(OidcError.InvalidBearerTokenResponse)); }); + + it("should throw invalid id token error when id_token is invalid", async () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + // invalid audience + aud: "something-else", + }); + await expect(() => + completeAuthorizationCodeGrant(code, { + clientId, + codeVerifier, + redirectUri, + delegatedAuthConfig, + nonce, + }), + ).rejects.toThrow(new Error(OidcError.InvalidIdToken)); + }); }); }); diff --git a/spec/unit/oidc/validate.spec.ts b/spec/unit/oidc/validate.spec.ts index d5091ed89f9..71e4aeb7c0a 100644 --- a/spec/unit/oidc/validate.spec.ts +++ b/spec/unit/oidc/validate.spec.ts @@ -14,11 +14,20 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { mocked } from "jest-mock"; +import jwtDecode from "jwt-decode"; + import { M_AUTHENTICATION } from "../../../src"; import { logger } from "../../../src/logger"; -import { validateOIDCIssuerWellKnown, validateWellKnownAuthentication } from "../../../src/oidc/validate"; +import { + validateIdToken, + validateOIDCIssuerWellKnown, + validateWellKnownAuthentication, +} from "../../../src/oidc/validate"; import { OidcError } from "../../../src/oidc/error"; +jest.mock("jwt-decode"); + describe("validateWellKnownAuthentication()", () => { const baseWk = { "m.homeserver": { @@ -194,3 +203,96 @@ describe("validateOIDCIssuerWellKnown", () => { expect(() => validateOIDCIssuerWellKnown(wk)).toThrow(OidcError.OpSupport); }); }); + +describe("validateIdToken()", () => { + const nonce = "test-nonce"; + const issuer = "https://auth.org/issuer"; + const clientId = "test-client-id"; + const idToken = "test-id-token"; + + const validDecodedIdToken = { + // nonce matches + nonce, + // not expired + exp: Date.now() / 1000 + 5555, + // audience is this client + aud: clientId, + // issuer matches + iss: issuer, + }; + beforeEach(() => { + mocked(jwtDecode).mockClear().mockReturnValue(validDecodedIdToken); + + jest.spyOn(logger, "error").mockClear(); + }); + + it("should throw when idToken is falsy", () => { + expect(() => validateIdToken(undefined, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + }); + + it("should throw when idToken cannot be decoded", () => { + mocked(jwtDecode).mockImplementation(() => { + throw new Error("oh no!"); + }); + expect(() => validateIdToken(undefined, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + }); + + it("should throw when issuer does not match", () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + iss: "https://badissuer.com", + }); + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid issuer")); + }); + + it("should throw when audience does not include clientId", () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + aud: "qwerty,uiop,asdf", + }); + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid audience")); + }); + + it("should throw when audience includes clientId and other audiences", () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + aud: `${clientId},uiop,asdf`, + }); + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid audience")); + }); + + it("should throw when nonce does not match", () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + nonce: "something else", + }); + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid nonce")); + }); + + it("should throw when token does not have an expiry", () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + exp: undefined, + }); + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid expiry")); + }); + + it("should throw when token is expired", () => { + mocked(jwtDecode).mockReturnValue({ + ...validDecodedIdToken, + // expired in the past + exp: Date.now() / 1000 - 777, + }); + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken)); + expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid expiry")); + }); + + it("should not throw for a valid id token", () => { + expect(() => validateIdToken(idToken, issuer, clientId, nonce)).not.toThrow(); + }); +}); diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index 26211a3d486..9b7fc19a47c 100644 --- a/src/oidc/authorize.ts +++ b/src/oidc/authorize.ts @@ -20,7 +20,7 @@ import { subtleCrypto, TextEncoder } from "../crypto/crypto"; import { logger } from "../logger"; import { randomString } from "../randomstring"; import { OidcError } from "./error"; -import { ValidatedIssuerConfig } from "./validate"; +import { validateIdToken, ValidatedIssuerConfig } from "./validate"; /** * Authorization parameters which are used in the authentication request of an OIDC auth code flow. @@ -173,11 +173,13 @@ export const completeAuthorizationCodeGrant = async ( codeVerifier, redirectUri, delegatedAuthConfig, + nonce, }: { clientId: string; codeVerifier: string; redirectUri: string; delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig; + nonce: string; }, ): Promise => { const params = new URLSearchParams(); @@ -203,6 +205,8 @@ export const completeAuthorizationCodeGrant = async ( const token = await response.json(); if (isValidBearerTokenResponse(token)) { + // throws when token is invalid + validateIdToken(token.id_token, delegatedAuthConfig.issuer, clientId, nonce); return normalizeBearerTokenResponseTokenType(token); } diff --git a/src/oidc/error.ts b/src/oidc/error.ts index 6e70283a6ca..233ae34c519 100644 --- a/src/oidc/error.ts +++ b/src/oidc/error.ts @@ -23,5 +23,6 @@ export enum OidcError { DynamicRegistrationFailed = "Dynamic registration failed", DynamicRegistrationInvalid = "Dynamic registration invalid response", CodeExchangeFailed = "Failed to exchange code for token", - InvalidBearerTokenResponse = "Invalid bearer token", + InvalidBearerTokenResponse = "Invalid bearer token response", + InvalidIdToken = "Invalid ID token", } diff --git a/src/oidc/validate.ts b/src/oidc/validate.ts index 09ecf5e609d..8519b2bf118 100644 --- a/src/oidc/validate.ts +++ b/src/oidc/validate.ts @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and limitations under the License. */ +import jwtDecode from "jwt-decode"; + import { IClientWellKnown, IDelegatedAuthConfig, M_AUTHENTICATION } from "../client"; import { logger } from "../logger"; import { OidcError } from "./error"; @@ -83,7 +85,7 @@ const requiredArrayValue = (wellKnown: Record, key: string, val }; /** - * Validates issue `.well-known/openid-configuration` + * Validates issuer `.well-known/openid-configuration` * As defined in RFC5785 https://openid.net/specs/openid-connect-discovery-1_0.html * validates that OP is compatible with Element's OIDC flow * @param wellKnown - json object @@ -116,3 +118,84 @@ export const validateOIDCIssuerWellKnown = (wellKnown: unknown): ValidatedIssuer logger.error("Issuer configuration not valid"); throw new Error(OidcError.OpSupport); }; + +/** + * Standard JWT claims. + * + * @see https://datatracker.ietf.org/doc/html/rfc7519#section-4.1 + */ +interface JwtClaims { + [claim: string]: unknown; + /** The "iss" (issuer) claim identifies the principal that issued the JWT. */ + iss?: string; + /** The "sub" (subject) claim identifies the principal that is the subject of the JWT. */ + sub?: string; + /** The "aud" (audience) claim identifies the recipients that the JWT is intended for. */ + aud?: string | string[]; + /** The "exp" (expiration time) claim identifies the expiration time on or after which the JWT MUST NOT be accepted for processing. */ + exp?: number; + // unused claims excluded +} +interface IdTokenClaims extends JwtClaims { + nonce?: string; +} + +const decodeIdToken = (token: string): IdTokenClaims => { + try { + return jwtDecode(token); + } catch (error) { + logger.error("Could not decode id_token", error); + throw error; + } +}; + +/** + * Validate idToken + * https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation + * @param idToken - id token from token endpoint + * @param issuer - issuer for the OP as found during discovery + * @param clientId - this client's id as registered with the OP + * @param nonce - nonce used in the authentication request + * @throws when id token is invalid + */ +export const validateIdToken = (idToken: string | undefined, issuer: string, clientId: string, nonce: string): void => { + try { + if (!idToken) { + throw new Error("No ID token"); + } + const claims = decodeIdToken(idToken); + + // The Issuer Identifier for the OpenID Provider MUST exactly match the value of the iss (issuer) Claim. + if (claims.iss !== issuer) { + throw new Error("Invalid issuer"); + } + /** + * The Client MUST validate that the aud (audience) Claim contains its client_id value registered at the Issuer identified by the iss (issuer) Claim as an audience. + * The aud (audience) Claim MAY contain an array with more than one element. + * The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience, or if it contains additional audiences not trusted by the Client. + * EW: Don't accept tokens with other untrusted audiences + * */ + if (claims.aud !== clientId) { + throw new Error("Invalid audience"); + } + + /** + * If a nonce value was sent in the Authentication Request, a nonce Claim MUST be present and its value checked + * to verify that it is the same value as the one that was sent in the Authentication Request. + */ + if (claims.nonce !== nonce) { + throw new Error("Invalid nonce"); + } + + /** + * The current time MUST be before the time represented by the exp Claim. + * exp is an epoch timestamp in seconds + * */ + if (!claims.exp || Date.now() > claims.exp * 1000) { + throw new Error("Invalid expiry"); + } + } catch (error) { + logger.error("Invalid ID token", error); + throw new Error(OidcError.InvalidIdToken); + } +}; diff --git a/yarn.lock b/yarn.lock index 51914809e7f..ac40bb8bc35 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5393,6 +5393,11 @@ jstransformer@1.0.0: is-promise "^2.0.0" promise "^7.0.1" +jwt-decode@^3.1.2: + version "3.1.2" + resolved "https://registry.yarnpkg.com/jwt-decode/-/jwt-decode-3.1.2.tgz#3fb319f3675a2df0c2895c8f5e9fa4b67b04ed59" + integrity sha512-UfpWE/VZn0iP50d8cz9NrZLM9lSWhcJ+0Gt/nm4by88UL+J1SiKN8/5dkjMmbEzwL2CAe+67GsegCbIKtbp75A== + kind-of@^3.0.2: version "3.2.2" resolved "https://registry.yarnpkg.com/kind-of/-/kind-of-3.2.2.tgz#31ea21a734bab9bbb0f32466d893aea51e4a3c64"