Skip to content

Commit 332a97f

Browse files
MackinnonBuckMackinnonBuck
committed
API review feedback
1 parent c21e32e commit 332a97f

29 files changed

+556
-390
lines changed

‎src/Identity/Core/src/DefaultPasskeyHandler.cs‎

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public async Task<PasskeyCreationOptionsResult> MakeCreationOptionsAsync(Passkey
6060
DisplayName = userEntity.DisplayName,
6161
},
6262
Challenge = BufferSource.FromBytes(challenge),
63-
Timeout = (uint)_options.Timeout.TotalMilliseconds,
63+
Timeout = (uint)_options.AuthenticatorTimeout.TotalMilliseconds,
6464
ExcludeCredentials = excludeCredentials,
6565
PubKeyCredParams = pubKeyCredParams,
6666
AuthenticatorSelection = new()
@@ -112,13 +112,13 @@ public async Task<PasskeyRequestOptionsResult> MakeRequestOptionsAsync(TUser? us
112112
ArgumentNullException.ThrowIfNull(httpContext);
113113

114114
var allowCredentials = await GetAllowCredentialsAsync().ConfigureAwait(false);
115-
var serverDomain = _options.ServerDomain ?? httpContext.Request.Host.Host;
115+
var serverDomain = GetServerDomain(httpContext);
116116
var challenge = RandomNumberGenerator.GetBytes(_options.ChallengeSize);
117117
var options = new PublicKeyCredentialRequestOptions
118118
{
119119
Challenge = BufferSource.FromBytes(challenge),
120120
RpId = serverDomain,
121-
Timeout = (uint)_options.Timeout.TotalMilliseconds,
121+
Timeout = (uint)_options.AuthenticatorTimeout.TotalMilliseconds,
122122
AllowCredentials = allowCredentials,
123123
UserVerification = _options.UserVerificationRequirement,
124124
};
@@ -307,15 +307,19 @@ await VerifyClientDataAsync(
307307
// 21-24. Determine the attestation statement format by performing a USASCII case-sensitive match on fmt against the set of supported WebAuthn
308308
// Attestation Statement Format Identifier values...
309309
// Handles all validation related to the attestation statement (21-24).
310-
var isAttestationStatementValid = await _options.VerifyAttestationStatement(new()
311-
{
312-
HttpContext = context.HttpContext,
313-
AttestationObject = attestationObjectMemory,
314-
ClientDataHash = clientDataHash,
315-
}).ConfigureAwait(false);
316-
if (!isAttestationStatementValid)
310+
if (_options.VerifyAttestationStatement is { } verifyAttestationStatement)
317311
{
318-
throw PasskeyException.InvalidAttestationStatement();
312+
var isAttestationStatementValid = await verifyAttestationStatement(new()
313+
{
314+
HttpContext = context.HttpContext,
315+
AttestationObject = attestationObjectMemory,
316+
ClientDataHash = clientDataHash,
317+
}).ConfigureAwait(false);
318+
319+
if (!isAttestationStatementValid)
320+
{
321+
throw PasskeyException.InvalidAttestationStatement();
322+
}
319323
}
320324

321325
// 25. Verify that the credentialId is <= 1023 bytes.
@@ -338,7 +342,6 @@ await VerifyClientDataAsync(
338342
var credentialRecord = new UserPasskeyInfo(
339343
credentialId,
340344
publicKey: attestedCredentialData.CredentialPublicKey.ToArray(),
341-
name: null,
342345
createdAt: DateTime.UtcNow,
343346
signCount: authenticatorData.SignCount,
344347
transports: response.Transports,
@@ -591,14 +594,7 @@ private async Task VerifyClientDataAsync(
591594
}
592595

593596
// Verify that the value of C.origin is an origin expected by the Relying Party.
594-
var originInfo = new PasskeyOriginValidationContext
595-
{
596-
HttpContext = httpContext,
597-
Origin = clientData.Origin,
598-
CrossOrigin = clientData.CrossOrigin == true,
599-
TopOrigin = clientData.TopOrigin,
600-
};
601-
var isOriginValid = await _options.ValidateOrigin(originInfo).ConfigureAwait(false);
597+
var isOriginValid = await ValidateOriginAsync(clientData, httpContext).ConfigureAwait(false);
602598
if (!isOriginValid)
603599
{
604600
throw PasskeyException.InvalidOrigin(clientData.Origin);
@@ -652,6 +648,32 @@ private void VerifyAuthenticatorData(
652648
}
653649
}
654650

651+
private ValueTask<bool> ValidateOriginAsync(CollectedClientData clientData, HttpContext httpContext)
652+
{
653+
if (_options.ValidateOrigin is { } validateOrigin)
654+
{
655+
// The user has overridden the default origin validation,
656+
// so we'll use that instead of the default behavior.
657+
return validateOrigin(new PasskeyOriginValidationContext
658+
{
659+
HttpContext = httpContext,
660+
Origin = clientData.Origin,
661+
CrossOrigin = clientData.CrossOrigin == true,
662+
TopOrigin = clientData.TopOrigin,
663+
});
664+
}
665+
666+
if (string.IsNullOrEmpty(clientData.Origin) ||
667+
clientData.CrossOrigin == true ||
668+
!Uri.TryCreate(clientData.Origin, UriKind.Absolute, out var originUri))
669+
{
670+
return ValueTask.FromResult(false);
671+
}
672+
673+
// Uri.Equals correctly handles string comparands.
674+
return ValueTask.FromResult(httpContext.Request.Headers.Origin is [var origin] && originUri.Equals(origin));
675+
}
676+
655677
private string GetServerDomain(HttpContext httpContext)
656678
=> _options.ServerDomain ?? httpContext.Request.Host.Host;
657679
}

‎src/Identity/Core/src/PasskeyAttestationStatementVerificationContext.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Identity;
1111
/// <remarks>
1212
/// See <see href="https://www.w3.org/TR/webauthn-3/#verification-procedure"/>.
1313
/// </remarks>
14-
public readonly struct PasskeyAttestationStatementVerificationContext
14+
public sealed class PasskeyAttestationStatementVerificationContext
1515
{
1616
/// <summary>
1717
/// Gets or sets the <see cref="HttpContext"/> for the current request.

‎src/Identity/Core/src/PasskeyOptions.cs‎

Lines changed: 95 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -9,69 +9,132 @@ namespace Microsoft.AspNetCore.Identity;
99
public class PasskeyOptions
1010
{
1111
/// <summary>
12-
/// Gets or sets the time that the server is willing to wait for a passkey operation to complete.
12+
/// Gets or sets the time that the browser should wait for the authenticator to provide a passkey.
1313
/// </summary>
1414
/// <remarks>
15+
/// <para>
16+
/// This option applies to both creating a new passkey and requesting an existing passkey.
17+
/// This is treated as a hint to the browser, and the browser may choose to ignore it.
18+
/// </para>
19+
/// <para>
1520
/// The default value is 5 minutes.
21+
/// </para>
22+
/// <para>
1623
/// See <see href="https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialcreationoptions-timeout"/>
1724
/// and <see href="https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialrequestoptions-timeout"/>.
25+
/// </para>
1826
/// </remarks>
19-
public TimeSpan Timeout { get; set; } = TimeSpan.FromMinutes(5);
27+
public TimeSpan AuthenticatorTimeout { get; set; } = TimeSpan.FromMinutes(5);
2028

2129
/// <summary>
22-
/// The size of the challenge in bytes sent to the client during WebAuthn attestation and assertion.
30+
/// Gets or sets the size of the challenge in bytes sent to the client during attestation and assertion.
2331
/// </summary>
2432
/// <remarks>
33+
/// <para>
34+
/// This option applies to both creating a new passkey and requesting an existing passkey.
35+
/// </para>
36+
/// <para>
2537
/// The default value is 32 bytes.
38+
/// </para>
39+
/// <para>
2640
/// See <see href="https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialcreationoptions-challenge"/>
2741
/// and <see href="https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialrequestoptions-challenge"/>.
42+
/// </para>
2843
/// </remarks>
2944
public int ChallengeSize { get; set; } = 32;
3045

3146
/// <summary>
32-
/// The effective domain of the server. Should be unique and will be used as the identity for the server.
47+
/// Gets or sets the effective domain of the server.
48+
/// This should be unique and will be used as the identity for the server.
3349
/// </summary>
3450
/// <remarks>
51+
/// <para>
52+
/// This option applies to both creating a new passkey and requesting an existing passkey.
53+
/// </para>
54+
/// <para>
3555
/// If left <see langword="null"/>, the server's origin may be used instead.
56+
/// </para>
57+
/// <para>
3658
/// See <see href="https://www.w3.org/TR/webauthn-3/#rp-id"/>.
59+
/// </para>
3760
/// </remarks>
3861
public string? ServerDomain { get; set; }
3962

4063
/// <summary>
4164
/// Gets or sets the user verification requirement.
4265
/// </summary>
4366
/// <remarks>
44-
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-userverificationrequirement"/>.
67+
/// <para>
68+
/// This option applies to both creating a new passkey and requesting an existing passkey.
69+
/// </para>
70+
/// <para>
4571
/// Possible values are "required", "preferred", and "discouraged".
72+
/// </para>
73+
/// <para>
4674
/// If left <see langword="null"/>, the browser defaults to "preferred".
75+
/// </para>
76+
/// <para>
77+
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-userverificationrequirement"/>.
78+
/// </para>
4779
/// </remarks>
4880
public string? UserVerificationRequirement { get; set; }
4981

5082
/// <summary>
5183
/// Gets or sets the extent to which the server desires to create a client-side discoverable credential.
5284
/// </summary>
5385
/// <remarks>
54-
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-residentkeyrequirement"/>.
86+
/// <para>
87+
/// This option only applies when creating a new passkey, and is not enforced on the server.
88+
/// </para>
89+
/// <para>
5590
/// Possible values are "discouraged", "preferred", or "required".
91+
/// </para>
92+
/// <para>
5693
/// If left <see langword="null"/>, the browser defaults to "preferred".
94+
/// </para>
95+
/// <para>
96+
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-residentkeyrequirement"/>.
97+
/// </para>
5798
/// </remarks>
5899
public string? ResidentKeyRequirement { get; set; }
59100

60101
/// <summary>
61102
/// Gets or sets the attestation conveyance preference.
62103
/// </summary>
63104
/// <remarks>
64-
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-attestationconveyancepreference"/>.
105+
/// <para>
106+
/// This option only applies when creating a new passkey, and already-registered passkeys are not affected by it.
107+
/// To validate the attestation statement of a passkey during passkey creation, provide a value for the
108+
/// <see cref="VerifyAttestationStatement"/> option.
109+
/// </para>
110+
/// <para>
111+
/// Possible values are "none", "indirect", "direct", and "enterprise".
112+
/// </para>
113+
/// <para>
65114
/// If left <see langword="null"/>, the browser defaults to "none".
115+
/// </para>
116+
/// <para>
117+
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-attestationconveyancepreference"/>.
118+
/// </para>
66119
/// </remarks>
67120
public string? AttestationConveyancePreference { get; set; }
68121

69122
/// <summary>
70-
/// Gets or sets the authenticator attachment.
123+
/// Gets or sets the allowed authenticator attachment.
71124
/// </summary>
72125
/// <remarks>
73-
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-authenticatorattachment"/>.
126+
/// <para>
127+
/// This option only applies when creating a new passkey, and already-registered passkeys are not affected by it.
128+
/// </para>
129+
/// <para>
130+
/// Possible values are "platform" and "cross-platform".
131+
/// </para>
132+
/// <para>
74133
/// If left <see langword="null"/>, any authenticator attachment modality is allowed.
134+
/// </para>
135+
/// <para>
136+
/// See <see href="https://www.w3.org/TR/webauthn-3/#enumdef-authenticatorattachment"/>.
137+
/// </para>
75138
/// </remarks>
76139
public string? AuthenticatorAttachment { get; set; }
77140

@@ -80,47 +143,42 @@ public class PasskeyOptions
80143
/// is allowed for passkey operations.
81144
/// </summary>
82145
/// <remarks>
83-
/// If <see langword="null"/> all supported algorithms are allowed.
146+
/// <para>
147+
/// This option only applies when creating a new passkey, and already-registered passkeys are not affected by it.
148+
/// </para>
149+
/// <para>
150+
/// If left <see langword="null"/>, all supported algorithms are allowed.
151+
/// </para>
152+
/// <para>
84153
/// See <see href="https://www.iana.org/assignments/cose/cose.xhtml#algorithms"/>.
154+
/// </para>
85155
/// </remarks>
86156
public Func<int, bool>? IsAllowedAlgorithm { get; set; }
87157

88158
/// <summary>
89159
/// Gets or sets a function that validates the origin of the request.
90160
/// </summary>
91161
/// <remarks>
92-
/// By default, this function disallows cross-origin requests and checks
93-
/// that the request's origin header matches the credential's origin.
162+
/// <para>
163+
/// This option applies to both creating a new passkey and requesting an existing passkey.
164+
/// </para>
165+
/// <para>
166+
/// If left <see langword="null"/>, cross-origin requests are disallowed, and the request is only
167+
/// considered valid if the request's origin header matches the credential's origin.
168+
/// </para>
94169
/// </remarks>
95-
public Func<PasskeyOriginValidationContext, Task<bool>> ValidateOrigin { get; set; } = DefaultValidateOrigin;
170+
public Func<PasskeyOriginValidationContext, ValueTask<bool>>? ValidateOrigin { get; set; }
96171

97172
/// <summary>
98173
/// Gets or sets a function that verifies the attestation statement of a passkey.
99174
/// </summary>
100175
/// <remarks>
101-
/// By default, this function does not perform any verification and always returns <see langword="true"/>.
176+
/// <para>
177+
/// This option only applies when creating a new passkey, and already-registered passkeys are not affected by it.
178+
/// </para>
179+
/// <para>
180+
/// If left <see langword="null"/>, this function does not perform any verification and always returns <see langword="true"/>.
181+
/// </para>
102182
/// </remarks>
103-
public Func<PasskeyAttestationStatementVerificationContext, Task<bool>> VerifyAttestationStatement { get; set; } = DefaultVerifyAttestationStatement;
104-
105-
private static Task<bool> DefaultValidateOrigin(PasskeyOriginValidationContext context)
106-
{
107-
var result = IsValidOrigin();
108-
return Task.FromResult(result);
109-
110-
bool IsValidOrigin()
111-
{
112-
if (string.IsNullOrEmpty(context.Origin) ||
113-
context.CrossOrigin ||
114-
!Uri.TryCreate(context.Origin, UriKind.Absolute, out var originUri))
115-
{
116-
return false;
117-
}
118-
119-
// Uri.Equals correctly handles string comparands.
120-
return context.HttpContext.Request.Headers.Origin is [var origin] && originUri.Equals(origin);
121-
}
122-
}
123-
124-
private static Task<bool> DefaultVerifyAttestationStatement(PasskeyAttestationStatementVerificationContext context)
125-
=> Task.FromResult(true);
183+
public Func<PasskeyAttestationStatementVerificationContext, ValueTask<bool>>? VerifyAttestationStatement { get; set; }
126184
}

‎src/Identity/Core/src/PasskeyOriginValidationContext.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Identity;
88
/// <summary>
99
/// Contains information used for determining whether a passkey's origin is valid.
1010
/// </summary>
11-
public readonly struct PasskeyOriginValidationContext
11+
public sealed class PasskeyOriginValidationContext
1212
{
1313
/// <summary>
1414
/// Gets or sets the HTTP context associated with the request.

‎src/Identity/Core/src/PublicAPI.Unshipped.txt‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ Microsoft.AspNetCore.Identity.PasskeyOptions.AttestationConveyancePreference.get
5959
Microsoft.AspNetCore.Identity.PasskeyOptions.AttestationConveyancePreference.set -> void
6060
Microsoft.AspNetCore.Identity.PasskeyOptions.AuthenticatorAttachment.get -> string?
6161
Microsoft.AspNetCore.Identity.PasskeyOptions.AuthenticatorAttachment.set -> void
62+
Microsoft.AspNetCore.Identity.PasskeyOptions.AuthenticatorTimeout.get -> System.TimeSpan
63+
Microsoft.AspNetCore.Identity.PasskeyOptions.AuthenticatorTimeout.set -> void
6264
Microsoft.AspNetCore.Identity.PasskeyOptions.ChallengeSize.get -> int
6365
Microsoft.AspNetCore.Identity.PasskeyOptions.ChallengeSize.set -> void
6466
Microsoft.AspNetCore.Identity.PasskeyOptions.IsAllowedAlgorithm.get -> System.Func<int, bool>?
@@ -68,13 +70,11 @@ Microsoft.AspNetCore.Identity.PasskeyOptions.ResidentKeyRequirement.get -> strin
6870
Microsoft.AspNetCore.Identity.PasskeyOptions.ResidentKeyRequirement.set -> void
6971
Microsoft.AspNetCore.Identity.PasskeyOptions.ServerDomain.get -> string?
7072
Microsoft.AspNetCore.Identity.PasskeyOptions.ServerDomain.set -> void
71-
Microsoft.AspNetCore.Identity.PasskeyOptions.Timeout.get -> System.TimeSpan
72-
Microsoft.AspNetCore.Identity.PasskeyOptions.Timeout.set -> void
7373
Microsoft.AspNetCore.Identity.PasskeyOptions.UserVerificationRequirement.get -> string?
7474
Microsoft.AspNetCore.Identity.PasskeyOptions.UserVerificationRequirement.set -> void
75-
Microsoft.AspNetCore.Identity.PasskeyOptions.ValidateOrigin.get -> System.Func<Microsoft.AspNetCore.Identity.PasskeyOriginValidationContext, System.Threading.Tasks.Task<bool>!>!
75+
Microsoft.AspNetCore.Identity.PasskeyOptions.ValidateOrigin.get -> System.Func<Microsoft.AspNetCore.Identity.PasskeyOriginValidationContext!, System.Threading.Tasks.ValueTask<bool>>?
7676
Microsoft.AspNetCore.Identity.PasskeyOptions.ValidateOrigin.set -> void
77-
Microsoft.AspNetCore.Identity.PasskeyOptions.VerifyAttestationStatement.get -> System.Func<Microsoft.AspNetCore.Identity.PasskeyAttestationStatementVerificationContext, System.Threading.Tasks.Task<bool>!>!
77+
Microsoft.AspNetCore.Identity.PasskeyOptions.VerifyAttestationStatement.get -> System.Func<Microsoft.AspNetCore.Identity.PasskeyAttestationStatementVerificationContext!, System.Threading.Tasks.ValueTask<bool>>?
7878
Microsoft.AspNetCore.Identity.PasskeyOptions.VerifyAttestationStatement.set -> void
7979
Microsoft.AspNetCore.Identity.PasskeyOriginValidationContext
8080
Microsoft.AspNetCore.Identity.PasskeyOriginValidationContext.CrossOrigin.get -> bool

‎src/Identity/EntityFrameworkCore/src/IdentityUserContext.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ internal virtual void OnModelCreatingVersion3(ModelBuilder builder)
287287
b.HasKey(p => p.CredentialId);
288288
b.ToTable("AspNetUserPasskeys");
289289
b.Property(p => p.CredentialId).HasMaxLength(1024); // Defined in WebAuthn spec to be no longer than 1023 bytes
290-
b.Property(p => p.PublicKey).HasMaxLength(1024); // Safe upper limit
290+
b.OwnsOne(p => p.Data).ToJson();
291291
});
292292
}
293293

0 commit comments

Comments
 (0)