-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pm 2032 troubleshoot actions #65
base: main
Are you sure you want to change the base?
Changes from all commits
d91e22e
9dc24de
f33570e
bd01206
0abba03
6d4aeff
d63588a
f2c6b03
26da1b2
ec0c93f
94073fa
07683f1
cd5635f
e3fcc5e
38f48e2
155e5d0
84f7fc8
d11fc46
3bda447
8101435
9580f7b
06b223c
eae9fa6
72420d0
97cfd1e
083b342
69af190
120b182
a28ea69
1ffd97e
0f44dda
3a738e3
70533a2
4f2ec30
17ff52d
15a5e06
367f58d
f601312
e79e295
ec6e30a
6c04eee
fa0bdf6
a38f719
f607774
1612e24
673f9fd
ed4b85c
03bb729
d62937a
cdabd17
5c4b14f
c5aedc9
9cb7b57
6f7efaa
ca10935
8087dda
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,112 @@ | ||
using Bit.Api.Auth.Models.Request.Accounts; | ||
using Bit.Api.Auth.Models.Request.Webauthn; | ||
using Bit.Api.Auth.Models.Response.WebAuthn; | ||
using Bit.Api.Models.Response; | ||
using Bit.Core; | ||
using Bit.Core.Auth.Models.Business.Tokenables; | ||
using Bit.Core.Auth.Repositories; | ||
using Bit.Core.Exceptions; | ||
using Bit.Core.Services; | ||
using Bit.Core.Tokens; | ||
using Bit.Core.Utilities; | ||
using Microsoft.AspNetCore.Authorization; | ||
using Microsoft.AspNetCore.Mvc; | ||
|
||
namespace Bit.Api.Auth.Controllers; | ||
|
||
[Route("webauthn")] | ||
[Authorize("Web")] | ||
[RequireFeature(FeatureFlagKeys.PasswordlessLogin)] | ||
public class WebAuthnController : Controller | ||
{ | ||
private readonly IUserService _userService; | ||
private readonly IWebAuthnCredentialRepository _credentialRepository; | ||
private readonly IDataProtectorTokenFactory<WebAuthnCredentialCreateOptionsTokenable> _createOptionsDataProtector; | ||
|
||
public WebAuthnController( | ||
IUserService userService, | ||
IWebAuthnCredentialRepository credentialRepository, | ||
IDataProtectorTokenFactory<WebAuthnCredentialCreateOptionsTokenable> createOptionsDataProtector) | ||
{ | ||
_userService = userService; | ||
_credentialRepository = credentialRepository; | ||
_createOptionsDataProtector = createOptionsDataProtector; | ||
} | ||
|
||
[HttpGet("")] | ||
public async Task<ListResponseModel<WebAuthnCredentialResponseModel>> Get() | ||
{ | ||
var user = await GetUserAsync(); | ||
var credentials = await _credentialRepository.GetManyByUserIdAsync(user.Id); | ||
|
||
return new ListResponseModel<WebAuthnCredentialResponseModel>(credentials.Select(c => new WebAuthnCredentialResponseModel(c))); | ||
} | ||
|
||
[HttpPost("options")] | ||
public async Task<WebAuthnCredentialCreateOptionsResponseModel> PostOptions([FromBody] SecretVerificationRequestModel model) | ||
{ | ||
var user = await VerifyUserAsync(model); | ||
var options = await _userService.StartWebAuthnLoginRegistrationAsync(user); | ||
|
||
var tokenable = new WebAuthnCredentialCreateOptionsTokenable(user, options); | ||
var token = _createOptionsDataProtector.Protect(tokenable); | ||
|
||
return new WebAuthnCredentialCreateOptionsResponseModel | ||
{ | ||
Options = options, | ||
Token = token | ||
}; | ||
} | ||
|
||
[HttpPost("")] | ||
public async Task Post([FromBody] WebAuthnCredentialRequestModel model) | ||
{ | ||
var user = await GetUserAsync(); | ||
var tokenable = _createOptionsDataProtector.Unprotect(model.Token); | ||
if (!tokenable.TokenIsValid(user)) | ||
{ | ||
throw new BadRequestException("The token associated with your request is expired. A valid token is required to continue."); | ||
} | ||
|
||
var success = await _userService.CompleteWebAuthLoginRegistrationAsync(user, model.Name, model.SupportsPrf, model.EncryptedUserKey, model.EncryptedPublicKey, model.EncryptedPrivateKey, tokenable.Options, model.DeviceResponse); | ||
if (!success) | ||
{ | ||
throw new BadRequestException("Unable to complete WebAuthn registration."); | ||
} | ||
} | ||
|
||
[HttpPost("{id}/delete")] | ||
public async Task Delete(Guid id, [FromBody] SecretVerificationRequestModel model) | ||
{ | ||
var user = await VerifyUserAsync(model); | ||
var credential = await _credentialRepository.GetByIdAsync(id, user.Id); | ||
if (credential == null) | ||
{ | ||
throw new NotFoundException("Credential not found."); | ||
} | ||
|
||
await _credentialRepository.DeleteAsync(credential); | ||
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. logic: Add error handling for the DeleteAsync operation |
||
} | ||
|
||
private async Task<Core.Entities.User> GetUserAsync() | ||
{ | ||
var user = await _userService.GetUserByPrincipalAsync(User); | ||
if (user == null) | ||
{ | ||
throw new UnauthorizedAccessException(); | ||
} | ||
return user; | ||
} | ||
|
||
private async Task<Core.Entities.User> VerifyUserAsync(SecretVerificationRequestModel model) | ||
{ | ||
var user = await GetUserAsync(); | ||
if (!await _userService.VerifySecretAsync(user, model.Secret)) | ||
{ | ||
await Task.Delay(Constants.FailedSecretVerificationDelay); | ||
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. logic: Potential timing attack vulnerability. Consider using a constant-time comparison for secret verification |
||
throw new BadRequestException(string.Empty, "User verification failed."); | ||
} | ||
|
||
return user; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
using Bit.Core.Utilities; | ||
using Fido2NetLib; | ||
|
||
namespace Bit.Api.Auth.Models.Request.Webauthn; | ||
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. style: Namespace 'Webauthn' is inconsistent with C# naming conventions. Consider changing to 'WebAuthn'. |
||
|
||
public class WebAuthnCredentialRequestModel | ||
{ | ||
[Required] | ||
public AuthenticatorAttestationRawResponse DeviceResponse { get; set; } | ||
|
||
[Required] | ||
public string Name { get; set; } | ||
|
||
[Required] | ||
public string Token { get; set; } | ||
|
||
[Required] | ||
public bool SupportsPrf { get; set; } | ||
|
||
[EncryptedString] | ||
[EncryptedStringLength(2000)] | ||
public string EncryptedUserKey { get; set; } | ||
|
||
[EncryptedString] | ||
[EncryptedStringLength(2000)] | ||
public string EncryptedPublicKey { get; set; } | ||
|
||
[EncryptedString] | ||
[EncryptedStringLength(2000)] | ||
public string EncryptedPrivateKey { get; set; } | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
using Bit.Core.Models.Api; | ||
using Fido2NetLib; | ||
|
||
namespace Bit.Api.Auth.Models.Response.WebAuthn; | ||
|
||
public class WebAuthnCredentialCreateOptionsResponseModel : ResponseModel | ||
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. style: Consider adding XML documentation comments to describe the purpose of this class and its properties |
||
{ | ||
private const string ResponseObj = "webauthnCredentialCreateOptions"; | ||
|
||
public WebAuthnCredentialCreateOptionsResponseModel() : base(ResponseObj) | ||
{ | ||
} | ||
|
||
public CredentialCreateOptions Options { get; set; } | ||
public string Token { get; set; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
using Bit.Core.Auth.Entities; | ||
using Bit.Core.Auth.Enums; | ||
using Bit.Core.Models.Api; | ||
|
||
namespace Bit.Api.Auth.Models.Response.WebAuthn; | ||
|
||
public class WebAuthnCredentialResponseModel : ResponseModel | ||
{ | ||
private const string ResponseObj = "webauthnCredential"; | ||
|
||
public WebAuthnCredentialResponseModel(WebAuthnCredential credential) : base(ResponseObj) | ||
{ | ||
Id = credential.Id.ToString(); | ||
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. style: Consider using Guid.ToString("N") for a more compact string representation without hyphens |
||
Name = credential.Name; | ||
PrfStatus = credential.GetPrfStatus(); | ||
} | ||
|
||
public string Id { get; set; } | ||
public string Name { get; set; } | ||
public WebAuthnPrfStatus PrfStatus { get; set; } | ||
Comment on lines
+18
to
+20
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. style: Properties should be read-only if they're only set in the constructor |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
using Bit.Core.Auth.Enums; | ||
using Bit.Core.Entities; | ||
using Bit.Core.Utilities; | ||
|
||
namespace Bit.Core.Auth.Entities; | ||
|
||
public class WebAuthnCredential : ITableObject<Guid> | ||
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. style: Consider adding XML documentation for the class to explain its purpose and usage |
||
{ | ||
public Guid Id { get; set; } | ||
public Guid UserId { get; set; } | ||
[MaxLength(50)] | ||
public string Name { get; set; } | ||
Comment on lines
+12
to
+13
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. style: Consider making Name property required using [Required] attribute |
||
[MaxLength(256)] | ||
public string PublicKey { get; set; } | ||
[MaxLength(256)] | ||
public string CredentialId { get; set; } | ||
public int Counter { get; set; } | ||
[MaxLength(20)] | ||
public string Type { get; set; } | ||
public Guid AaGuid { get; set; } | ||
[MaxLength(2000)] | ||
public string EncryptedUserKey { get; set; } | ||
[MaxLength(2000)] | ||
public string EncryptedPrivateKey { get; set; } | ||
[MaxLength(2000)] | ||
public string EncryptedPublicKey { get; set; } | ||
public bool SupportsPrf { get; set; } | ||
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. logic: SupportsPrf should be initialized to false by default |
||
public DateTime CreationDate { get; internal set; } = DateTime.UtcNow; | ||
public DateTime RevisionDate { get; internal set; } = DateTime.UtcNow; | ||
|
||
public void SetNewId() | ||
{ | ||
Id = CoreHelpers.GenerateComb(); | ||
} | ||
|
||
public WebAuthnPrfStatus GetPrfStatus() | ||
{ | ||
if (SupportsPrf && EncryptedUserKey != null && EncryptedPrivateKey != null && EncryptedPublicKey != null) | ||
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. style: Use null-coalescing operator (??) instead of null checks for better readability |
||
{ | ||
return WebAuthnPrfStatus.Enabled; | ||
} | ||
else if (SupportsPrf) | ||
{ | ||
return WebAuthnPrfStatus.Supported; | ||
} | ||
|
||
return WebAuthnPrfStatus.Unsupported; | ||
} | ||
Comment on lines
+37
to
+49
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. style: GetPrfStatus method could be simplified using pattern matching |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
namespace Bit.Core.Auth.Enums; | ||
|
||
public enum WebAuthnLoginAssertionOptionsScope | ||
{ | ||
Authentication = 0, | ||
PrfRegistration = 1 | ||
} | ||
Comment on lines
+3
to
+7
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. style: Consider adding XML documentation comments to describe the purpose of this enum and its values |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
namespace Bit.Core.Auth.Enums; | ||
|
||
public enum WebAuthnPrfStatus | ||
{ | ||
Enabled = 0, | ||
Supported = 1, | ||
Unsupported = 2 | ||
Comment on lines
+5
to
+7
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. logic: Verify that the order of these enum values aligns with the intended logic in the WebAuthn implementation |
||
} | ||
Comment on lines
+3
to
+8
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. style: Consider adding XML documentation comments to describe the purpose of this enum and each of its values |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
| ||
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. style: Remove this empty line at the beginning of the file. |
||
using Bit.Core.Models.Api; | ||
using Fido2NetLib; | ||
|
||
namespace Bit.Core.Auth.Models.Api.Response.Accounts; | ||
|
||
public class WebAuthnLoginAssertionOptionsResponseModel : ResponseModel | ||
{ | ||
private const string ResponseObj = "webAuthnLoginAssertionOptions"; | ||
|
||
public WebAuthnLoginAssertionOptionsResponseModel() : base(ResponseObj) | ||
{ | ||
} | ||
|
||
public AssertionOptions Options { get; set; } | ||
public string Token { get; set; } | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,12 @@ public UserDecryptionOptions() : base("userDecryptionOptions") | |
/// </summary> | ||
public bool HasMasterPassword { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the WebAuthn PRF decryption keys. | ||
/// </summary> | ||
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
public WebAuthnPrfDecryptionOption? WebAuthnPrfOptions { get; set; } | ||
Comment on lines
+22
to
+23
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. style: Consider using init-only property for WebAuthnPrfOptions to ensure immutability after initialization |
||
|
||
/// <summary> | ||
/// Gets or sets information regarding this users trusted device decryption setup. | ||
/// </summary> | ||
|
@@ -29,6 +35,20 @@ public UserDecryptionOptions() : base("userDecryptionOptions") | |
public KeyConnectorUserDecryptionOption? KeyConnectorOption { get; set; } | ||
} | ||
|
||
public class WebAuthnPrfDecryptionOption | ||
{ | ||
public string EncryptedPrivateKey { get; } | ||
public string EncryptedUserKey { get; } | ||
|
||
public WebAuthnPrfDecryptionOption( | ||
string encryptedPrivateKey, | ||
string encryptedUserKey) | ||
{ | ||
EncryptedPrivateKey = encryptedPrivateKey; | ||
EncryptedUserKey = encryptedUserKey; | ||
} | ||
} | ||
Comment on lines
+38
to
+50
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. style: Add XML documentation comments for WebAuthnPrfDecryptionOption class and its properties |
||
|
||
public class TrustedDeviceUserDecryptionOption | ||
{ | ||
public bool HasAdminApproval { get; } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
using System.Text.Json.Serialization; | ||
using Bit.Core.Entities; | ||
using Bit.Core.Tokens; | ||
using Fido2NetLib; | ||
|
||
namespace Bit.Core.Auth.Models.Business.Tokenables; | ||
|
||
public class WebAuthnCredentialCreateOptionsTokenable : ExpiringTokenable | ||
{ | ||
// 7 minutes = max webauthn timeout (6 minutes) + slack for miscellaneous delays | ||
private const double _tokenLifetimeInHours = (double)7 / 60; | ||
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. style: Consider using TimeSpan.FromMinutes(7) for clarity |
||
public const string ClearTextPrefix = "BWWebAuthnCredentialCreateOptions_"; | ||
public const string DataProtectorPurpose = "WebAuthnCredentialCreateDataProtector"; | ||
public const string TokenIdentifier = "WebAuthnCredentialCreateOptionsToken"; | ||
|
||
public string Identifier { get; set; } = TokenIdentifier; | ||
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. style: Identifier property can be made read-only |
||
public Guid? UserId { get; set; } | ||
public CredentialCreateOptions Options { get; set; } | ||
|
||
[JsonConstructor] | ||
public WebAuthnCredentialCreateOptionsTokenable() | ||
{ | ||
ExpirationDate = DateTime.UtcNow.AddHours(_tokenLifetimeInHours); | ||
} | ||
|
||
public WebAuthnCredentialCreateOptionsTokenable(User user, CredentialCreateOptions options) : this() | ||
{ | ||
UserId = user?.Id; | ||
Options = options; | ||
} | ||
|
||
public bool TokenIsValid(User user) | ||
{ | ||
if (!Valid || user == null) | ||
{ | ||
return false; | ||
} | ||
|
||
return UserId == user.Id; | ||
} | ||
Comment on lines
+32
to
+40
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. style: TokenIsValid(User) method may be redundant with base class Valid property |
||
|
||
protected override bool TokenIsValid() => Identifier == TokenIdentifier && UserId != null && Options != null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Add authorization check to ensure the user can only access their own credentials