-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(auth-validator): [Auth/PM-22975] Client Version Validator #6588
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
1c4fd6c
22e9e5b
1af2fba
47a26bb
a82b31c
7d71ee2
851f963
756ae5e
7874ec7
91af02b
59d9d7b
7897485
d0c5333
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,6 @@ | ||
| ๏ปฟnamespace Bit.Core.KeyManagement; | ||
|
|
||
| public static class Constants | ||
| { | ||
| public static readonly Version MinimumClientVersion = new Version("2025.11.0"); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
| using Bit.Core.KeyManagement.Queries.Interfaces; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Queries; | ||
|
|
||
| public class GetMinimumClientVersionForUserQuery(IIsV2EncryptionUserQuery isV2EncryptionUserQuery) | ||
| : IGetMinimumClientVersionForUserQuery | ||
| { | ||
| public async Task<Version?> Run(User? user) | ||
| { | ||
| if (user == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| if (await isV2EncryptionUserQuery.Run(user)) | ||
|
Contributor
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. ๐ญ Error handling consideration - If
The current behavior (1) seems reasonable for detecting corruption, but consider whether it could be weaponized. If a user's account gets into an invalid state, they'll be locked out until support fixes it. |
||
| { | ||
| return Constants.MinimumClientVersion; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Queries.Interfaces; | ||
|
|
||
| public interface IGetMinimumClientVersionForUserQuery | ||
| { | ||
| Task<Version?> Run(User? user); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Queries.Interfaces; | ||
|
|
||
| public interface IIsV2EncryptionUserQuery | ||
| { | ||
| Task<bool> Run(User user); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.KeyManagement.Queries.Interfaces; | ||
| using Bit.Core.KeyManagement.Repositories; | ||
| using Bit.Core.KeyManagement.Utilities; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Queries; | ||
|
|
||
| public class IsV2EncryptionUserQuery(IUserSignatureKeyPairRepository userSignatureKeyPairRepository) | ||
| : IIsV2EncryptionUserQuery | ||
| { | ||
| public async Task<bool> Run(User user) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(user); | ||
|
|
||
| var hasSignatureKeyPair = await userSignatureKeyPairRepository.GetByUserIdAsync(user.Id) != null; | ||
| var isPrivateKeyEncryptionV2 = | ||
| !string.IsNullOrWhiteSpace(user.PrivateKey) && | ||
| EncryptionParsing.GetEncryptionType(user.PrivateKey) == EncryptionType.XChaCha20Poly1305_B64; | ||
|
|
||
| return hasSignatureKeyPair switch | ||
| { | ||
| // Valid v2 user | ||
| true when isPrivateKeyEncryptionV2 => true, | ||
| // Valid v1 user | ||
| false when !isPrivateKeyEncryptionV2 => false, | ||
| _ => throw new InvalidOperationException( | ||
|
Contributor
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. โ Cryptographic state validation error lacks detail - The exception message doesn't specify which invalid state was detected, making debugging difficult. Consider: _ => throw new InvalidOperationException(
$"User {user.Id} is in an invalid encryption state: " +
$"HasSignatureKeyPair={hasSignatureKeyPair}, " +
$"IsPrivateKeyV2={isPrivateKeyEncryptionV2}. " +
"Expected both true (v2) or both false (v1).")This provides actionable information for support/debugging without exposing sensitive data. |
||
| "User is in an invalid state for key rotation. User has a signature key pair, but the private key is not in v2 format, or vice versa.") | ||
| }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| ๏ปฟusing Bit.Core.Enums; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Utilities; | ||
|
|
||
| public static class EncryptionParsing | ||
| { | ||
| /// <summary> | ||
| /// Helper method to convert an encryption type string to an enum value. | ||
| /// </summary> | ||
| public static EncryptionType GetEncryptionType(string encString) | ||
| { | ||
| var parts = encString.Split('.'); | ||
| if (parts.Length == 1) | ||
|
Contributor
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. if (parts.Length == 1)
{
throw new ArgumentException("Encryption type string must contain a type prefix and data separated by '.'");
}And for the second case: throw new ArgumentException($"Unrecognized encryption type: '{parts[0]}'");
Contributor
Author
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. This is code copied from KM, I'm hesitant to make more changes to their work. |
||
| { | ||
| throw new ArgumentException("Invalid encryption type string."); | ||
| } | ||
| if (byte.TryParse(parts[0], out var encryptionTypeNumber)) | ||
| { | ||
| if (Enum.IsDefined(typeof(EncryptionType), encryptionTypeNumber)) | ||
| { | ||
| return (EncryptionType)encryptionTypeNumber; | ||
| } | ||
| } | ||
| throw new ArgumentException("Invalid encryption type string."); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ public abstract class BaseRequestValidator<T> where T : class | |
| private readonly IUserRepository _userRepository; | ||
| private readonly IAuthRequestRepository _authRequestRepository; | ||
| private readonly IMailService _mailService; | ||
| private readonly IClientVersionValidator _clientVersionValidator; | ||
|
|
||
| protected ICurrentContext CurrentContext { get; } | ||
| protected IPolicyService PolicyService { get; } | ||
|
|
@@ -68,7 +69,8 @@ public BaseRequestValidator( | |
| IPolicyRequirementQuery policyRequirementQuery, | ||
| IAuthRequestRepository authRequestRepository, | ||
| IMailService mailService, | ||
| IUserAccountKeysQuery userAccountKeysQuery | ||
| IUserAccountKeysQuery userAccountKeysQuery, | ||
| IClientVersionValidator clientVersionValidator | ||
| ) | ||
| { | ||
| _userManager = userManager; | ||
|
|
@@ -89,6 +91,7 @@ IUserAccountKeysQuery userAccountKeysQuery | |
| _authRequestRepository = authRequestRepository; | ||
| _mailService = mailService; | ||
| _accountKeysQuery = userAccountKeysQuery; | ||
| _clientVersionValidator = clientVersionValidator; | ||
| } | ||
|
|
||
| protected async Task ValidateAsync(T context, ValidatedTokenRequest request, | ||
|
|
@@ -108,7 +111,8 @@ await BuildSuccessResultAsync(validatorContext.User, context, validatorContext.D | |
| } | ||
| else | ||
| { | ||
| // 1. We need to check if the user's master password hash is correct. | ||
| // 1. We need to check if the user is legitimate via the contextually appropriate mechanism | ||
| // (webauthn, password, custom token, etc.). | ||
| var valid = await ValidateContextAsync(context, validatorContext); | ||
| var user = validatorContext.User; | ||
| if (!valid) | ||
|
|
@@ -119,6 +123,17 @@ await BuildSuccessResultAsync(validatorContext.User, context, validatorContext.D | |
| return; | ||
| } | ||
|
|
||
| // 1.5 Now check the version number of the client. Do this after ValidateContextAsync so that | ||
| // we prevent account enumeration. If we were to do this before ValidateContextAsync, then attackers | ||
| // could use a known invalid client version and make a request for a user (before we know if they have | ||
| // demonstrated ownership of the account via correct credentials) and identify if they exist by getting | ||
| // an error response back from the validator saying the user is not compatible with the client. | ||
|
Comment on lines
+126
to
+130
Contributor
Author
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. ๐ญ Could someone double check my explanation here?
Patrick-Pimentel-Bitwarden marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| var clientVersionValid = await ValidateClientVersionAsync(context, validatorContext); | ||
| if (!clientVersionValid) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // 2. Decide if this user belongs to an organization that requires SSO. | ||
| validatorContext.SsoRequired = await RequireSsoLoginAsync(user, request.GrantType); | ||
| if (validatorContext.SsoRequired) | ||
|
|
@@ -258,7 +273,8 @@ private Func<Task<bool>>[] DetermineValidationOrder(T context, ValidatedTokenReq | |
| // validation to perform the recovery as part of scheme validation based on the request. | ||
| return | ||
| [ | ||
| () => ValidateMasterPasswordAsync(context, validatorContext), | ||
| () => ValidateGrantSpecificContext(context, validatorContext), | ||
| () => ValidateClientVersionAsync(context, validatorContext), | ||
| () => ValidateTwoFactorAsync(context, request, validatorContext), | ||
| () => ValidateSsoAsync(context, request, validatorContext), | ||
| () => ValidateNewDeviceAsync(context, request, validatorContext), | ||
|
|
@@ -271,7 +287,8 @@ private Func<Task<bool>>[] DetermineValidationOrder(T context, ValidatedTokenReq | |
| // The typical validation scenario. | ||
| return | ||
| [ | ||
| () => ValidateMasterPasswordAsync(context, validatorContext), | ||
| () => ValidateGrantSpecificContext(context, validatorContext), | ||
| () => ValidateClientVersionAsync(context, validatorContext), | ||
| () => ValidateSsoAsync(context, request, validatorContext), | ||
| () => ValidateTwoFactorAsync(context, request, validatorContext), | ||
| () => ValidateNewDeviceAsync(context, request, validatorContext), | ||
|
|
@@ -324,12 +341,30 @@ private static async Task<bool> ProcessValidatorsAsync(params Func<Task<bool>>[] | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates the user's Master Password hash. | ||
| /// Validates whether the client version is compatible for the user attempting to authenticate. | ||
| /// New authentications only; refresh/device grants are handled elsewhere. | ||
| /// </summary> | ||
| /// <returns>true if the scheme successfully passed validation, otherwise false.</returns> | ||
| private async Task<bool> ValidateClientVersionAsync(T context, CustomValidatorRequestContext validatorContext) | ||
| { | ||
| var ok = await _clientVersionValidator.ValidateAsync(validatorContext.User, validatorContext); | ||
| if (ok) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| SetValidationErrorResult(context, validatorContext); | ||
| await LogFailedLoginEvent(validatorContext.User, EventType.User_FailedLogIn); | ||
| return false; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates the user's master password, webauthen, or custom token request via the appropriate context validator. | ||
| /// </summary> | ||
| /// <param name="context">The current request context.</param> | ||
| /// <param name="validatorContext"><see cref="Bit.Identity.IdentityServer.CustomValidatorRequestContext" /></param> | ||
| /// <returns>true if the scheme successfully passed validation, otherwise false.</returns> | ||
| private async Task<bool> ValidateMasterPasswordAsync(T context, CustomValidatorRequestContext validatorContext) | ||
| private async Task<bool> ValidateGrantSpecificContext(T context, CustomValidatorRequestContext validatorContext) | ||
| { | ||
| var valid = await ValidateContextAsync(context, validatorContext); | ||
| var user = validatorContext.User; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| ๏ปฟusing Bit.Core.Context; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.KeyManagement.Queries.Interfaces; | ||
| using Bit.Core.Models.Api; | ||
| using Duende.IdentityServer.Validation; | ||
|
|
||
| namespace Bit.Identity.IdentityServer.RequestValidators; | ||
|
|
||
| public interface IClientVersionValidator | ||
| { | ||
| Task<bool> ValidateAsync(User user, CustomValidatorRequestContext requestContext); | ||
| } | ||
|
|
||
| public class ClientVersionValidator( | ||
| ICurrentContext currentContext, | ||
| IGetMinimumClientVersionForUserQuery getMinimumClientVersionForUserQuery) | ||
| : IClientVersionValidator | ||
| { | ||
| private static readonly string UpgradeMessage = "Please update your app to continue using Bitwarden"; | ||
|
|
||
| public async Task<bool> ValidateAsync(User? user, CustomValidatorRequestContext requestContext) | ||
|
Contributor
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. ๐ญ The null check for
Which behavior is intentional here? If the current fail-open behavior is intentional for defense-in-depth, consider adding a comment explaining the reasoning. |
||
| { | ||
| if (user == null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var clientVersion = currentContext.ClientVersion; | ||
| var minVersion = await getMinimumClientVersionForUserQuery.Run(user); | ||
|
|
||
| // Fail-open if headers are missing or no restriction | ||
| if (minVersion == null) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (clientVersion < minVersion) | ||
| { | ||
| requestContext.ValidationErrorResult = new ValidationResult | ||
| { | ||
| Error = "invalid_client_version", | ||
| ErrorDescription = UpgradeMessage, | ||
| IsError = true | ||
| }; | ||
| requestContext.CustomResponse = new Dictionary<string, object> | ||
| { | ||
| { "ErrorModel", new ErrorResponseModel(UpgradeMessage) } | ||
| }; | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.