From 416fd0903073e79ca1d522ac3216bb2cca2b75cd Mon Sep 17 00:00:00 2001 From: Deepak Prabhakara Date: Fri, 3 Jan 2025 11:06:55 +0000 Subject: [PATCH] Store encrypted profile information with no way to access it without the key which is sent to client and never stored on the server (#3469) * generate a per code/token encryption key and send it to the client, store the encrypted value so that PII exposure of storing profile is dropped to zero * lint fix * fixed tests * debug * fixed missing stub * fixed token check * added --enable-source-maps * fixed flag * removed wrong flag * fixed tests * tweaked stubbing of randomBytes --- npm/src/controller/oauth.ts | 58 +++++++++++-- npm/test/sso/fixture.ts | 35 +++++--- npm/test/sso/saml_idp_oauth.test.ts | 127 ++++++++++++++++++---------- 3 files changed, 157 insertions(+), 63 deletions(-) diff --git a/npm/src/controller/oauth.ts b/npm/src/controller/oauth.ts index e3a14c6e5..d22c34593 100644 --- a/npm/src/controller/oauth.ts +++ b/npm/src/controller/oauth.ts @@ -48,9 +48,30 @@ import { SSOHandler } from './sso-handler'; import { ValidateOption, extractSAMLResponseAttributes } from '../saml/lib'; import { oidcClientConfig } from './oauth/oidc-client'; import { App } from '../ee/identity-federation/app'; +import * as encrypter from '../db/encrypter'; +import { Encrypted } from '../typings'; const deflateRawAsync = promisify(deflateRaw); +function encrypt(val: any) { + const genKey = crypto.randomBytes(32); + const hexKey = genKey.toString('hex'); + const encVal = encrypter.encrypt(JSON.stringify(val), genKey); + return { + hexKey, + encVal, + }; +} + +function decrypt(res: Encrypted, encryptionKey: string) { + const encKey = Buffer.from(encryptionKey, 'hex'); + if (res.iv && res.tag) { + return JSON.parse(encrypter.decrypt(res.value, res.iv, res.tag, encKey)); + } + + return JSON.parse(res.value); +} + export class OAuthController implements IOAuthController { private connectionStore: Storable; private sessionStore: Storable; @@ -1053,9 +1074,11 @@ export class OAuthController implements IOAuthController { codeVal['session'] = session; } - await this.codeStore.put(code, codeVal); + const { hexKey, encVal } = encrypt(codeVal); - return code; + await this.codeStore.put(code, encVal); + + return hexKey + '.' + code; } /** @@ -1149,7 +1172,18 @@ export class OAuthController implements IOAuthController { throw new JacksonError('Please specify code', 400); } - const codeVal = await this.codeStore.get(code); + const codes = code.split('.'); + if (codes.length !== 2) { + throw new JacksonError('Invalid code', 403); + } + + const encCodeVal = await this.codeStore.get(codes[1]); + if (!encCodeVal) { + throw new JacksonError('Invalid code', 403); + } + + const codeVal = decrypt(encCodeVal, codes[0]); + if (!codeVal || !codeVal.profile) { throw new JacksonError('Invalid code', 403); } @@ -1256,7 +1290,9 @@ export class OAuthController implements IOAuthController { tokenVal.claims.sub = codeVal.profile.claims.id; } - await this.tokenStore.put(token, tokenVal); + const { hexKey, encVal } = encrypt(tokenVal); + + await this.tokenStore.put(token, encVal); // delete the code try { @@ -1267,7 +1303,7 @@ export class OAuthController implements IOAuthController { } const tokenResponse: OAuthTokenRes = { - access_token: token, + access_token: hexKey + '.' + token, token_type: 'bearer', expires_in: this.opts.db.ttl!, }; @@ -1331,7 +1367,17 @@ export class OAuthController implements IOAuthController { * } */ public async userInfo(token: string): Promise { - const rsp = await this.tokenStore.get(token); + const tokens = token.split('.'); + if (tokens.length !== 2) { + throw new JacksonError('Invalid token', 403); + } + + const encRsp = await this.tokenStore.get(tokens[1]); + if (!encRsp) { + throw new JacksonError('Invalid token', 403); + } + + const rsp = decrypt(encRsp, tokens[0]); metrics.increment('oauthUserInfo'); diff --git a/npm/test/sso/fixture.ts b/npm/test/sso/fixture.ts index 6ad1be846..6de3d031e 100644 --- a/npm/test/sso/fixture.ts +++ b/npm/test/sso/fixture.ts @@ -1,3 +1,4 @@ +import crypto from 'crypto'; import { OAuthReqBody, OAuthReqBodyWithAccessType, @@ -11,6 +12,13 @@ import boxyhqNoentityID from './data/metadata/noentityID/boxyhq-noentityID'; import exampleOidc from './data/metadata/example.oidc'; import invalidssodescriptor from './data/metadata/invalidSSODescriptor/invalidssodescriptor'; +export const code = '1234567890'; +export const token = '24c1550190dd6a5a9bd6fe2a8ff69d593121c7b9'; +export const genKey = crypto.randomBytes(32); +export const iv = crypto.randomBytes(12); +export const clientCode = genKey.toString('hex') + '.' + code; +export const clientToken = genKey.toString('hex') + '.' + token; + // BEGIN: Fixtures for authorize export const authz_request_normal: Partial = { redirect_uri: boxyhq.defaultRedirectUrl, @@ -97,7 +105,7 @@ export const invalid_tenant_product = (product?, tenant?): Partial = { grant_type: 'authorization_code', @@ -139,21 +146,21 @@ export const bodyWithInvalidRedirectUri: Partial = { grant_type: 'authorization_code', client_id: `tenant=${boxyhq.tenant}&product=${boxyhq.product}`, client_secret: CLIENT_SECRET_VERIFIER, - code: CODE, + code: clientCode, redirect_uri: 'http://example.com', }; export const bodyWithMissingRedirectUri: Partial = { grant_type: 'authorization_code', client_id: `tenant=${boxyhq.tenant}&product=${boxyhq.product}`, client_secret: CLIENT_SECRET_VERIFIER, - code: CODE, + code: clientCode, }; //encoded clientId and wrong secret export const bodyWithInvalidClientSecret: Partial = { grant_type: 'authorization_code', client_id: `tenant=${boxyhq.tenant}&product=${boxyhq.product}`, client_secret: 'dummy', - code: CODE, + code: clientCode, redirect_uri: boxyhq.defaultRedirectUrl, }; //unencoded clientId with wrong secret @@ -165,7 +172,7 @@ export const bodyWithUnencodedClientId_InvalidClientSecret_gen = (connectionReco grant_type: 'authorization_code', client_id: connectionRecord.clientID, client_secret: 'dummy', - code: CODE, + code: clientCode, redirect_uri: boxyhq.defaultRedirectUrl, }; }; @@ -174,7 +181,7 @@ export const bodyWithDummyCredentials: Partial = { grant_type: 'authorization_code', client_id: `dummy`, client_secret: 'dummy', - code: CODE, + code: clientCode, redirect_uri: boxyhq.defaultRedirectUrl, }; @@ -182,7 +189,7 @@ export const token_req_encoded_client_id: Partial = { grant_type: 'authorization_code', client_id: `tenant=${boxyhq.tenant}&product=${boxyhq.product}`, client_secret: CLIENT_SECRET_VERIFIER, - code: CODE, + code: clientCode, redirect_uri: boxyhq.defaultRedirectUrl, }; @@ -194,42 +201,42 @@ export const token_req_unencoded_client_id_gen = (connectionRecords) => { grant_type: 'authorization_code', client_id: connectionRecord.clientID, client_secret: connectionRecord.clientSecret, - code: CODE, + code: clientCode, redirect_uri: boxyhq.defaultRedirectUrl, }; }; export const token_req_dummy_client_id_idp_saml_login_wrong_secretverifier = { grant_type: 'authorization_code', - code: CODE, + code: clientCode, client_id: 'dummy', client_secret: 'TOP-SECRET-WRONG', }; export const token_req_dummy_client_id_idp_saml_login = { grant_type: 'authorization_code', - code: CODE, + code: clientCode, client_id: 'dummy', client_secret: 'TOP-SECRET', }; export const token_req_encoded_client_id_idp_saml_login = { grant_type: 'authorization_code', - code: CODE, + code: clientCode, client_id: 'tenant=boxyhq.com&product=crm', client_secret: 'TOP-SECRET', }; export const token_req_encoded_client_id_idp_saml_login_wrong_secretverifier = { grant_type: 'authorization_code', - code: CODE, + code: clientCode, client_id: 'tenant=boxyhq.com&product=crm', client_secret: 'TOP-SECRET-WRONG', }; export const token_req = { grant_type: 'authorization_code', - code: CODE, + code: clientCode, redirect_uri: boxyhq.defaultRedirectUrl, }; diff --git a/npm/test/sso/saml_idp_oauth.test.ts b/npm/test/sso/saml_idp_oauth.test.ts index 14a9e1bbb..d07293cd7 100644 --- a/npm/test/sso/saml_idp_oauth.test.ts +++ b/npm/test/sso/saml_idp_oauth.test.ts @@ -41,6 +41,12 @@ import { token_req_encoded_client_id_idp_saml_login, token_req_dummy_client_id_idp_saml_login_wrong_secretverifier, token_req_encoded_client_id_idp_saml_login_wrong_secretverifier, + code, + token, + genKey, + iv, + clientCode, + clientToken, } from './fixture'; import { addSSOConnections, jacksonOptions } from '../utils'; import boxyhq from './data/metadata/boxyhq'; @@ -51,15 +57,68 @@ let idpEnabledConnectionAPIController: IConnectionAPIController; //idp initiated let idpEnabledOAuthController: IOAuthController; let keyPair: jose.GenerateKeyPairResult; -const code = '1234567890'; -const token = '24c1550190dd6a5a9bd6fe2a8ff69d593121c7b9'; - const metadataPath = path.join(__dirname, '/data/metadata'); let connections: Array = []; let code_verifier: string; let code_challenge: string; +function _stubRandomBytes(codeOrToken: string) { + return ( + sinon + .stub(crypto, 'randomBytes') + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(codeOrToken) + .onSecondCall() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(genKey) + .onThirdCall() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(iv) + ); +} + +function stubRandomBytesCode() { + return _stubRandomBytes(code); +} + +function stubRandomBytesToken() { + return _stubRandomBytes(token); +} + +function stubRandomBytesAll() { + return ( + sinon + .stub(crypto, 'randomBytes') + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(code) + .onSecondCall() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(genKey) + .onThirdCall() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(iv) + .onCall(3) + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(token) + .onCall(4) + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(genKey) + .onCall(5) + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .returns(iv) + ); +} + tap.before(async () => { const client = await import('openid-client'); code_verifier = client.randomPKCECodeVerifier(); @@ -308,16 +367,17 @@ tap.test('samlResponse()', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code); + const stubRandomBytes = stubRandomBytesCode(); const response = await oauthController.samlResponse(responseBody); const params = new URLSearchParams(new URL(response.redirect_url!).search); t.ok(stubValidate.calledOnce, 'validate called once'); - t.ok(stubRandomBytes.calledOnce, 'randomBytes called once'); + t.ok( + stubRandomBytes.calledThrice, + 'samlResponse randomBytes called thrice: ' + stubRandomBytes.callCount + ); t.ok('redirect_url' in response, 'response contains redirect_url'); t.ok(params.has('code'), 'query string includes code'); t.ok(params.has('state'), 'query string includes state'); @@ -472,21 +532,16 @@ tap.test('token()', async (t) => { async (t) => { t.test('encoded client_id', async (t) => { const body = token_req_encoded_client_id; - const stubRandomBytes = sinon - .stub(crypto, 'randomBytes') - .onFirstCall() - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - .returns(token); + const stubRandomBytes = stubRandomBytesToken(); const response = await oauthController.token(body); - t.ok(stubRandomBytes.calledOnce, 'randomBytes called once'); + t.ok(stubRandomBytes.calledThrice, 'token randomBytes called thrice: ' + stubRandomBytes.callCount); t.ok('access_token' in response, 'includes access_token'); t.ok('token_type' in response, 'includes token_type'); t.ok('expires_in' in response, 'includes expires_in'); t.notOk('id_token' in response, 'does not include id_token'); - t.match(response.access_token, token); + t.match(response.access_token, clientToken); t.match(response.token_type, 'bearer'); t.match(response.expires_in, 300); @@ -518,9 +573,7 @@ tap.test('token()', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); await oauthController.samlResponse(responseBody); @@ -532,7 +585,7 @@ tap.test('token()', async (t) => { t.ok('token_type' in tokenRes, 'includes token_type'); t.ok('expires_in' in tokenRes, 'includes expires_in'); t.notOk('id_token' in tokenRes, 'does not include id_token'); - t.match(tokenRes.access_token, token); + t.match(tokenRes.access_token, clientToken); t.match(tokenRes.token_type, 'bearer'); t.match(tokenRes.expires_in, 300); @@ -575,9 +628,7 @@ tap.test('token()', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); await oauthController.samlResponse(responseBody); @@ -644,9 +695,7 @@ tap.test('token()', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); await oauthController.samlResponse(responseBody); @@ -703,12 +752,10 @@ tap.test('IdP initiated flow', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); const { redirect_url } = await idpEnabledOAuthController.samlResponse(responseBody); - t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), code); + t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), clientCode); const body = token_req_encoded_client_id_idp_saml_login_wrong_secretverifier; @@ -736,12 +783,10 @@ tap.test('IdP initiated flow', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); const { redirect_url } = await idpEnabledOAuthController.samlResponse(responseBody); - t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), code); + t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), clientCode); const body = token_req_dummy_client_id_idp_saml_login_wrong_secretverifier; @@ -769,12 +814,10 @@ tap.test('IdP initiated flow', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); const { redirect_url } = await idpEnabledOAuthController.samlResponse(responseBody); - t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), code); + t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), clientCode); const body = token_req_encoded_client_id_idp_saml_login; @@ -782,7 +825,7 @@ tap.test('IdP initiated flow', async (t) => { t.ok('access_token' in tokenRes, 'includes access_token'); t.ok('token_type' in tokenRes, 'includes token_type'); t.ok('expires_in' in tokenRes, 'includes expires_in'); - t.equal(tokenRes.access_token, token); + t.equal(tokenRes.access_token, clientToken); t.equal(tokenRes.token_type, 'bearer'); t.equal(tokenRes.expires_in, 300); const profile = await idpEnabledOAuthController.userInfo(tokenRes.access_token); @@ -807,12 +850,10 @@ tap.test('IdP initiated flow', async (t) => { sessionIndex: '', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const stubRandomBytes = sinon.stub(crypto, 'randomBytes').returns(code).onSecondCall().returns(token); + const stubRandomBytes = stubRandomBytesAll(); const { redirect_url } = await idpEnabledOAuthController.samlResponse(responseBody); - t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), code); + t.equal(new URLSearchParams(new URL(redirect_url!).search).get('code'), clientCode); const body = token_req_dummy_client_id_idp_saml_login; @@ -820,7 +861,7 @@ tap.test('IdP initiated flow', async (t) => { t.ok('access_token' in tokenRes, 'includes access_token'); t.ok('token_type' in tokenRes, 'includes token_type'); t.ok('expires_in' in tokenRes, 'includes expires_in'); - t.equal(tokenRes.access_token, token); + t.equal(tokenRes.access_token, clientToken); t.equal(tokenRes.token_type, 'bearer'); t.equal(tokenRes.expires_in, 300); const profile = await idpEnabledOAuthController.userInfo(tokenRes.access_token);