Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ describe("LoginStrategy", () => {
tokenService.decodeAccessToken.calledWith(accessToken).mockResolvedValue(decodedToken);

passwordPreloginService.getPreloginData$.mockReturnValue(
of(new PasswordPreloginData(PBKDF2KdfConfig.createDefault())),
of(new PasswordPreloginData(PBKDF2KdfConfig.createDefault(), "prelogin-salt")),
);
legacyCompatKeyService.makeMasterKey.mockResolvedValue({} as any);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
} from "@bitwarden/common/auth/password-prelogin";
import { TwoFactorService } from "@bitwarden/common/auth/two-factor";
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { AccountCryptographicStateService } from "@bitwarden/common/key-management/account-cryptography/account-cryptographic-state.service";
import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service";
import { FakeMasterPasswordService } from "@bitwarden/common/key-management/master-password/services/fake-master-password.service";
Expand Down Expand Up @@ -51,6 +52,9 @@ import { PasswordLoginStrategy, PasswordLoginStrategyData } from "./password-log
const email = "hello@world.com";
const masterPassword = "password";
const hashedPassword = "HASHED_PASSWORD";
// Deliberately not equal to `email`. The whole point of PM-27060 is that the server dictates the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ : leaving a comment w/ the PM-27060 reference is worse vs just explaining that server dictates salt.

// KDF salt, so a fixture where salt === email would pass no matter which one the strategy used.
const preloginSalt = "server.normalized+salt@world.com";
const masterKey = new SymmetricCryptoKey(
Utils.fromB64ToArray(
"N2KWjlLpfi5uHjv+YcfUKIpZ1l+W+6HRensmIqD+BFYBf6N/dvFpJfWwYnVBdgFCK2tJTAIMLhqzIQQEUmGFgg==",
Expand All @@ -62,6 +66,17 @@ const masterPasswordPolicyResponse = new MasterPasswordPolicyResponse({
EnforceOnLogin: true,
MinLength: 8,
});
const kdfConfig = PBKDF2KdfConfig.createDefault();

function credentialsWithPrefetchedData(salt: string = preloginSalt) {
return new PasswordLoginCredentials(
email,
masterPassword,
undefined,
undefined,
new PasswordPreloginData(kdfConfig, salt),
);
}

describe("PasswordLoginStrategy", () => {
let accountService: FakeAccountService;
Expand Down Expand Up @@ -127,10 +142,14 @@ describe("PasswordLoginStrategy", () => {
});

passwordPreloginService.getPreloginData$.mockReturnValue(
of(new PasswordPreloginData(PBKDF2KdfConfig.createDefault())),
of(new PasswordPreloginData(PBKDF2KdfConfig.createDefault(), preloginSalt)),
);
legacyCompatKeyService.makeMasterKey.mockResolvedValue(masterKey);

// Default to the flag off so the pre-PM-27060 behavior stays the baseline; tests that exercise
// the SDK prelogin path opt in explicitly.
configService.getFeatureFlag.mockResolvedValue(false);

legacyCompatKeyService.hashMasterKey
.calledWith(masterPassword, expect.anything())
.mockResolvedValue(hashedPassword);
Expand Down Expand Up @@ -231,14 +250,7 @@ describe("PasswordLoginStrategy", () => {
});

it("does not call getPreloginData$ when preFetchedPreloginData is provided", async () => {
const preloginData = new PasswordPreloginData(PBKDF2KdfConfig.createDefault());
const credentialsWithPrefetch = new PasswordLoginCredentials(
email,
masterPassword,
undefined,
undefined,
preloginData,
);
const credentialsWithPrefetch = credentialsWithPrefetchedData();

await passwordLoginStrategy.logIn(credentialsWithPrefetch);

Expand All @@ -260,6 +272,129 @@ describe("PasswordLoginStrategy", () => {

expect(passwordPreloginService.clearCache).toHaveBeenCalledTimes(1);
});

// PM-27060: when prelogin comes from the SDK, the server dictates the KDF salt and the client
// must derive the master key from it rather than from the email the user typed. The flag gates
// this so it can be switched off if normalization diverges during the transition.
describe("salt selection", () => {
it("reads the PM27060_PasswordPreloginFromSdk flag", async () => {
await passwordLoginStrategy.logIn(credentials);

expect(configService.getFeatureFlag).toHaveBeenCalledWith(
FeatureFlag.PM27060_PasswordPreloginFromSdk,
);
});

describe("when the flag is on", () => {
beforeEach(() => {
configService.getFeatureFlag.mockResolvedValue(true);
});

it("derives the master key from the prelogin salt for prefetched data", async () => {
await passwordLoginStrategy.logIn(credentialsWithPrefetchedData());

expect(legacyCompatKeyService.makeMasterKey).toHaveBeenCalledWith(
masterPassword,
preloginSalt,
kdfConfig,
);
expect(legacyCompatKeyService.makeMasterKey).not.toHaveBeenCalledWith(
masterPassword,
email,
expect.anything(),
);
});

it("derives the master key from the prelogin salt for freshly fetched data", async () => {
// credentials from the outer beforeEach carries no prefetched data, so the strategy
// fetches via passwordPreloginService, which is stubbed to return preloginSalt.
await passwordLoginStrategy.logIn(credentials);

expect(legacyCompatKeyService.makeMasterKey).toHaveBeenCalledWith(
masterPassword,
preloginSalt,
PBKDF2KdfConfig.createDefault(),
);
expect(legacyCompatKeyService.makeMasterKey).not.toHaveBeenCalledWith(
masterPassword,
email,
expect.anything(),
);
});

it("passes the salt through verbatim without re-normalizing it", async () => {
// The server owns normalization under PM-27060. Trimming or lower-casing here would
// produce a different master key than the one the vault was encrypted with.
const unnormalizedSalt = " MiXeD.Case@World.Com ";

await passwordLoginStrategy.logIn(credentialsWithPrefetchedData(unnormalizedSalt));

expect(legacyCompatKeyService.makeMasterKey).toHaveBeenCalledWith(
masterPassword,
unnormalizedSalt,
kdfConfig,
);
});
Comment thread
JaredSnider-Bitwarden marked this conversation as resolved.

it("passes an absent salt straight through", async () => {
// PM-28143: Salt is nullable server-side during the transition. This documents current
// behavior โ€” the strategy does not fall back to the email and does not throw.
// Built inline rather than via the helper, whose default parameter would substitute
// preloginSalt for an explicit undefined.
const noSaltCredentials = new PasswordLoginCredentials(
email,
masterPassword,
undefined,
undefined,
new PasswordPreloginData(kdfConfig, undefined as unknown as string),
);

await passwordLoginStrategy.logIn(noSaltCredentials);

expect(legacyCompatKeyService.makeMasterKey).toHaveBeenCalledWith(
masterPassword,
undefined,
kdfConfig,
);
});
Comment on lines +339 to +359

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ : another unsupported server test case.

});

describe("when the flag is off", () => {
beforeEach(() => {
configService.getFeatureFlag.mockResolvedValue(false);
});

it("derives the master key from the entered email for prefetched data", async () => {
await passwordLoginStrategy.logIn(credentialsWithPrefetchedData());

expect(legacyCompatKeyService.makeMasterKey).toHaveBeenCalledWith(
masterPassword,
email,
kdfConfig,
);
expect(legacyCompatKeyService.makeMasterKey).not.toHaveBeenCalledWith(
masterPassword,
preloginSalt,
expect.anything(),
);
});

it("derives the master key from the entered email for freshly fetched data", async () => {
await passwordLoginStrategy.logIn(credentials);

expect(legacyCompatKeyService.makeMasterKey).toHaveBeenCalledWith(
masterPassword,
email,
PBKDF2KdfConfig.createDefault(),
);
expect(legacyCompatKeyService.makeMasterKey).not.toHaveBeenCalledWith(
masterPassword,
preloginSalt,
expect.anything(),
);
});
});
});
});

describe("evaluateMasterPasswordIfRequired", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
PasswordPreloginData,
PasswordPreloginService,
} from "@bitwarden/common/auth/password-prelogin";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength";
import { UserId } from "@bitwarden/common/types/guid";
Expand Down Expand Up @@ -136,13 +137,29 @@ export class PasswordLoginStrategy extends LoginStrategy {
email: string,
preFetchedPreloginData?: PasswordPreloginData,
): Promise<MasterKey> {
const useSdkForPrelogin = await this.configService.getFeatureFlag(
FeatureFlag.PM27060_PasswordPreloginFromSdk,
);

// if we have prefetched prelogin data, use it
if (preFetchedPreloginData) {
return this.legacyCompatKeyService.makeMasterKey(
masterPassword,
email,
preFetchedPreloginData.kdfConfig,
);
// If we are using the sdk to fetch the prelogin data, only then do we want to
// use the salt that is passed back from the prelogin response in building the master key.
// This gives us the ability to turn off the feature of using the returned salt from salt
// in the event of bad normalization occurring during the transition.
if (useSdkForPrelogin) {
return this.legacyCompatKeyService.makeMasterKey(
masterPassword,
preFetchedPreloginData.salt,
preFetchedPreloginData.kdfConfig,
);
Comment on lines +150 to +155

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

โ“ QUESTION: makeMasterKey re-normalizes the salt โ€” is the server salt guaranteed to already be trimmed and lower-cased?

Details

makeMasterKey applies its own normalization before deriving:

// libs/legacy-crypto/src/services/legacy-compat-key.service.ts:86
email = email.trim().toLowerCase();

So the server-supplied salt is not used verbatim, despite the spec comment ("passes the salt through verbatim without re-normalizing itโ€ฆ Trimming or lower-casing here would produce a different master key"). If the server ever dictates a salt whose casing or whitespace differs from trim().toLowerCase(), the client derives a different master key than intended โ€” precisely the divergence PM-27060 is meant to eliminate.

If server normalization is guaranteed to match, consider updating the spec comment so it doesn't assert a guarantee the runtime doesn't provide. If not, the salt needs a derivation path that skips the legacy normalization.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Patrick-Pimentel-Bitwarden , this is something we have to consider.

} else {
return this.legacyCompatKeyService.makeMasterKey(
masterPassword,
email,
preFetchedPreloginData.kdfConfig,
);
}
Comment on lines +150 to +162

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ : It's a bit more clear to me if we use this ternary construction + many less lines of code.

// Flag lets us revert to email-derived salt if SDK salt encounters issues
const salt = useSdkForPrelogin ? preloginData.salt : email;
return this.legacyCompatKeyService.makeMasterKey(masterPassword, salt, preloginData.kdfConfig);

}

// No prefetched data โ€” fetch now. PasswordPreloginData.fromResponse validates the KDF config.
Expand All @@ -152,7 +169,23 @@ export class PasswordLoginStrategy extends LoginStrategy {
throw new Error("KDF config is required");
}

return this.legacyCompatKeyService.makeMasterKey(masterPassword, email, preloginData.kdfConfig);
// If we are using the sdk to fetch the prelogin data, only then do we want to
// use the salt that is passed back from the prelogin response in building the master key.
// This gives us the ability to turn off the feature of using the returned salt from salt
// in the event of bad normalization occurring during the transition.
if (useSdkForPrelogin) {
return this.legacyCompatKeyService.makeMasterKey(
masterPassword,
preloginData.salt,
preloginData.kdfConfig,
);
} else {
return this.legacyCompatKeyService.makeMasterKey(
masterPassword,
email,
preloginData.kdfConfig,
);
}
Comment on lines +172 to +188

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

}

private async evaluateMasterPasswordIfRequired(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ import { UserDecryptionOptionsService } from "../user-decryption-options/user-de
import { LoginStrategyService } from "./login-strategy.service";
import { CacheData } from "./login-strategy.state";

const argon2PreloginData = new PasswordPreloginData(new Argon2KdfConfig(2, 16, 1));
const argon2PreloginData = new PasswordPreloginData(new Argon2KdfConfig(2, 16, 1), "prelogin-salt");

describe("LoginStrategyService", () => {
let sut: LoginStrategyService;
Expand Down Expand Up @@ -162,7 +162,7 @@ describe("LoginStrategyService", () => {
});

passwordPreloginService.getPreloginData$.mockReturnValue(
of(new PasswordPreloginData(PBKDF2KdfConfig.createDefault())),
of(new PasswordPreloginData(PBKDF2KdfConfig.createDefault(), "prelogin-salt")),
);
legacyCompatKeyService.makeMasterKey.mockResolvedValue({} as any);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,26 @@ describe("DefaultPasswordPreloginService", () => {
const emailB = "b@example.com";
const identityUrl = "https://identity.bitwarden.com";

// The API and SDK paths return different salts so tests can prove which source was used.
const apiSalt = "api-salt";
const sdkSalt = "sdk-salt";

// PBKDF2 is used as a stand-in throughout; KDF type coverage is in password-prelogin.model.spec.ts.
const response = new PasswordPreloginResponse({
Kdf: 0,
KdfIterations: PBKDF2KdfConfig.ITERATIONS.defaultValue,
KdfSettings: { KdfType: 0, Iterations: PBKDF2KdfConfig.ITERATIONS.defaultValue },
Salt: apiSalt,
});
const sdkResponse: SdkPasswordPreloginResponse = {
kdf: { pBKDF2: { iterations: PBKDF2KdfConfig.ITERATIONS.defaultValue } },
salt: "test-salt",
salt: sdkSalt,
};
const expectedData = new PasswordPreloginData(
new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue),
apiSalt,
);
const expectedSdkData = new PasswordPreloginData(
new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue),
sdkSalt,
);

beforeEach(() => {
Expand Down Expand Up @@ -93,10 +102,45 @@ describe("DefaultPasswordPreloginService", () => {

const result = await firstValueFrom(sut.getPreloginData$(email));

expect(result).toEqual(expectedData);
expect(result).toEqual(expectedSdkData);
expect(apiService.getPreloginData).not.toHaveBeenCalled();
});

it("carries the SDK-supplied salt onto the model when the flag is on", async () => {
configService.getFeatureFlag.mockResolvedValue(true);

const result = await firstValueFrom(sut.getPreloginData$(email));

// PasswordLoginStrategy derives the master key from this salt when the flag is on, so it
// must come from the SDK response rather than the email the caller passed in.
expect(result.salt).toBe(sdkSalt);
expect(result.salt).not.toBe(email);
});

it("carries the API-supplied salt onto the model when the flag is off", async () => {
const result = await firstValueFrom(sut.getPreloginData$(email));

// The salt is still mapped when the flag is off โ€” PasswordLoginStrategy simply ignores it
// and derives from the entered email instead.
expect(result.salt).toBe(apiSalt);
});
Comment on lines +109 to +126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ : these two tests are duplicative of the two tests above them.


it("emits an undefined salt when the SDK omits one", async () => {
// PM-28143: salt is nullable while the server transition is in flight. Documents current
// behavior โ€” the service does not substitute a fallback.
configService.getFeatureFlag.mockResolvedValue(true);
sdkService.client.auth
.mockDeep()
.login.mockDeep()
.get_password_prelogin.mockResolvedValue({
kdf: { pBKDF2: { iterations: PBKDF2KdfConfig.ITERATIONS.defaultValue } },
} as SdkPasswordPreloginResponse);

const result = await firstValueFrom(sut.getPreloginData$(email));

expect(result.salt).toBeUndefined();
});
Comment on lines +128 to +142

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

โš ๏ธ : this test is documenting a server state that we no longer support. All supported servers send salt so... I think we delete this.


it("checks the feature flag with the expected key", async () => {
await firstValueFrom(sut.getPreloginData$(email));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,6 @@ export class DefaultPasswordPreloginService implements PasswordPreloginService {
const sdkResponse: SdkPasswordPreloginResponse = await loginClient.get_password_prelogin(email);
const kdfConfig = fromSdkKdfConfig(sdkResponse.kdf);
kdfConfig.validateKdfConfigForPrelogin();
return new PasswordPreloginData(kdfConfig);
return new PasswordPreloginData(kdfConfig, sdkResponse.salt);
}
}
Loading
Loading