From 43b8f5881b82607fbe8d188025bc96f2778a3124 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Wed, 15 Feb 2023 11:11:28 +0800 Subject: [PATCH] feat!: remove implicit flow and hybrid flow to ensure security (#97) --- packages/connector-oidc/README.md | 78 +-------- .../connector-oidc/docs/config-template.json | 1 - packages/connector-oidc/package.extend.json | 1 + packages/connector-oidc/src/index.ts | 36 ++-- packages/connector-oidc/src/types.test.ts | 46 +---- packages/connector-oidc/src/types.ts | 160 +----------------- packages/connector-oidc/src/utils.ts | 80 +-------- pnpm-lock.yaml | 2 + 8 files changed, 23 insertions(+), 381 deletions(-) diff --git a/packages/connector-oidc/README.md b/packages/connector-oidc/README.md index fc0880e5..a428a7e5 100644 --- a/packages/connector-oidc/README.md +++ b/packages/connector-oidc/README.md @@ -18,8 +18,6 @@ For detailed steps on registering a Google App for Logto's social sign-in use, p ## Compose the connector JSON -You can choose which OIDC authorization flow to use by configuring `oidcFlowType` as either 'AuthorizationCode', 'Implicit' or 'Hybrid'. - `scope` determines the resource you can access to after a successful authorization. `clientId` and `clientSecret` can be found at your Google app details page. @@ -32,6 +30,8 @@ Since an authentication request is required for all different flow types, an `au Here are some examples of OIDC connector config JSON connected to Google. Other vendor's config could vary. +You may be curious as to why a standard OIDC protocol supports both the implicit and hybrid flows, yet the Logto connector only supports the authorization flow. It has been determined that the implicit and hybrid flows are less secure than the authorization flow. Due to Logto's focus on security, it only supports the authorization flow for the highest level of security for its users, despite its slightly less convenient nature. + ### Authorization Code Flow `responseType` can only be 'code' with authorization code flow, so we make it optional and a default value will be automatically filled. @@ -40,7 +40,6 @@ Here are some examples of OIDC connector config JSON connected to Google. Other ```json { - "oidcFlowType": "AuthorizationCode", "scope": "profile email", "responseType": "", "clientId": "", @@ -68,77 +67,6 @@ Here are some examples of OIDC connector config JSON connected to Google. Other } ``` -### Implicit Flow - -`responseType` of implicit flow should either be 'id_token token' or 'id_token'. - -`tokenEndpoint` in this flow is not required. - -```json -{ - "oidcFlowType": "Implicit", - "scope": "profile email", - "responseType": "", - "clientId": "", - "clientSecret": "", - "authorizationEndpoint": "", - "idTokenVerificationConfig": { - "jwksUri": "", - "issuer": "", - }, - "authRequestOptionalConfig": { - "responseMode": "", - "display": "", - "prompt": "", - "maxAge": "", - "uiLocales": "", - "idTokenHint": "", - "loginHint": "", - "acrValues": "", - },, - "customConfig": { - "customParameter1": "", - "customParameter2": "" - } -} -``` - -### Hybrid Flow - -`responseType` in hybrid flow can be one of 'id_token code', 'id_token code token', 'code token'. - -If 'id_token' is not presented in `responseType`, then `tokenEndpoint` is required for fetching `idToken` via token request. - -```json -{ - "oidcFlowType": "Implicit", - "scope": "profile email", - "responseType": "", - "clientId": "", - "clientSecret": "", - "authorizationEndpoint": "", - "tokenEndpoint": "", - "idTokenVerificationConfig": { - "jwksUri": "", - "issuer": "", - }, - "authRequestOptionalConfig": { - "responseMode": "", - "display": "", - "prompt": "", - "maxAge": "", - "uiLocales": "", - "idTokenHint": "", - "loginHint": "", - "acrValues": "", - },, - "customConfig": { - "customParameter1": "", - "customParameter2": "" - } -} -``` - > ℹ️ **Note** > > For all flow types, we provided an OPTIONAL `customConfig` key to put your customize parameters. @@ -148,11 +76,11 @@ If 'id_token' is not presented in `responseType`, then `tokenEndpoint` is requir | Name | Type | Required | |-------------------------------------|-------------------------------------|-----------| -| oidcFlowType | enum | True | | scope | string | True | | clientId | string | True | | clientSecret | string | True | | authorizationEndpoint | string | True | +| tokenEndpoint | string | True | | idTokenVerificationConfig | IdTokenVerificationConfig | True | | authRequestOptionalConfig | AuthRequestOptionalConfig | False | | customConfig | Record | False | diff --git a/packages/connector-oidc/docs/config-template.json b/packages/connector-oidc/docs/config-template.json index 69fca909..e84b365e 100644 --- a/packages/connector-oidc/docs/config-template.json +++ b/packages/connector-oidc/docs/config-template.json @@ -1,5 +1,4 @@ { - "oidcFlowType": "AuthorizationCode", "authorizationEndpoint": "", "tokenEndpoint": "", "clientId": "", diff --git a/packages/connector-oidc/package.extend.json b/packages/connector-oidc/package.extend.json index 68fbbff0..b349292d 100644 --- a/packages/connector-oidc/package.extend.json +++ b/packages/connector-oidc/package.extend.json @@ -4,6 +4,7 @@ "version": "1.0.0-beta.18", "description": "OIDC standard connector implementation.", "dependencies": { + "@logto/core-kit": "1.0.0-beta.30", "jose": "^4.3.8", "nanoid": "^4.0.0" } diff --git a/packages/connector-oidc/src/index.ts b/packages/connector-oidc/src/index.ts index 24a50c52..8a8cec78 100644 --- a/packages/connector-oidc/src/index.ts +++ b/packages/connector-oidc/src/index.ts @@ -11,6 +11,7 @@ import { validateConfig, ConnectorType, } from '@logto/connector-kit'; +import { generateStandardId } from '@logto/core-kit'; import { assert, conditional, pick } from '@silverhand/essentials'; import { HTTPError } from 'got'; import { createRemoteJWKSet, jwtVerify } from 'jose'; @@ -18,16 +19,10 @@ import snakecaseKeys from 'snakecase-keys'; import { defaultMetadata } from './constant.js'; import type { OidcConfig } from './types.js'; -import { idTokenProfileStandardClaimsGuard, oidcConfigGuard, OidcFlowType } from './types.js'; -import { - buildIdGenerator, - isIdTokenInResponseType, - getAuthorizationCodeFlowIdToken, - getImplicitFlowIdToken, - getHybridFlowIdToken, -} from './utils.js'; +import { idTokenProfileStandardClaimsGuard, oidcConfigGuard } from './types.js'; +import { getIdToken } from './utils.js'; -const generateNonce = () => buildIdGenerator(12)(); +const generateNonce = () => generateStandardId(); const getAuthorizationUri = (getConfig: GetConnectorConfig): GetAuthorizationUri => @@ -37,7 +32,6 @@ const getAuthorizationUri = const parsedConfig = oidcConfigGuard.parse(config); const nonce = generateNonce(); - const needNonce = isIdTokenInResponseType(parsedConfig.responseType); assert( setSession, @@ -56,26 +50,13 @@ const getAuthorizationUri = ...authRequestOptionalConfig, ...customConfig, }), - ...(needNonce ? { nonce } : {}), + nonce, redirect_uri: redirectUri, }); return `${parsedConfig.authorizationEndpoint}?${queryParameters.toString()}`; }; -const getIdToken = async (config: OidcConfig, data: unknown, redirectUri?: string) => { - if (config.oidcFlowType === OidcFlowType.AuthorizationCode) { - return getAuthorizationCodeFlowIdToken(config, data, redirectUri); - } - - if (config.oidcFlowType === OidcFlowType.Implicit) { - return getImplicitFlowIdToken(data); - } - - // Hybrid Flow - return getHybridFlowIdToken(config, data, redirectUri); -}; - const getUserInfo = (getConfig: GetConnectorConfig): GetUserInfo => // eslint-disable-next-line complexity @@ -92,6 +73,13 @@ const getUserInfo = ); const { nonce: validationNonce, redirectUri } = await getSession(); + assert( + redirectUri, + new ConnectorError(ConnectorErrorCodes.General, { + message: "CAN NOT find 'redirectUri' from connector session.", + }) + ); + const { id_token: idToken } = await getIdToken(parsedConfig, data, redirectUri); if (!idToken) { diff --git a/packages/connector-oidc/src/types.test.ts b/packages/connector-oidc/src/types.test.ts index cf8775e0..d6e1b088 100644 --- a/packages/connector-oidc/src/types.test.ts +++ b/packages/connector-oidc/src/types.test.ts @@ -1,8 +1,4 @@ -import { - scopePostProcessor, - implicitFlowResponsePostProcessor, - hybridFlowResponsePostProcessor, -} from './types.js'; +import { scopePostProcessor } from './types.js'; describe('scopePostProcessor', () => { it('`openid` will be added if not exists (with empty string)', () => { @@ -17,43 +13,3 @@ describe('scopePostProcessor', () => { expect(scopePostProcessor('profile openid')).toEqual('profile openid'); }); }); - -describe('implicitFlowResponsePostProcessor', () => { - it('return fully space-deliminated response type', () => { - expect(implicitFlowResponsePostProcessor('')).toEqual('id_token token'); - }); - - it('throws when required `id_token` is not presented', () => { - expect(() => implicitFlowResponsePostProcessor('token')).toThrow(); - }); - - it('throws when invalid type is not presented', () => { - expect(() => implicitFlowResponsePostProcessor('id_token hello')).toThrow(); - }); - - it('return original response type', () => { - expect(implicitFlowResponsePostProcessor('id_token')).toEqual('id_token'); - }); -}); - -describe('hybridFlowResponsePostProcessor', () => { - it('return fully space-deliminated response type', () => { - expect(hybridFlowResponsePostProcessor('')).toEqual('id_token code token'); - }); - - it('throws when required `code` is not presented', () => { - expect(() => hybridFlowResponsePostProcessor('id_token token')).toThrow(); - }); - - it('throws when invalid type is not presented', () => { - expect(() => hybridFlowResponsePostProcessor('code hello')).toThrow(); - }); - - it('throws when `token` or `id_token` is not presented', () => { - expect(() => hybridFlowResponsePostProcessor('code')).toThrow(); - }); - - it('return original response type', () => { - expect(hybridFlowResponsePostProcessor('id_token code token')).toEqual('id_token code token'); - }); -}); diff --git a/packages/connector-oidc/src/types.ts b/packages/connector-oidc/src/types.ts index b9359db3..51de1054 100644 --- a/packages/connector-oidc/src/types.ts +++ b/packages/connector-oidc/src/types.ts @@ -3,12 +3,6 @@ import { z } from 'zod'; const scopeOpenid = 'openid' as const; export const delimiter = /[ +]/; -const allImplicitFlowResponseTypes = ['id_token', 'token']; -const requiredImplicitFlowResponseTypes = new Set(['id_token']); - -const allHybridFlowResponseTypes = ['id_token', 'code', 'token']; -const requiredHybridFlowResponseTypes = new Set(['code']); - // Space-delimited 'scope' MUST contain 'openid', see https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth export const scopePostProcessor = (scope: string) => { const splitScopes = scope.split(delimiter).filter(Boolean); @@ -20,93 +14,6 @@ export const scopePostProcessor = (scope: string) => { return scope; }; -export const implicitFlowResponsePostProcessor = (responseType: string) => { - const splitResponseTypes = responseType.split(delimiter).filter(Boolean); - - if (splitResponseTypes.length === 0) { - return allImplicitFlowResponseTypes.join(' '); - } - - if ( - !splitResponseTypes.some((singleResponseType) => - requiredImplicitFlowResponseTypes.has(singleResponseType) - ) - ) { - throw new TypeError( - `Required responseType ${JSON.stringify( - Array.from(requiredImplicitFlowResponseTypes) - )} is not presented!` - ); - } - - if ( - !splitResponseTypes.every((singleResponseType) => - allImplicitFlowResponseTypes.includes(singleResponseType) - ) - ) { - throw new TypeError( - `Some responseType ${JSON.stringify( - splitResponseTypes.filter( - (singleResponseType) => !allImplicitFlowResponseTypes.includes(singleResponseType) - ) - )} is invalid.` - ); - } - - return splitResponseTypes.join(' '); -}; - -export const hybridFlowResponsePostProcessor = (responseType: string) => { - const splitResponseTypes = responseType.split(delimiter).filter(Boolean); - - if (splitResponseTypes.length === 0) { - return allHybridFlowResponseTypes.join(' '); - } - - if ( - !splitResponseTypes.some((singleResponseType) => - requiredHybridFlowResponseTypes.has(singleResponseType) - ) - ) { - throw new TypeError( - `Required responseType ${JSON.stringify( - splitResponseTypes.filter( - (singleResponseType) => !allHybridFlowResponseTypes.includes(singleResponseType) - ) - )} is not presented!` - ); - } - - if ( - !splitResponseTypes.every((singleResponseType) => - allHybridFlowResponseTypes.includes(singleResponseType) - ) - ) { - throw new TypeError( - `Some responseType ${JSON.stringify( - splitResponseTypes.filter( - (singleResponseType) => !allHybridFlowResponseTypes.includes(singleResponseType) - ) - )} is invalid.` - ); - } - - if (splitResponseTypes.length === 1) { - throw new TypeError("At least one of 'token' and 'id_token' is needed."); - } - - // For hybrid flow, 'code' is always required in response type, at least one another response type among 'token' and 'id_token' is required. - return splitResponseTypes.join(' '); -}; - -export enum OidcFlowType { - AuthorizationCode = 'AuthorizationCode', - Implicit = 'Implicit', - Hybrid = 'Hybrid', -} - -export const oidcFlowTypeGuard = z.nativeEnum(OidcFlowType); - // See https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims. // We only concern a subset of them, and social identity provider usually does not provide a complete set of them. export const idTokenProfileStandardClaimsGuard = z.object({ @@ -177,9 +84,8 @@ export const idTokenVerificationConfigGuard = z.object({ jwksUri: z.string() }). export type IdTokenVerificationConfig = z.infer; -export const authorizationCodeConfigGuard = z +export const oidcConfigGuard = z .object({ - oidcFlowType: z.literal(OidcFlowType.AuthorizationCode), responseType: z.literal('code').optional().default('code'), grantType: z.literal('authorization_code').optional().default('authorization_code'), scope: z.string().transform(scopePostProcessor), @@ -190,50 +96,6 @@ export const authorizationCodeConfigGuard = z .merge(endpointConfigGuard) .merge(clientConfigGuard); -export type AuthorizationCodeConfig = z.infer; - -export const implicitConfigGuard = z - .object({ - oidcFlowType: z.literal(OidcFlowType.Implicit), - responseType: z - .string() - .optional() - .default('id_token') - .transform(implicitFlowResponsePostProcessor), - scope: z.string().transform(scopePostProcessor), - idTokenVerificationConfig: idTokenVerificationConfigGuard, - authRequestOptionalConfig: authRequestOptionalConfigGuard.optional(), - customConfig: z.record(z.string()).optional(), - }) - .merge(endpointConfigGuard.pick({ authorizationEndpoint: true })) - .merge(clientConfigGuard); - -export const hybridConfigGuard = z - .object({ - oidcFlowType: z.literal(OidcFlowType.Hybrid), - responseType: z - .string() - .optional() - .default('code id_token token') - .transform(hybridFlowResponsePostProcessor), - grantType: z.literal('authorization_code').optional().default('authorization_code'), - scope: z.string().transform(scopePostProcessor), - idTokenVerificationConfig: idTokenVerificationConfigGuard, - authRequestOptionalConfig: authRequestOptionalConfigGuard.optional(), - customConfig: z.record(z.string()).optional(), - }) - .merge(endpointConfigGuard.pick({ authorizationEndpoint: true })) - .merge(endpointConfigGuard.pick({ tokenEndpoint: true }).partial()) - .merge(clientConfigGuard); - -export type HybridConfig = z.infer; - -export const oidcConfigGuard = z.discriminatedUnion('oidcFlowType', [ - authorizationCodeConfigGuard, - implicitConfigGuard, - hybridConfigGuard, -]); - export type OidcConfig = z.infer; export const authResponseGuard = z @@ -245,26 +107,6 @@ export const authResponseGuard = z export type AuthResponse = z.infer; -export const implicitAuthResponseGuard = z - .object({ - id_token: z.string(), - access_token: z.string().optional(), - token_type: z.string().optional(), - expires_in: z.string().optional(), - scope: z.string().optional(), - state: z.string().optional(), - }) - .catchall(z.string()); - -export const hybridAuthResponseGuard = z - .object({ - code: z.string(), - id_token: z.string().optional(), - access_token: z.string().optional(), - token_type: z.string().optional(), - }) - .catchall(z.string()); - export const accessTokenResponseGuard = z.object({ id_token: z.string(), access_token: z.string().optional(), diff --git a/packages/connector-oidc/src/utils.ts b/packages/connector-oidc/src/utils.ts index d6032051..c0ecfbcf 100644 --- a/packages/connector-oidc/src/utils.ts +++ b/packages/connector-oidc/src/utils.ts @@ -2,22 +2,11 @@ import { ConnectorError, ConnectorErrorCodes, parseJson } from '@logto/connector import { assert, pick } from '@silverhand/essentials'; import type { Response } from 'got'; import { got, HTTPError } from 'got'; -import { customAlphabet } from 'nanoid'; import snakecaseKeys from 'snakecase-keys'; import { defaultTimeout } from './constant.js'; -import type { AccessTokenResponse, AuthorizationCodeConfig, HybridConfig } from './types.js'; -import { - accessTokenResponseGuard, - delimiter, - authResponseGuard, - implicitAuthResponseGuard, - hybridAuthResponseGuard, -} from './types.js'; - -const alphabet = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; -// FIXME @darcy: Temporary use this workaround, this change has been made in @logto/core-kit but has not been published yet. -export const buildIdGenerator = (size: number) => customAlphabet(alphabet, size); +import type { AccessTokenResponse, OidcConfig } from './types.js'; +import { accessTokenResponseGuard, delimiter, authResponseGuard } from './types.js'; export const accessTokenRequester = async ( tokenEndpoint: string, @@ -63,11 +52,7 @@ export const isIdTokenInResponseType = (responseType: string) => { return responseType.split(delimiter).includes('id_token'); }; -export const getAuthorizationCodeFlowIdToken = async ( - config: AuthorizationCodeConfig, - data: unknown, - redirectUri?: string -) => { +export const getIdToken = async (config: OidcConfig, data: unknown, redirectUri: string) => { const result = authResponseGuard.safeParse(data); if (!result.success) { @@ -76,65 +61,6 @@ export const getAuthorizationCodeFlowIdToken = async ( const { code } = result.data; - assert( - redirectUri, - new ConnectorError(ConnectorErrorCodes.General, { - message: "CAN NOT find 'redirectUri' from connector session.", - }) - ); - - const { customConfig, ...rest } = config; - - const parameterObject = snakecaseKeys({ - ...pick(rest, 'grantType', 'clientId', 'clientSecret'), - ...customConfig, - code, - redirectUri, - }); - - return accessTokenRequester(config.tokenEndpoint, parameterObject); -}; - -export const getImplicitFlowIdToken = async (data: unknown) => { - const result = implicitAuthResponseGuard.safeParse(data); - - if (!result.success) { - throw new ConnectorError(ConnectorErrorCodes.General, data); - } - - return result.data; -}; - -export const getHybridFlowIdToken = async ( - config: HybridConfig, - data: unknown, - redirectUri?: string -) => { - assert( - redirectUri, - new ConnectorError(ConnectorErrorCodes.General, { - message: "CAN NOT find 'redirectUri' from connector session.", - }) - ); - const result = hybridAuthResponseGuard.safeParse(data); - - if (!result.success) { - throw new ConnectorError(ConnectorErrorCodes.General, data); - } - - if (result.data.id_token) { - return result.data; - } - - assert( - config.tokenEndpoint, - new ConnectorError(ConnectorErrorCodes.InvalidConfig, { - message: "'tokenEndpoint' should be provided since 'id_token' is missed in response type.", - }) - ); - - const { code } = result.data; - const { customConfig, ...rest } = config; const parameterObject = snakecaseKeys({ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6932fa50..e15a915f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1137,6 +1137,7 @@ importers: specifiers: '@jest/types': ^29.3.1 '@logto/connector-kit': 1.0.0-rc.1 + '@logto/core-kit': 1.0.0-beta.30 '@rollup/plugin-commonjs': ^24.0.0 '@rollup/plugin-json': ^6.0.0 '@rollup/plugin-node-resolve': ^15.0.1 @@ -1165,6 +1166,7 @@ importers: zod: ^3.20.2 dependencies: '@logto/connector-kit': 1.0.0-rc.1 + '@logto/core-kit': 1.0.0-beta.30 '@silverhand/essentials': 2.1.0 got: 12.5.3 jose: 4.9.2