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

Set the WebAuthN rpId to the COOKIE_DOMAIN if set #457

Merged
merged 2 commits into from
May 9, 2023

Conversation

dd32
Copy link
Contributor

@dd32 dd32 commented May 9, 2023

By default the WebAuthN Passkey is scoped to the origin domain, for example, example.org

If a Multisite install however shares it's cookies with subdomains, attempting to login via subdomain.example.org will fail to match as the origin doesn't match the keys ID.

On single-site, COOKIE_DOMAIN is normally false. On Multisite, it's usually set to either a) The domain, or b) on subdomains, the parent domain. The ltrim() is used for sites that support older browsers which needed the leading ..

This allows for the WebAuthN Passkey to be setup via a subdomain or the parent domain, and used on all of it's subdomains or parent domain.

Minimal testing has been done, and further testing is required.

Fixes #456

@dd32 dd32 marked this pull request as ready for review May 9, 2023 02:38
@dd32 dd32 requested a review from sjinks as a code owner May 9, 2023 02:38
@sjinks
Copy link
Owner

sjinks commented May 9, 2023

Thank you, I will test this today. My concern here is whether it is going to work with U2F.

@dd32
Copy link
Contributor Author

dd32 commented May 9, 2023

I'm not sure what impact this will have upon existing registered keys either, although, I think it should be fine if it matches the default assumed value by browsers.

dd32 added 2 commits May 9, 2023 09:05
By default the WebAuthN Passkey is scoped to the origin domain, for example, example.org

If a Multisite install however shares it's cookies with subdomains, attempting to login via subdomain.example.org will fail to match as the origin doesn't match the keys ID.

On single-site, COOKIE_DOMAIN is normally false. On Multisite, it's usually set to either a) The domain, or b) on subdomains, the parent domain. The ltrim() is used for sites that support older browsers which needed the leading `.`.

This allows for the WebAuthN Passkey to be setup via a subdomain or the parent domain, and used on all of it's subdomains or parent domain.
@sjinks sjinks changed the base branch from master to fix-rpid May 9, 2023 09:51
@sjinks sjinks merged commit 5a27417 into sjinks:fix-rpid May 9, 2023
@sjinks
Copy link
Owner

sjinks commented May 9, 2023

I have merged this into a local branch to make it easier for me to test.

$builder->setRelyingParty( new RelyingParty( get_bloginfo( 'name' ), self::get_u2f_app_id() ) );
$relay = new RelyingParty( get_bloginfo( 'name' ), self::get_u2f_app_id() );
if ( \COOKIE_DOMAIN ) {
$relay->setId( ltrim( \COOKIE_DOMAIN, '.' ) );
Copy link

Choose a reason for hiding this comment

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

What do you think about adding a filter on this value?

Copy link
Owner

Choose a reason for hiding this comment

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

@iandunn if you think it makes sense to use a filter here, I can add it, no problem.

But setting the relying party ID to anything else than the top level domain will break login.

Ref: https://w3c.github.io/webauthn/#rp-id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But setting the relying party ID to anything else than the top level domain will break login.

To be clear, subdomain.example.org IS fine, but only if it should be scoped to the subdomain and subdomains-of-the-subdomain.

adding a filter

FWIW I think a filter on the return value from the entire function might be the best option, to allow 100% overriding and custommization of all options.

Copy link
Owner

Choose a reason for hiding this comment

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

@dd32 I missed this comment :-(

I have added a PR; could you please take a look: #483

PS: if I don't respond to comments/mentions, please feel free to ping me in A8C Slack.

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.

Multisite login only works on domain where key was registered
3 participants