Skip to content

Commit

Permalink
[PM-8107] Remove Duo v2 from server (#4934)
Browse files Browse the repository at this point in the history 
refactor(TwoFactorAuthentication): Remove references to old Duo SDK version 2 code and replace them with the Duo SDK version 4 supported library DuoUniversal code.

Increased unit test coverage in the Two Factor Authentication code space. We opted to use DI instead of Inheritance for the Duo and OrganizaitonDuo two factor tokens to increase testability, since creating a testing mock of the Duo.Client was non-trivial.

Reviewed-by: @JaredSnider-Bitwarden
  • Loading branch information
@ike-kottlowski
ike-kottlowski authored Nov 18, 2024
1 parent e16cad5 commit ab5d473
Show file tree
Hide file tree
Showing 36 changed files with 1,411 additions and 1,368 deletions.
44 changes: 6 additions & 38 deletions src/Api/Auth/Controllers/TwoFactorController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@
using Bit.Api.Models.Request;
using Bit.Api.Models.Response;
using Bit.Core.Auth.Enums;
using Bit.Core.Auth.Identity.TokenProviders;
using Bit.Core.Auth.LoginFeatures.PasswordlessLogin.Interfaces;
using Bit.Core.Auth.Models.Business.Tokenables;
using Bit.Core.Auth.Utilities;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Tokens;
using Bit.Core.Utilities;
using Fido2NetLib;
Expand All @@ -29,34 +28,31 @@ public class TwoFactorController : Controller
private readonly IUserService _userService;
private readonly IOrganizationRepository _organizationRepository;
private readonly IOrganizationService _organizationService;
private readonly GlobalSettings _globalSettings;
private readonly UserManager<User> _userManager;
private readonly ICurrentContext _currentContext;
private readonly IVerifyAuthRequestCommand _verifyAuthRequestCommand;
private readonly IFeatureService _featureService;
private readonly IDuoUniversalTokenService _duoUniversalTokenService;
private readonly IDataProtectorTokenFactory<TwoFactorAuthenticatorUserVerificationTokenable> _twoFactorAuthenticatorDataProtector;
private readonly IDataProtectorTokenFactory<SsoEmail2faSessionTokenable> _ssoEmailTwoFactorSessionDataProtector;

public TwoFactorController(
IUserService userService,
IOrganizationRepository organizationRepository,
IOrganizationService organizationService,
GlobalSettings globalSettings,
UserManager<User> userManager,
ICurrentContext currentContext,
IVerifyAuthRequestCommand verifyAuthRequestCommand,
IFeatureService featureService,
IDuoUniversalTokenService duoUniversalConfigService,
IDataProtectorTokenFactory<TwoFactorAuthenticatorUserVerificationTokenable> twoFactorAuthenticatorDataProtector,
IDataProtectorTokenFactory<SsoEmail2faSessionTokenable> ssoEmailTwoFactorSessionDataProtector)
{
_userService = userService;
_organizationRepository = organizationRepository;
_organizationService = organizationService;
_globalSettings = globalSettings;
_userManager = userManager;
_currentContext = currentContext;
_verifyAuthRequestCommand = verifyAuthRequestCommand;
_featureService = featureService;
_duoUniversalTokenService = duoUniversalConfigService;
_twoFactorAuthenticatorDataProtector = twoFactorAuthenticatorDataProtector;
_ssoEmailTwoFactorSessionDataProtector = ssoEmailTwoFactorSessionDataProtector;
}
Expand Down Expand Up @@ -184,21 +180,7 @@ public async Task<TwoFactorDuoResponseModel> GetDuo([FromBody] SecretVerificatio
public async Task<TwoFactorDuoResponseModel> PutDuo([FromBody] UpdateTwoFactorDuoRequestModel model)
{
var user = await CheckAsync(model, true);
try
{
// for backwards compatibility - will be removed with PM-8107
DuoApi duoApi = null;
if (model.ClientId != null && model.ClientSecret != null)
{
duoApi = new DuoApi(model.ClientId, model.ClientSecret, model.Host);
}
else
{
duoApi = new DuoApi(model.IntegrationKey, model.SecretKey, model.Host);
}
await duoApi.JSONApiCall("GET", "/auth/v2/check");
}
catch (DuoException)
if (!await _duoUniversalTokenService.ValidateDuoConfiguration(model.ClientSecret, model.ClientId, model.Host))
{
throw new BadRequestException(
"Duo configuration settings are not valid. Please re-check the Duo Admin panel.");
Expand Down Expand Up @@ -241,21 +223,7 @@ public async Task<TwoFactorDuoResponseModel> PutOrganizationDuo(string id,
}

var organization = await _organizationRepository.GetByIdAsync(orgIdGuid) ?? throw new NotFoundException();
try
{
// for backwards compatibility - will be removed with PM-8107
DuoApi duoApi = null;
if (model.ClientId != null && model.ClientSecret != null)
{
duoApi = new DuoApi(model.ClientId, model.ClientSecret, model.Host);
}
else
{
duoApi = new DuoApi(model.IntegrationKey, model.SecretKey, model.Host);
}
await duoApi.JSONApiCall("GET", "/auth/v2/check");
}
catch (DuoException)
if (!await _duoUniversalTokenService.ValidateDuoConfiguration(model.ClientId, model.ClientSecret, model.Host))
{
throw new BadRequestException(
"Duo configuration settings are not valid. Please re-check the Duo Admin panel.");
Expand Down
63 changes: 18 additions & 45 deletions src/Api/Auth/Models/Request/TwoFactorRequestModels.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
using Bit.Api.Auth.Models.Request.Accounts;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.Auth.Enums;
using Bit.Core.Auth.Identity.TokenProviders;
using Bit.Core.Auth.Models;
using Bit.Core.Auth.Utilities;
using Bit.Core.Entities;
using Fido2NetLib;

Expand Down Expand Up @@ -43,44 +43,34 @@ public User ToUser(User existingUser)
public class UpdateTwoFactorDuoRequestModel : SecretVerificationRequestModel, IValidatableObject
{
/*
To support both v2 and v4 we need to remove the required annotation from the properties.
todo - the required annotation will be added back in PM-8107.
String lengths based on Duo's documentation
https://github.com/duosecurity/duo_universal_csharp/blob/main/DuoUniversal/Client.cs
*/
[StringLength(50)]
[Required]
[StringLength(20, MinimumLength = 20, ErrorMessage = "Client Id must be exactly 20 characters.")]
public string ClientId { get; set; }
[StringLength(50)]
[Required]
[StringLength(40, MinimumLength = 40, ErrorMessage = "Client Secret must be exactly 40 characters.")]
public string ClientSecret { get; set; }
//todo - will remove SKey and IKey with PM-8107
[StringLength(50)]
public string IntegrationKey { get; set; }
//todo - will remove SKey and IKey with PM-8107
[StringLength(50)]
public string SecretKey { get; set; }
[Required]
[StringLength(50)]
public string Host { get; set; }

public User ToUser(User existingUser)
{
var providers = existingUser.GetTwoFactorProviders();
if (providers == null)
{
providers = new Dictionary<TwoFactorProviderType, TwoFactorProvider>();
providers = [];
}
else if (providers.ContainsKey(TwoFactorProviderType.Duo))
{
providers.Remove(TwoFactorProviderType.Duo);
}

Temporary_SyncDuoParams();

providers.Add(TwoFactorProviderType.Duo, new TwoFactorProvider
{
MetaData = new Dictionary<string, object>
{
//todo - will remove SKey and IKey with PM-8107
["SKey"] = SecretKey,
["IKey"] = IntegrationKey,
["ClientSecret"] = ClientSecret,
["ClientId"] = ClientId,
["Host"] = Host
Expand All @@ -96,22 +86,17 @@ public Organization ToOrganization(Organization existingOrg)
var providers = existingOrg.GetTwoFactorProviders();
if (providers == null)
{
providers = new Dictionary<TwoFactorProviderType, TwoFactorProvider>();
providers = [];
}
else if (providers.ContainsKey(TwoFactorProviderType.OrganizationDuo))
{
providers.Remove(TwoFactorProviderType.OrganizationDuo);
}

Temporary_SyncDuoParams();

providers.Add(TwoFactorProviderType.OrganizationDuo, new TwoFactorProvider
{
MetaData = new Dictionary<string, object>
{
//todo - will remove SKey and IKey with PM-8107
["SKey"] = SecretKey,
["IKey"] = IntegrationKey,
["ClientSecret"] = ClientSecret,
["ClientId"] = ClientId,
["Host"] = Host
Expand All @@ -124,34 +109,22 @@ public Organization ToOrganization(Organization existingOrg)

public override IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
if (!DuoApi.ValidHost(Host))
var results = new List<ValidationResult>();
if (string.IsNullOrWhiteSpace(ClientId))
{
yield return new ValidationResult("Host is invalid.", [nameof(Host)]);
results.Add(new ValidationResult("ClientId is required.", [nameof(ClientId)]));
}
if (string.IsNullOrWhiteSpace(ClientSecret) && string.IsNullOrWhiteSpace(ClientId) &&
string.IsNullOrWhiteSpace(SecretKey) && string.IsNullOrWhiteSpace(IntegrationKey))
{
yield return new ValidationResult("Neither v2 or v4 values are valid.", [nameof(IntegrationKey), nameof(SecretKey), nameof(ClientSecret), nameof(ClientId)]);
}
}

/*
use this method to ensure that both v2 params and v4 params are in sync
todo will be removed in pm-8107
*/
private void Temporary_SyncDuoParams()
{
// Even if IKey and SKey exist prioritize v4 params ClientId and ClientSecret
if (!string.IsNullOrWhiteSpace(ClientSecret) && !string.IsNullOrWhiteSpace(ClientId))
if (string.IsNullOrWhiteSpace(ClientSecret))
{
SecretKey = ClientSecret;
IntegrationKey = ClientId;
results.Add(new ValidationResult("ClientSecret is required.", [nameof(ClientSecret)]));
}
else if (!string.IsNullOrWhiteSpace(SecretKey) && !string.IsNullOrWhiteSpace(IntegrationKey))

if (string.IsNullOrWhiteSpace(Host) || !DuoUniversalTokenService.ValidDuoHost(Host))
{
ClientSecret = SecretKey;
ClientId = IntegrationKey;
results.Add(new ValidationResult("Host is invalid.", [nameof(Host)]));
}
return results;
}
}

Expand Down
71 changes: 7 additions & 64 deletions src/Api/Auth/Models/Response/TwoFactor/TwoFactorDuoResponseModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,37 +13,26 @@ public class TwoFactorDuoResponseModel : ResponseModel
public TwoFactorDuoResponseModel(User user)
: base(ResponseObj)
{
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}
ArgumentNullException.ThrowIfNull(user);

var provider = user.GetTwoFactorProvider(TwoFactorProviderType.Duo);
Build(provider);
}

public TwoFactorDuoResponseModel(Organization org)
public TwoFactorDuoResponseModel(Organization organization)
: base(ResponseObj)
{
if (org == null)
{
throw new ArgumentNullException(nameof(org));
}
ArgumentNullException.ThrowIfNull(organization);

var provider = org.GetTwoFactorProvider(TwoFactorProviderType.OrganizationDuo);
var provider = organization.GetTwoFactorProvider(TwoFactorProviderType.OrganizationDuo);
Build(provider);
}

public bool Enabled { get; set; }
public string Host { get; set; }
//TODO - will remove SecretKey with PM-8107
public string SecretKey { get; set; }
//TODO - will remove IntegrationKey with PM-8107
public string IntegrationKey { get; set; }
public string ClientSecret { get; set; }
public string ClientId { get; set; }

// updated build to assist in the EDD migration for the Duo 2FA provider
private void Build(TwoFactorProvider provider)
{
if (provider?.MetaData != null && provider.MetaData.Count > 0)
Expand All @@ -54,36 +43,13 @@ private void Build(TwoFactorProvider provider)
{
Host = (string)host;
}

//todo - will remove SKey and IKey with PM-8107
// check Skey and IKey first if they exist
if (provider.MetaData.TryGetValue("SKey", out var sKey))
{
ClientSecret = MaskKey((string)sKey);
SecretKey = MaskKey((string)sKey);
}
if (provider.MetaData.TryGetValue("IKey", out var iKey))
{
IntegrationKey = (string)iKey;
ClientId = (string)iKey;
}

// Even if IKey and SKey exist prioritize v4 params ClientId and ClientSecret
if (provider.MetaData.TryGetValue("ClientSecret", out var clientSecret))
{
if (!string.IsNullOrWhiteSpace((string)clientSecret))
{
ClientSecret = MaskKey((string)clientSecret);
SecretKey = MaskKey((string)clientSecret);
}
ClientSecret = MaskSecret((string)clientSecret);
}
if (provider.MetaData.TryGetValue("ClientId", out var clientId))
{
if (!string.IsNullOrWhiteSpace((string)clientId))
{
ClientId = (string)clientId;
IntegrationKey = (string)clientId;
}
ClientId = (string)clientId;
}
}
else
Expand All @@ -92,30 +58,7 @@ private void Build(TwoFactorProvider provider)
}
}

/*
use this method to ensure that both v2 params and v4 params are in sync
todo will be removed in pm-8107
*/
private void Temporary_SyncDuoParams()
{
// Even if IKey and SKey exist prioritize v4 params ClientId and ClientSecret
if (!string.IsNullOrWhiteSpace(ClientSecret) && !string.IsNullOrWhiteSpace(ClientId))
{
SecretKey = ClientSecret;
IntegrationKey = ClientId;
}
else if (!string.IsNullOrWhiteSpace(SecretKey) && !string.IsNullOrWhiteSpace(IntegrationKey))
{
ClientSecret = SecretKey;
ClientId = IntegrationKey;
}
else
{
throw new InvalidDataException("Invalid Duo parameters.");
}
}

private static string MaskKey(string key)
private static string MaskSecret(string key)
{
if (string.IsNullOrWhiteSpace(key) || key.Length <= 6)
{
Expand Down
3 changes: 1 addition & 2 deletions src/Api/Startup.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
using Bit.SharedWeb.Utilities;
using Microsoft.AspNetCore.Diagnostics.HealthChecks;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Bit.Core.Auth.Identity;
using Bit.Core.Auth.UserFeatures;
using Bit.Core.Entities;
using Bit.Core.Billing.Extensions;
Expand All @@ -32,10 +31,10 @@
using Bit.Core.Vault.Entities;
using Bit.Api.Auth.Models.Request.WebAuthn;
using Bit.Core.Auth.Models.Data;
using Bit.Core.Auth.Identity.TokenProviders;
using Bit.Core.Tools.ReportFeatures;



#if !OSS
using Bit.Commercial.Core.SecretsManager;
using Bit.Commercial.Core.Utilities;
Expand Down
Loading

0 comments on commit ab5d473

Please sign in to comment.