-
Notifications
You must be signed in to change notification settings - Fork 154
[PM-40314] fix: Enforce Master Password policy on SSO/TDE vault unlock #2980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| { | ||
| "access_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTY5MDg4NzksInN1YiI6IjEzNTEyNDY3LTljZmUtNDNiMC05NjlmLTA3NTM0MDg0NzY0YiIsIm5hbWUiOiJCaXR3YXJkZW4gVXNlciIsImVtYWlsIjoidXNlckBiaXR3YXJkZW4uY29tIiwiZW1haWxfdmVyaWZpZWQiOnRydWUsImlhdCI6MTUxNjIzOTAyMiwicHJlbWl1bSI6ZmFsc2UsImFtciI6WyJBcHBsaWNhdGlvbiJdfQ.KDqC8kUaOAgBiUY8eeLa0a4xYWN8GmheXTFXmataFwM", | ||
| "expires_in": 3600, | ||
| "token_type": "Bearer", | ||
| "refresh_token": "REFRESH_TOKEN", | ||
| "scope": "api offline_access", | ||
| "MasterPasswordPolicy": { | ||
| "MinComplexity": 1, | ||
| "MinLength": 22, | ||
| "RequireLower": true, | ||
| "RequireUpper": true, | ||
| "RequireNumbers": false, | ||
| "RequireSpecial": false, | ||
| "EnforceOnLogin": true, | ||
| "Object": "masterPasswordPolicy" | ||
| }, | ||
| "ForcePasswordReset": false, | ||
| "Kdf": 0, | ||
| "KdfIterations": 600000, | ||
| "KdfMemory": null, | ||
| "KdfParallelism": null, | ||
| "UserDecryptionOptions": { | ||
| "HasMasterPassword": true, | ||
| "TrustedDeviceOption": { | ||
| "EncryptedPrivateKey": "private-key", | ||
| "EncryptedUserKey": "user-key", | ||
| "HasAdminApproval": false, | ||
| "HasLoginApprovingDevice": true, | ||
| "HasManageResetPasswordPermission": false | ||
| }, | ||
| "Object": "userDecryptionOptions" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,18 @@ protocol AuthService { | |
| /// | ||
| func answerLoginRequest(_ request: LoginRequest, approve: Bool) async throws | ||
|
|
||
| /// Checks the supplied master password against any active organization Master Password | ||
| /// policy after the vault has been unlocked with it, setting `forcePasswordResetReason` if | ||
| /// the password does not satisfy the policy. Uses the policy captured during the most recent | ||
| /// SSO/TDE login for this email if one is cached, otherwise falls back to whatever policy | ||
| /// data is already available via `PolicyService`. | ||
| /// | ||
| /// - Parameters: | ||
| /// - email: The email of the account that was unlocked. | ||
| /// - masterPassword: The master password used to unlock the vault. | ||
| /// | ||
| func checkMasterPasswordPolicyAfterUnlock(email: String, masterPassword: String) async throws | ||
|
|
||
| /// Check the status of the pending login request for the unauthenticated user. | ||
| /// | ||
| func checkPendingLoginRequest(withId id: String) async throws -> LoginRequest | ||
|
|
@@ -355,6 +367,12 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng | |
| /// the callback the `ASWebAuthenticationSession` is configured to match. | ||
| private var singleSignOnCallbackUrl: String { callbackUrl(for: .singleSignOn) } | ||
|
|
||
| /// The master password policy captured from the identity token response during an SSO/TDE | ||
| /// login, cached (keyed by email) until the user unlocks the vault with their master | ||
| /// password. Only consumed when the email matches the account being unlocked, to avoid a | ||
| /// stale policy from one account leaking into a later unlock for a different account. | ||
| private var ssoMasterPasswordPolicy: (email: String, policy: MasterPasswordPolicyResponseModel?)? | ||
|
|
||
|
Comment on lines
+370
to
+375
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π€ I think there's an edge case here where it would fail where you have a user with two SSO accounts on different servers but using the same |
||
| /// The service used by the application to manage account state. | ||
| private let stateService: StateService | ||
|
|
||
|
|
@@ -456,6 +474,19 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng | |
| _ = try await authAPIService.answerLoginRequest(loginRequest.id, requestModel: requestModel) | ||
| } | ||
|
|
||
| func checkMasterPasswordPolicyAfterUnlock(email: String, masterPassword: String) async throws { | ||
| let cached = ssoMasterPasswordPolicy | ||
| ssoMasterPasswordPolicy = nil | ||
| let policy = cached?.email == email ? cached?.policy : nil | ||
|
|
||
| try await checkMasterPasswordPolicies( | ||
| isPreAuth: false, | ||
| masterPassword: masterPassword, | ||
| masterPasswordPolicy: policy, | ||
| username: email, | ||
| ) | ||
| } | ||
|
|
||
| func checkPendingLoginRequest(withId id: String) async throws -> LoginRequest { | ||
| guard let loginWithDeviceData else { throw AuthError.missingLoginWithDeviceData } | ||
|
|
||
|
|
@@ -756,6 +787,12 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng | |
| email: email, | ||
| ) | ||
|
|
||
| // Cache the master password policy so it can be checked once the user unlocks the vault | ||
| // with their master password (covers both the direct master password unlock method and | ||
| // the TDE "approve with master password" flow, which is reached via the | ||
| // `AuthError.requireDecryptionOptions` throw below). | ||
| ssoMasterPasswordPolicy = (email, response.masterPasswordPolicy) | ||
|
|
||
| return try await unlockMethod(for: response) | ||
| } | ||
|
|
||
|
|
@@ -779,6 +816,17 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng | |
| // Get the identity token to log in to Bitwarden. | ||
| let response = try await getIdentityTokenResponse(email: email, request: twoFactorRequest) | ||
|
|
||
| // Cache the master password policy so it can be checked once the user unlocks the vault | ||
| // with their master password. Only applies to SSO/TDE logins that also required two-factor | ||
| // authentication, where the policy from the first (pre-2FA) identity token response was | ||
| // never captured because that attempt threw `twoFactorRequired` before reaching the | ||
| // caching in `loginWithSingleSignOn`. Plain password logins already have their policy | ||
| // checked above via `preAuthForcePasswordResetReason`, so caching it again here would | ||
| // just cause it to be redundantly re-checked once the vault is unlocked. | ||
| if case .authorizationCode = twoFactorRequest.authenticationMethod { | ||
| ssoMasterPasswordPolicy = (email, response.masterPasswordPolicy) | ||
| } | ||
|
|
||
| // If it's assigned then we need to update the required reset password and remove the cache. | ||
| if preAuthForcePasswordResetReason != nil { | ||
| try await stateService.setForcePasswordResetReason(.weakMasterPasswordOnLogin) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
β QUESTION: Running this on every master-password unlock means the Update Master Password screen can now be triggered from inside an app extension.
Trace and consideration
VaultUnlockProcessor.unlockVault()callsunlockVaultWithPasswordand thencoordinator.handleEvent(.didCompleteAuth), which routes throughAuthRouter.completeAuthRedirect():The
forcePasswordResetReasoncheck sits above theisInAppExtensionguard, so this path applies in the AutoFill/Share/Action extensions too. An SSO/TDE user whose master password violates the policy β precisely the population this PR targets β may well perform their first master-password unlock inside the AutoFill extension. They would then be dropped into Update Master Password mid-autofill (which triggerssettingsRepository.fetchSync()and, on success,logout) instead of getting their credential.This was reachable before only via a reason already persisted from a main-app login; this change makes the extension itself a place where the reason first gets set. Was the extension context considered? If enforcement should be main-app-only, gating the new call (or the router's redirect) on
isInAppExtensionwould keep autofill unaffected β the reason would still be set on the next main-app unlock. NoteAppProcessor.removeMasterPasswordalready applies exactly this kind of guard.The PR description discusses the vault-timeout re-lock flow but not extensions, so flagging in case it was an unconsidered side effect rather than an intentional trade-off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps something to raise to Product.