webauthn_rp_id not correctly passed to webauthn verify when allow cross domain passkeys

[butsjoh]

In our multi tenant setup we also have a general domain app.xxx.com for authentication that then forwards to the actual tenant. Tenant themselves also have a domain to login client_1.xxx.com, client_2.xxx.com, ...

We where setting up the passkeys on each tenant domain successfully using the webauthn feature but when we tried to login on the app.xxx.com domain the passkeys where not working (on the tenant specific auth page they worked fine). After some digging we realised it has something todo with the rp_id (https://stackoverflow.com/questions/55090589/webauthn-across-multiple-subdomain#comment96973384_55090589). So we modified the webauthn_rp_id that the feature offers to xxx.com.

When we then started the setup of the passkey again it failed on the tenant domains. The error we got back was a Webauthn::RpIdVerificationError. So we then started looking at the implementation of the webauthn feature and discovered there are 2 methods: valid_webauthn_credential_auth? and valid_new_webauthn_credential? that both had the following code:

# Hack around inability to override expected_origin
origin = webauthn_origin
webauthn_credential.response.define_singleton_method(:verify) do |expected_challenge, expected_origin = nil, **kw|
  super(expected_challenge, expected_origin || origin, **kw)
end

When we look at the code of the webauthn gem (see https://github.com/cedarcode/webauthn-ruby/blob/master/lib/webauthn/authenticator_response.rb#L27) we clearly see that passing the rp_id is also an option.

So i think whenever you set a custom rp_id with webauthn_rp_id it should also be passed to that verify. The following code patched for the 2 methods valid_webauthn_credential_auth? and valid_new_webauthn_credential? made it work in our situation and allowed for cross domain passkeys.

# Hack around inability to override expected_origin
 origin = webauthn_origin
 rp_id = webauthn_rp_id # MODIFIED
 webauthn_credential.response.define_singleton_method(:verify) do |expected_challenge, expected_origin = nil, **kw|
   kw[:rp_id] = rp_id # MODIFIED
   super(expected_challenge, expected_origin || origin, **kw)
 end

I did not have the time yet to provide a proper PR with this fix yet but first would like a confirmation that this fix would be not breaking other situations. We tested it and it works and i think this is according to the spec: https://www.w3.org/TR/webauthn/#relying-party-identifier

[jeremyevans]

The fix proposed seems reasonable to me. I think we could make that change along with a test where webauthn_rp_id is set to something different than webauthn_origin.

[jeremyevans]

OK, I have a working test that uses a different rp_id than the origin. Doing some final testing, and assuming no problems, I'll merge it in a bit.

[jeremyevans]

Fixed by d65d69b

[butsjoh]

thnx ! 👍

[butsjoh]

Is there a release scheduled for the changes that are currently on master? Our patch is holding but we like to get it cleaned up :)

[jeremyevans]

I'm planning for a release tomorrow.