Skip to content
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

[PM-16603] Implement userkey rotation v2 #12646

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ <h1>{{ "changeMasterPassword" | i18n }}</h1>
[(ngModel)]="masterPasswordHint"
/>
</div>
<button type="submit" buttonType="primary" bitButton [loading]="form.loading">
<button type="submit" buttonType="primary" bitButton [loading]="loading">
{{ "changeMasterPassword" | i18n }}
</button>
</form>
Expand Down
137 changes: 135 additions & 2 deletions apps/web/src/app/auth/settings/change-password.component.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Code changes here affect both the old process and the v2 process.

Ideally, when we're writing things behind a feature flag, we don't want to be making significant changes to pre-existing logic/code.

๐Ÿ’ญ What about branching off the feature flag at a higher level?

  async submit() {
    if (
      this.masterPasswordHint != null &&
      this.masterPasswordHint.toLowerCase() === this.masterPassword.toLowerCase()
    ) {
      this.toastService.showToast({
        variant: "error",
        title: this.i18nService.t("errorOccurred"),
        message: this.i18nService.t("hintEqualsPassword"),
      });
      return;
    }

    this.leakedPassword = false;
    if (this.checkForBreaches) {
      this.leakedPassword = (await this.auditService.passwordLeaked(this.masterPassword)) > 0;
    }

    if (this.userkeyRotationV2) {
      await this.submitV2();
    } else {
      await super.submit();
    }
  }

  async submitV2() {
    //Process things how we want it to work in the future.
  }

This would also make it a good time to extract what we think should be in its on service and use the new service only in v2.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
import { PasswordRequest } from "@bitwarden/common/auth/models/request/password.request";
import { getUserId } from "@bitwarden/common/auth/services/account.service";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
Expand All @@ -35,11 +37,13 @@
extends BaseChangePasswordComponent
implements OnInit, OnDestroy
{
loading = false;

Check warning on line 40 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L40

Added line #L40 was not covered by tests
rotateUserKey = false;
currentMasterPassword: string;
masterPasswordHint: string;
checkForBreaches = true;
characterMinimumMessage = "";
userkeyRotationV2 = false;

Check warning on line 46 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L46

Added line #L46 was not covered by tests

constructor(
i18nService: I18nService,
Expand All @@ -56,9 +60,10 @@
private userVerificationService: UserVerificationService,
private keyRotationService: UserKeyRotationService,
kdfConfigService: KdfConfigService,
masterPasswordService: InternalMasterPasswordServiceAbstraction,
protected masterPasswordService: InternalMasterPasswordServiceAbstraction,
accountService: AccountService,
toastService: ToastService,
private configService: ConfigService,

Check warning on line 66 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L66

Added line #L66 was not covered by tests
) {
super(
i18nService,
Expand All @@ -75,6 +80,8 @@
}

async ngOnInit() {
this.userkeyRotationV2 = await this.configService.getFeatureFlag(FeatureFlag.UserKeyRotationV2);

Check warning on line 83 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L83

Added line #L83 was not covered by tests

if (!(await this.userVerificationService.hasMasterPassword())) {
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
Expand Down Expand Up @@ -135,6 +142,132 @@
}

async submit() {
if (this.userkeyRotationV2) {
await this.submitNew();

Check warning on line 146 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L146

Added line #L146 was not covered by tests
} else {
await this.submitOld();

Check warning on line 148 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L148

Added line #L148 was not covered by tests
}
}

async submitNew() {
if (this.currentMasterPassword == null || this.currentMasterPassword === "") {
this.toastService.showToast({

Check warning on line 154 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L154

Added line #L154 was not covered by tests
variant: "error",
title: this.i18nService.t("errorOccurred"),
message: this.i18nService.t("masterPasswordRequired"),
});
return false;

Check warning on line 159 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L159

Added line #L159 was not covered by tests
}

this.loading = true;

Check warning on line 162 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L162

Added line #L162 was not covered by tests
if (
this.masterPasswordHint != null &&
this.masterPasswordHint.toLowerCase() === this.masterPassword.toLowerCase()
) {
this.toastService.showToast({

Check warning on line 167 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L167

Added line #L167 was not covered by tests
variant: "error",
title: this.i18nService.t("errorOccurred"),
message: this.i18nService.t("hintEqualsPassword"),
});
this.loading = false;
return;

Check warning on line 173 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L172-L173

Added lines #L172 - L173 were not covered by tests
}

this.leakedPassword = false;

Check warning on line 176 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L176

Added line #L176 was not covered by tests
if (this.checkForBreaches) {
this.leakedPassword = (await this.auditService.passwordLeaked(this.masterPassword)) > 0;

Check warning on line 178 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L178

Added line #L178 was not covered by tests
}

if (!(await this.strongPassword())) {
this.loading = false;
return;

Check warning on line 183 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L182-L183

Added lines #L182 - L183 were not covered by tests
}

try {

Check warning on line 186 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L186

Added line #L186 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing where this check that was getting ran in the old flow setupSubmitActions is?

   if (this.currentMasterPassword == null || this.currentMasterPassword === "") {
      this.toastService.showToast({
        variant: "error",
        title: this.i18nService.t("errorOccurred"),
        message: this.i18nService.t("masterPasswordRequired"),
      });
      return false;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I split the two submit flows now, I added this on top of the new one.

if (this.rotateUserKey) {
await this.syncService.fullSync(true);
const user = await firstValueFrom(this.accountService.activeAccount$);
await this.keyRotationService.rotateUserKeyMasterPasswordAndEncryptedData(

Check warning on line 190 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L188-L190

Added lines #L188 - L190 were not covered by tests
this.currentMasterPassword,
this.masterPassword,
user,
this.masterPasswordHint,
);
} else {
await this.updatePassword(this.masterPassword);

Check warning on line 197 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L197

Added line #L197 was not covered by tests
}
} catch (e) {
this.toastService.showToast({

Check warning on line 200 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L200

Added line #L200 was not covered by tests
variant: "error",
title: this.i18nService.t("errorOccurred"),
message: e.message,
});
}
this.loading = false;

Check warning on line 206 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L206

Added line #L206 was not covered by tests
}

// todo: move this to a service
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved
// https://bitwarden.atlassian.net/browse/PM-17108
private async updatePassword(newMasterPassword: string) {
const currentMasterPassword = this.currentMasterPassword;
const { userId, email } = await firstValueFrom(

Check warning on line 213 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L212-L213

Added lines #L212 - L213 were not covered by tests
this.accountService.activeAccount$.pipe(map((a) => ({ userId: a?.id, email: a?.email }))),
);
const kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(userId));

Check warning on line 216 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L216

Added line #L216 was not covered by tests

const currentMasterKey = await this.keyService.makeMasterKey(

Check warning on line 218 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L218

Added line #L218 was not covered by tests
currentMasterPassword,
email,
kdfConfig,
);
const decryptedUserKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(

Check warning on line 223 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L223

Added line #L223 was not covered by tests
currentMasterKey,
userId,
);
if (decryptedUserKey == null) {
this.toastService.showToast({

Check warning on line 228 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L228

Added line #L228 was not covered by tests
variant: "error",
title: null,
message: this.i18nService.t("invalidMasterPassword"),
});
return;

Check warning on line 233 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L233

Added line #L233 was not covered by tests
}

const newMasterKey = await this.keyService.makeMasterKey(newMasterPassword, email, kdfConfig);
const newMasterKeyEncryptedUserKey = await this.keyService.encryptUserKeyWithMasterKey(

Check warning on line 237 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L236-L237

Added lines #L236 - L237 were not covered by tests
newMasterKey,
decryptedUserKey,
);

const request = new PasswordRequest();
request.masterPasswordHash = await this.keyService.hashMasterKey(

Check warning on line 243 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L242-L243

Added lines #L242 - L243 were not covered by tests
this.currentMasterPassword,
currentMasterKey,
);
request.masterPasswordHint = this.masterPasswordHint;
request.newMasterPasswordHash = await this.keyService.hashMasterKey(

Check warning on line 248 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L247-L248

Added lines #L247 - L248 were not covered by tests
newMasterPassword,
newMasterKey,
);
request.key = newMasterKeyEncryptedUserKey[1].encryptedString;
try {
await this.apiService.postPassword(request);
this.toastService.showToast({

Check warning on line 255 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L252-L255

Added lines #L252 - L255 were not covered by tests
variant: "success",
title: this.i18nService.t("masterPasswordChanged"),
message: this.i18nService.t("masterPasswordChangedDesc"),
});
this.messagingService.send("logout");

Check warning on line 260 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L260

Added line #L260 was not covered by tests
} catch {
this.toastService.showToast({

Check warning on line 262 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L262

Added line #L262 was not covered by tests
variant: "error",
title: null,
message: this.i18nService.t("errorOccurred"),
});
}
}

async submitOld() {
if (
this.masterPasswordHint != null &&
this.masterPasswordHint.toLowerCase() === this.masterPassword.toLowerCase()
Expand Down Expand Up @@ -240,6 +373,6 @@

private async updateKey() {
const user = await firstValueFrom(this.accountService.activeAccount$);
await this.keyRotationService.rotateUserKeyAndEncryptedData(this.masterPassword, user);
await this.keyRotationService.rotateUserKeyAndEncryptedDataLegacy(this.masterPassword, user);

Check warning on line 376 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L376

Added line #L376 was not covered by tests
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export class AccountKeysRequest {
// Other keys encrypted by the userkey
userKeyEncryptedAccountPrivateKey: string;
accountPublicKey: string;

constructor(userKeyEncryptedAccountPrivateKey: string, accountPublicKey: string) {
this.userKeyEncryptedAccountPrivateKey = userKeyEncryptedAccountPrivateKey;
this.accountPublicKey = accountPublicKey;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { Argon2KdfConfig, KdfConfig, KdfType } from "@bitwarden/key-management";

export class MasterPasswordUnlockDataRequest {
kdfType: KdfType = KdfType.PBKDF2_SHA256;
kdfIterations: number = 0;
kdfMemory?: number;
kdfParallelism?: number;

email: string;
masterKeyAuthenticationHash: string;

masterKeyEncryptedUserKey: string;

masterPasswordHint?: string;

constructor(
kdfConfig: KdfConfig,
email: string,
masterKeyAuthenticationHash: string,
masterKeyEncryptedUserKey: string,
masterPasswordHash?: string,
) {
this.kdfType = kdfConfig.kdfType;
this.kdfIterations = kdfConfig.iterations;
if (kdfConfig.kdfType === KdfType.Argon2id) {
this.kdfMemory = (kdfConfig as Argon2KdfConfig).memory;
this.kdfParallelism = (kdfConfig as Argon2KdfConfig).parallelism;

Check warning on line 27 in apps/web/src/app/key-management/key-rotation/request/master-password-unlock-data.request.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/key-management/key-rotation/request/master-password-unlock-data.request.ts#L26-L27

Added lines #L26 - L27 were not covered by tests
}

this.email = email;
this.masterKeyAuthenticationHash = masterKeyAuthenticationHash;
this.masterKeyEncryptedUserKey = masterKeyEncryptedUserKey;
this.masterPasswordHint = masterPasswordHash;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { AccountKeysRequest } from "./account-keys.request";
import { UnlockDataRequest } from "./unlock-data.request";
import { UserDataRequest as AccountDataRequest } from "./userdata.request";

export class RotateUserAccountKeysRequest {
constructor(
accountUnlockData: UnlockDataRequest,
accountKeys: AccountKeysRequest,
accountData: AccountDataRequest,
oldMasterKeyAuthenticationHash: string,
) {
this.accountUnlockData = accountUnlockData;
this.accountKeys = accountKeys;
this.accountData = accountData;
this.oldMasterKeyAuthenticationHash = oldMasterKeyAuthenticationHash;
}

// Authentication for the request
oldMasterKeyAuthenticationHash: string;

// All methods to get to the userkey
accountUnlockData: UnlockDataRequest;

// Other keys encrypted by the userkey
accountKeys: AccountKeysRequest;

// User vault data encrypted by the userkey
accountData: AccountDataRequest;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { OrganizationUserResetPasswordWithIdRequest } from "@bitwarden/admin-console/common";
import { WebauthnRotateCredentialRequest } from "@bitwarden/common/auth/models/request/webauthn-rotate-credential.request";

import { EmergencyAccessWithIdRequest } from "../../../auth/emergency-access/request/emergency-access-update.request";

import { MasterPasswordUnlockDataRequest } from "./master-password-unlock-data.request";

export class UnlockDataRequest {
// All methods to get to the userkey
masterPasswordUnlockData: MasterPasswordUnlockDataRequest;
emergencyAccessUnlockData: EmergencyAccessWithIdRequest[];
organizationAccountRecoveryUnlockData: OrganizationUserResetPasswordWithIdRequest[];
passkeyUnlockData: WebauthnRotateCredentialRequest[];

constructor(
masterPasswordUnlockData: MasterPasswordUnlockDataRequest,
emergencyAccessUnlockData: EmergencyAccessWithIdRequest[],
organizationAccountRecoveryUnlockData: OrganizationUserResetPasswordWithIdRequest[],
passkeyUnlockData: WebauthnRotateCredentialRequest[],
) {
this.masterPasswordUnlockData = masterPasswordUnlockData;
this.emergencyAccessUnlockData = emergencyAccessUnlockData;
this.organizationAccountRecoveryUnlockData = organizationAccountRecoveryUnlockData;
this.passkeyUnlockData = passkeyUnlockData;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { SendWithIdRequest } from "@bitwarden/common/tools/send/models/request/send-with-id.request";
import { CipherWithIdRequest } from "@bitwarden/common/vault/models/request/cipher-with-id.request";
import { FolderWithIdRequest } from "@bitwarden/common/vault/models/request/folder-with-id.request";

export class UserDataRequest {
ciphers: CipherWithIdRequest[];
folders: FolderWithIdRequest[];
sends: SendWithIdRequest[];

constructor(
ciphers: CipherWithIdRequest[],
folders: FolderWithIdRequest[],
sends: SendWithIdRequest[],
) {
this.ciphers = ciphers;
this.folders = folders;
this.sends = sends;
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since our API design is expecting empty arrays should we add in some handling for null and undefined here to make sure we only pass empty arrays or the appropriate array of data objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking in the rotateUserKeyMasterPasswordAndEncryptedData now, and throwing if any of them is null.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import { ApiService } from "@bitwarden/common/abstractions/api.service";

import { RotateUserAccountKeysRequest } from "./request/rotate-user-account-keys.request";
import { UpdateKeyRequest } from "./request/update-key.request";

@Injectable()
Expand All @@ -11,4 +12,14 @@
postUserKeyUpdate(request: UpdateKeyRequest): Promise<any> {
return this.apiService.send("POST", "/accounts/key", request, true, false);
}

postUserKeyUpdateV2(request: RotateUserAccountKeysRequest): Promise<any> {
return this.apiService.send(

Check warning on line 17 in apps/web/src/app/key-management/key-rotation/user-key-rotation-api.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/key-management/key-rotation/user-key-rotation-api.service.ts#L17

Added line #L17 was not covered by tests
"POST",
"/accounts/key-management/rotate-user-account-keys",
request,
true,
false,
);
}
}
Loading
Loading