Repository navigation
WIP: Support device public key and passkeys #356
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
Changes from 9 commits
91d7056
c685ec3
b14ff27
c499134
134e01f
eb40bd4
8555ab6
f8fe767
4f82fa3
c64bd03
d969c66
0131ee7
457412d
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 |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ namespace Fido2Demo; | |
| public class TestController : Controller | ||
| { | ||
| /* CONFORMANCE TESTING ENDPOINTS */ | ||
| private static readonly DevelopmentInMemoryStore DemoStorage = new (); | ||
| private static readonly DevelopmentInMemoryStore _demoStorage = new (); | ||
|
|
||
| private readonly IFido2 _fido2; | ||
| private readonly string _origin; | ||
|
|
@@ -56,15 +56,15 @@ public JsonResult MakeCredentialOptionsTest([FromBody] TEST_MakeCredentialParams | |
| } | ||
|
|
||
| // 1. Get user from DB by username (in our example, auto create missing users) | ||
| var user = DemoStorage.GetOrAddUser(opts.Username, () => new Fido2User | ||
| var user = _demoStorage.GetOrAddUser(opts.Username, () => new Fido2User | ||
| { | ||
| DisplayName = opts.DisplayName, | ||
| Name = opts.Username, | ||
| Id = username // byte representation of userID is required | ||
| }); | ||
|
|
||
| // 2. Get user existing keys by username | ||
| var existingKeys = DemoStorage.GetCredentialsByUser(user).Select(c => c.Descriptor).ToList(); | ||
| var existingKeys = _demoStorage.GetCredentialsByUser(user).Select(c => c.Descriptor).ToList(); | ||
|
|
||
| //var exts = new AuthenticationExtensionsClientInputs() { Extensions = true, UserVerificationIndex = true, Location = true, UserVerificationMethod = true, BiometricAuthenticatorPerformanceBounds = new AuthenticatorBiometricPerfBounds { FAR = float.MaxValue, FRR = float.MaxValue } }; | ||
| var exts = new AuthenticationExtensionsClientInputs() { }; | ||
|
|
@@ -83,7 +83,7 @@ public JsonResult MakeCredentialOptionsTest([FromBody] TEST_MakeCredentialParams | |
|
|
||
| [HttpPost] | ||
| [Route("/attestation/result")] | ||
| public async Task<JsonResult> MakeCredentialResultTest([FromBody] AuthenticatorAttestationRawResponse attestationResponse, CancellationToken cancellationToken) | ||
| public async Task<JsonResult> MakeCredentialResultTestAsync([FromBody] AuthenticatorAttestationRawResponse attestationResponse, CancellationToken cancellationToken) | ||
| { | ||
|
|
||
| // 1. get the options we sent the client | ||
|
|
@@ -93,20 +93,20 @@ public async Task<JsonResult> MakeCredentialResultTest([FromBody] AuthenticatorA | |
| // 2. Create callback so that lib can verify credential id is unique to this user | ||
| IsCredentialIdUniqueToUserAsyncDelegate callback = static async (args, cancellationToken) => | ||
| { | ||
| var users = await DemoStorage.GetUsersByCredentialIdAsync(args.CredentialId, cancellationToken); | ||
| var users = await _demoStorage.GetUsersByCredentialIdAsync(args.CredentialId, cancellationToken); | ||
| return users.Count <= 0; | ||
| }; | ||
|
|
||
| // 2. Verify and make the credentials | ||
| var success = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback, cancellationToken: cancellationToken); | ||
|
|
||
| // 3. Store the credentials in db | ||
| DemoStorage.AddCredentialToUser(options.User, new StoredCredential | ||
| _demoStorage.AddCredentialToUser(options.User, new StoredCredential | ||
| { | ||
| Descriptor = new PublicKeyCredentialDescriptor(success.Result.CredentialId), | ||
| PublicKey = success.Result.PublicKey, | ||
| UserHandle = success.Result.User.Id, | ||
| SignatureCounter = success.Result.Counter | ||
| SignCount = success.Result.Counter | ||
| }); | ||
|
|
||
| // 4. return "ok" to the client | ||
|
|
@@ -119,12 +119,12 @@ public IActionResult AssertionOptionsTest([FromBody] TEST_AssertionClientParams | |
| { | ||
| var username = assertionClientParams.Username; | ||
| // 1. Get user from DB | ||
| var user = DemoStorage.GetUser(username); | ||
| var user = _demoStorage.GetUser(username); | ||
| if (user == null) | ||
| return NotFound("username was not registered"); | ||
|
|
||
| // 2. Get registered credentials from database | ||
| var existingCredentials = DemoStorage.GetCredentialsByUser(user).Select(c => c.Descriptor).ToList(); | ||
| var existingCredentials = _demoStorage.GetCredentialsByUser(user).Select(c => c.Descriptor).ToList(); | ||
|
|
||
| var uv = assertionClientParams.UserVerification; | ||
| if (null != assertionClientParams.authenticatorSelection) | ||
|
|
@@ -154,30 +154,33 @@ public IActionResult AssertionOptionsTest([FromBody] TEST_AssertionClientParams | |
|
|
||
| [HttpPost] | ||
| [Route("/assertion/result")] | ||
| public async Task<JsonResult> MakeAssertionTest([FromBody] AuthenticatorAssertionRawResponse clientResponse, CancellationToken cancellationToken) | ||
| public async Task<JsonResult> MakeAssertionTestAsync([FromBody] AuthenticatorAssertionRawResponse clientResponse, CancellationToken cancellationToken) | ||
| { | ||
| // 1. Get the assertion options we sent the client | ||
| var jsonOptions = HttpContext.Session.GetString("fido2.assertionOptions"); | ||
| var options = AssertionOptions.FromJson(jsonOptions); | ||
|
|
||
| // 2. Get registered credential from database | ||
| var creds = DemoStorage.GetCredentialById(clientResponse.Id); | ||
| var creds = _demoStorage.GetCredentialById(clientResponse.Id); | ||
|
|
||
| // 3. Get credential counter from database | ||
| var storedCounter = creds.SignatureCounter; | ||
|
|
||
| // 4. Create callback to check if userhandle owns the credentialId | ||
| IsUserHandleOwnerOfCredentialIdAsync callback = static async (args, cancellationToken) => | ||
| { | ||
| var storedCreds = await DemoStorage.GetCredentialsByUserHandleAsync(args.UserHandle, cancellationToken); | ||
| var storedCreds = await _demoStorage.GetCredentialsByUserHandleAsync(args.UserHandle, cancellationToken); | ||
| return storedCreds.Exists(c => c.Descriptor.Id.SequenceEqual(args.CredentialId)); | ||
| }; | ||
|
|
||
| // 5. Make the assertion | ||
| var res = await _fido2.MakeAssertionAsync(clientResponse, options, creds.PublicKey, storedCounter, callback, cancellationToken: cancellationToken); | ||
| var res = await _fido2.MakeAssertionAsync(clientResponse, options, creds.PublicKey, creds.DevicePublicKeys, storedCounter, callback, cancellationToken: cancellationToken); | ||
|
|
||
| // 6. Store the updated counter | ||
| DemoStorage.UpdateCounter(res.CredentialId, res.Counter); | ||
| _demoStorage.UpdateCounter(res.CredentialId, res.Counter); | ||
|
|
||
| if (res.DevicePublicKey is not null) | ||
| creds.DevicePublicKeys.Add(res.DevicePublicKey); | ||
|
Collaborator
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. I don't understand what we are doing here?
Collaborator
Author
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. Basically if a new device public key was discovered in the assertion flow, add it to the list of device public keys associated with this credential.
Collaborator
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. Hmm... I think I understand @aseigler. Am I right to think that before saving the DPK the RP would perhaps check some policy or trigger a risk based ceremony before trusting that DPK and then saving/updating the storage with
Collaborator
Author
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. Yeah, basically same as registering a new credential against an existing user, but way more complicated.
Collaborator
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. Alright makes sense, I was just confused about how we used it in our demo. When I get the time I'll add a |
||
|
|
||
| var testRes = new | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,18 +9,40 @@ namespace Fido2NetLib.Objects; | |
| /// </summary> | ||
| public class AttestationVerificationSuccess : AssertionVerificationResult | ||
| { | ||
| [JsonConverter(typeof(Base64UrlConverter))] | ||
| public byte[] PublicKey { get; set; } | ||
|
|
||
| public Fido2User User { get; set; } | ||
|
|
||
| public string CredType { get; set; } | ||
|
|
||
| public Guid AaGuid { get; set; } | ||
|
|
||
| #nullable enable | ||
| public X509Certificate2? AttestationCertificate { get; set; } | ||
| #nullable disable | ||
|
|
||
| public X509Certificate2[] AttestationCertificateChain { get; set; } | ||
| /// <summary> | ||
| /// The type of the public key credential source. | ||
| /// </summary> | ||
| public PublicKeyCredentialType Type { get; set; } = PublicKeyCredentialType.PublicKey; | ||
| /// <summary> | ||
| /// The Credential ID of the public key credential source. | ||
| /// </summary> | ||
| public byte[] Id { get; set; } | ||
|
Collaborator
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. Do we want to use the same JsonConverter as we do on PublicKey?
Collaborator
Author
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. Already done in AuthenticatorAttestationRawResponse, no? |
||
| /// <summary> | ||
| /// The credential public key of the public key credential source. | ||
| /// </summary> | ||
| [JsonConverter(typeof(Base64UrlConverter))] | ||
| public byte[] PublicKey { get; set; } | ||
| /// <summary> | ||
| /// The value returned from getTransports() when the public key credential source was registered. | ||
| /// </summary> | ||
| public AuthenticatorTransport[] Transports { get; set; } | ||
| /// <summary> | ||
| /// The value of the BE flag when the public key credential source was created. | ||
| /// </summary> | ||
| public bool BE { get; set; } | ||
| /// <summary> | ||
| /// The value of the attestationObject attribute when the public key credential source was registered. | ||
| /// Storing this enables the Relying Party to reference the credential's attestation statement at a later time. | ||
| /// </summary> | ||
| public byte[] AttestationObject { get; set; } | ||
|
Collaborator
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. Do we want to use JsonConverter on this and
Collaborator
Author
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. Already done in AuthenticatorAttestationRawResponse, no? |
||
| /// <summary> | ||
| /// The value of the clientDataJSON attribute when the public key credential source was registered. | ||
| /// Storing this in combination with the above attestationObject item enables the Relying Party to re-verify the attestation signature at a later time. | ||
| /// </summary> | ||
| public byte[] AttestationClientDataJSON { get; set; } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| namespace Fido2NetLib.Objects; | ||
|
|
||
| using System; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| public sealed class AuthenticationExtensionsDevicePublicKeyInputs | ||
| { | ||
| [JsonPropertyName("attestation")] | ||
| public string Attestation { get; set; } = "none"; | ||
|
|
||
| [JsonPropertyName("attestationFormats")] | ||
| public string[] AttestationFormats { get; set; } = Array.Empty<string>(); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
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 argue that these be allowed by default to support a happier upgrade path. Some RPs may not care about these statuses at all, and so their existing configuration would break?
I know this comment is against the Demo project, but the Configuration default would also be false if this wasn't decided / set at startup.
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 hear ya. This is a enterprise vs consumer setting in my eyes, and I personally am enterprise focused. When this PR is in a release it will be part of a major update, and we are expecting that will contain some breaking changes. Regardless of which way it goes by default, there will be notes to help guide implementers.
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 agree, I think my main concern is that this may block typical use cases.
For an example (even thinking enterprise), you may wish for your users to have a passkey that's backed up and set this setting. However, they would then be unable to add a physical key (Yubikey et al). I know this is the whole point of "Passkeys" to not need it, but I feel that it's more of a per-key decision than one that should be set globally for the RP?
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.
@aseigler I agree that we should default to true. Given that the average developer might not know what this option do (and should be True for them), while I think we can expect enterprise developers have more knowledge and know to turn it off because of their own research/policy for deploying fido2.
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.
They would actually be able to register a typical security key with this setting true or false, as those do not set BE flag. Currently only certain platform authenticators do. It's undefined how the platform is protecting those BE keys and where they are going to be restored to. That's why in a situation where the RP cares (enterprise), they might wish to restrict BE keys from being registered at all on the platform, or maybe allow some blessed authenticators but not others (would require additional work to support this). I guess there might be a reason an RP would only want to allow BE keys? That's not the result or intent of this implementation, at least currently.
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.
Understand the Enterprise point of view.
From a more consumer-focused point of view (ie. specifically the well marketed "Passkeys" aspect) we may want to accept only Passkeys at first, to ensure that users are not able to lose access very easily. Whilst it's not perfect and most authenticators don't give on where the keys are from / stored, knowing that they're backed up allows it to be reasonably assumed that the key is durable and isn't going to disappear like in the case of a physical key loss. For example, we explicitly advise the user to register with a backed up authenticator (specifically iCloud backed Passkeys or their Android phone).
I might be wrong in assuming that this is where this library is going to see most of its use in the short-mid term, but that's my 2c on why this default would be the wrong one.
Uh oh!
There was an error while loading. Please reload this page.
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.
First thing: https://shouldicapitalizepasskey.com
Sounds crazy to me, but if that's what people want to do with this it's easy enough to switch the logic for BE and BS so instead of true/false the policy can be allow/disallow/require for both registration and authentication flows with the defaults being the most "normal" allow but giving the RP admin the ability to ability for more strict control.