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.

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

Pass a list of allowed origins instead of a single origin #184

Merged
merged 16 commits into from
Apr 2, 2024

Conversation

arianvp
Copy link
Contributor

@arianvp arianvp commented Mar 25, 2024

Android and iOS also support WebAuthn just like browsers.

The apps will use their AppStore/PlayStore AppID as the origin. This means we need to allow a list of origins instead of a single origin.

Apple uses https://developer.apple.com/documentation/xcode/supporting-associated-domains to link the app origin to the RpId

Google uses an assetlinks.json file:
https://developers.google.com/identity/fido/android/native-apps#interoperability_with_your_website

Android and iOS also support WebAuthn just like browsers.

The apps will use their AppStore/PlayStore AppID as the origin. This
means we need to allow a list of origins instead of a single origin.

Apple uses https://developer.apple.com/documentation/xcode/supporting-associated-domains to link
the app origin to the RpId

Google uses an assetlinks.json file:
https://developers.google.com/identity/fido/android/native-apps#interoperability_with_your_website
@infinisil
Copy link
Member

Multiple origins looks like a Webauthn 3 (which is still a draft) specific notion, see https://www.w3.org/TR/webauthn-3/#sctn-validating-origin, this doesn't seem to be documented in Webauthn 2.

This library currently only implements Webauthn 2, but this seems very benign, and since it's already used in practice, it sounds fine to add.

Please add some comments to the code/docs to explain this context.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Overall this is looking good! Made a bunch of suggestions for more consistency, especially regarding imports (there were some duplicate qualified imports of NonEmpty too), but also some other minor things

server/src/Main.hs Outdated Show resolved Hide resolved
src/Crypto/WebAuthn/Operation/Authentication.hs Outdated Show resolved Hide resolved
server/src/Main.hs Outdated Show resolved Hide resolved
server/src/Main.hs Outdated Show resolved Hide resolved
server/src/Main.hs Outdated Show resolved Hide resolved
tests/Main.hs Outdated Show resolved Hide resolved
tests/Main.hs Outdated Show resolved Hide resolved
tests/Main.hs Outdated Show resolved Hide resolved
src/Crypto/WebAuthn/Operation/Authentication.hs Outdated Show resolved Hide resolved
src/Crypto/WebAuthn/Operation/Registration.hs Outdated Show resolved Hide resolved
@arianvp arianvp requested a review from infinisil March 28, 2024 02:56
changelog.md Outdated Show resolved Hide resolved
@infinisil
Copy link
Member

Would also be neat to have a test with two origins ;)

arianvp and others added 5 commits March 28, 2024 10:43
@arianvp
Copy link
Contributor Author

arianvp commented Mar 28, 2024

I added some quickcheck properties now. Hope this suffices? :D PTAL

@arianvp arianvp requested a review from infinisil March 28, 2024 23:31
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looking good to me!

@infinisil infinisil merged commit baa6496 into tweag:master Apr 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants