-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add passkeys to ASP.NET Core Identity #62112
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?
Conversation
FYI I have a PR adding metrics to identity here: #62078. Whoever merges second will need to react and add counters and tags for passkey signins. |
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.
Pull Request Overview
This PR adds support for passkeys to ASP.NET Core Identity by extending the existing identity stores and sign‐in flows. Key changes include:
- Introducing new generic abstractions and methods for managing passkeys in both UserStore and UserOnlyStore.
- Updating IdentityUserContext model building to support passkeys under Identity Schema Version 3.
- Extending SignInManager with new APIs for passkey sign in and for configuring/retrieving passkey creation and request options.
Reviewed Changes
Copilot reviewed 111 out of 111 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Identity/EntityFrameworkCore/src/UserStore.cs | Added a new generic UserStore overload and methods to create, find, update, and remove user passkeys. |
src/Identity/EntityFrameworkCore/src/UserOnlyStore.cs | Extended the store for users without roles to support passkey operations along with checks for DB support. |
src/Identity/EntityFrameworkCore/src/IdentityUserContext.cs | Modified OnModelCreating to include a new entity for passkeys when using Identity Schema Version 3. |
src/Identity/Core/src/SignInManager.cs | Introduced new methods for passkey sign in and configuration of passkey creation/request options. |
PublicAPI.Unshipped.txt and build files | Updated the public API surface and project configuration to expose passkey-related features. |
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.
Some comments to help with reviewing.
src/Identity/Extensions.Core/src/IPasskeyRequestContextProvider.cs
Outdated
Show resolved
Hide resolved
public interface IPasskeyOriginValidator | ||
{ | ||
/// <summary> | ||
/// Determines whether the specified origin is valid for passkey operations. | ||
/// </summary> | ||
/// <param name="originInfo">Information about the passkey's origin.</param> | ||
/// <returns><c>true</c> if the origin is valid; otherwise, <c>false</c>.</returns> | ||
bool IsValidOrigin(PasskeyOriginInfo originInfo); | ||
} |
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.
This interface exists because the meaning of a "valid origin" can vary widely between applications. See https://www.w3.org/TR/webauthn-3/#sctn-validating-origin.
/// <summary> | ||
/// Adds a new passkey credential in the store for the specified <paramref name="user"/>, | ||
/// or updates an existing passkey. | ||
/// </summary> | ||
/// <param name="user">The user to create the passkey credential for.</param> | ||
/// <param name="passkey">The passkey to add.</param> | ||
/// <param name="cancellationToken">The <see cref="CancellationToken"/> used to propagate notifications that the operation should be canceled.</param> | ||
/// <returns>The <see cref="Task"/> that represents the asynchronous operation.</returns> | ||
Task SetPasskeyAsync(TUser user, UserPasskeyInfo passkey, CancellationToken cancellationToken); |
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.
Considering splitting this out into AddPasskeyAsync
and UpdatePasskeyAsync
.
/// <summary> | ||
/// Gets the JSON representation of the options. | ||
/// </summary> | ||
/// <remarks> | ||
/// The structure of the JSON string matches the description in the WebAuthn specification. | ||
/// See <see href="https://www.w3.org/TR/webauthn-3/#dictdef-publickeycredentialcreationoptionsjson"/>. | ||
/// </remarks> | ||
public string AsJson() | ||
=> _optionsJson; |
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.
This PR currently just exposes the credential creation/request options as a JSON string rather than returning a strongly-typed object. That way we're not locked into making the entirety of the Passkeys/
folder public
. In the future, if we wanted to e.g., write a hand-crafted JSON writer that constructed a string without first building a .NET representation, we could do that.
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.
Making this method virtual
would allow users to override its logic if needed.
/// <summary> | ||
/// Gets or sets whether the passkey has a verified user. | ||
/// </summary> | ||
public virtual bool IsUserVerified { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets whether the passkey is eligible for backup. | ||
/// </summary> | ||
public virtual bool IsBackupEligible { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets whether the passkey is currently backed up. | ||
/// </summary> | ||
public virtual bool IsBackedUp { get; set; } |
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.
These flags are all present on the authenticator data flags. We could just store the flags byte directly so that we don't introduce another schema change in the future, should we find ourselves wanting to store more flags.
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.
These flags (per the specification) must be persisted and validated during authentication processing for previously created passkeys.
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.
The specification states in the Credential Backup State section:
"The value of the BE flag is set during authenticatorMakeCredential operation and MUST NOT change."
This means:
- The BE (Backup Eligible) flag must remain unchanged after Passkey creation.
- If BE was initially 0 (false), the BS (Backup State) flag must never be 1 (true).
Storing these flags in binary form would be impractical. The current implementation (keeping them as separate values) provides the right balance of correctness and usability.
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.
Thanks for the feedback, @vanbukin.
Storing these flags in binary form would be impractical. The current implementation (keeping them as separate values) provides the right balance of correctness and usability.
I agree it's more usable to keep them as separate properties. The main reason for my initial suggestion was to avoid cases where spec changes require additional flags to be stored, incurring an update to this model and a migration in customers' apps.
For example, the BS and BE flags weren't defined in the WebAuthn Level 2 spec, but were added in the Level 3 spec. A similar adjustment could theoretically be made in the future.
async function createCredential(optionsJSON) { | ||
// See: https://www.w3.org/TR/webauthn-2/#sctn-registering-a-new-credential | ||
|
||
// 1. Let options be a new PublicKeyCredentialCreationOptions structure configured to | ||
// the Relying Party’s needs for the ceremony. | ||
// See: https://www.w3.org/TR/webauthn-3/#dom-publickeycredential-parsecreationoptionsfromjson | ||
const options = PublicKeyCredential.parseCreationOptionsFromJSON(optionsJSON); | ||
|
||
// 2. Call navigator.credentials.create() and pass options as the publicKey option. | ||
// Let credential be the result of the successfully resolved promise. | ||
// If the promise is rejected, abort the ceremony with a user-visible error, | ||
// or otherwise guide the user experience as might be determinable from the | ||
// context available in the rejected promise. | ||
const credential = await navigator.credentials.create({ publicKey: options }); | ||
|
||
// 3. Let response be credential.response. If response is not an instance of | ||
// AuthenticatorAttestationResponse, abort the ceremony with a user-visible error. | ||
if (!(credential?.response instanceof AuthenticatorAttestationResponse)) { | ||
throw new Error('The authenticator failed to provide a valid credential.'); | ||
} | ||
|
||
// Continue the ceremony on the server. | ||
// See: https://www.w3.org/TR/webauthn-3/#dom-publickeycredential-tojson | ||
return JSON.stringify(credential); | ||
} |
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.
We could move this code to a package, but it's so small that I think it's safe to include it in the template.
However, I'm considering changing the options
parameter here to represent the top-level object passed to the navigator.credentials.create()
function instead of representing the publicKey
property. That's because the WebAuthn level 3 spec references another top-level property, mediation
in the ceremony, and we might want a way to configure it (and other not-yet-introduced properties) in the future.
Also, this script assumes it's loaded as part of a full page navigation, not an enhanced navigation. This strategy currently works because the script gets rendered as a result of a non-enhanced form post.
[ | ||
new(COSEAlgorithmIdentifier.ES256), | ||
new(COSEAlgorithmIdentifier.PS256), | ||
new(COSEAlgorithmIdentifier.ES384), | ||
new(COSEAlgorithmIdentifier.PS384), | ||
new(COSEAlgorithmIdentifier.PS512), | ||
new(COSEAlgorithmIdentifier.RS256), | ||
new(COSEAlgorithmIdentifier.ES512), | ||
new(COSEAlgorithmIdentifier.RS384), | ||
new(COSEAlgorithmIdentifier.RS512), | ||
]; |
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.
This list does not include RS1, even though the FIDO conformance testing tool checks for it. Maybe we should make this list configurable.
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.
I imagine RS1 is RSASHA1, so I think it's ok to leave it out, but @blowdart might care.
Is there any provision for passkey integration into identity API endpoints via IdentityApiEndpointRouteBuilderExtensions? |
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.
Some initial review feedback.
I need to do a more detailed pass, specially over the crypto stuff.
Overall looks good, and I'm happy to see this landing as it is a great improvement for security. The main pieces of feedback I have are:
- Layering -> I think a lot of this code should go into Identity directly rather than Core.
- UserManager vs SignInManager: Some of the code might fit better in SignInManager, specially if it requires access to HttpContext (or you are accessing it behind an abstraction).
- Maybe collect the HttpContext based info you need in SignInManager and call UserManager with it.
- Json strings through the API or S.T.J types on the DTOs. My understanding is that part of it is to avoid serializing/deserializing things and making additional types public.
- An option here could be to create an "opaque" type that contains the Json data. Maybe with a
GetRawData
or something like that which returns the original JSON string. That way in the future you can add fields to the type if you want/need to and use those directly over the string, and anyone who needs access to the data, can get the string and deserialize it. (which is no different than what they have to do today, isn't it?)
- An option here could be to create an "opaque" type that contains the Json data. Maybe with a
...tes/content/BlazorWeb-CSharp/BlazorWeb-CSharp/Components/Account/Shared/PasskeyHandler.razor
Outdated
Show resolved
Hide resolved
var user = Input.Email is { Length: > 0 } email | ||
? await UserManager.FindByEmailAsync(email) | ||
: null; | ||
|
||
var passkeyRequestArgs = new PasskeyRequestArgs<ApplicationUser> | ||
{ | ||
User = user, | ||
}; | ||
var options = await SignInManager.ConfigurePasskeyRequestOptionsAsync(passkeyRequestArgs); |
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.
Might make sense for ConfigurePasskeyRequestOptionsAsync
to take in the email as a separate parameter and perform this bit of logic internally to set the user.
That way you avoid having to inject the UserManager in addition to the sign in manager.
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.
Identity doesn't require that users have an email though, right? Or is the suggestion to add a convenience method that wraps the one accepting a TUser
directly?
...jectTemplates/content/BlazorWeb-CSharp/BlazorWeb-CSharp/Components/Account/Pages/Login.razor
Outdated
Show resolved
Hide resolved
/// <param name="originalOptionsJson">The JSON representation of the original passkey creation options provided to the browser.</param> | ||
/// <param name="userManager">The <see cref="UserManager{TUser}"/> to retrieve user information from.</param> | ||
/// <returns>A task object representing the asynchronous operation containing the <see cref="PasskeyAssertionResult{TUser}"/>.</returns> | ||
Task<PasskeyAssertionResult<TUser>> PerformAssertionAsync(TUser? user, string credentialJson, string originalOptionsJson, UserManager<TUser> userManager); |
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.
For all these APIs, dealing in the serialized JSON string feels "strange". I'm not 100% sure how big these strings are, but I would think it makes more sense for these payloads to get deserialized into DTOs and passed in as objects rather than having the guts of Identity handle the serialization concerns.
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.
The main reason I did it this way is to avoid exposing the details of the WebAuthn spec as public C# API. But I agree it feels unusual. Maybe we could do something similar to the PasskeyCreationOptions
type added in this PR, which exposes a small number of strongly-typed properties but has a method to get the underlying JSON representation.
src/Identity/Extensions.Core/src/IPasskeyRequestContextProvider.cs
Outdated
Show resolved
Hide resolved
/// <remarks> | ||
/// See <see href="https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialrequestoptions-extensions"/>. | ||
/// </remarks> | ||
public JsonElement? Extensions { get; set; } |
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.
Using JsonElement here is a bit "strange", is there another type that could be used instead? (IDictionary<string,object>) or similar?
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.
Not sure it's possible - this property gets forwarded to a PublicKeyCredentialRequestOptions
object, whose sole purpose is to get serialized into a JSON payload that gets passed to the JavaScript navigator.credentials.get()
method.
This JSON serialization happens internally within Identity and uses the JSON source generator (the customer doesn't get to specify their own JsonSerializerOptions
because the serialization mechanism is an internal implementation detail).
So, if the Extensions
property was an IDictionary<string, object>
, our implementation would throw for any dictionary values we haven't anticipated in the IdentityJsonSerializerContext
. Whereas, the current approach allows the developer to either create a JsonElement
directly or serialize another .NET type into one via JsonSerializer.SerializeToElement()
, where they can pass their own JsonSerializerOptions
.
All that said, there might be a better way of handling this than JsonElement
that I haven't thought of.
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.
I don't think JsonElement is that strange. We've been using it a lot in the MCP cshar-sdk repo for things like InputSchema. If the goal is to serialize/deserialize extensions as JSON, IDictionary<string,object>
feels inappropriate because the object
would probably just JsonElements most of the time anyway.
This does prevent us from switching to a different JSON library in the future or using something other than JSON for this, but I highly doubt we'd switch from System.Text.Json, and it appears the webauthn standard requires JSON for extensions.
@eiriktsarpalis Do you think JsonElement makes sense?
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.
Whereas, the current approach allows the developer to either create a JsonElement directly
You can't just create a JsonElement. But instead, you need to get a JsonDocument and get the JsonElement out of that.
Would using JsonNode
work better here if we expect people to create these directly?
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.
It depends on the use case. I have a preference for JsonElement
if I want the value to be immutable but JsonNode
/JsonObject
is easier to manipulate. I find JsonElement?
to be somewhat awkward because you then three separate representations for null
, default(JsonElement)
, and JsonElement
values representing JSON nulls whereas JsonNode?
simply uses null
for all three.
src/Identity/Extensions.Core/src/Passkeys/BufferSourceJsonConverter.cs
Outdated
Show resolved
Hide resolved
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.
Consider using Authenticate with a passkey through form autofill with autocomplete="username webauthn"
for a better user experience.
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.
Should we make it possible to register with just a passkey and not a password with the Blazor project template? I'm fine if we do this as a follow up.
I'm also a fan of the layering adjustment that Javier suggested to move the HttpContext-related stuff up to the AspNetCore package and moving more stuff from UserManager to the SignInManager.
src/ProjectTemplates/test/Templates.Blazor.Tests/BlazorTemplateTest.cs
Outdated
Show resolved
Hide resolved
src/ProjectTemplates/test/Templates.Blazor.Tests/BlazorWebTemplateTest.cs
Outdated
Show resolved
Hide resolved
|
||
internal static class PasskeyExceptionExtensions | ||
{ | ||
extension(PasskeyException) |
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.
Cool use of the new syntax, but why not just make these normal internal static methods? Also, would application level code ever need to do a runtime check of the kind of PasskeyException? Would it make sense for there to be an PasskeyExceptionReason enum or something like that? The names of these methods seem like they could be good enum values.
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.
Cool use of the new syntax, but why not just make these normal internal static methods?
I thought it might be better to separate out these helper methods from the core PasskeyException
declaration, but it was also an excuse to use static extension members :) I'm fine with moving them back if that's preferable.
Also, would application level code ever need to do a runtime check of the kind of PasskeyException? Would it make sense for there to be an PasskeyExceptionReason enum or something like that? The names of these methods seem like they could be good enum values.
Hm, maybe. We do something similar in IdentityErrorDescriber
. I'm just now sure how common it'll be to programmatically react to different passkey validation failures. It might be useful for metrics, though. For example, if the server keeps rejecting credentials and the developer wants to find out why, reporting the exception "reason" might be helpful? I wonder if we should consider this as a potential follow-up item.
|
||
internal class DefaultPasskeyOriginValidator : IPasskeyOriginValidator | ||
{ | ||
private readonly IHttpContextAccessor _httpContextAccessor; |
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.
IMO if we can avoid sprinkling IHttpContextAccessor
it's better. Services like the SignInManager use it because they are very "user facing" and we want to minimize dependencies and complexity, but in general, it's an invitation for failure whenever these things are used out of context.
My "recommendation" would be to either pass the HttpContext explicitly as a parameter from the call-site or have SignInManager
(or equivalent) construct an instance of the implementation and pass it as a parameter to the methods that need it. (If you end up with too many parameters you can group them in a context object).
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.
I've updated IPasskeyHandler
methods to accept context objects that contain an HttpContext
provided by SignInManager
.
public static EventId UserCannotSignInWithoutConfirmedAccount = new EventId(4, "UserCannotSignInWithoutConfirmedAccount"); | ||
public static EventId TwoFactorSecurityStampValidationFailed = new EventId(5, "TwoFactorSecurityStampValidationFailed"); | ||
public static readonly EventId UserCannotSignInWithoutConfirmedEmail = new(0, "UserCannotSignInWithoutConfirmedEmail"); | ||
public static readonly EventId SecurityStampValidationFailed = new(0, "SecurityStampValidationFailed"); |
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.
Is the duplicate EventId here expected?
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.
Hm, not sure - the duplicates were present prior to this PR. Changing it might technically be breaking? Might be worth fixing anyway though.
IPasskeyOriginValidator originValidator, | ||
IPasskeyAttestationStatementVerifier attestationStatementVerifier) |
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.
Rather than having these two separate interfaces.
Would it make sense to have virtual methods on this type?
Also, if we don't expect to have multiple implementations of IPassKeyHandler, would it make sense to just have an abstract base class instead?
The benefit is that should ever anyone want to customize these behaviors, they only have to override the expected method, rather than creating a separate implementation of one or more interfaces, registering it in DI and so on.
Maybe we can follow a pattern for things that require HttpContext where we provide a ValidationOptions object that exposes different properties that can be set to callbacks that we invoke at the right time.
Other libraries like the Oidc library follow this pattern.
It gets rid of potentially 3 different interfaces, and you still have the ability to customize things in the same way.
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.
Also, if we don't expect to have multiple implementations of IPassKeyHandler, would it make sense to just have an abstract base class instead?
It might be possible that third-party libraries want to provide an IPasskeyHandler
implementation solely based on their library, in which case it might make less sense to extend a base class.
But I do like the idea of removing IPasskeyAttestationStatementValidator
and IPasskeyOriginVerifier
and instead just making them virtual methods on our default passkey handler. It makes sense because the reasons those interfaces exist are due to decisions made in the DefaultPasskeyHandler
implementation of IPasskeyHandler
. If you were to provide a different IPasskeyHandler
, it might choose to not utilize those interfaces in the same way.
|
||
namespace Microsoft.AspNetCore.Identity; | ||
|
||
internal sealed class BufferSourceJsonConverter : JsonConverter<BufferSource> |
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.
Is this needed because S.T.J doesn't support ReadOnlyMemory?
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.
STJ actually does support ReadOnlyMemory<byte>
, but it uses base64 encoding, not base64url encoding. Browser WebAuthn APIs use base64url encoding when converting byte arrays to/from strings.
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.
This would be a reasonable thing to bring to S.T.J. The reason I brought up in the first place is that this is non-trivial logic that we are copying/putting here, when this could ideally be a built-in JsonConverter
that got applied.
/// <remarks> | ||
/// See <see href="https://www.w3.org/TR/webauthn-3/#dom-authenticatorselectioncriteria-userverification"/>. | ||
/// </remarks> | ||
public string UserVerification { get; set; } = "preferred"; |
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.
Is this the thing that determines whether the user needs to provide proof? (via biometrics, pin or similar mechanism). Is the default set by the spec? or do we have freedom? I would think that we would want this to be required.
Otherwise, the passkey can be used without confirmation that the user is whom they deem to be. For example, cases where the device is left unlocked (at a public location or forgotten).
Just thinking that being conservative here is better. I haven't seen a site using passkeys that doesn't require me to use a pin or biometric to use the credential.
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.
The default value for userVerification is preferred. You likely set a PIN/biometric on your device so it will always request it.
When userVerification is preferred, the user experience depends on whether or not a PIN is set or a fingerprint is enrolled on the user’s security key.
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.
@mguinness is right - the spec indicates that "preferred" is the default when not specified, and the C# property here just reflects that. If user verification is possible, it will be performed. However, we're free to set our defaults to whatever we think is best, so I'm open to considering "required" as being the default.
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.
I would consider this as part of the Threat Model. I think being conservative here is a good default.
/// The <see cref="Task"/> that represents the asynchronous operation, containing the <see cref="IdentityResult"/> | ||
/// of the operation. | ||
/// </returns> | ||
public virtual async Task<IdentityResult> RemovePasskeyAsync(TUser user, byte[] credentialId) |
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.
For all these methods, maybe use ROM<byte>
unless we have a good reason to pass byte[] around?
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.
Yeah, I think that would work. My original reasoning for using byte[]
was because the passkey EF Core entity type (IdentityUserPasskey
) needs to store a byte[]
credential ID, but none of these methods accepting a credentialId
update the entity, so I think ROM should be fine. I'll try that.
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.
Ah, it's not possible because expression trees can't reference ref structs, and you need to access ReadOnlyMemory<byte>.Span
when doing the ID equality check in the query for the DbSet
.
We could still change the UserManager
APIs to work with ReadOnlyMemory<byte>
, but we'd need to eventually copy the buffer to a byte[]
anyway. Keeping the argument as a byte[]
enables the caller to avoid this unnecessary copy, especially if they already have a byte[]
to provide in the first place.
...ontent/BlazorWeb-CSharp/BlazorWeb-CSharp/Components/Account/Pages/Manage/RenamePasskey.razor
Outdated
Show resolved
Hide resolved
} | ||
this.form.submit(); | ||
} | ||
}); |
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.
Is this a JS Module because of enhanced nav?
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.
That's one reason, yeah. Making it a custom element like this also allows the developer to put multiple <PasskeySubmit/>
components on the same page, if they want. I guess you could still directly render a script from the PasskeySubmit
component while using a custom element, but given that that's not a supported pattern in all cases, I think it seems better to use a JS initializer. Happy to consider alternatives, though :)
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.
I was a bit skeptical about loading this script on all pages and not only identity, but I understand the constraints that we are operating within, so the initializer seems like a good trade-off. I just wanted to confirm.
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.
I actually revised this a bit - I thought it was better organized to have the JS code "closer" to the corresponding C#, so this logic is now in PasskeySubmit.razor.js
and it's loaded directly from App.razor
.
...ctTemplates/content/BlazorWeb-CSharp/BlazorWeb-CSharp/wwwroot/BlazorWeb-CSharp.lib.module.js
Outdated
Show resolved
Hide resolved
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.
This looks great! I'll save some of my API feedback for when we do API review on #62287 and more people can participate.
/// Gets or sets whether the credential creation request was initiated from | ||
/// a different origin than the one associated with the relying party. | ||
/// </summary> | ||
public bool? CrossOrigin { get; init; } |
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.
Can we make this non-nullable by just treating null as false? And could we do the same for PasskeyOriginInfo?
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.
Possibly, but the spec explicitly states that the member is optional: https://www.w3.org/TR/webauthn-3/#dom-collectedclientdata-crossorigin, and the verbiage in the spec's registration/authentication steps alludes to the fact that there are three states by stating that the server must check whether CrossOrigin
is both present and set to true.
However, if the only two states we care about are "present and set to true" and the opposite, then a non-nullable bool
still works 🙂 My intention was just to have the C# representation reflect what's described in the spec as accurately as possible.
Is your concern with making it nullable primarily from an API perspective? If so, I should note that all the types in the Passkeys
folder, including this one, are internal
.
Anyway, I don't feel too strongly about this, so I'm fine with making it non-nullable and writing that it's optional in a <remark>
in the XML docs, if that's preferable!
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.
You're right that I was mostly concerned about the public API surface, so the nullability of the property in CollectedClientData doesn't matter as much to me. I followed it back from PasskeyOriginInfo where I think it is public. I just figured that if there's no practical difference between null and false, it would be easier for people to understand if we made it non-nullable. I don't feel super strongly, but either way a <remark>
on the public property explaining the meaning of null vs. false would be nice.
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.
Good point about PasskeyOriginInfo
being public. I've just changed PasskeyOriginInfo.CrossOrigin
to be a non-nullable bool
, but left CollectedClientData.CrossOrigin
as a bool?
just so it's clear in the code where the "loss of information" occurs as we convert it to a bool
.
// For future-proofing, we pass a PasskeyOriginInfo to the origin validator so that we're able to add more properties to | ||
// it later. | ||
var originInfo = new PasskeyOriginInfo(clientData.Origin, clientData.CrossOrigin); | ||
var isOriginValid = await IsValidOriginAsync(originInfo, context.HttpContext).ConfigureAwait(false); |
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.
Do we care that the origin where we receive the request for the challenge is the same as the origin we are validating the attestation from?
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.
I don't think so - the browser enforces origin isolation when invoking navigator.credentials.create()
, so the server just has to check that the origin included in the returned credential matches an origin that the server expects.
// 11. Verify that the value of C.challenge equals the base64url encoding of originalOptions.challenge. | ||
if (!clientData.Challenge.FixedTimeEquals(originalOptions.Challenge)) | ||
{ | ||
throw PasskeyException.InvalidChallenge(); | ||
} | ||
|
||
// 12-14. Verify that the value of C.origin is an origin expected by the Relying Party. | ||
// NOTE: The level 3 draft permits having multiple origins and validating the "top origin" when a cross-origin request is made. | ||
// For future-proofing, we pass a PasskeyOriginInfo to the origin validator so that we're able to add more properties to | ||
// it later. | ||
var originInfo = new PasskeyOriginInfo(clientData.Origin, clientData.CrossOrigin == true); | ||
var isOriginValid = await IsValidOriginAsync(originInfo, context.HttpContext).ConfigureAwait(false); | ||
if (!isOriginValid) | ||
{ | ||
throw PasskeyException.InvalidOrigin(clientData.Origin); | ||
} | ||
|
||
// NOTE: The level 2 spec requires token binding validation, but the level 3 spec does not. | ||
// We'll just validate that the token binding object doesn't have an unexpected format. | ||
if (clientData.TokenBinding is { } tokenBinding) | ||
{ | ||
var status = tokenBinding.Status; | ||
if (!string.Equals("supported", status, StringComparison.Ordinal) && | ||
!string.Equals("present", status, StringComparison.Ordinal) && | ||
!string.Equals("not-supported", status, StringComparison.Ordinal)) | ||
{ | ||
throw PasskeyException.InvalidTokenBindingStatus(status); | ||
} | ||
} | ||
|
||
// 15. Verify that the rpIdHash in authData is the SHA-256 hash of the RP ID expected by the Relying Party. | ||
var rpIdHash = SHA256.HashData(Encoding.UTF8.GetBytes(originalOptions.RpId ?? string.Empty)); | ||
if (!CryptographicOperations.FixedTimeEquals(authenticatorData.RpIdHash.Span, rpIdHash.AsSpan())) | ||
{ | ||
throw PasskeyException.InvalidRelyingPartyIDHash(); | ||
} | ||
|
||
// 16. Verify that the UP bit of the flags in authData is set. | ||
if (!authenticatorData.IsUserPresent) | ||
{ | ||
throw PasskeyException.UserNotPresent(); | ||
} | ||
|
||
// 17. If user verification was determined to be required, verify that the UV bit of the flags in authData is set. | ||
// Otherwise, ignore the value of the UV flag. | ||
if (string.Equals("required", originalOptions.UserVerification, StringComparison.Ordinal) && !authenticatorData.IsUserVerified) | ||
{ | ||
throw PasskeyException.UserNotVerified(); | ||
} | ||
|
||
// 18. If the BE bit of the flags in authData is not set, verify that the BS bit is not set. | ||
if (!authenticatorData.IsBackupEligible && authenticatorData.IsBackedUp) | ||
{ | ||
throw PasskeyException.NotBackupEligibleYetBackedUp(); | ||
} | ||
|
||
// 19. If the credential backup state is used as part of Relying Party business logic or policy, let currentBe and currentBs | ||
// be the values of the BE and BS bits, respectively, of the flags in authData. Compare currentBe and currentBs with | ||
// credentialRecord.backupEligible and credentialRecord.backupState: | ||
// 1. If credentialRecord.backupEligible is set, verify that currentBe is set. | ||
// 2. If credentialRecord.backupEligible is not set, verify that currentBe is not set. | ||
// 3. Apply Relying Party policy, if any. | ||
if (storedPasskey.IsBackupEligible && !authenticatorData.IsBackupEligible) | ||
{ | ||
throw PasskeyException.ExpectedBackupEligibleCredential(); | ||
} | ||
if (!storedPasskey.IsBackupEligible && authenticatorData.IsBackupEligible) | ||
{ | ||
throw PasskeyException.ExpectedBackupIneligibleCredential(); | ||
} | ||
if (authenticatorData.IsBackedUp && _passkeyOptions.BackedUpCredentialPolicy is PasskeyOptions.CredentialBackupPolicy.Disallowed) | ||
{ | ||
throw PasskeyException.BackupDisallowedYetBackedUp(); | ||
} | ||
if (!authenticatorData.IsBackedUp && _passkeyOptions.BackedUpCredentialPolicy is PasskeyOptions.CredentialBackupPolicy.Required) | ||
{ | ||
throw PasskeyException.BackupRequiredYetNotBackedUp(); | ||
} | ||
|
||
// 20. Let clientDataHash be the result of computing a hash over the cData using SHA-256. | ||
var clientDataHash = SHA256.HashData(response.ClientDataJSON.AsSpan()); | ||
|
||
// 21. Using credentialRecord.publicKey, verify that sig is a valid signature over the binary concatenation of authData and hash. | ||
byte[] data = [.. response.AuthenticatorData.AsSpan(), .. clientDataHash]; | ||
var cpk = new CredentialPublicKey(storedPasskey.PublicKey); | ||
if (!cpk.Verify(data, response.Signature.AsSpan())) | ||
{ | ||
throw PasskeyException.InvalidAssertionSignature(); | ||
} | ||
|
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.
I believe these steps are common to the steps we perform for attestation. It might make sense to create a helper method ValidateCommonSteps and have the logic in a single place.
For sensitive logic like this it pays off to have a single implementation rather than running the risk of introducing variations/bugs over time.
It also makes it easier to understand and validate the important unique steps of each flow.
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.
Agreed, particularly on the point that it can help reduce the chance of bugs.
I had a common helper like that at one point, but I removed it because it became a pain to modify (and was arguably harder to read) because:
- There are sometimes subtle variations between the attestation and assertion steps, which meant the helper had to be parameterized in non-obvious ways.
- The common steps aren't contiguous. They can be made so, but then the implementation appears to differ even more from the spec.
- The comments referencing the spec naturally become out of sync between attestation and assertion.
But I was thinking it might help to create smaller methods representing the atomic steps, and those could be called from the attestation or assertion procedures, wherever it makes sense to. Does that sound fine? I think it strikes a good balance between clearly following the spec and sharing common code to reduce bugs.
return SignInResult.Failed; | ||
} | ||
|
||
var setPasskeyResult = await UserManager.SetPasskeyAsync(assertionResult.User, assertionResult.Passkey); |
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.
This is the thing that updates the sign count on the database, isn't it? So everytime the user signs in it gets incremented?
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.
Yes, correct.
/// <returns> | ||
/// A task object representing the asynchronous operation containing the <see cref="PasskeyCreationOptions"/>. | ||
/// </returns> | ||
public virtual async Task<PasskeyCreationOptions> GeneratePasskeyCreationOptionsAsync(PasskeyCreationArgs creationArgs) |
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.
Is my understanding correct that there are two pairs of Generate/Configure methods and that esentially 1 pair is used for registering a new passkey and the other one for performing an attestation? If that's the case, I think adding a comment would make it clearer.
All methods have very similar names and makes it somewhat confusing. Maybe the naming of the method could be different. GeneratePKRegistrationOptions
and GeneratePKAttestationOptions
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.
Is my understanding correct that there are two pairs of Generate/Configure methods and that esentially 1 pair is used for registering a new passkey and the other one for performing an attestation? If that's the case, I think adding a comment would make it clearer.
One pair ("creation") is for generating options used for attestation/registration, and the other pair ("request") is for generating options used for assertion/authentication. Maybe that's what you meant. I'll add new comments to clarify that.
The creation/request terms come from the WebAuthn spec. Do you think it's OK to deviate from that and stick with attestation/assertion terminology? I agree it might make things clearer, as someone less familiar with WebAuthn might not understand the correlation between creation+attestation and request+assertion.
This also might be a good discussion for API review.
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.
By the way, the difference between "configure" and "generate" this context is that "generate" actually produces the options, and "configure" stores them in the HttpContext via HttpContext.SignInAsync()
. Does that seem clear enough? Maybe that's also an API review question.
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.
Looks great!
this.internals.form.addEventListener('submit', (event) => { | ||
if (event.submitter?.name === '__passkeySubmit') { | ||
event.preventDefault(); | ||
this.obtainCredentialAndReSubmit(); | ||
} | ||
}); |
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.
One thought that I had is we could maybe leverage the constraint validation API. Setup a custom validity to "Retrieving credential" or something similar, and then upon the credential promise resolving. Setting the value in the form data and removing the custom validity state.
That would save us this bit and the bit below, as the form can only ever be submitted once we've retrieved the passkey.
/// <summary> | ||
/// The default passkey handler. | ||
/// </summary> | ||
public partial class DefaultPasskeyHandler<TUser> : IPasskeyHandler<TUser> |
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.
Is there are a reason, why there is no cancellationToken usage in here, even though some of the APIs support it?
Add passkeys to ASP.NET Core Identity
This PR adds passkey support to ASP.NET Core Identity.
Description
Following is a summary of the changes in this PR:
Microsoft.AspNetCore.Identity.EntityFrameworkCore
SignInManager
andUserManager
for passkey management and sign inNote that the goal of this PR is to add support for passkey authentication in ASP.NET Core Identity. While it implements core WebAuthn functionality, it does not provide a complete and general-purpose WebAuthn/FIDO2 library. The public API surface is limited in order to enable long-term stability of the feature. Targeted extensibility points were added to enable functionality not implemented by default, most notably attestation statement validation. This allows the use of third-party libraries to fill the missing gaps, when desired. Community feedback may result in additional extensibility APIs being added in the future.
This PR includes E2E tests validating that a passkey can be registered and used for logging in. I'll add unit tests after we agree on the design to avoid churn.
Fixes #53467