WIP: Support device public key and passkeys #356

aseigler · 2022-12-13T02:12:21Z

Resolves #334 (and #350).

codecov-commenter · 2022-12-14T03:01:54Z

Codecov Report

Merging #356 (457412d) into master (5e72564) will decrease coverage by 2.11%.
The diff coverage is 42.91%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
- Coverage   76.80%   74.70%   -2.11%     
==========================================
  Files          89       92       +3     
  Lines        2531     2728     +197     
  Branches      427      456      +29     
==========================================
+ Hits         1944     2038      +94     
- Misses        466      577     +111     
+ Partials      121      113       -8

Impacted Files	Coverage Δ
...ls/Objects/AuthenticationExtensionsClientInputs.cs	`14.28% <0.00%> (-5.72%)`	⬇️
...s/AuthenticationExtensionsDevicePublicKeyInputs.cs	`0.00% <0.00%> (ø)`
Src/Fido2/AuthenticatorResponse.cs	`57.14% <ø> (ø)`
Src/Fido2/DevelopmentInMemoryStore.cs	`0.00% <0.00%> (ø)`
Src/Fido2/Objects/AttestedCredentialData.cs	`82.14% <0.00%> (-3.32%)`	⬇️
Src/Fido2/AuthenticatorAssertionResponse.cs	`43.04% <23.07%> (-7.81%)`	⬇️
...ido2/Objects/DevicePublicKeyAuthenticatorOutput.cs	`41.37% <41.37%> (ø)`
...s/Objects/AuthenticationExtensionsClientOutputs.cs	`85.71% <50.00%> (-14.29%)`	⬇️
Src/Fido2/AuthenticatorAttestationResponse.cs	`76.00% <61.53%> (-10.96%)`	⬇️
...2.Models/Objects/AttestationVerificationSuccess.cs	`90.00% <85.71%> (-10.00%)`	⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Src/Fido2.Models/Objects/AuthenticationExtensionsDevicePublicKeyInputs.cs

Src/Fido2.Models/Objects/AuthenticationExtensionsDevicePublicKeyOutputs.cs

Src/Fido2/Objects/AuthenticatorData.cs

Src/Fido2/Objects/DevicePublicKeyAuthenticatorOutput.cs

Src/Fido2/Objects/AttestedCredentialData.cs

Src/Fido2/DevelopmentInMemoryStore.cs

Src/Fido2/AuthenticatorAssertionResponse.cs

Src/Fido2.Models/Objects/PublicKeyCredentialType.cs

Src/Fido2.Models/Objects/AssertionVerificationResult.cs

iamcarbon · 2023-01-18T01:21:50Z

Hi @aseigler -- anything functionality left here?

into feat-passkey

aseigler · 2023-01-21T03:56:54Z

Hi @aseigler -- anything functionality left here?

Not sure, need to do another pass on the DPK things to make sure I've got all the logic straight. The passkey area I think is good to go.

Src/Fido2/Objects/DevicePublicKeyAuthenticatorOutput.cs

aseigler · 2023-01-27T03:15:21Z

Still need to make RP configurable BE/BS options and plumb those into registration and login flows before passkey is done.

DPK looks like it has object names that have been updated.

Reminder to create CredentialRecord object, move existing StoredCredential members there and hand around that credential record instead of parts of it in and out of login flow.

Reminder to remove the rest of the references to token binding.

…on flows

rudiv · 2023-02-19T16:04:38Z

Demo/Startup.cs

@@ -52,6 +52,8 @@ public void ConfigureServices(IServiceCollection services)
            options.Origins = Configuration.GetSection("fido2:origins").Get<HashSet<string>>();
            options.TimestampDriftTolerance = Configuration.GetValue<int>("fido2:timestampDriftTolerance");
            options.MDSCacheDirPath = Configuration["fido2:MDSCacheDirPath"];
+            options.AllowBackupEligibleCredential = Configuration.GetValue<bool>("fido2:allowBackupEligibleCredential");


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.

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.

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.

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?

@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.

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.

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.

From a more consumer-focused point of view (ie. specifically the well marketed "Passkeys" aspect) we may want to accept only Passkeys at first

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.

iamcarbon · 2023-03-04T01:07:27Z

@aseigler Anything else left here before we merge?

aseigler · 2023-03-04T03:35:20Z

@aseigler Anything else left here before we merge?

I think the only thing that hasn't been gotten to in the list was moving StoredCredential to CredentialRecord but that can be done separately, no need to hold this up for that.

Update BE/BS policy logic Add more assertion tests, move more error strings to error messages Get metadata into assertion flow for DPK

Dnnzk · 2023-05-06T09:13:52Z

Hey! Any news on this?

abergs

Great work on these two large features. I have some comments, but I'm happy with the changes.

abergs · 2023-06-21T13:03:34Z

Demo/TestController.cs

+        _demoStorage.UpdateCounter(res.CredentialId, res.Counter);
+
+        if (res.DevicePublicKey is not null)
+            creds.DevicePublicKeys.Add(res.DevicePublicKey);


I don't understand what we are doing here?

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.

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 res.DevicePublicKey but in our demo we just add it directly to our in memory storage?

Yeah, basically same as registering a new credential against an existing user, but way more complicated.

Alright makes sense, I was just confused about how we used it in our demo. When I get the time I'll add a // TODO: Check policy if we allow new devices, if-so, store it to db

abergs · 2023-06-21T18:08:53Z

Src/Fido2/AuthenticatorAttestationResponse.cs

+            Id = authData.AttestedCredentialData.CredentialID,
+            PublicKey = authData.AttestedCredentialData.CredentialPublicKey.GetBytes(),
+            SignCount = authData.SignCount,
+            //Transports = result of response.getTransports();


Why do we not return Transports anymore?

We didn't return transports before, in fact we didn't even collect it in the JavaScript at all, I added in this but it looks like it needs to be added to all of the other registration JavaScript files, and collect transports in AuthenticatorAttestationRawResponse.ResponseData, and then populate transports in the controller from the raw response. There are a few reasons to collect transports, to help with user facing dialogs, but also to compare what transports are being used against a policy, or comparing what transports should be available according to the authenticator metadata. Basically this is incomplete at the moment.

abergs · 2023-06-21T18:11:56Z

Src/Fido2/DevelopmentInMemoryStore.cs

    public byte[] UserId { get; set; }
+
    public PublicKeyCredentialDescriptor Descriptor { get; set; }


Should we deprecate the descriptor or do we still have a use for it? (Now that we've added Id, PublicKey, Type to the class itself)

Yeah, I don't see a reason to store both Id and Descriptor.

abergs · 2023-06-21T18:14:59Z

Src/Fido2.Models/Objects/AttestationVerificationSuccess.cs

+    /// <summary>
+    /// The Credential ID of the public key credential source.
+    /// </summary>
+    public byte[] Id { get; set; }


Do we want to use the same JsonConverter as we do on PublicKey?

Already done in AuthenticatorAttestationRawResponse, no?

abergs · 2023-06-21T18:15:31Z

Src/Fido2.Models/Objects/AttestationVerificationSuccess.cs

+    /// 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; }


Do we want to use JsonConverter on this and AttestationClientDataJSON?

Already done in AuthenticatorAttestationRawResponse, no?

Start DPK and passkey support

91d7056

aseigler added the feature New feature label Dec 13, 2022

aseigler self-assigned this Dec 13, 2022

aseigler marked this pull request as draft December 13, 2022 13:21

Merge branch 'master' into feat-passkey

c685ec3

iamcarbon reviewed Jan 11, 2023

View reviewed changes

iamcarbon mentioned this pull request Jan 18, 2023

Strongly type AaGuid #362

Merged

aseigler added 3 commits January 20, 2023 22:48

Cleanup

b14ff27

Merge branch 'feat-passkey' of https://github.com/aseigler/fido2-net-lib

c499134

into feat-passkey

Merge branch 'master' into feat-passkey

134e01f

iamcarbon reviewed Jan 24, 2023

View reviewed changes

Src/Fido2/Objects/DevicePublicKeyAuthenticatorOutput.cs Show resolved Hide resolved

iamcarbon suggested changes Jan 24, 2023

View reviewed changes

aseigler added 3 commits January 26, 2023 18:52

Merge

eb40bd4

Finish merge

8555ab6

Additional cleanup

f8fe767

aseigler added 3 commits February 11, 2023 18:18

Make BE and BS config options plumb them into attestation and asserti…

4f82fa3

…on flows

Update DPK verification names to match current spec verbiage

c64bd03

Remove token binding

d969c66

rudiv reviewed Feb 19, 2023

View reviewed changes

Set default to allow backed up credentials

0131ee7

aseigler marked this pull request as ready for review March 4, 2023 03:35

aseigler requested review from abergs and iamcarbon March 4, 2023 15:48

aseigler marked this pull request as draft March 6, 2023 13:22

Switch BE/BS config options from bool to enum

457412d

Update BE/BS policy logic Add more assertion tests, move more error strings to error messages Get metadata into assertion flow for DPK

aseigler marked this pull request as ready for review April 9, 2023 16:16

abergs mentioned this pull request Jun 8, 2023

Status of Project #396

Closed

abergs mentioned this pull request Jun 21, 2023

NuGet Package is out of date #400

Closed

abergs approved these changes Jun 21, 2023

View reviewed changes

abergs merged commit 7b92f4a into passwordless-lib:master Jun 21, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Support device public key and passkeys #356

WIP: Support device public key and passkeys #356

aseigler commented Dec 13, 2022 •

edited

codecov-commenter commented Dec 14, 2022 •

edited

iamcarbon commented Jan 18, 2023 •

edited

aseigler commented Jan 21, 2023

aseigler commented Jan 27, 2023

rudiv Feb 19, 2023 •

edited

aseigler Feb 20, 2023

rudiv Feb 20, 2023

abergs Feb 24, 2023

aseigler Mar 4, 2023

rudiv Mar 6, 2023

aseigler Mar 6, 2023 •

edited

iamcarbon commented Mar 4, 2023

aseigler commented Mar 4, 2023

Dnnzk commented May 6, 2023

abergs left a comment

abergs Jun 21, 2023

aseigler Jun 26, 2023

abergs Jun 26, 2023 •

edited

aseigler Jun 26, 2023

abergs Jun 26, 2023

abergs Jun 21, 2023

aseigler Jun 26, 2023

abergs Jun 21, 2023 •

edited

aseigler Jun 26, 2023

abergs Jun 21, 2023

aseigler Jun 26, 2023

abergs Jun 21, 2023

aseigler Jun 26, 2023

		public byte[] UserId { get; set; }

		public PublicKeyCredentialDescriptor Descriptor { get; set; }

WIP: Support device public key and passkeys #356

WIP: Support device public key and passkeys #356

Conversation

aseigler commented Dec 13, 2022 • edited

codecov-commenter commented Dec 14, 2022 • edited

Codecov Report

iamcarbon commented Jan 18, 2023 • edited

aseigler commented Jan 21, 2023

aseigler commented Jan 27, 2023

rudiv Feb 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseigler Mar 6, 2023 • edited

Choose a reason for hiding this comment

iamcarbon commented Mar 4, 2023

aseigler commented Mar 4, 2023

Dnnzk commented May 6, 2023

abergs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abergs Jun 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abergs Jun 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseigler commented Dec 13, 2022 •

edited

codecov-commenter commented Dec 14, 2022 •

edited

iamcarbon commented Jan 18, 2023 •

edited

rudiv Feb 19, 2023 •

edited

aseigler Mar 6, 2023 •

edited

abergs Jun 26, 2023 •

edited

abergs Jun 21, 2023 •

edited