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

Changes from 1 commit
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
2 changes: 1 addition & 1 deletion Src/Fido2/AuthenticatorResponse.cs
View file
Open in desktop
Expand Up @@ -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)))
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 {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)}");
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