-
Notifications
You must be signed in to change notification settings - Fork 0
Pm 2023 fido2 authentication #73
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
d91e22e
9dc24de
f33570e
bd01206
0abba03
6d4aeff
74e56ee
cdf531d
00d92ed
f8fd97e
f8ee4fa
ebec5cb
88dce90
556b758
ee32d5a
73b7a00
d63588a
f2c6b03
26da1b2
ec0c93f
258afae
6aff50a
e8ab5d7
85db96c
934dd82
74d8f2e
40e56ef
e718989
f17ea48
59704b9
75edd13
c7ee102
590713f
0a9544c
204a63f
97048ea
84cd8c7
158b20d
1205e9f
2f64464
91ff3e5
edc4840
c33249d
0233685
94073fa
1f1cbd5
7d3582d
8754e0a
4e8fd85
9b1ac4a
5d11d06
07683f1
cd5635f
e3fcc5e
38f48e2
3c7d2cc
7b358ad
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,110 @@ | ||
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 Microsoft.AspNetCore.Authorization; | ||
using Microsoft.AspNetCore.Mvc; | ||
|
||
namespace Bit.Api.Auth.Controllers; | ||
|
||
[Route("webauthn")] | ||
[Authorize("Web")] | ||
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.UserKey, model.PrfPublicKey, model.PrfPrivateKey, model.SupportsPrf, 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); | ||
} | ||
|
||
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); | ||
throw new BadRequestException(string.Empty, "User verification failed."); | ||
} | ||
|
||
return user; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
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: Consider using 'WebAuthn' instead of 'Webauthn' in the namespace for consistency with the class name |
||
|
||
public class WebAuthnCredentialRequestModel | ||
{ | ||
[Required] | ||
public AuthenticatorAttestationRawResponse DeviceResponse { get; set; } | ||
|
||
[Required] | ||
public string Name { 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. style: Add length constraint to Name property |
||
|
||
[Required] | ||
public string Token { get; set; } | ||
|
||
[Required] | ||
public bool SupportsPrf { get; set; } | ||
|
||
public string UserKey { get; set; } | ||
public string PrfPublicKey { get; set; } | ||
public string PrfPrivateKey { 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 and usage of this class |
||
{ | ||
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; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
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> | ||
{ | ||
public Guid Id { get; set; } | ||
public Guid UserId { get; set; } | ||
[MaxLength(50)] | ||
public string Name { get; set; } | ||
[MaxLength(256)] | ||
public string PublicKey { get; set; } | ||
[MaxLength(256)] | ||
public string DescriptorId { get; set; } | ||
public int Counter { get; set; } | ||
[MaxLength(20)] | ||
public string Type { get; set; } | ||
public Guid AaGuid { get; set; } | ||
public string UserKey { get; set; } | ||
public string PrfPublicKey { get; set; } | ||
public string PrfPrivateKey { get; set; } | ||
Comment on lines
+22
to
+24
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: UserKey, PrfPublicKey, and PrfPrivateKey should likely have [MaxLength] attributes |
||
public bool SupportsPrf { get; set; } | ||
public DateTime CreationDate { get; internal set; } = DateTime.UtcNow; | ||
public DateTime RevisionDate { get; internal set; } = DateTime.UtcNow; | ||
Comment on lines
+26
to
+27
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 DateTimeOffset instead of DateTime for better timezone handling |
||
|
||
public void SetNewId() | ||
{ | ||
Id = CoreHelpers.GenerateComb(); | ||
} | ||
|
||
public WebAuthnPrfStatus GetPrfStatus() | ||
{ | ||
if (SupportsPrf && PrfPublicKey != null && PrfPrivateKey != null) | ||
{ | ||
return WebAuthnPrfStatus.Enabled; | ||
} | ||
else if (SupportsPrf) | ||
{ | ||
return WebAuthnPrfStatus.Supported; | ||
} | ||
|
||
return WebAuthnPrfStatus.Unsupported; | ||
} | ||
Comment on lines
+34
to
+46
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 a switch expression |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
namespace Bit.Core.Auth.Enums; | ||
|
||
public enum WebAuthnPrfStatus | ||
{ | ||
Enabled = 0, | ||
Supported = 1, | ||
Unsupported = 2 | ||
} | ||
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 each enum value |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
|
||
namespace Bit.Core.Auth.Models.Api.Request.Accounts; | ||
|
||
public class WebauthnCredentialAssertionOptionsRequestModel | ||
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: Class name inconsistent with file name (Webauthn vs WebAuthn) |
||
{ | ||
[EmailAddress] | ||
[StringLength(256)] | ||
public string Email { 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. style: Email property should be nullable (string?) for C# 8.0+ nullable reference types compatibility |
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
using Fido2NetLib; | ||
|
||
namespace Bit.Core.Auth.Models.Api.Request.Accounts; | ||
|
||
public class WebauthnCredentialAssertionRequestModel | ||
{ | ||
[Required] | ||
public AuthenticatorAssertionRawResponse DeviceResponse { get; set; } | ||
|
||
[Required] | ||
public string Token { get; set; } | ||
} | ||
Comment on lines
+6
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 adding XML documentation comments to describe the purpose of this class and its properties |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
using Bit.Core.Models.Api; | ||
using Fido2NetLib; | ||
|
||
namespace Bit.Core.Auth.Models.Api.Response.Accounts; | ||
|
||
public class WebAuthnCredentialAssertionOptionsResponseModel : 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: Class could benefit from XML documentation for better API understanding |
||
{ | ||
private const string ResponseObj = "webauthnCredentialAssertionOptions"; | ||
|
||
public WebAuthnCredentialAssertionOptionsResponseModel() : base(ResponseObj) | ||
{ | ||
} | ||
|
||
public AssertionOptions Options { get; set; } | ||
public string Token { get; set; } | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
using Bit.Core.Models.Api; | ||
|
||
namespace Bit.Core.Auth.Models.Api.Response.Accounts; | ||
|
||
public class WebAuthnCredentialAssertionResponseModel : 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 for the class |
||
{ | ||
private const string ResponseObj = "webauthnCredentialAssertion"; | ||
|
||
public WebAuthnCredentialAssertionResponseModel() : base(ResponseObj) | ||
{ | ||
} | ||
|
||
public string Token { 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. style: Add XML documentation for the Token property |
||
} | ||
|
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 WebAuthnCredentialAssertionOptionsTokenable : 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 better readability |
||
public const string ClearTextPrefix = "BWWebAuthnCredentialAssertionOptions_"; | ||
public const string DataProtectorPurpose = "WebAuthnCredentialAssertionDataProtector"; | ||
public const string TokenIdentifier = "WebAuthnCredentialAssertionOptionsToken"; | ||
|
||
public string Identifier { get; set; } = TokenIdentifier; | ||
public Guid? UserId { get; set; } | ||
public AssertionOptions Options { get; set; } | ||
|
||
[JsonConstructor] | ||
public WebAuthnCredentialAssertionOptionsTokenable() | ||
{ | ||
ExpirationDate = DateTime.UtcNow.AddHours(_tokenLifetimeInHours); | ||
} | ||
|
||
public WebAuthnCredentialAssertionOptionsTokenable(User user, AssertionOptions options) : this() | ||
{ | ||
UserId = user?.Id; | ||
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 null reference exception if user is null |
||
Options = options; | ||
} | ||
|
||
public bool TokenIsValid(User user) | ||
{ | ||
if (!Valid || user == 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: Consider separating the null check for user to improve clarity |
||
{ | ||
return false; | ||
} | ||
|
||
return UserId == user.Id; | ||
} | ||
|
||
protected override bool TokenIsValid() => Identifier == TokenIdentifier && UserId != null && Options != null; | ||
} | ||
|
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; | ||
public const string ClearTextPrefix = "BWWebAuthnCredentialCreateOptions_"; | ||
public const string DataProtectorPurpose = "WebAuthnCredentialCreateDataProtector"; | ||
public const string TokenIdentifier = "WebAuthnCredentialCreateOptionsToken"; | ||
|
||
public string Identifier { get; set; } = TokenIdentifier; | ||
public Guid? UserId { get; set; } | ||
public CredentialCreateOptions Options { get; set; } | ||
|
||
[JsonConstructor] | ||
public WebAuthnCredentialCreateOptionsTokenable() | ||
{ | ||
ExpirationDate = DateTime.UtcNow.AddHours(_tokenLifetimeInHours); | ||
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 DateTimeOffset.UtcNow instead of DateTime.UtcNow for better timezone handling |
||
} | ||
|
||
public WebAuthnCredentialCreateOptionsTokenable(User user, CredentialCreateOptions options) : this() | ||
{ | ||
UserId = user?.Id; | ||
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 null reference exception if user is null |
||
Options = options; | ||
} | ||
|
||
public bool TokenIsValid(User user) | ||
{ | ||
if (!Valid || user == null) | ||
{ | ||
return false; | ||
} | ||
|
||
return UserId == user.Id; | ||
} | ||
|
||
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: Potential timing attack vulnerability. Consider using a constant-time comparison