Support Multiple Origins #237

Hinton · 2021-08-18T19:04:41Z

Objective

Native android applications supports the FIDO2 protocol, but diverges slightly from the WebAuthN spec in that they set the origin field in the Attestation and Assertion to android:apk-key-hash:<hash>. Since setting Fido2Configuration.Origin to this value would cause web clients to fail we need a way to handle multiple allowed origins.

This PR deprecates the Fido2Configuration.Origin property and replace it with Fido2Configuration.Origins. It falls back gracefully to Origin should Origins not be set.

I aimed to not modifying any public API, but unfortunately both AuthenticatorAssertionResponse and AuthenticatorResponse signature was changed slightly to support a list of origins. Let me know if I should look into adding new methods instead to ensure backwards compatibility.

Noteworthy Code Changes

Src/Fido2.Models/Fido2Configuration.cs:
Src/Fido2/AuthenticatorAssertionResponse.cs: Descriptions of file changes should be descriptive but brief, code should "speak for itself" however this enables the author to further describe the logic or theory behind the change.
Src/Fido2/AuthenticatorResponse.cs: Changed BaseVerify to accept an list of origins.
Src/Fido2/Fido2NetLib.cs: Changed to use Origins instead of Origin. This file seemed to contain mixed linebreaks. When saving they all changed to LF (\n). Let me know if we want to avoid these change)

Resolves: #220

dnfadmin · 2021-08-18T19:04:54Z

All CLA requirements met.

codecov-commenter · 2021-08-18T19:11:27Z

Codecov Report

Merging #237 (71efd25) into master (1ea74bc) will decrease coverage by 0.20%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   79.31%   79.10%   -0.21%     
==========================================
  Files          82       83       +1     
  Lines        3161     3173      +12     
==========================================
+ Hits         2507     2510       +3     
- Misses        654      663       +9

Impacted Files	Coverage Δ
Src/Fido2.Models/Fido2Configuration.cs	`72.22% <66.66%> (-9.60%)`	⬇️
Src/Fido2.Models/StringExtensions.cs	`100.00% <100.00%> (ø)`
Src/Fido2/AuthenticatorAssertionResponse.cs	`77.94% <100.00%> (ø)`
Src/Fido2/AuthenticatorAttestationResponse.cs	`88.70% <100.00%> (ø)`
Src/Fido2/AuthenticatorResponse.cs	`64.10% <100.00%> (-15.90%)`	⬇️
Src/Fido2/Fido2NetLib.cs	`81.25% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea74bc...71efd25. Read the comment docs.

abergs · 2021-08-19T14:41:44Z

Src/Fido2/AuthenticatorResponse.cs


            // 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)))


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.

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

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.

abergs · 2021-08-19T14:45:26Z

Src/Fido2/AuthenticatorResponse.cs

@@ -73,7 +73,7 @@ protected void BaseVerify(List<string> expectedOrigins, byte[] originalChallenge

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


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

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

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.

aseigler · 2021-09-01T18:43:17Z

So all the unit tests seem ok, but this PR breaks conformance tests currently. This is what I am seeing:

Fido2NetLib.Fido2VerificationException: Fully qualified origin https://localhost:44329 of https://localhost:44329 not equal to fully qualified original origin  (1)
   at Fido2NetLib.AuthenticatorResponse.BaseVerify(HashSet`1 fullyQualifiedExpectedOrigins, Byte[] originalChallenge, Byte[] requestTokenBindingId) in source\repos\fido2-net-lib\Src\Fido2\AuthenticatorResponse.cs:line 81
   at Fido2NetLib.AuthenticatorAttestationResponse.VerifyAsync(CredentialCreateOptions originalOptions, Fido2Configuration config, IsCredentialIdUniqueToUserAsyncDelegate isCredentialIdUniqueToUser, IMetadataService metadataService, Byte[] requestTokenBindingId) in source\repos\fido2-net-lib\Src\Fido2\AuthenticatorAttestationResponse.cs:line 83
   at Fido2NetLib.Fido2.MakeNewCredentialAsync(AuthenticatorAttestationRawResponse attestationResponse, CredentialCreateOptions origChallenge, IsCredentialIdUniqueToUserAsyncDelegate isCredentialIdUniqueToUser, Byte[] requestTokenBindingId) in source\repos\fido2-net-lib\Src\Fido2\Fido2NetLib.cs:line 74
   at Fido2Demo.TestController.MakeCredentialResultTest(AuthenticatorAttestationRawResponse attestationResponse) in source\repos\fido2-net-lib\Demo\TestController.cs:line 106

Hinton · 2021-09-08T12:11:29Z

@aseigler Hmm, I get a similar error if I leave Origin and Origins empty in services.AddFido2. I don't know much about the conformance process, but is it possible the origin is set to an empty value in appsettings.json?

We ended up not needing these changes after all, mainly due to iOS not natively supporting WebAuthn outside Safari (and AsWebAuthenticationSession), to minimize the dual maintenance we went with the same approach on Android. This allowed us to keep using only a single Origin. However I still believe there's value in getting this PR merged as it would add support for the native fido2 apis on Android.

…of origin

…yte[]

aseigler · 2021-11-18T00:45:04Z

I am happy with this now. Thanks!

Hinton · 2021-11-18T09:32:38Z

@aseigler Thanks for the help in getting this merged.

Hinton added 3 commits August 18, 2021 20:14

Add support for using multiple origins

d570df5

Improve error message by converting the lists to strings

5f3d7aa

Changed demo to use origins.

b689d81

abergs mentioned this pull request Aug 19, 2021

Checking Origin #220

Closed

abergs requested review from aseigler and abergs August 19, 2021 14:36

abergs reviewed Aug 19, 2021

View reviewed changes

Change Origins to HashSet

ceb8db2

Hinton requested a review from abergs August 20, 2021 09:15

mpbw2 mentioned this pull request Aug 27, 2021

FIDO2 WebAuthn support for mobile bitwarden/mobile#1519

Merged

aseigler added feature New feature enhancement Enhancements or general improvements labels Sep 1, 2021

aseigler mentioned this pull request Nov 15, 2021

Release 3.0 #264

Closed

aseigler added 4 commits November 17, 2021 09:33

Merge branch 'master' into feature/multiple-origins

f998a17

Fix conformance test controller, update tests to use origins instead …

2ab90c6

…of origin

Remove obsolete MDSAccessKey property

8ff3e1d

Update BaseVerify() parameters to use ReadOnlySpan<byte> instead of b…

71efd25

…yte[]

aseigler approved these changes Nov 17, 2021

View reviewed changes

aseigler self-assigned this Nov 17, 2021

aseigler merged commit 25a3013 into passwordless-lib:master Nov 18, 2021

aseigler mentioned this pull request Jan 9, 2023

Adding support for android devices go-webauthn/webauthn#92

Closed

aseigler mentioned this pull request Mar 9, 2023

Automatically add https scheme to fully qualify origin #385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Multiple Origins #237

Support Multiple Origins #237

Hinton commented Aug 18, 2021 •

edited

dnfadmin commented Aug 18, 2021 •

edited

codecov-commenter commented Aug 18, 2021 •

edited

abergs Aug 19, 2021

Hinton Aug 19, 2021

Hinton Aug 20, 2021

abergs Aug 19, 2021

Hinton Aug 19, 2021

abergs Aug 19, 2021

aseigler commented Sep 1, 2021

Hinton commented Sep 8, 2021 •

edited

aseigler commented Nov 18, 2021

Hinton commented Nov 18, 2021

Support Multiple Origins #237

Support Multiple Origins #237

Conversation

Hinton commented Aug 18, 2021 • edited

Objective

Noteworthy Code Changes

dnfadmin commented Aug 18, 2021 • edited

codecov-commenter commented Aug 18, 2021 • edited

Codecov Report

abergs Aug 19, 2021

Choose a reason for hiding this comment

Hinton Aug 19, 2021

Choose a reason for hiding this comment

Hinton Aug 20, 2021

Choose a reason for hiding this comment

abergs Aug 19, 2021

Choose a reason for hiding this comment

Hinton Aug 19, 2021

Choose a reason for hiding this comment

abergs Aug 19, 2021

Choose a reason for hiding this comment

aseigler commented Sep 1, 2021

Hinton commented Sep 8, 2021 • edited

aseigler commented Nov 18, 2021

Hinton commented Nov 18, 2021

Hinton commented Aug 18, 2021 •

edited

dnfadmin commented Aug 18, 2021 •

edited

codecov-commenter commented Aug 18, 2021 •

edited

Hinton commented Sep 8, 2021 •

edited