Allow specifying expected origin and rp_id when verifying public key credentials

[jeremyevans]

The lack of support for this is the reason Rodauth currently overrides internal WebAuthn methods. I would like Rodauth to stop doing that, by having WebAuthn officially support this.

I've tested this with the following patch to Rodauth:

diff --git a/lib/rodauth/features/webauthn.rb b/lib/rodauth/features/webauthn.rb
index 93d4530..4a67db5 100644
--- a/lib/rodauth/features/webauthn.rb
+++ b/lib/rodauth/features/webauthn.rb
@@ -244,6 +244,11 @@ module Rodauth
       end
     end

+    webauthn_fixed = WebAuthn::PublicKeyCredentialWithAttestation.instance_method(:verify).parameters.include?([:key, :expected_origin]) &&
+                     WebAuthn::PublicKeyCredentialWithAssertion.instance_method(:verify).parameters.include?([:key, :expected_origin])
+    define_method(:webauthn_fixed?){webauthn_fixed}
+    private :webauthn_fixed?
+
     def webauthn_auth_form_path
       webauthn_auth_path
     end
@@ -312,11 +317,12 @@ module Rodauth
     end

     def valid_new_webauthn_credential?(webauthn_credential)
+      kw = webauthn_fixed? ? {expected_origin: webauthn_origin, rp_id: webauthn_rp_id} : {}
       _override_webauthn_credential_response_verify(webauthn_credential)
       (challenge = param_or_nil(webauthn_setup_challenge_param)) &&
         (hmac = param_or_nil(webauthn_setup_challenge_hmac_param)) &&
         (timing_safe_eql?(compute_hmac(challenge), hmac) || (hmac_secret_rotation? && timing_safe_eql?(compute_old_hmac(challenge), hmac))) &&
-        webauthn_credential.verify(challenge)
+        webauthn_credential.verify(challenge, **kw)
     end

     def webauthn_credential_options_for_get
@@ -362,12 +368,17 @@ module Rodauth
     def valid_webauthn_credential_auth?(webauthn_credential)
       ds = webauthn_keys_ds.where(webauthn_keys_webauthn_id_column => webauthn_credential.id)
       pub_key, sign_count = ds.get([webauthn_keys_public_key_column, webauthn_keys_sign_count_column])
+      kw = {public_key: pub_key, sign_count: sign_count}
+      if webauthn_fixed?
+        kw[:expected_origin] = webauthn_origin
+        kw[:rp_id] = webauthn_rp_id
+      end

       _override_webauthn_credential_response_verify(webauthn_credential)
       (challenge = param_or_nil(webauthn_auth_challenge_param)) &&
         (hmac = param_or_nil(webauthn_auth_challenge_hmac_param)) &&
         (timing_safe_eql?(compute_hmac(challenge), hmac) || (hmac_secret_rotation? && timing_safe_eql?(compute_old_hmac(challenge), hmac))) &&
-        webauthn_credential.verify(challenge, public_key: pub_key, sign_count: sign_count) &&
+        webauthn_credential.verify(challenge, **kw) &&
         ds.update(
           webauthn_keys_sign_count_column => Integer(webauthn_credential.sign_count),
           webauthn_keys_last_use_column => Sequel::CURRENT_TIMESTAMP
@@ -409,6 +420,11 @@ module Rodauth

     private

+    if webauthn_fixed
+      def _override_webauthn_credential_response_verify(webauthn_credential)
+        # Nothing
+      end
+    else
       def _override_webauthn_credential_response_verify(webauthn_credential)
         # Hack around inability to override expected_origin and rp_id
         origin = webauthn_origin
@@ -418,6 +434,7 @@ module Rodauth
           super(expected_challenge, expected_origin || origin, **kw)
         end
       end
+    end

     def _two_factor_auth_links
       links = super

[santiagorodriguez96]

Hello @jeremyevans!

First of all, thank you for the PR ❤️

However, this seems to be duplicate of #345 and #365 (some issues asking for that feature where open too such as #285, #320, #344 and #375).

Our gem has released support for handling multiple origins in v3.0.0. Please refer to our advanced configuration docs to see how to our gem provides a way for achieving that. We have moved away voluntarily from passing the expected origin, rp_id and others directly to the verification methods after discussion in #285.

Please let me know if that suits your use case and any questions you might have!

[jeremyevans]

Switching to the new approach, if possible, would certainly require more significant changes to Rodauth, and I'm not sure I could keep Rodauth backwards compatible with such changes.

IMO, a design approach that promotes one API for simple cases but requires switching to a completely separate API for more advanced cases is suboptimal. One should aim for an approach that allows for gradual code changes in response to more gradually more complex needs.

I can see telling users to use the new WebAuthn::RelyingParty for cases where the response #verify doesn't handle their needs, but since this change just passes existing arguments from the credential #verify to the response #verify, it seems low risk and makes it much easier to existing users to handle separate origins/relying party ids.

In any case, this isn't a big deal to me. Rodauth has always monkey-patched to the response #verify method to work correctly, and it can continue to do so. So it's fine with me if you close this.

[santiagorodriguez96]

Switching to the new approach, if possible, would certainly require more significant changes to Rodauth, and I'm not sure I could keep Rodauth backwards compatible with such changes.

IMO, a design approach that promotes one API for simple cases but requires switching to a completely separate API for more advanced cases is suboptimal. One should aim for an approach that allows for gradual code changes in response to more gradually more complex needs.

I can see telling users to use the new WebAuthn::RelyingParty for cases where the response #verify doesn't handle their needs, but since this change just passes existing arguments from the credential #verify to the response #verify, it seems low risk and makes it much easier to existing users to handle separate origins/relying party ids.

In any case, this isn't a big deal to me. Rodauth has always monkey-patched to the response #verify method to work correctly, and it can continue to do so. So it's fine with me if you close this.

@jeremyevans I managed to come up with a POC for using our official approach for supporting multiple domains in rodauth. It feels to me that that should be all you need to do, but I might be missing something as I have zero knowledge about rodauth and how it works 😞

[jeremyevans]

@santiagorodriguez96 Thank you very much. I'm happy to see that I was wrong, it wasn't a big change to switch to the new approach (though keeping compatibility will be a bit ugly until webauthn v2 support can be dropped). I'm going to close this.