mirror of
https://github.com/bitwarden/server.git
synced 2024-12-22 16:57:36 +01:00
[PM-9826] Remove validation from 2fa GET and mask sensitive data (#4526)
* remove validation from 2fa GET and mask sensitive data
* skip verification check on put email
* disable verification on send-email and reenable on put email
* validate authenticator on set instead of get
* Revert "validate authenticator on set instead of get"
This reverts commit 7bf2084531
.
* fix tests
* fix more tests
* Narrow scope of verify bypass
* Defaulted to false on VerifySecretAsync
* fix default param value
---------
Co-authored-by: Ike Kottlowski <ikottlowski@bitwarden.com>
Co-authored-by: Todd Martin <tmartin@bitwarden.com>
This commit is contained in:
parent
4f4750a0a6
commit
091c03a90c
@ -93,7 +93,7 @@ public class TwoFactorController : Controller
|
||||
public async Task<TwoFactorAuthenticatorResponseModel> GetAuthenticator(
|
||||
[FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, false);
|
||||
var user = await CheckAsync(model, false, false);
|
||||
var response = new TwoFactorAuthenticatorResponseModel(user);
|
||||
return response;
|
||||
}
|
||||
@ -121,7 +121,7 @@ public class TwoFactorController : Controller
|
||||
[HttpPost("get-yubikey")]
|
||||
public async Task<TwoFactorYubiKeyResponseModel> GetYubiKey([FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, true);
|
||||
var user = await CheckAsync(model, true, false);
|
||||
var response = new TwoFactorYubiKeyResponseModel(user);
|
||||
return response;
|
||||
}
|
||||
@ -147,7 +147,7 @@ public class TwoFactorController : Controller
|
||||
[HttpPost("get-duo")]
|
||||
public async Task<TwoFactorDuoResponseModel> GetDuo([FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, true);
|
||||
var user = await CheckAsync(model, true, false);
|
||||
var response = new TwoFactorDuoResponseModel(user);
|
||||
return response;
|
||||
}
|
||||
@ -187,7 +187,7 @@ public class TwoFactorController : Controller
|
||||
public async Task<TwoFactorDuoResponseModel> GetOrganizationDuo(string id,
|
||||
[FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
await CheckAsync(model, false);
|
||||
await CheckAsync(model, false, false);
|
||||
|
||||
var orgIdGuid = new Guid(id);
|
||||
if (!await _currentContext.ManagePolicies(orgIdGuid))
|
||||
@ -244,7 +244,7 @@ public class TwoFactorController : Controller
|
||||
[HttpPost("get-webauthn")]
|
||||
public async Task<TwoFactorWebAuthnResponseModel> GetWebAuthn([FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, false);
|
||||
var user = await CheckAsync(model, false, false);
|
||||
var response = new TwoFactorWebAuthnResponseModel(user);
|
||||
return response;
|
||||
}
|
||||
@ -253,7 +253,7 @@ public class TwoFactorController : Controller
|
||||
[ApiExplorerSettings(IgnoreApi = true)] // Disable Swagger due to CredentialCreateOptions not converting properly
|
||||
public async Task<CredentialCreateOptions> GetWebAuthnChallenge([FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, false);
|
||||
var user = await CheckAsync(model, false, false);
|
||||
var reg = await _userService.StartWebAuthnRegistrationAsync(user);
|
||||
return reg;
|
||||
}
|
||||
@ -288,7 +288,7 @@ public class TwoFactorController : Controller
|
||||
[HttpPost("get-email")]
|
||||
public async Task<TwoFactorEmailResponseModel> GetEmail([FromBody] SecretVerificationRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, false);
|
||||
var user = await CheckAsync(model, false, false);
|
||||
var response = new TwoFactorEmailResponseModel(user);
|
||||
return response;
|
||||
}
|
||||
@ -296,7 +296,7 @@ public class TwoFactorController : Controller
|
||||
[HttpPost("send-email")]
|
||||
public async Task SendEmail([FromBody] TwoFactorEmailRequestModel model)
|
||||
{
|
||||
var user = await CheckAsync(model, false);
|
||||
var user = await CheckAsync(model, false, false);
|
||||
model.ToUser(user);
|
||||
await _userService.SendTwoFactorEmailAsync(user);
|
||||
}
|
||||
@ -433,7 +433,7 @@ public class TwoFactorController : Controller
|
||||
return Task.FromResult(new DeviceVerificationResponseModel(false, false));
|
||||
}
|
||||
|
||||
private async Task<User> CheckAsync(SecretVerificationRequestModel model, bool premium)
|
||||
private async Task<User> CheckAsync(SecretVerificationRequestModel model, bool premium, bool isSetMethod = true)
|
||||
{
|
||||
var user = await _userService.GetUserByPrincipalAsync(User);
|
||||
if (user == null)
|
||||
@ -441,7 +441,7 @@ public class TwoFactorController : Controller
|
||||
throw new UnauthorizedAccessException();
|
||||
}
|
||||
|
||||
if (!await _userService.VerifySecretAsync(user, model.Secret))
|
||||
if (!await _userService.VerifySecretAsync(user, model.Secret, isSetMethod))
|
||||
{
|
||||
await Task.Delay(2000);
|
||||
throw new BadRequestException(string.Empty, "User verification failed.");
|
||||
|
@ -59,8 +59,8 @@ public class TwoFactorDuoResponseModel : ResponseModel
|
||||
// check Skey and IKey first if they exist
|
||||
if (provider.MetaData.TryGetValue("SKey", out var sKey))
|
||||
{
|
||||
ClientSecret = (string)sKey;
|
||||
SecretKey = (string)sKey;
|
||||
ClientSecret = MaskKey((string)sKey);
|
||||
SecretKey = MaskKey((string)sKey);
|
||||
}
|
||||
if (provider.MetaData.TryGetValue("IKey", out var iKey))
|
||||
{
|
||||
@ -73,8 +73,8 @@ public class TwoFactorDuoResponseModel : ResponseModel
|
||||
{
|
||||
if (!string.IsNullOrWhiteSpace((string)clientSecret))
|
||||
{
|
||||
ClientSecret = (string)clientSecret;
|
||||
SecretKey = (string)clientSecret;
|
||||
ClientSecret = MaskKey((string)clientSecret);
|
||||
SecretKey = MaskKey((string)clientSecret);
|
||||
}
|
||||
}
|
||||
if (provider.MetaData.TryGetValue("ClientId", out var clientId))
|
||||
@ -114,4 +114,15 @@ public class TwoFactorDuoResponseModel : ResponseModel
|
||||
throw new InvalidDataException("Invalid Duo parameters.");
|
||||
}
|
||||
}
|
||||
|
||||
private static string MaskKey(string key)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(key) || key.Length <= 6)
|
||||
{
|
||||
return key;
|
||||
}
|
||||
|
||||
// Mask all but the first 6 characters.
|
||||
return string.Concat(key.AsSpan(0, 6), new string('*', key.Length - 6));
|
||||
}
|
||||
}
|
||||
|
@ -75,7 +75,7 @@ public interface IUserService
|
||||
string GetUserName(ClaimsPrincipal principal);
|
||||
Task SendOTPAsync(User user);
|
||||
Task<bool> VerifyOTPAsync(User user, string token);
|
||||
Task<bool> VerifySecretAsync(User user, string secret);
|
||||
Task<bool> VerifySecretAsync(User user, string secret, bool isSettingMFA = false);
|
||||
|
||||
|
||||
void SetTwoFactorProvider(User user, TwoFactorProviderType type, bool setEnabled = true);
|
||||
|
@ -1342,7 +1342,7 @@ public class UserService : UserManager<User>, IUserService, IDisposable
|
||||
"otp:" + user.Email, token);
|
||||
}
|
||||
|
||||
public async Task<bool> VerifySecretAsync(User user, string secret)
|
||||
public async Task<bool> VerifySecretAsync(User user, string secret, bool isSettingMFA = false)
|
||||
{
|
||||
bool isVerified;
|
||||
if (user.HasMasterPassword())
|
||||
@ -1354,6 +1354,12 @@ public class UserService : UserManager<User>, IUserService, IDisposable
|
||||
isVerified = await CheckPasswordAsync(user, secret) ||
|
||||
await VerifyOTPAsync(user, secret);
|
||||
}
|
||||
else if (isSettingMFA)
|
||||
{
|
||||
// this is temporary to allow users to view their MFA settings without invalidating email TOTP
|
||||
// Will be removed with PM-9925
|
||||
isVerified = true;
|
||||
}
|
||||
else
|
||||
{
|
||||
// If they don't have a password at all they can only do OTP
|
||||
|
@ -21,9 +21,9 @@ public class OrganizationTwoFactorDuoResponseModelTests
|
||||
// Assert if v4 data Ikey and Skey are set to clientId and clientSecret
|
||||
Assert.NotNull(model);
|
||||
Assert.Equal("clientId", model.ClientId);
|
||||
Assert.Equal("clientSecret", model.ClientSecret);
|
||||
Assert.Equal("secret************", model.ClientSecret);
|
||||
Assert.Equal("clientId", model.IntegrationKey);
|
||||
Assert.Equal("clientSecret", model.SecretKey);
|
||||
Assert.Equal("secret************", model.SecretKey);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
@ -57,9 +57,9 @@ public class OrganizationTwoFactorDuoResponseModelTests
|
||||
/// Assert Even if both versions are present priority is given to v4 data
|
||||
Assert.NotNull(model);
|
||||
Assert.Equal("clientId", model.ClientId);
|
||||
Assert.Equal("clientSecret", model.ClientSecret);
|
||||
Assert.Equal("secret************", model.ClientSecret);
|
||||
Assert.Equal("clientId", model.IntegrationKey);
|
||||
Assert.Equal("clientSecret", model.SecretKey);
|
||||
Assert.Equal("secret************", model.SecretKey);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
@ -92,12 +92,14 @@ public class OrganizationTwoFactorDuoResponseModelTests
|
||||
|
||||
private string GetTwoFactorOrganizationDuoProvidersJson()
|
||||
{
|
||||
return "{\"6\":{\"Enabled\":true,\"MetaData\":{\"SKey\":\"SKey\",\"IKey\":\"IKey\",\"ClientSecret\":\"clientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
return
|
||||
"{\"6\":{\"Enabled\":true,\"MetaData\":{\"SKey\":\"SKey\",\"IKey\":\"IKey\",\"ClientSecret\":\"secretClientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
}
|
||||
|
||||
private string GetTwoFactorOrganizationDuoV4ProvidersJson()
|
||||
{
|
||||
return "{\"6\":{\"Enabled\":true,\"MetaData\":{\"ClientSecret\":\"clientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
return
|
||||
"{\"6\":{\"Enabled\":true,\"MetaData\":{\"ClientSecret\":\"secretClientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
}
|
||||
|
||||
private string GetTwoFactorOrganizationDuoV2ProvidersJson()
|
||||
|
@ -21,9 +21,9 @@ public class UserTwoFactorDuoResponseModelTests
|
||||
// Assert if v4 data Ikey and Skey are set to clientId and clientSecret
|
||||
Assert.NotNull(model);
|
||||
Assert.Equal("clientId", model.ClientId);
|
||||
Assert.Equal("clientSecret", model.ClientSecret);
|
||||
Assert.Equal("secret************", model.ClientSecret);
|
||||
Assert.Equal("clientId", model.IntegrationKey);
|
||||
Assert.Equal("clientSecret", model.SecretKey);
|
||||
Assert.Equal("secret************", model.SecretKey);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
@ -57,9 +57,9 @@ public class UserTwoFactorDuoResponseModelTests
|
||||
// Assert Even if both versions are present priority is given to v4 data
|
||||
Assert.NotNull(model);
|
||||
Assert.Equal("clientId", model.ClientId);
|
||||
Assert.Equal("clientSecret", model.ClientSecret);
|
||||
Assert.Equal("secret************", model.ClientSecret);
|
||||
Assert.Equal("clientId", model.IntegrationKey);
|
||||
Assert.Equal("clientSecret", model.SecretKey);
|
||||
Assert.Equal("secret************", model.SecretKey);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
@ -92,12 +92,14 @@ public class UserTwoFactorDuoResponseModelTests
|
||||
|
||||
private string GetTwoFactorDuoProvidersJson()
|
||||
{
|
||||
return "{\"2\":{\"Enabled\":true,\"MetaData\":{\"SKey\":\"SKey\",\"IKey\":\"IKey\",\"ClientSecret\":\"clientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
return
|
||||
"{\"2\":{\"Enabled\":true,\"MetaData\":{\"SKey\":\"SKey\",\"IKey\":\"IKey\",\"ClientSecret\":\"secretClientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
}
|
||||
|
||||
private string GetTwoFactorDuoV4ProvidersJson()
|
||||
{
|
||||
return "{\"2\":{\"Enabled\":true,\"MetaData\":{\"ClientSecret\":\"clientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
return
|
||||
"{\"2\":{\"Enabled\":true,\"MetaData\":{\"ClientSecret\":\"secretClientSecret\",\"ClientId\":\"clientId\",\"Host\":\"example.com\"}}}";
|
||||
}
|
||||
|
||||
private string GetTwoFactorDuoV2ProvidersJson()
|
||||
|
Loading…
Reference in New Issue
Block a user