From e215828e8550d5d81bbd4de12e805c99f48bf7bc Mon Sep 17 00:00:00 2001 From: Todd Martin <106564991+trmartin4@users.noreply.github.com> Date: Thu, 24 Aug 2023 21:30:52 -0400 Subject: [PATCH] [PM-3533] Support onboarding Key Connector users with existing master passwords (#6082) * Added checks for new KeyConnector URL in all references to the legacy one. * Updated KeyConnector logoutCallback to be a Promise * Removed extra dependencies from KeyConnectorService * Made the logout callback async. * Adjusted logic to handle having a master password. * Updated not to return error if master key is not found. * Undid change to callback to reduce scope of this change. * Cleaned up functions. * Updated tests. * Updated comments. * Updated comments. * Updated to use getKeyConnectorUrl helper. --- .../src/services/jslib-services.module.ts | 2 - .../sso-login.strategy.spec.ts | 8 +-- .../login-strategies/sso-login.strategy.ts | 57 +++++++++++++++---- .../auth/services/key-connector.service.ts | 14 ++++- 4 files changed, 61 insertions(+), 20 deletions(-) diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index dcc8b5e3bc..14b26ca43d 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -572,8 +572,6 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; LogService, OrganizationServiceAbstraction, CryptoFunctionServiceAbstraction, - SyncNotifierServiceAbstraction, - MessagingServiceAbstraction, LOGOUT_CALLBACK, ], }, diff --git a/libs/common/src/auth/login-strategies/sso-login.strategy.spec.ts b/libs/common/src/auth/login-strategies/sso-login.strategy.spec.ts index 78d0a491c3..099a3a02a2 100644 --- a/libs/common/src/auth/login-strategies/sso-login.strategy.spec.ts +++ b/libs/common/src/auth/login-strategies/sso-login.strategy.spec.ts @@ -266,11 +266,11 @@ describe("SsoLogInStrategy", () => { describe("Key Connector", () => { let tokenResponse: IdentityTokenResponse; beforeEach(() => { - tokenResponse = identityTokenResponseFactory(); + tokenResponse = identityTokenResponseFactory(null, { HasMasterPassword: false }); tokenResponse.keyConnectorUrl = keyConnectorUrl; }); - it("gets and sets the master key if Key Connector is enabled", async () => { + it("gets and sets the master key if Key Connector is enabled and the user doesn't have a master password", async () => { const masterKey = new SymmetricCryptoKey( new Uint8Array(64).buffer as CsprngArray ) as MasterKey; @@ -283,7 +283,7 @@ describe("SsoLogInStrategy", () => { expect(keyConnectorService.setMasterKeyFromUrl).toHaveBeenCalledWith(keyConnectorUrl); }); - it("converts new SSO user to Key Connector on first login", async () => { + it("converts new SSO user with no master password to Key Connector on first login", async () => { tokenResponse.key = null; apiService.postIdentityToken.mockResolvedValue(tokenResponse); @@ -296,7 +296,7 @@ describe("SsoLogInStrategy", () => { ); }); - it("decrypts and sets the user key if Key Connector is enabled", async () => { + it("decrypts and sets the user key if Key Connector is enabled and the user doesn't have a master password", async () => { const userKey = new SymmetricCryptoKey(new Uint8Array(64).buffer as CsprngArray) as UserKey; const masterKey = new SymmetricCryptoKey( new Uint8Array(64).buffer as CsprngArray diff --git a/libs/common/src/auth/login-strategies/sso-login.strategy.ts b/libs/common/src/auth/login-strategies/sso-login.strategy.ts index 3bfa5ce01f..3e9a7e33f3 100644 --- a/libs/common/src/auth/login-strategies/sso-login.strategy.ts +++ b/libs/common/src/auth/login-strategies/sso-login.strategy.ts @@ -77,19 +77,50 @@ export class SsoLogInStrategy extends LogInStrategy { } protected override async setMasterKey(tokenResponse: IdentityTokenResponse) { - // TODO: discuss how this is no longer true with TDE - // eventually we’ll need to support migration of existing TDE users to Key Connector - const newSsoUser = tokenResponse.key == null; - - if (tokenResponse.keyConnectorUrl != null) { - if (!newSsoUser) { - await this.keyConnectorService.setMasterKeyFromUrl(tokenResponse.keyConnectorUrl); - } else { + // The only way we can be setting a master key at this point is if we are using Key Connector. + // First, check to make sure that we should do so based on the token response. + if (this.shouldSetMasterKeyFromKeyConnector(tokenResponse)) { + // If we're here, we know that the user should use Key Connector (they have a KeyConnectorUrl) and does not have a master password. + // We can now check the key on the token response to see whether they are a brand new user or an existing user. + // The presence of a masterKeyEncryptedUserKey indicates that the user has already been provisioned in Key Connector. + const newSsoUser = tokenResponse.key == null; + if (newSsoUser) { await this.keyConnectorService.convertNewSsoUserToKeyConnector(tokenResponse, this.orgId); + } else { + const keyConnectorUrl = this.getKeyConnectorUrl(tokenResponse); + await this.keyConnectorService.setMasterKeyFromUrl(keyConnectorUrl); } } } + /** + * Determines if it is possible set the `masterKey` from Key Connector. + * @param tokenResponse + * @returns `true` if the master key can be set from Key Connector, `false` otherwise + */ + private shouldSetMasterKeyFromKeyConnector(tokenResponse: IdentityTokenResponse): boolean { + const userDecryptionOptions = tokenResponse?.userDecryptionOptions; + + // If the user has a master password, this means that they need to migrate to Key Connector, so we won't set the key here. + // We default to false here because old server versions won't have hasMasterPassword and in that case we want to rely solely on the keyConnectorUrl. + // TODO: remove null default after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3537) + const userHasMasterPassword = userDecryptionOptions?.hasMasterPassword ?? false; + + const keyConnectorUrl = this.getKeyConnectorUrl(tokenResponse); + + // In order for us to set the master key from Key Connector, we need to have a Key Connector URL + // and the user must not have a master password. + return keyConnectorUrl != null && !userHasMasterPassword; + } + + private getKeyConnectorUrl(tokenResponse: IdentityTokenResponse): string { + // TODO: remove tokenResponse.keyConnectorUrl reference after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3537) + const userDecryptionOptions = tokenResponse?.userDecryptionOptions; + return ( + tokenResponse.keyConnectorUrl ?? userDecryptionOptions?.keyConnectorOption?.keyConnectorUrl + ); + } + // TODO: future passkey login strategy will need to support setting user key (decrypting via TDE or admin approval request) // so might be worth moving this logic to a common place (base login strategy or a separate service?) protected override async setUserKey(tokenResponse: IdentityTokenResponse): Promise { @@ -117,9 +148,8 @@ export class SsoLogInStrategy extends LogInStrategy { await this.trySetUserKeyWithDeviceKey(tokenResponse); } } else if ( - // TODO: remove tokenResponse.keyConnectorUrl when it's deprecated masterKeyEncryptedUserKey != null && - (tokenResponse.keyConnectorUrl || userDecryptionOptions?.keyConnectorOption?.keyConnectorUrl) + this.getKeyConnectorUrl(tokenResponse) != null ) { // Key connector enabled for user await this.trySetUserKeyWithMasterKey(); @@ -208,12 +238,15 @@ export class SsoLogInStrategy extends LogInStrategy { private async trySetUserKeyWithMasterKey(): Promise { const masterKey = await this.cryptoService.getMasterKey(); + // There is a scenario in which the master key is not set here. That will occur if the user + // has a master password and is using Key Connector. In that case, we cannot set the master key + // because the user hasn't entered their master password yet. + // Instead, we'll return here and let the migration to Key Connector handle setting the master key. if (!masterKey) { - throw new Error("Master key not found"); + return; } const userKey = await this.cryptoService.decryptUserKeyWithMasterKey(masterKey); - await this.cryptoService.setUserKey(userKey); } diff --git a/libs/common/src/auth/services/key-connector.service.ts b/libs/common/src/auth/services/key-connector.service.ts index 24bf76f76b..7aa41a9857 100644 --- a/libs/common/src/auth/services/key-connector.service.ts +++ b/libs/common/src/auth/services/key-connector.service.ts @@ -24,7 +24,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private logService: LogService, private organizationService: OrganizationService, private cryptoFunctionService: CryptoFunctionService, - private logoutCallback: (expired: boolean, userId?: string) => void + private logoutCallback: (expired: boolean, userId?: string) => Promise ) {} setUsesKeyConnector(usesKeyConnector: boolean) { @@ -84,7 +84,15 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { } async convertNewSsoUserToKeyConnector(tokenResponse: IdentityTokenResponse, orgId: string) { - const { kdf, kdfIterations, kdfMemory, kdfParallelism, keyConnectorUrl } = tokenResponse; + // TODO: Remove after tokenResponse.keyConnectorUrl is deprecated in 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3537) + const { + kdf, + kdfIterations, + kdfMemory, + kdfParallelism, + keyConnectorUrl: legacyKeyConnectorUrl, + userDecryptionOptions, + } = tokenResponse; const password = await this.cryptoFunctionService.randomBytes(64); const kdfConfig = new KdfConfig(kdfIterations, kdfMemory, kdfParallelism); @@ -104,6 +112,8 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { const [pubKey, privKey] = await this.cryptoService.makeKeyPair(); try { + const keyConnectorUrl = + legacyKeyConnectorUrl ?? userDecryptionOptions?.keyConnectorOption?.keyConnectorUrl; await this.apiService.postUserKeyToKeyConnector(keyConnectorUrl, keyConnectorRequest); } catch (e) { this.handleKeyConnectorError(e);