feat(auth-validator): [Auth/PM-22975] Client Version Validator #6588
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
…hups to baserequest validator.
…e white spaces as well as the renaming of a file
…ixing the rename to the correct file.
…ng a little to be more clear.
Patrick-Pimentel-Bitwarden
requested review from
JaredSnider-Bitwarden and
mzieniukbw
|
Claude finished @Patrick-Pimentel-Bitwarden's task —— View job Code Review CompleteTasks
Comprehensive Code Review - PR #6588: Client Version ValidatorOverall Assessment: REQUEST CHANGESThis PR implements critical security infrastructure to prevent v2 encryption users from logging in with incompatible old clients. The core approach is sound, with proper account enumeration prevention through post-credential validation. However, there are critical test coverage gaps and an important consideration about the breaking change impact. Finding 1: Missing test coverage for User.HasV2Encryption() edge casesLocation: Details and fixThe Missing test scenarios:
Why this matters:
Recommendation: [Theory]
[InlineData(TestEncryptionConstants.V2PrivateKey, 2, true)] // Valid v2
[InlineData(TestEncryptionConstants.V2PrivateKey, 1, false)] // v2 key, v1 version
[InlineData(TestEncryptionConstants.AES256_CBC_HMAC_Encstring, 2, false)] // v1 key, v2 version
[InlineData(null, 2, false)] // Null private key
[InlineData("", 2, false)] // Empty private key
[InlineData("invalid", 2, false)] // Malformed key (triggers exception)
public void HasV2Encryption_VariousStates_ReturnsExpected(string privateKey, int? securityVersion, bool expected)
{
var user = new User
{
PrivateKey = privateKey,
SecurityVersion = securityVersion
};
Assert.Equal(expected, user.HasV2Encryption());
}This ensures the validator's core dependency behaves correctly under all conditions. Finding 2: Blocking all clients without version header is a breaking changeLocation: Details and considerationThe validator rejects ALL authentication attempts when the if (clientVersion == null)
{
FillRequestContextWithErrorData(requestContext, "version_header_missing", _versionHeaderMissing);
return false;
}Impact:
Security rationale from code comments:
Questions to address:
Alternative approach (if breaking change is problematic): Version? minVersion = user.HasV2Encryption() ? Constants.MinimumClientVersionForV2Encryption : null;
if (minVersion == null)
{
return true; // v1 user - no version check needed
}
// v2 user - header is required
if (clientVersion == null)
{
FillRequestContextWithErrorData(requestContext, "version_header_missing", _versionHeaderMissing);
return false;
}This maintains backward compatibility for v1 users while enforcing strict version requirements for v2 users. If the breaking change is intentional: Document the rollout plan and client version requirements in the PR description. Finding 3: Account enumeration prevention validationLocation: Verification detailsThe PR correctly prevents account enumeration by placing client version validation AFTER credential validation: // 1. Validate credentials first
var valid = await ValidateContextAsync(context, validatorContext);
if (!valid) { return; }
// 1.5 Now check client version (after proving account ownership)
var clientVersionValid = await ValidateClientVersionAsync(context, validatorContext);Why this is correct:
Feature flag path also safe: No changes needed - the implementation correctly prevents account enumeration. Finding 4: Failed login event logging may misrepresent user intentLocation: Details and improvementWhen client version validation fails, the code logs a failed login event: SetValidationErrorResult(context, validatorContext);
await LogFailedLoginEvent(validatorContext.User, EventType.User_FailedLogIn);Issue:
Suggestion: await LogFailedLoginEvent(validatorContext.User, EventType.User_ClientVersionRejected);
// OR
_logger.LogInformation("User {UserId} blocked due to outdated client version {Version}",
user.Id, clientVersion);This distinguishes between security-relevant failed logins and UX issues requiring user action. Alternatively: If failed login counting is intentional (to prevent brute force with old clients), document this decision in comments. Finding 5: MinimumClientVersionForV2Encryption constant locationLocation: Discussion pointThe minimum client version is hardcoded: public static readonly Version MinimumClientVersionForV2Encryption = new("2025.11.0");Questions:
Consideration: Not blocking for this PR, but worth considering for future-proofing. Finding 6: Code quality - Extraction of EncryptionParsing utilityLocation: Verification detailsThe extraction of Strengths:
Test coverage verified:
This refactoring improves code reusability and maintainability without introducing issues. Finding 7: Integration test coverageLocation: Verification detailsIntegration tests provide good end-to-end coverage: Tested scenarios:
Good practices observed:
Minor gap: No integration test for v1 user on old client (should allow). This is covered by unit tests but would be valuable for regression prevention. Not blocking, but consider adding: [Theory, BitAutoData, RegisterFinishRequestModelCustomize]
public async Task TokenEndpoint_V1User_OnOldClientVersion_Succeeds(...)
{
// v1 user (no v2 encryption) should authenticate regardless of client version
}Security Compliance ChecklistBased on Bitwarden's security requirements from CLAUDE.md:
Recommendations SummaryMust fix before merge (Critical):
Should address (Important): Consider (Suggested): Questions for team: Final VerdictThe PR implements a critical security control with proper account enumeration prevention and good test coverage for most components. However, the missing test coverage for the core Once test coverage is added and the breaking change is documented/adjusted, this will be ready for merge. |
Patrick-Pimentel-Bitwarden
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs
Outdated
Show resolved
Hide resolved
src/Identity/IdentityServer/RequestValidators/ClientVersionValidator.cs
Outdated
Show resolved
Hide resolved
src/Core/KeyManagement/Queries/GetMinimumClientVersionForUserQuery.cs
Outdated
Show resolved
Hide resolved
|
I am seeing 100 failures in tests, double checking what is going on, consider holding off on approving. |
|
|
||
| namespace Bit.Core.KeyManagement.Queries; | ||
|
|
||
| public class IsV2EncryptionUserQuery(IUserSignatureKeyPairRepository userSignatureKeyPairRepository) |
There was a problem hiding this comment.
Ok, so having thought about this a bit, I think we should remove this query. We can do either user.GetSecurityVersion() >= 2 or implement "IsV2User" on the user class which does the same.
There was a problem hiding this comment.
So I took this feedback and tweaked it a little. I introduced a narrower scoped check:
user.IsSecurityVersionTwo()
I felt this was more defensive because now we have breadcrumbed places where we are doing branching logic based on a V2 encryption requirement.
While I am happy to be using this simpler approach than the IsV2User query I want to recognize this is a deviation from the ticket's original tech breakdown and should this be discussed to confirm this wouldn't create any issues?
See the changes in 753670d
There was a problem hiding this comment.
So after a round of discussion around the software engineering approach to this solution we arrived at the work that was done f719763
Have a look @quexten and let me know what you think. This approach keeps some attachment to looking at the private key structure which we feel is a more defensive approach than just looking at the SecurityVersion column.
There was a problem hiding this comment.
I guess it's a little confusing, but a V2 is every version after and including 2, not just 2. But it's OK to leave it as is right now, since we don't yet have a direct plan for implementing one of the features that would migrate a V2 user to a security version that is larger than 2.
I'm ok with the solution as-is, thanks for spending the time here!
…edback from km. Removed IsV2User in favor of checking the security version on the user.
| var sut = new GetMinimumClientVersionForUserQuery(); | ||
| var version = await sut.Run(new User | ||
| { | ||
| SecurityVersion = 1, |
There was a problem hiding this comment.
Could we add a test for security version being null?
There was a problem hiding this comment.
good catch!
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
Approving for KM, one optional request for a test
… tests and added comment.
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Really nice work on this! I have a few questions and change requests below:
src/Core/KeyManagement/Queries/GetMinimumClientVersionForUserQuery.cs
Outdated
Show resolved
Hide resolved
src/Identity/IdentityServer/RequestValidators/ClientVersionValidator.cs
Outdated
Show resolved
Hide resolved
…lidator to return false on null.
…th removal of cqrs approach in favor of static user checks. Also fixed tests
…cryption parsing tests
…bs and updated test for encryption parsing tests.
…omment to make more sense.
… the header present now blocks users from validating
…tion name change.
|
|
||
| // Simple stubs for other encstring versions used by parsing tests | ||
| public const string AES256_CBC_B64_Encstring = "0.stub"; | ||
| public const string AES128_CBC_HMACSHA256_B64_Encstring = "1.stub"; |
There was a problem hiding this comment.
Noting: We don't support this encstring type anymore, it does not exist in the real world and client / sdk support was removed.
There was a problem hiding this comment.
Let's remove it then
There was a problem hiding this comment.
Or mark it as deprecated
| public const string AES256_CBC_HMAC_EmptySuffix = "2."; | ||
| public const string RSA2048_OAEPSHA256_B64_Encstring = "3.stub"; | ||
| public const string RSA2048_OAEPSHA1_B64_Encstring = "4.stub"; | ||
| public const string RSA2048_OAEPSHA256_HMACSHA256_B64_Encstring = "5.stub"; |
There was a problem hiding this comment.
Noting: Between the RSA types, only type 4 is known to exist in the real world.
There was a problem hiding this comment.
Would you like me to leave this comment amongst the testing constants? Or is there another action you would like me to take?
There was a problem hiding this comment.
Let's only add ones that we expect to use
There was a problem hiding this comment.
Or we can just keep these as examples
| public const string RSA2048_OAEPSHA256_B64_Encstring = "3.stub"; | ||
| public const string RSA2048_OAEPSHA1_B64_Encstring = "4.stub"; | ||
| public const string RSA2048_OAEPSHA256_HMACSHA256_B64_Encstring = "5.stub"; | ||
| public const string RSA2048_OAEPSHA1_HMACSHA256_B64_Encstring = "6.stub"; |
There was a problem hiding this comment.
Could we add a Type 7 string?
…ges to test encryption constants.
… client version validation.
NEEDS TESTING ON FEATURE BRANCH
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22975
📔 Objective
The objective of this ticket is to create a client version validator that checks the header data on a token request and prevent users who have migrated to v2 from logging in with older client versions.
📸 Screenshots
Validator.Working.mov
Non.Rotated.Key.Working.Fine.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes