Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support Multiple Origins #237

Merged
merged 8 commits into from Nov 18, 2021
Merged

Support Multiple Origins #237

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d570df5
Add support for using multiple origins
Hinton Aug 18, 2021
5f3d7aa
Improve error message by converting the lists to strings
Hinton Aug 18, 2021
b689d81
Changed demo to use origins.
Hinton Aug 18, 2021
ceb8db2
Change Origins to HashSet
Hinton Aug 20, 2021
f998a17
Merge branch 'master' into feature/multiple-origins
aseigler Nov 17, 2021
2ab90c6
Fix conformance test controller, update tests to use origins instead …
aseigler Nov 17, 2021
8ff3e1d
Remove obsolete MDSAccessKey property
aseigler Nov 17, 2021
71efd25
Update BaseVerify() parameters to use ReadOnlySpan<byte> instead of b…
aseigler Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 25 additions & 12 deletions Src/Fido2.Models/Fido2Configuration.cs
View file
Open in desktop
@@ -1,5 +1,6 @@
using System.Net.Http;

using System;
using System.Collections.Generic;

namespace Fido2NetLib
{
public class Fido2Configuration
Expand Down Expand Up @@ -38,16 +39,26 @@ public class Fido2Configuration
/// <summary>
/// Server origin, including protocol host and port.
/// </summary>
public string Origin { get; set; }

/// <summary>
/// MDSAccessKey
/// </summary>
public string MDSAccessKey { get; set; }

/// <summary>
/// MDSCacheDirPath
/// </summary>
[Obsolete("This property is obsolete. Use Origins instead.")]
public string Origin { get; set; }

/// <summary>
/// Server origins, including protocol host and port.
/// </summary>
public List<string> Origins
{
get => _origins ?? new List<string>{ Origin };
set => _origins = value;
}

/// <summary>
/// MDSAccessKey
/// </summary>
public string MDSAccessKey { get; set; }

/// <summary>
/// MDSCacheDirPath
/// </summary>
public string MDSCacheDirPath { get; set; }

/// <summary>
Expand All @@ -56,5 +67,7 @@ public class Fido2Configuration
public Fido2Configuration()
{
}

private List<string> _origins;
}
}
5 changes: 3 additions & 2 deletions Src/Fido2/AuthenticatorAssertionResponse.cs
View file
Open in desktop
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Cryptography;
using System.Text;
Expand Down Expand Up @@ -50,13 +51,13 @@ public static AuthenticatorAssertionResponse Parse(AuthenticatorAssertionRawResp
/// <param name="requestTokenBindingId"></param>
public async Task<AssertionVerificationResult> VerifyAsync(
AssertionOptions options,
string expectedOrigin,
List<string> expectedOrigins,
byte[] storedPublicKey,
uint storedSignatureCounter,
IsUserHandleOwnerOfCredentialIdAsync isUserHandleOwnerOfCredId,
byte[] requestTokenBindingId)
{
BaseVerify(expectedOrigin, options.Challenge, requestTokenBindingId);
BaseVerify(expectedOrigins, options.Challenge, requestTokenBindingId);

if (Raw.Type != PublicKeyCredentialType.PublicKey)
throw new Fido2VerificationException("AssertionResponse Type is not set to public-key");
Expand Down
2 changes: 1 addition & 1 deletion Src/Fido2/AuthenticatorAttestationResponse.cs
View file
Open in desktop
Expand Up @@ -80,7 +80,7 @@ public async Task<AttestationVerificationSuccess> VerifyAsync(CredentialCreateOp
// 5. Verify that the value of C.origin matches the Relying Party's origin.
// 6. Verify that the value of C.tokenBinding.status matches the state of Token Binding for the TLS connection over which the assertion was obtained.
// If Token Binding was used on that TLS connection, also verify that C.tokenBinding.id matches the base64url encoding of the Token Binding ID for the connection.
BaseVerify(config.Origin, originalOptions.Challenge, requestTokenBindingId);
BaseVerify(config.Origins, originalOptions.Challenge, requestTokenBindingId);

if (Raw.Id == null || Raw.Id.Length == 0)
throw new Fido2VerificationException("AttestationResponse is missing Id");
Expand Down
9 changes: 5 additions & 4 deletions Src/Fido2/AuthenticatorResponse.cs
View file
Open in desktop
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Newtonsoft.Json;
Expand Down Expand Up @@ -55,7 +56,7 @@ private AuthenticatorResponse()

// todo: add TokenBinding https://www.w3.org/TR/webauthn/#dictdef-tokenbinding

protected void BaseVerify(string expectedOrigin, byte[] originalChallenge, byte[] requestTokenBindingId)
protected void BaseVerify(List<string> expectedOrigins, byte[] originalChallenge, byte[] requestTokenBindingId)
{
if (Type != "webauthn.create" && Type != "webauthn.get")
throw new Fido2VerificationException($"Type not equal to 'webauthn.create' or 'webauthn.get'. Was: '{Type}'");
Expand All @@ -68,11 +69,11 @@ protected void BaseVerify(string expectedOrigin, byte[] originalChallenge, byte[
throw new Fido2VerificationException("Challenge not equal to original challenge");

var fullyQualifiedOrigin = FullyQualifiedOrigin(Origin);
var fullyQualifiedExpectedOrigin = FullyQualifiedOrigin(expectedOrigin);
var fullyQualifiedExpectedOrigins = expectedOrigins.Select(FullyQualifiedOrigin);

// 5. Verify that the value of C.origin matches the Relying Party's origin.
if (!string.Equals(fullyQualifiedOrigin, fullyQualifiedExpectedOrigin, StringComparison.OrdinalIgnoreCase))
throw new Fido2VerificationException($"Fully qualified origin {fullyQualifiedOrigin} of {Origin} not equal to fully qualified original origin {fullyQualifiedExpectedOrigin} of {expectedOrigin}");
if (!fullyQualifiedExpectedOrigins.Any(o => string.Equals(fullyQualifiedOrigin, o, StringComparison.OrdinalIgnoreCase)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not a dealbreaker, I'm wondering if a HashSet would be better than a List?
My gut feeling is that it would be able to do a lookup without iterating (like linq Any does).

It is mostly relevant if the list of origins grow due to large number of unique origins.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, just need to make sure to normalize the ordinal before comparing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to HashSet, however to avoid iterating and converting them to FullyQualifiedOrigins each time, I had to store them in the Fido2Configuration object. I'm not completely happy with it, but I don't see any other place to put it.

throw new Fido2VerificationException($"Fully qualified origin {fullyQualifiedOrigin} of {Origin} not equal to fully qualified original origin {string.Join(", ", fullyQualifiedExpectedOrigins)} of {string.Join(", ", expectedOrigins)}");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a .Take(MAX_ORIGINS_TO_PRINT) to the list before string joining to stop a very long list of origins to generate huge strings in the exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, any suggestion of what it should be set to?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 5-10 should be enough.
Our original intention was to make sure the developer understod what value we were expecting. I think we could include a count just to avoid confusion now that we are hiding some.


// 6. Verify that the value of C.tokenBinding.status matches the state of Token Binding for the TLS connection over which the assertion was obtained.
// If Token Binding was used on that TLS connection, also verify that C.tokenBinding.id matches the base64url encoding of the Token Binding ID for the connection.
Expand Down
58 changes: 29 additions & 29 deletions Src/Fido2/Fido2NetLib.cs
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
using System.Security.Cryptography;
using System.Threading.Tasks;
using Fido2NetLib.Objects;

namespace Fido2NetLib
{
/// <summary>
Expand All @@ -17,22 +17,22 @@ public partial class Fido2 : IFido2
private readonly IMetadataService _metadataService;

public Fido2(
Fido2Configuration config,
Fido2Configuration config,
IMetadataService metadataService = null)
{
_config = config;
_crypto = RandomNumberGenerator.Create();
_metadataService = metadataService;
}
}

/// <summary>
/// Returns CredentialCreateOptions including a challenge to be sent to the browser/authr to create new credentials
/// </summary>
/// <returns></returns>
/// <param name="excludeCredentials">Recommended. This member is intended for use by Relying Parties that wish to limit the creation of multiple credentials for the same account on a single authenticator.The client is requested to return an error if the new credential would be created on an authenticator that also contains one of the credentials enumerated in this parameter.</param>
public CredentialCreateOptions RequestNewCredential(
Fido2User user,
List<PublicKeyCredentialDescriptor> excludeCredentials,
public CredentialCreateOptions RequestNewCredential(
Fido2User user,
List<PublicKeyCredentialDescriptor> excludeCredentials,
AuthenticationExtensionsClientInputs extensions = null)
{
return RequestNewCredential(user, excludeCredentials, AuthenticatorSelection.Default, AttestationConveyancePreference.None, extensions);
Expand All @@ -44,11 +44,11 @@ public partial class Fido2 : IFido2
/// <returns></returns>
/// <param name="attestationPreference">This member is intended for use by Relying Parties that wish to express their preference for attestation conveyance. The default is none.</param>
/// <param name="excludeCredentials">Recommended. This member is intended for use by Relying Parties that wish to limit the creation of multiple credentials for the same account on a single authenticator.The client is requested to return an error if the new credential would be created on an authenticator that also contains one of the credentials enumerated in this parameter.</param>
public CredentialCreateOptions RequestNewCredential(
Fido2User user,
List<PublicKeyCredentialDescriptor> excludeCredentials,
AuthenticatorSelection authenticatorSelection,
AttestationConveyancePreference attestationPreference,
public CredentialCreateOptions RequestNewCredential(
Fido2User user,
List<PublicKeyCredentialDescriptor> excludeCredentials,
AuthenticatorSelection authenticatorSelection,
AttestationConveyancePreference attestationPreference,
AuthenticationExtensionsClientInputs extensions = null)
{
var challenge = new byte[_config.ChallengeSize];
Expand All @@ -64,10 +64,10 @@ public partial class Fido2 : IFido2
/// <param name="attestationResponse"></param>
/// <param name="origChallenge"></param>
/// <returns></returns>
public async Task<CredentialMakeResult> MakeNewCredentialAsync(
AuthenticatorAttestationRawResponse attestationResponse,
CredentialCreateOptions origChallenge,
IsCredentialIdUniqueToUserAsyncDelegate isCredentialIdUniqueToUser,
public async Task<CredentialMakeResult> MakeNewCredentialAsync(
AuthenticatorAttestationRawResponse attestationResponse,
CredentialCreateOptions origChallenge,
IsCredentialIdUniqueToUserAsyncDelegate isCredentialIdUniqueToUser,
byte[] requestTokenBindingId = null)
{
var parsedResponse = AuthenticatorAttestationResponse.Parse(attestationResponse);
Expand All @@ -86,9 +86,9 @@ public partial class Fido2 : IFido2
/// Returns AssertionOptions including a challenge to the browser/authr to assert existing credentials and authenticate a user.
/// </summary>
/// <returns></returns>
public AssertionOptions GetAssertionOptions(
IEnumerable<PublicKeyCredentialDescriptor> allowedCredentials,
UserVerificationRequirement? userVerification,
public AssertionOptions GetAssertionOptions(
IEnumerable<PublicKeyCredentialDescriptor> allowedCredentials,
UserVerificationRequirement? userVerification,
AuthenticationExtensionsClientInputs extensions = null)
{
var challenge = new byte[_config.ChallengeSize];
Expand All @@ -102,21 +102,21 @@ public partial class Fido2 : IFido2
/// Verifies the assertion response from the browser/authr to assert existing credentials and authenticate a user.
/// </summary>
/// <returns></returns>
public async Task<AssertionVerificationResult> MakeAssertionAsync(
AuthenticatorAssertionRawResponse assertionResponse,
AssertionOptions originalOptions,
byte[] storedPublicKey,
uint storedSignatureCounter,
IsUserHandleOwnerOfCredentialIdAsync isUserHandleOwnerOfCredentialIdCallback,
public async Task<AssertionVerificationResult> MakeAssertionAsync(
AuthenticatorAssertionRawResponse assertionResponse,
AssertionOptions originalOptions,
byte[] storedPublicKey,
uint storedSignatureCounter,
IsUserHandleOwnerOfCredentialIdAsync isUserHandleOwnerOfCredentialIdCallback,
byte[] requestTokenBindingId = null)
{
var parsedResponse = AuthenticatorAssertionResponse.Parse(assertionResponse);

var result = await parsedResponse.VerifyAsync(originalOptions,
_config.Origin,
storedPublicKey,
storedSignatureCounter,
isUserHandleOwnerOfCredentialIdCallback,
var result = await parsedResponse.VerifyAsync(originalOptions,
_config.Origins,
storedPublicKey,
storedSignatureCounter,
isUserHandleOwnerOfCredentialIdCallback,
requestTokenBindingId);

return result;
Expand Down