From be083df4acd1fc2632fbdfdb3681e68b0242ae57 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 5 Jul 2023 16:29:21 +1200 Subject: [PATCH 01/10] use oidc-client-ts during oidc discovery --- package.json | 1 + spec/unit/oidc/validate.spec.ts | 1 + src/autodiscovery.ts | 70 +++++++++++++++++++++++++++++++-- src/oidc/discovery.ts | 58 +++++++++++++++++++++++++++ src/oidc/validate.ts | 30 ++++++++++++++ yarn.lock | 13 ++++++ 6 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 src/oidc/discovery.ts diff --git a/package.json b/package.json index 0610917f020..eaa8051b8fa 100644 --- a/package.json +++ b/package.json @@ -63,6 +63,7 @@ "loglevel": "^1.7.1", "matrix-events-sdk": "0.0.1", "matrix-widget-api": "^1.3.1", + "oidc-client-ts": "^2.2.4", "p-retry": "4", "sdp-transform": "^2.14.1", "unhomoglyph": "^1.0.6", diff --git a/spec/unit/oidc/validate.spec.ts b/spec/unit/oidc/validate.spec.ts index 71e4aeb7c0a..3574aa9605a 100644 --- a/spec/unit/oidc/validate.spec.ts +++ b/spec/unit/oidc/validate.spec.ts @@ -129,6 +129,7 @@ describe("validateOIDCIssuerWellKnown", () => { authorization_endpoint: "https://test.org/authorize", token_endpoint: "https://authorize.org/token", registration_endpoint: "https://authorize.org/regsiter", + revocation_endpoint: "https://authorize.org/regsiter", response_types_supported: ["code"], grant_types_supported: ["authorization_code"], code_challenge_methods_supported: ["S256"], diff --git a/src/autodiscovery.ts b/src/autodiscovery.ts index f9cf0398c2b..70caf2eb5d9 100644 --- a/src/autodiscovery.ts +++ b/src/autodiscovery.ts @@ -15,10 +15,18 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { SigningKey } from "oidc-client-ts"; + import { IClientWellKnown, IWellKnownConfig, IDelegatedAuthConfig, IServerVersions, M_AUTHENTICATION } from "./client"; import { logger } from "./logger"; import { MatrixError, Method, timeoutSignal } from "./http-api"; -import { ValidatedIssuerConfig, validateOIDCIssuerWellKnown, validateWellKnownAuthentication } from "./oidc/validate"; +import { discoverAndValidateAuthenticationConfig } from "./oidc/discovery"; +import { + ValidatedIssuerConfig, + ValidatedIssuerMetadata, + validateOIDCIssuerWellKnown, + validateWellKnownAuthentication, +} from "./oidc/validate"; import { OidcError } from "./oidc/error"; // Dev note: Auto discovery is part of the spec. @@ -50,12 +58,26 @@ interface AutoDiscoveryState { } interface WellKnownConfig extends Omit, AutoDiscoveryState {} +/** + * @deprecated in favour of OidcAuthorityConfig + */ interface DelegatedAuthConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig, AutoDiscoveryState {} +/** + * @experimental + */ +export interface OidcAuthorityConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig, AutoDiscoveryState { + metadata?: ValidatedIssuerMetadata; + signingKeys?: SigningKey[]; +} + export interface ClientConfig extends Omit { "m.homeserver": WellKnownConfig; "m.identity_server": WellKnownConfig; - "m.authentication"?: DelegatedAuthConfig | AutoDiscoveryState; + /** + * @experimental + */ + "m.authentication"?: OidcAuthorityConfig | AutoDiscoveryState; } /** @@ -262,7 +284,7 @@ export class AutoDiscovery { } }); - const authConfig = await this.validateDiscoveryAuthenticationConfig(wellknown); + const authConfig = await this.discoverAndValidateAuthenticationConfig(wellknown); clientConfig[M_AUTHENTICATION.stable!] = authConfig; // Step 8: Give the config to the caller (finally) @@ -271,6 +293,7 @@ export class AutoDiscovery { /** * Validate delegated auth configuration + * @deprecated use discoverAndValidateAuthenticationConfig * - m.authentication config is present and valid * - delegated auth issuer openid-configuration is reachable * - delegated auth issuer openid-configuration is configured correctly for us @@ -319,6 +342,47 @@ export class AutoDiscovery { } } + /** + * Validate delegated auth configuration + * - m.authentication config is present and valid + * - delegated auth issuer openid-configuration is reachable + * - delegated auth issuer openid-configuration is configured correctly for us + * When successful, validated authentication metadata and optionally signing keys will be returned + * Any errors are caught, and AutoDiscoveryState returned with error + * @param wellKnown - configuration object as returned + * by the .well-known auto-discovery endpoint + * @returns Config or failure result + */ + public static async discoverAndValidateAuthenticationConfig( + wellKnown: IClientWellKnown, + ): Promise { + try { + const result = await discoverAndValidateAuthenticationConfig(wellKnown); + + // include this for backwards compatibility + const validatedIssuerConfig = validateOIDCIssuerWellKnown(result.metadata); + + const response = { + state: AutoDiscoveryAction.SUCCESS, + error: null, + ...validatedIssuerConfig, + ...result, + }; + return response; + } catch (error) { + const errorMessage = (error as Error).message as unknown as OidcError; + const errorType = Object.values(OidcError).includes(errorMessage) ? errorMessage : OidcError.General; + + const state = + errorType === OidcError.NotSupported ? AutoDiscoveryAction.IGNORE : AutoDiscoveryAction.FAIL_ERROR; + + return { + state, + error: errorType, + }; + } + } + /** * Attempts to automatically discover client configuration information * prior to logging in. Such information includes the homeserver URL diff --git a/src/oidc/discovery.ts b/src/oidc/discovery.ts new file mode 100644 index 00000000000..7162d9a541f --- /dev/null +++ b/src/oidc/discovery.ts @@ -0,0 +1,58 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { MetadataService, OidcClientSettingsStore, SigningKey } from "oidc-client-ts"; + +import { IClientWellKnown } from "../client"; +import { isValidatedIssuerMetadata, ValidatedIssuerMetadata, validateWellKnownAuthentication } from "./validate"; + +/** + * @experimental + * Discover and validate delegated auth configuration + * - m.authentication config is present and valid + * - delegated auth issuer openid-configuration is reachable + * - delegated auth issuer openid-configuration is configured correctly for us + * When successful, validated metadata is returned + * @param wellKnown - configuration object as returned + * by the .well-known auto-discovery endpoint + * @returns validated authentication metadata and optionally signing keys + * @throws when delegated auth config is invalid or unreachable + */ +export const discoverAndValidateAuthenticationConfig = async ( + wellKnown: IClientWellKnown, +): Promise<{ + metadata: ValidatedIssuerMetadata; + signingKeys?: SigningKey[]; +}> => { + const homeserverAuthenticationConfig = validateWellKnownAuthentication(wellKnown); + + // create a temporary settings store so we can use metadata service for discovery + const settings = new OidcClientSettingsStore({ + authority: homeserverAuthenticationConfig.issuer, + redirect_uri: "", // Not known yet, this is here to make the type checker happy + client_id: "", // Not known yet, this is here to make the type checker happy + }); + const metadataService = new MetadataService(settings); + const metadata = await metadataService.getMetadata(); + const signingKeys = (await metadataService.getSigningKeys()) ?? undefined; + + isValidatedIssuerMetadata(metadata); + + return { + metadata, + signingKeys, + }; +}; diff --git a/src/oidc/validate.ts b/src/oidc/validate.ts index 8519b2bf118..d724634ce2e 100644 --- a/src/oidc/validate.ts +++ b/src/oidc/validate.ts @@ -15,6 +15,7 @@ limitations under the License. */ import jwtDecode from "jwt-decode"; +import { OidcMetadata } from "oidc-client-ts"; import { IClientWellKnown, IDelegatedAuthConfig, M_AUTHENTICATION } from "../client"; import { logger } from "../logger"; @@ -101,6 +102,7 @@ export const validateOIDCIssuerWellKnown = (wellKnown: unknown): ValidatedIssuer const isInvalid = [ requiredStringProperty(wellKnown, "authorization_endpoint"), requiredStringProperty(wellKnown, "token_endpoint"), + requiredStringProperty(wellKnown, "revocation_endpoint"), optionalStringProperty(wellKnown, "registration_endpoint"), requiredArrayValue(wellKnown, "response_types_supported", "code"), requiredArrayValue(wellKnown, "grant_types_supported", "authorization_code"), @@ -119,6 +121,34 @@ export const validateOIDCIssuerWellKnown = (wellKnown: unknown): ValidatedIssuer throw new Error(OidcError.OpSupport); }; +/** + * Metadata from OIDC authority discovery + * With validated properties required in type + */ +export type ValidatedIssuerMetadata = Partial & + Pick< + OidcMetadata, + | "authorization_endpoint" + | "token_endpoint" + | "registration_endpoint" + | "response_types_supported" + | "grant_types_supported" + | "code_challenge_methods_supported" + >; + +/** + * Wraps validateOIDCIssuerWellKnown in a type assertion + * that asserts expected properties are present + * (Typescript assertions cannot be arrow functions) + * @param metadata - issuer openid-configuration response + * @throws when metadata validation fails + */ +export function isValidatedIssuerMetadata( + metadata: Partial, +): asserts metadata is ValidatedIssuerMetadata { + validateOIDCIssuerWellKnown(metadata); +} + /** * Standard JWT claims. * diff --git a/yarn.lock b/yarn.lock index ac40bb8bc35..ec421221353 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3115,6 +3115,11 @@ crypto-browserify@^3.0.0: randombytes "^2.0.0" randomfill "^1.0.3" +crypto-js@^4.1.1: + version "4.1.1" + resolved "https://registry.yarnpkg.com/crypto-js/-/crypto-js-4.1.1.tgz#9e485bcf03521041bd85844786b83fb7619736cf" + integrity sha512-o2JlM7ydqd3Qk9CA0L4NL6mTzU2sdx96a+oOfPu8Mkl/PK51vSyoi8/rQ8NknZtk44vq15lmhAj9CIAGwgeWKw== + cssom@^0.5.0: version "0.5.0" resolved "https://registry.yarnpkg.com/cssom/-/cssom-0.5.0.tgz#d254fa92cd8b6fbd83811b9fbaed34663cc17c36" @@ -5920,6 +5925,14 @@ object.values@^1.1.6: define-properties "^1.1.4" es-abstract "^1.20.4" +oidc-client-ts@^2.2.4: + version "2.2.4" + resolved "https://registry.yarnpkg.com/oidc-client-ts/-/oidc-client-ts-2.2.4.tgz#7d86b5efe2248f3637a6f3a0ee1af86764aea125" + integrity sha512-nOZwIomju+AmXObl5Oq5PjrES/qTt8bLsENJCIydVgi9TEWk7SCkOU6X3RNkY7yfySRM1OJJvDKdREZdmnDT2g== + dependencies: + crypto-js "^4.1.1" + jwt-decode "^3.1.2" + once@^1.3.0, once@^1.4.0: version "1.4.0" resolved "https://registry.yarnpkg.com/once/-/once-1.4.0.tgz#583b1aa775961d4b113ac17d9c50baef9dd76bd1" From 755630258b68c0421f06ea1d7ec1e89faa279d0a Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 5 Jul 2023 17:28:15 +1200 Subject: [PATCH 02/10] export new type for auth config --- src/autodiscovery.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/autodiscovery.ts b/src/autodiscovery.ts index 70caf2eb5d9..c79f077d928 100644 --- a/src/autodiscovery.ts +++ b/src/autodiscovery.ts @@ -66,7 +66,7 @@ interface DelegatedAuthConfig extends IDelegatedAuthConfig, ValidatedIssuerConfi /** * @experimental */ -export interface OidcAuthorityConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig, AutoDiscoveryState { +export interface OidcAuthorityConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig { metadata?: ValidatedIssuerMetadata; signingKeys?: SigningKey[]; } @@ -77,7 +77,7 @@ export interface ClientConfig extends Omit { + ): Promise<(OidcAuthorityConfig & AutoDiscoveryState) | AutoDiscoveryState> { try { const result = await discoverAndValidateAuthenticationConfig(wellKnown); From 1f3fa36f8eb52c0f2f48890821a7efe4857aa548 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 5 Jul 2023 17:44:55 +1200 Subject: [PATCH 03/10] deprecate generateAuthorizationUrl in favour of generateOidcAuthorizationUrl --- src/oidc/authorize.ts | 26 ++++++++++++++++++++++++++ src/oidc/discovery.ts | 13 ++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index 9b7fc19a47c..ff7d1d529c3 100644 --- a/src/oidc/authorize.ts +++ b/src/oidc/authorize.ts @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { OidcClient, OidcClientSettings, WebStorageStateStore } from "oidc-client-ts"; + import { IDelegatedAuthConfig } from "../client"; import { Method } from "../http-api"; import { subtleCrypto, TextEncoder } from "../crypto/crypto"; @@ -74,6 +76,7 @@ export const generateAuthorizationParams = ({ redirectUri }: { redirectUri: stri }); /** + * @deprecated use generateOidcAuthorizationUrl * Generate a URL to attempt authorization with the OP * See https://openid.net/specs/openid-connect-basic-1_0.html#CodeRequest * @param authorizationUrl - endpoint to attempt authorization with the OP @@ -101,6 +104,29 @@ export const generateAuthorizationUrl = async ( return url.toString(); }; +/** + * @experimental + * Generate a URL to attempt authorization with the OP + * See https://openid.net/specs/openid-connect-basic-1_0.html#CodeRequest + * @param oidcClientSettings - oidc configuration + * @param homeserverName - used as state + * @returns a Promise with the url as a string + */ +export const generateOidcAuthorizationUrl = async ( + oidcClientSettings: OidcClientSettings, + homeserverName: string, +): Promise => { + const oidcClient = new OidcClient({ + ...oidcClientSettings, + stateStore: new WebStorageStateStore({ store: window.sessionStorage }), + }); + const request = await oidcClient.createSigninRequest({ + state: { host: homeserverName }, + }); + + return request.url; +}; + /** * The expected response type from the token endpoint during authorization code flow * Normalized to always use capitalized 'Bearer' for token_type diff --git a/src/oidc/discovery.ts b/src/oidc/discovery.ts index 7162d9a541f..0756dbf55aa 100644 --- a/src/oidc/discovery.ts +++ b/src/oidc/discovery.ts @@ -16,7 +16,7 @@ limitations under the License. import { MetadataService, OidcClientSettingsStore, SigningKey } from "oidc-client-ts"; -import { IClientWellKnown } from "../client"; +import { IClientWellKnown, IDelegatedAuthConfig } from "../client"; import { isValidatedIssuerMetadata, ValidatedIssuerMetadata, validateWellKnownAuthentication } from "./validate"; /** @@ -33,10 +33,12 @@ import { isValidatedIssuerMetadata, ValidatedIssuerMetadata, validateWellKnownAu */ export const discoverAndValidateAuthenticationConfig = async ( wellKnown: IClientWellKnown, -): Promise<{ - metadata: ValidatedIssuerMetadata; - signingKeys?: SigningKey[]; -}> => { +): Promise< + IDelegatedAuthConfig & { + metadata: ValidatedIssuerMetadata; + signingKeys?: SigningKey[]; + } +> => { const homeserverAuthenticationConfig = validateWellKnownAuthentication(wellKnown); // create a temporary settings store so we can use metadata service for discovery @@ -52,6 +54,7 @@ export const discoverAndValidateAuthenticationConfig = async ( isValidatedIssuerMetadata(metadata); return { + ...homeserverAuthenticationConfig, metadata, signingKeys, }; From b8086a104e1335ded9fed2e764a969bab1117d85 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 13:13:56 +1200 Subject: [PATCH 04/10] testing util for oidc configurations --- spec/test-utils/oidc.ts | 52 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 spec/test-utils/oidc.ts diff --git a/spec/test-utils/oidc.ts b/spec/test-utils/oidc.ts new file mode 100644 index 00000000000..c982a483080 --- /dev/null +++ b/spec/test-utils/oidc.ts @@ -0,0 +1,52 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { OidcAuthorityConfig } from "../../src"; +import { ValidatedIssuerMetadata } from "../../src/oidc/validate"; + +/** + * Makes a valid OidcAuthorityConfig with minimum valid values + * @param issuer used as the base for all other urls + * @returns OidcAuthorityConfig + */ +export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcAuthorityConfig => { + const metadata = mockOpenIdConfiguration(issuer); + + return { + issuer, + account: issuer + "account", + registrationEndpoint: metadata.registration_endpoint, + authorizationEndpoint: metadata.authorization_endpoint, + tokenEndpoint: metadata.token_endpoint, + metadata, + }; +}; + +/** + * Useful for mocking /.well-known/openid-configuration + * @param issuer used as the base for all other urls + * @returns ValidatedIssuerMetadata + */ +export const mockOpenIdConfiguration = (issuer = "https://auth.org/"): ValidatedIssuerMetadata => ({ + issuer, + revocation_endpoint: issuer + "revoke", + token_endpoint: issuer + "token", + authorization_endpoint: issuer + "auth", + registration_endpoint: issuer + "registration", + response_types_supported: ["query"], + grant_types_supported: ["code", "refreshToken"], + code_challenge_methods_supported: ["S256"], +}); From 613b83bf3273beb4cdeeaca540d3c65cfb44cef0 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 13:21:14 +1200 Subject: [PATCH 05/10] test generateOidcAuthorizationUrl --- spec/test-utils/oidc.ts | 8 ++--- spec/unit/oidc/authorize.spec.ts | 54 +++++++++++++++++++++----------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/spec/test-utils/oidc.ts b/spec/test-utils/oidc.ts index c982a483080..57cd0e4b3b4 100644 --- a/spec/test-utils/oidc.ts +++ b/spec/test-utils/oidc.ts @@ -14,15 +14,15 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { OidcAuthorityConfig } from "../../src"; +import { OidcClientConfig } from "../../src"; import { ValidatedIssuerMetadata } from "../../src/oidc/validate"; /** - * Makes a valid OidcAuthorityConfig with minimum valid values + * Makes a valid OidcClientConfig with minimum valid values * @param issuer used as the base for all other urls - * @returns OidcAuthorityConfig + * @returns OidcClientConfig */ -export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcAuthorityConfig => { +export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClientConfig => { const metadata = mockOpenIdConfiguration(issuer); return { diff --git a/spec/unit/oidc/authorize.spec.ts b/spec/unit/oidc/authorize.spec.ts index d773954889a..773d7071dd9 100644 --- a/spec/unit/oidc/authorize.spec.ts +++ b/spec/unit/oidc/authorize.spec.ts @@ -1,3 +1,7 @@ +/** + * @jest-environment jsdom + */ + /* Copyright 2023 The Matrix.org Foundation C.I.C. @@ -25,8 +29,10 @@ import { completeAuthorizationCodeGrant, generateAuthorizationParams, generateAuthorizationUrl, + generateOidcAuthorizationUrl, } from "../../../src/oidc/authorize"; import { OidcError } from "../../../src/oidc/error"; +import { makeDelegatedAuthConfig, mockOpenIdConfiguration } from "../../test-utils/oidc"; jest.mock("jwt-decode"); @@ -34,20 +40,16 @@ jest.mock("jwt-decode"); const realSubtleCrypto = crypto.subtleCrypto; describe("oidc authorization", () => { - const issuer = "https://auth.com/"; - const authorizationEndpoint = "https://auth.com/authorization"; - const tokenEndpoint = "https://auth.com/token"; - const delegatedAuthConfig = { - issuer, - registrationEndpoint: issuer + "registration", - authorizationEndpoint: issuer + "auth", - tokenEndpoint, - }; + const delegatedAuthConfig = makeDelegatedAuthConfig(); + const authorizationEndpoint = delegatedAuthConfig.metadata.authorization_endpoint; + const tokenEndpoint = delegatedAuthConfig.metadata.token_endpoint; const clientId = "xyz789"; const baseUrl = "https://test.com"; beforeAll(() => { jest.spyOn(logger, "warn"); + + fetchMock.get(delegatedAuthConfig.issuer + ".well-known/openid-configuration", mockOpenIdConfiguration()); }); afterEach(() => { @@ -97,20 +99,36 @@ describe("oidc authorization", () => { "A secure context is required to generate code challenge. Using plain text code challenge", ); }); + }); + + describe("generateOidcAuthorizationUrl()", () => { + it("should generate url with correct parameters", async () => { + const nonce = "abc123"; + + const metadata = delegatedAuthConfig.metadata; - it("uses a s256 code challenge when crypto is available", async () => { - jest.spyOn(crypto.subtleCrypto, "digest"); - const authorizationParams = generateAuthorizationParams({ redirectUri: baseUrl }); const authUrl = new URL( - await generateAuthorizationUrl(authorizationEndpoint, clientId, authorizationParams), + await generateOidcAuthorizationUrl({ + metadata, + homeserverUrl: baseUrl, + clientId, + redirectUri: baseUrl, + nonce, + }), ); - const codeChallenge = authUrl.searchParams.get("code_challenge"); - expect(crypto.subtleCrypto.digest).toHaveBeenCalledWith("SHA-256", expect.any(Object)); + expect(authUrl.searchParams.get("response_mode")).toEqual("query"); + expect(authUrl.searchParams.get("response_type")).toEqual("code"); + expect(authUrl.searchParams.get("client_id")).toEqual(clientId); + expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); + // scope minus the 10char random device id at the end + expect(authUrl.searchParams.get("scope")!.slice(0, -10)).toEqual( + "openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:", + ); + expect(authUrl.searchParams.get("state")).toBeTruthy(); + expect(authUrl.searchParams.get("nonce")).toEqual(nonce); - // didn't use plain text code challenge - expect(authorizationParams.codeVerifier).not.toEqual(codeChallenge); - expect(codeChallenge).toBeTruthy(); + expect(authUrl.searchParams.get("code_challenge")).toBeTruthy(); }); }); From 93e6100db455a09bdbacf2be0ee8a3aaa55a90a3 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 13:21:21 +1200 Subject: [PATCH 06/10] lint --- src/autodiscovery.ts | 10 +++++----- src/oidc/authorize.ts | 42 +++++++++++++++++++++++++++++++++--------- src/oidc/validate.ts | 2 ++ 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/autodiscovery.ts b/src/autodiscovery.ts index c79f077d928..757f14ffff2 100644 --- a/src/autodiscovery.ts +++ b/src/autodiscovery.ts @@ -59,15 +59,15 @@ interface AutoDiscoveryState { interface WellKnownConfig extends Omit, AutoDiscoveryState {} /** - * @deprecated in favour of OidcAuthorityConfig + * @deprecated in favour of OidcClientConfig */ interface DelegatedAuthConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig, AutoDiscoveryState {} /** * @experimental */ -export interface OidcAuthorityConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig { - metadata?: ValidatedIssuerMetadata; +export interface OidcClientConfig extends IDelegatedAuthConfig, ValidatedIssuerConfig { + metadata: ValidatedIssuerMetadata; signingKeys?: SigningKey[]; } @@ -77,7 +77,7 @@ export interface ClientConfig extends Omit { + ): Promise<(OidcClientConfig & AutoDiscoveryState) | AutoDiscoveryState> { try { const result = await discoverAndValidateAuthenticationConfig(wellKnown); diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index ff7d1d529c3..f448e91d15d 100644 --- a/src/oidc/authorize.ts +++ b/src/oidc/authorize.ts @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { OidcClient, OidcClientSettings, WebStorageStateStore } from "oidc-client-ts"; +import { OidcClient, WebStorageStateStore } from "oidc-client-ts"; import { IDelegatedAuthConfig } from "../client"; import { Method } from "../http-api"; @@ -22,7 +22,7 @@ import { subtleCrypto, TextEncoder } from "../crypto/crypto"; import { logger } from "../logger"; import { randomString } from "../randomstring"; import { OidcError } from "./error"; -import { validateIdToken, ValidatedIssuerConfig } from "./validate"; +import { validateIdToken, ValidatedIssuerConfig, ValidatedIssuerMetadata } from "./validate"; /** * Authorization parameters which are used in the authentication request of an OIDC auth code flow. @@ -37,6 +37,11 @@ export type AuthorizationParams = { nonce: string; }; +/** + * @experimental + * Generate the scope used in authorization request with OIDC OP + * @returns scope + */ const generateScope = (): string => { const deviceId = randomString(10); return `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:${deviceId}`; @@ -112,16 +117,35 @@ export const generateAuthorizationUrl = async ( * @param homeserverName - used as state * @returns a Promise with the url as a string */ -export const generateOidcAuthorizationUrl = async ( - oidcClientSettings: OidcClientSettings, - homeserverName: string, -): Promise => { +export const generateOidcAuthorizationUrl = async ({ + metadata, + redirectUri, + clientId, + homeserverUrl, + nonce, +}: { + clientId: string; + metadata: ValidatedIssuerMetadata; + homeserverUrl: string; + redirectUri: string; + nonce: string; +}): Promise => { + const scope = await generateScope(); const oidcClient = new OidcClient({ - ...oidcClientSettings, - stateStore: new WebStorageStateStore({ store: window.sessionStorage }), + ...metadata, + client_id: clientId, + redirect_uri: redirectUri, + authority: metadata.issuer, + response_mode: "query", + response_type: "code", + extraQueryParams: { + nonce, + }, + scope, + stateStore: new WebStorageStateStore({ prefix: "mx_", store: window.sessionStorage }), }); const request = await oidcClient.createSigninRequest({ - state: { host: homeserverName }, + state: { host: homeserverUrl }, }); return request.url; diff --git a/src/oidc/validate.ts b/src/oidc/validate.ts index d724634ce2e..a59bec22f7d 100644 --- a/src/oidc/validate.ts +++ b/src/oidc/validate.ts @@ -128,9 +128,11 @@ export const validateOIDCIssuerWellKnown = (wellKnown: unknown): ValidatedIssuer export type ValidatedIssuerMetadata = Partial & Pick< OidcMetadata, + | "issuer" | "authorization_endpoint" | "token_endpoint" | "registration_endpoint" + | "revocation_endpoint" | "response_types_supported" | "grant_types_supported" | "code_challenge_methods_supported" From 43fe0dc78bbd4c271f5c37d49f51415293fc1e60 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 16:11:26 +1200 Subject: [PATCH 07/10] test discovery --- spec/test-utils/oidc.ts | 5 +- spec/unit/autodiscovery.spec.ts | 82 +++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/spec/test-utils/oidc.ts b/spec/test-utils/oidc.ts index 57cd0e4b3b4..0bd97442daf 100644 --- a/spec/test-utils/oidc.ts +++ b/spec/test-utils/oidc.ts @@ -46,7 +46,8 @@ export const mockOpenIdConfiguration = (issuer = "https://auth.org/"): Validated token_endpoint: issuer + "token", authorization_endpoint: issuer + "auth", registration_endpoint: issuer + "registration", - response_types_supported: ["query"], - grant_types_supported: ["code", "refreshToken"], + jwks_uri: issuer + "jwks", + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], code_challenge_methods_supported: ["S256"], }); diff --git a/spec/unit/autodiscovery.spec.ts b/spec/unit/autodiscovery.spec.ts index e2e2c30824f..2ffe9be1a68 100644 --- a/spec/unit/autodiscovery.spec.ts +++ b/spec/unit/autodiscovery.spec.ts @@ -15,10 +15,17 @@ See the License for the specific language governing permissions and limitations under the License. */ +import fetchMock from "fetch-mock-jest"; import MockHttpBackend from "matrix-mock-request"; +import { M_AUTHENTICATION } from "../../src"; import { AutoDiscovery } from "../../src/autodiscovery"; import { OidcError } from "../../src/oidc/error"; +import { makeDelegatedAuthConfig } from "../test-utils/oidc"; + +// keep to reset the fetch function after using MockHttpBackend +// @ts-ignore private property +const realAutoDiscoveryFetch: typeof global.fetch = AutoDiscovery.fetchFn; describe("AutoDiscovery", function () { const getHttpBackend = (): MockHttpBackend => { @@ -27,6 +34,10 @@ describe("AutoDiscovery", function () { return httpBackend; }; + afterAll(() => { + AutoDiscovery.setFetchFn(realAutoDiscoveryFetch); + }); + it("should throw an error when no domain is specified", function () { getHttpBackend(); return Promise.all([ @@ -855,4 +866,75 @@ describe("AutoDiscovery", function () { }), ]); }); + + describe("m.authentication", () => { + const homeserverName = "example.org"; + const homeserverUrl = "https://chat.example.org/"; + const issuer = "https://auth.org/"; + + beforeAll(() => { + // make these tests independent from fetch mocking above + AutoDiscovery.setFetchFn(realAutoDiscoveryFetch); + }); + + beforeEach(() => { + fetchMock.resetBehavior(); + fetchMock.get(`${homeserverUrl}_matrix/client/versions`, { versions: ["r0.0.1"] }); + + fetchMock.get("https://example.org/.well-known/matrix/client", { + "m.homeserver": { + // Note: we also expect this test to trim the trailing slash + base_url: "https://chat.example.org/", + }, + "m.authentication": { + issuer, + }, + }); + }); + + it("should return valid authentication configuration", async () => { + const config = makeDelegatedAuthConfig(issuer); + + fetchMock.get(`${config.metadata.issuer}.well-known/openid-configuration`, config.metadata); + fetchMock.get(`${config.metadata.issuer}jwks`, { + status: 200, + headers: { + "Content-Type": "application/json", + }, + keys: [], + }); + + const result = await AutoDiscovery.findClientConfig(homeserverName); + + expect(result[M_AUTHENTICATION.stable!]).toEqual({ + state: AutoDiscovery.SUCCESS, + ...config, + signingKeys: [], + account: undefined, + error: null, + }); + }); + + it("should set state to error for invalid authentication configuration", async () => { + const config = makeDelegatedAuthConfig(issuer); + // authorization_code is required + config.metadata.grant_types_supported = ["openid"]; + + fetchMock.get(`${config.metadata.issuer}.well-known/openid-configuration`, config.metadata); + fetchMock.get(`${config.metadata.issuer}jwks`, { + status: 200, + headers: { + "Content-Type": "application/json", + }, + keys: [], + }); + + const result = await AutoDiscovery.findClientConfig(homeserverName); + + expect(result[M_AUTHENTICATION.stable!]).toEqual({ + state: AutoDiscovery.FAIL_ERROR, + error: OidcError.OpSupport, + }); + }); + }); }); From b46017c6a2bfa8d5f14d55e1a9852371054e6368 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 16:39:40 +1200 Subject: [PATCH 08/10] dont pass whole client wellknown to oidc validation funcs --- spec/unit/oidc/validate.spec.ts | 34 +++++++++++++-------------------- src/autodiscovery.ts | 6 ++++-- src/oidc/discovery.ts | 6 +++--- src/oidc/validate.ts | 6 ++---- 4 files changed, 22 insertions(+), 30 deletions(-) diff --git a/spec/unit/oidc/validate.spec.ts b/spec/unit/oidc/validate.spec.ts index 3574aa9605a..f177d0d5a58 100644 --- a/spec/unit/oidc/validate.spec.ts +++ b/spec/unit/oidc/validate.spec.ts @@ -35,7 +35,7 @@ describe("validateWellKnownAuthentication()", () => { }, }; it("should throw not supported error when wellKnown has no m.authentication section", () => { - expect(() => validateWellKnownAuthentication(baseWk)).toThrow(OidcError.NotSupported); + expect(() => validateWellKnownAuthentication(undefined)).toThrow(OidcError.NotSupported); }); it("should throw misconfigured error when authentication issuer is not a string", () => { @@ -45,7 +45,9 @@ describe("validateWellKnownAuthentication()", () => { issuer: { url: "test.com" }, }, }; - expect(() => validateWellKnownAuthentication(wk)).toThrow(OidcError.Misconfigured); + expect(() => validateWellKnownAuthentication(wk[M_AUTHENTICATION.stable!] as any)).toThrow( + OidcError.Misconfigured, + ); }); it("should throw misconfigured error when authentication account is not a string", () => { @@ -56,7 +58,9 @@ describe("validateWellKnownAuthentication()", () => { account: { url: "test" }, }, }; - expect(() => validateWellKnownAuthentication(wk)).toThrow(OidcError.Misconfigured); + expect(() => validateWellKnownAuthentication(wk[M_AUTHENTICATION.stable!] as any)).toThrow( + OidcError.Misconfigured, + ); }); it("should throw misconfigured error when authentication account is false", () => { @@ -67,7 +71,9 @@ describe("validateWellKnownAuthentication()", () => { account: false, }, }; - expect(() => validateWellKnownAuthentication(wk)).toThrow(OidcError.Misconfigured); + expect(() => validateWellKnownAuthentication(wk[M_AUTHENTICATION.stable!] as any)).toThrow( + OidcError.Misconfigured, + ); }); it("should return valid config when wk uses stable m.authentication", () => { @@ -78,7 +84,7 @@ describe("validateWellKnownAuthentication()", () => { account: "account.com", }, }; - expect(validateWellKnownAuthentication(wk)).toEqual({ + expect(validateWellKnownAuthentication(wk[M_AUTHENTICATION.stable!])).toEqual({ issuer: "test.com", account: "account.com", }); @@ -91,7 +97,7 @@ describe("validateWellKnownAuthentication()", () => { issuer: "test.com", }, }; - expect(validateWellKnownAuthentication(wk)).toEqual({ + expect(validateWellKnownAuthentication(wk[M_AUTHENTICATION.stable!])).toEqual({ issuer: "test.com", }); }); @@ -104,22 +110,8 @@ describe("validateWellKnownAuthentication()", () => { somethingElse: "test", }, }; - expect(validateWellKnownAuthentication(wk)).toEqual({ - issuer: "test.com", - }); - }); - - it("should return valid config when wk uses unstable prefix for m.authentication", () => { - const wk = { - ...baseWk, - [M_AUTHENTICATION.unstable!]: { - issuer: "test.com", - account: "account.com", - }, - }; - expect(validateWellKnownAuthentication(wk)).toEqual({ + expect(validateWellKnownAuthentication(wk[M_AUTHENTICATION.stable!])).toEqual({ issuer: "test.com", - account: "account.com", }); }); }); diff --git a/src/autodiscovery.ts b/src/autodiscovery.ts index 757f14ffff2..b7a16f10702 100644 --- a/src/autodiscovery.ts +++ b/src/autodiscovery.ts @@ -307,7 +307,8 @@ export class AutoDiscovery { wellKnown: IClientWellKnown, ): Promise { try { - const homeserverAuthenticationConfig = validateWellKnownAuthentication(wellKnown); + const authentication = M_AUTHENTICATION.findIn(wellKnown) || undefined; + const homeserverAuthenticationConfig = validateWellKnownAuthentication(authentication); const issuerOpenIdConfigUrl = `${this.sanitizeWellKnownUrl( homeserverAuthenticationConfig.issuer, @@ -357,7 +358,8 @@ export class AutoDiscovery { wellKnown: IClientWellKnown, ): Promise<(OidcClientConfig & AutoDiscoveryState) | AutoDiscoveryState> { try { - const result = await discoverAndValidateAuthenticationConfig(wellKnown); + const authentication = M_AUTHENTICATION.findIn(wellKnown) || undefined; + const result = await discoverAndValidateAuthenticationConfig(authentication); // include this for backwards compatibility const validatedIssuerConfig = validateOIDCIssuerWellKnown(result.metadata); diff --git a/src/oidc/discovery.ts b/src/oidc/discovery.ts index 0756dbf55aa..76aaeea8054 100644 --- a/src/oidc/discovery.ts +++ b/src/oidc/discovery.ts @@ -16,7 +16,7 @@ limitations under the License. import { MetadataService, OidcClientSettingsStore, SigningKey } from "oidc-client-ts"; -import { IClientWellKnown, IDelegatedAuthConfig } from "../client"; +import { IDelegatedAuthConfig } from "../client"; import { isValidatedIssuerMetadata, ValidatedIssuerMetadata, validateWellKnownAuthentication } from "./validate"; /** @@ -32,14 +32,14 @@ import { isValidatedIssuerMetadata, ValidatedIssuerMetadata, validateWellKnownAu * @throws when delegated auth config is invalid or unreachable */ export const discoverAndValidateAuthenticationConfig = async ( - wellKnown: IClientWellKnown, + authenticationConfig?: IDelegatedAuthConfig, ): Promise< IDelegatedAuthConfig & { metadata: ValidatedIssuerMetadata; signingKeys?: SigningKey[]; } > => { - const homeserverAuthenticationConfig = validateWellKnownAuthentication(wellKnown); + const homeserverAuthenticationConfig = validateWellKnownAuthentication(authenticationConfig); // create a temporary settings store so we can use metadata service for discovery const settings = new OidcClientSettingsStore({ diff --git a/src/oidc/validate.ts b/src/oidc/validate.ts index a59bec22f7d..458bbca812f 100644 --- a/src/oidc/validate.ts +++ b/src/oidc/validate.ts @@ -17,7 +17,7 @@ limitations under the License. import jwtDecode from "jwt-decode"; import { OidcMetadata } from "oidc-client-ts"; -import { IClientWellKnown, IDelegatedAuthConfig, M_AUTHENTICATION } from "../client"; +import { IDelegatedAuthConfig } from "../client"; import { logger } from "../logger"; import { OidcError } from "./error"; @@ -40,9 +40,7 @@ export type ValidatedIssuerConfig = { * @returns config - when present and valid * @throws when config is not found or invalid */ -export const validateWellKnownAuthentication = (wellKnown: IClientWellKnown): IDelegatedAuthConfig => { - const authentication = M_AUTHENTICATION.findIn(wellKnown); - +export const validateWellKnownAuthentication = (authentication?: IDelegatedAuthConfig): IDelegatedAuthConfig => { if (!authentication) { throw new Error(OidcError.NotSupported); } From 300c972a93ea4dd851b8abd2e86c50ab000928c4 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 17:28:06 +1200 Subject: [PATCH 09/10] add nonce --- src/oidc/authorize.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index f448e91d15d..f8be1bc94df 100644 --- a/src/oidc/authorize.ts +++ b/src/oidc/authorize.ts @@ -123,12 +123,14 @@ export const generateOidcAuthorizationUrl = async ({ clientId, homeserverUrl, nonce, + state, }: { clientId: string; metadata: ValidatedIssuerMetadata; homeserverUrl: string; redirectUri: string; nonce: string; + state: string; }): Promise => { const scope = await generateScope(); const oidcClient = new OidcClient({ @@ -138,14 +140,12 @@ export const generateOidcAuthorizationUrl = async ({ authority: metadata.issuer, response_mode: "query", response_type: "code", - extraQueryParams: { - nonce, - }, scope, - stateStore: new WebStorageStateStore({ prefix: "mx_", store: window.sessionStorage }), + stateStore: new WebStorageStateStore({ prefix: "mx_oidc_", store: window.sessionStorage }), }); const request = await oidcClient.createSigninRequest({ - state: { host: homeserverUrl }, + state, + nonce, }); return request.url; From 64d6ef1db2311e844d27fda7179a0c45d58c0862 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 7 Jul 2023 15:27:37 +1200 Subject: [PATCH 10/10] use client userState for homeserver --- src/oidc/authorize.ts | 9 +++++---- src/oidc/validate.ts | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index f8be1bc94df..8dca760c5cf 100644 --- a/src/oidc/authorize.ts +++ b/src/oidc/authorize.ts @@ -22,7 +22,7 @@ import { subtleCrypto, TextEncoder } from "../crypto/crypto"; import { logger } from "../logger"; import { randomString } from "../randomstring"; import { OidcError } from "./error"; -import { validateIdToken, ValidatedIssuerConfig, ValidatedIssuerMetadata } from "./validate"; +import { validateIdToken, ValidatedIssuerConfig, ValidatedIssuerMetadata, UserState } from "./validate"; /** * Authorization parameters which are used in the authentication request of an OIDC auth code flow. @@ -122,15 +122,15 @@ export const generateOidcAuthorizationUrl = async ({ redirectUri, clientId, homeserverUrl, + identityServerUrl, nonce, - state, }: { clientId: string; metadata: ValidatedIssuerMetadata; homeserverUrl: string; + identityServerUrl?: string; redirectUri: string; nonce: string; - state: string; }): Promise => { const scope = await generateScope(); const oidcClient = new OidcClient({ @@ -143,8 +143,9 @@ export const generateOidcAuthorizationUrl = async ({ scope, stateStore: new WebStorageStateStore({ prefix: "mx_oidc_", store: window.sessionStorage }), }); + const userState: UserState = { homeserverUrl, nonce, identityServerUrl }; const request = await oidcClient.createSigninRequest({ - state, + state: userState, nonce, }); diff --git a/src/oidc/validate.ts b/src/oidc/validate.ts index 458bbca812f..1db4ba85491 100644 --- a/src/oidc/validate.ts +++ b/src/oidc/validate.ts @@ -229,3 +229,19 @@ export const validateIdToken = (idToken: string | undefined, issuer: string, cli throw new Error(OidcError.InvalidIdToken); } }; + +/** + * State we ask OidcClient to store when starting oidc authorization flow (in `generateOidcAuthorizationUrl`) + * so that we can access it on return from the OP and complete login + */ +export type UserState = { + /** + * Remember which server we were trying to login to + */ + homeserverUrl: string; + identityServerUrl?: string; + /** + * Used to validate id token + */ + nonce: string; +};