feat(salt): [PM-27060] - #22381
Conversation
… have the salt present for key derivation when using the sdk for prelogin.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR threads the server-supplied prelogin salt through Code Review Details
The salt-selection branch is duplicated in both the prefetched and freshly-fetched paths (including a copy-pasted four-line comment) and could collapse to a single PR Metadata Assessment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (useSdkForPrelogin) { | ||
| return this.legacyCompatKeyService.makeMasterKey( | ||
| masterPassword, | ||
| preFetchedPreloginData.salt, | ||
| preFetchedPreloginData.kdfConfig, | ||
| ); |
There was a problem hiding this comment.
❓ 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.
There was a problem hiding this comment.
@Patrick-Pimentel-Bitwarden , this is something we have to consider.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22381 +/- ##
=======================================
Coverage 53.45% 53.46%
=======================================
Files 4275 4275
Lines 135303 135308 +5
Branches 21326 21327 +1
=======================================
+ Hits 72328 72336 +8
+ Misses 57688 57685 -3
Partials 5287 5287 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| * The salt the server dictates for master key derivation. Only consumed when | ||
| * {@link FeatureFlag.PM27060_PasswordPreloginFromSdk} is on; callers otherwise derive from the | ||
| * user-entered email. Nullable while PM-28143 is in flight — the server does not populate it on | ||
| * every deployment. |
There was a problem hiding this comment.
Nit: Claude loves to make fragile comments like this where it details that something is only consumed while the flag is enabled - it should just document what it is - generally not usage sites unless there is a very good reason. Also, the PM-28143 comment is not useful. All supported servers send the salt now even though it is optional from the server response model perspective (which we could have just made non-nullish from the start since they were additive model changes on the respond model side).
There was a problem hiding this comment.
This comment applies elsewhere where we mention PM-28143 as well.
| 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); | ||
| }); |
There was a problem hiding this comment.
| 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(); | ||
| }); |
There was a problem hiding this comment.
| if (useSdkForPrelogin) { | ||
| return this.legacyCompatKeyService.makeMasterKey( | ||
| masterPassword, | ||
| preFetchedPreloginData.salt, | ||
| preFetchedPreloginData.kdfConfig, | ||
| ); | ||
| } else { | ||
| return this.legacyCompatKeyService.makeMasterKey( | ||
| masterPassword, | ||
| email, | ||
| preFetchedPreloginData.kdfConfig, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎨 : 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);
| // 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, | ||
| ); | ||
| } |
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
🎨 : leaving a comment w/ the PM-27060 reference is worse vs just explaining that server dictates salt.
| 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, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
2 participants
🎟️ Tracking
📔 Objective
Added in logic so the prelogin response will have the salt present for key derivation when using the sdk for prelogin.
📸 Screenshots