feat: allow multiple origins set per RelyingParty #431

obroshnij · 2024-09-06T09:51:16Z

As described in the issue #428
when building a Relying Party that is supposed to be used by iOS and Android native clients, we need to be able to specify multiple allowed origins per RP (https://developer.android.com/identity/sign-in/credential-manager#verify-origin)
This is yet not supported by this library though is also part of standard specification https://w3c.github.io/webauthn/#sctn-validating-origin

Current workaround is to fallback to creating multiple RP abstractions per expected client platform (Android, iOS, etc) and select one of them based on User-Agent header (for example). That is of course troublesome and can actually be avoided.

This PR allows a relying party to specify either string for origin or array of strings like in the example:

# config/initializers/webauthn.rb

WebAuthn.configure do |config|
  # This value needs to match `window.location.origin` evaluated by
  # the User Agent during registration and authentication ceremonies.
  # config.origin = "https://auth.example.com/"
  config.origin = [
    "https://auth.example.com/",
    "android:apk-key-hash:blablablablablalblalla"
  ]

  # Relying Party name for display purposes
  config.rp_name = "Example Inc."
  config.rp_id  = "example.com"
end

The only drawback of this approach is that in case an array is used for config.origin, - it's impossssible to "guess" relying party by origin (which maybe we should not even do?) thus having array there and no explicitly set config.rp_id would result in RpIdVerificationError which imo should be expected.

Hope this PR makes sense. Please let me know should there be any change requests/questions

santiagorodriguez96

Hey @obroshnij 👋 thank you so much for your time and effort!

I like where this is going!

I will try to give a proper test to this in the next week to see how it works in a real app. I'll let you know once I've done it 🙂

Then again, thanks!

lib/webauthn/authenticator_response.rb

spec/webauthn/authenticator_attestation_response_spec.rb

obroshnij · 2024-09-09T12:31:30Z

@santiagorodriguez96 thanks a lot for a review and feedback. I have address the points that you brought earlier. Please let me know if there is still anything I should improve

obroshnij · 2024-09-18T17:47:18Z

@santiagorodriguez96 I know you guys are pretty busy, but just so that I can also manage my expectations, is there a chance to get this patch merged any time soon?

santiagorodriguez96

Hey @obroshnij! Thank you for taking the time and the patience for addressing the comments. Sorry that it took me some time to get back to you again.

I've left a couple more comments in order to keep this moving forward 🙂

Then again thank you!

lib/webauthn/authenticator_response.rb

lib/webauthn/relying_party.rb

spec/webauthn/authenticator_attestation_response_spec.rb

lib/webauthn/authenticator_response.rb

README.md

lib/webauthn/relying_party.rb

lib/webauthn/version.rb

lib/webauthn/authenticator_response.rb

lib/webauthn/relying_party.rb

lib/webauthn/authenticator_response.rb

obroshnij · 2024-11-05T19:59:24Z

hey @santiagorodriguez96 thanks a lot for another review. I have addressed the points you brought up and I think it should be ready for the next review

obroshnij · 2024-11-08T20:08:11Z

hi @santiagorodriguez96 thanks for running the CI here. I have fixed the rubocop issues now

santiagorodriguez96 · 2024-11-08T20:22:59Z

Hi @obroshnij! The PR looks good to me!

I'd want to hold off for merging as my plan was to release this as part of v3.3 as we have a lot of features coming in v3.2 too and this one is already a pretty big one 😕

I'll try to release v3.2 and v3.3 in the following days 🙂

obroshnij · 2024-11-08T20:42:04Z

Hi @obroshnij! The PR looks good to me!

I'd want to hold off for merging as my plan was to release this as part of v3.3 as we have a lot of features coming in v3.2 too and this one is already a pretty big one 😕

I'll try to release v3.2 and v3.3 in the following days 🙂

awesome, I'm looking forward to that 🚀

obroshnij · 2025-01-10T18:51:51Z

hi @santiagorodriguez96 do you know approximately when this PR may end up in master?

alycit · 2025-01-22T16:37:25Z

@santiagorodriguez96 any update here on when this will get merged? My team is also running up against this issue.

obroshnij · 2025-01-22T16:41:19Z

@alycit feel free to grab my fork in the meantime to unblock yourself. I'm trying to keep that one updated

anaclair · 2025-01-23T20:38:10Z

@alycit feel free to grab my fork in the meantime to unblock yourself. I'm trying to keep that one updated

We really appreciate this!

Would you consider releasing your fork as a gem?

obroshnij · 2025-02-04T19:09:36Z

@alycit feel free to grab my fork in the meantime to unblock yourself. I'm trying to keep that one updated

We really appreciate this!

Would you consider releasing your fork as a gem?

no, sorry. I think you can use it without that. But publishing it as a gem would mean that me or someone would have to support it and update in future. Also I'm afraid it would undermine the amount of work authors of the original gem have invested.
I would still hope to see this PR released

brauliomartinezlm · 2025-01-22T01:00:58Z

lib/webauthn/authenticator_response.rb

+    # @return [Boolean]
+    # @param [Array<String>] expected_origin
+    # Validate if one of the allowed origins configured for RP is matching the one received from client


While these annotations might be valuable in many cases, I don't think our gem's codebase has adopted them as a practice. For the sake of consistency, would you mind removing them from this and other methods in the diff?

yes, you're right - removed them

brauliomartinezlm · 2025-02-05T14:59:13Z

lib/webauthn/relying_party.rb

+        )
+      end
+
+      @origin = new_origin


If we're define the writer ourselves, wouldn't it be better to have this define @allowed_origins instead of even defining@origin?

That will allow us to avoid this and keep a dual data type handling inside the AuthenticatorResponse class that handles verification.

thanks @brauliomartinezlm that is actually a good idea imo

brauliomartinezlm · 2025-02-05T15:21:44Z

lib/webauthn/authenticator_response.rb

+      case expected_origin
+      when Array
+        URI.parse(expected_origin.first).host if expected_origin.size == 1
+      when String
+        URI.parse(expected_origin).host
+      end


Related to my comment below, if we transform the origin setter to set allowed_origins, we should just use allowed_origins in across this file and always expect to be an array. In such case, I would only handle this branch of logic:

URI.parse(expected_origin.first).host if expected_origin.size == 1

brauliomartinezlm · 2025-02-05T15:27:31Z

README.md

@@ -106,6 +106,17 @@ WebAuthn.configure do |config|
  # the User Agent during registration and authentication ceremonies.
  config.origin = "https://auth.example.com"

+  # For the case when a relying party has multiple origins


If we're gonna put a deprecation warning and change how this is gonna be used, we actually should directly remove the config.origin and only put as a viable option config.allowed_origins = ["https://auth.example.com"]. We don't need to be explicit about multiplicity in it. It's easily inferred and we have a whole section talking about RPs instantiation which is still supported.

In that context, I would just remove the rest for the added comments of this block of changes.

adjusted, thank you

brauliomartinezlm · 2025-02-05T15:38:57Z

Hi @obroshnij 👋

First of all, I really appreciate your work on this PR. Contributions and discussions are always welcome and appreciated ❤️

This PR took me a while to process. I apologize for the delay in me writing this comment. While @santiagorodriguez96 quickly interacted with you on it, there was always a design decision in the middle that took me ages (with the little time I can invest into OSS) to get my mind wrapped around it.

Given the gem's design on Relaying Party instantiation, I always felt this was going against that. I'm still pretty doubtful to be honest.

However, I just had a meeting with @santiagorodriguez96 and we decided to move forward with the change proposed in this PR and how it affects the API. Webauthn specs is clearly talking about an array of expected origins in the validation step for origin values and that made me reflect in favor of it.

I just left you a few comments that I think are important for us to introduce this change. Again, I do apologize for asking them so down the road.

Also I'm afraid it would undermine the amount of work authors of the original gem have invested.

Lastly, this comment showed a considerable amount of respect. Very appreciated. Not that common in the OSS realm so it's nice to acknowledge when seen 👍

Looking forward to unblock folks that are relying on the fork you did and have them back in the official distro.

obroshnij · 2025-02-07T19:00:50Z

hi @brauliomartinezlm
I appreciate your feedback and already addressed the changes you requested. They all make sense and now I think made the code even more robust.
Yeah, OSS contributions are sometimes challenging but should definitely be appreciated. This gem is well designed and well thought of and that definitely deserves a 👍
Happy to move it forward and looking forward to more feedback

brauliomartinezlm

Thank you so much @obroshnij for your last comment and addressing the comments and feedback.
Left you another round of comments that are entering the nit picky space but will get the PR ready to get merged.
Again, really appreciate your collaboration 🙏

brauliomartinezlm · 2025-02-11T13:19:02Z

lib/webauthn/authenticator_response.rb

+      # NOTE: we are trying to guess from 'expected_origin' only in case it's a single origin
+      # (array that contains a single element)
+      # rp_id should either be explicitly set or guessed from only a single origin


I feel I'd remove the comments we added for these methods. You did a good job on making them simple enough so they are easily understandable. While I know it's common practice to comment code we've followed the self documented code and only use comments and annotations for strictly necessary cases. I don't honestly think we're in that scenario here.

brauliomartinezlm · 2025-02-11T13:45:16Z

README.md

@@ -104,7 +104,12 @@ For a Rails application this would go in `config/initializers/webauthn.rb`.
 WebAuthn.configure do |config|
  # This value needs to match `window.location.origin` evaluated by
  # the User Agent during registration and authentication ceremonies.
-  config.origin = "https://auth.example.com"
+  # Multiple origins can be used in more complex scenarios (e.g. different subdomains, native client sending android:apk-key-hash:... etc.)


I'd reword this comment to:

Multiple origins can be used when needed. Using more than one will imply you MUST configure rp_id explicitely. If you need your credentials to be bound to a single origin but you have more than one tenant, please see [our Advanced Configuration section](https://github.com/cedarcode/webauthn-ruby/blob/master/docs/advanced_configuration.md) instead of adding multiple origins.

and remove the empty line below.

awesome, thanks for the hint

brauliomartinezlm · 2025-02-11T13:46:31Z

README.md

+  # Multiple origins can be used in more complex scenarios (e.g. different subdomains, native client sending android:apk-key-hash:... etc.)
+
+  config.allowed_origins = [
+    "https://auth.example.com",
+    "android:apk-key-hash:blablablablablalblalla"
+  ]


I'd leave this example configuration using one single element (the first one) because I'm assuming that is the majority of our gem users use cases.

obroshnij · 2025-02-12T01:32:47Z

Thank you so much @obroshnij for your last comment and addressing the comments and feedback. Left you another round of comments that are entering the nit picky space but will get the PR ready to get merged. Again, really appreciate your collaboration 🙏

Thanks @brauliomartinezlm , that sounds great! Appreciate your collaboration. I have addressed the concerns you mentioned. Looking forward to making this change happen!

brauliomartinezlm

Thank you for the changes @obroshnij !
Last round, very picky from my side but straightforward I hope.
After that I'm done, I promise 🙏 😄

brauliomartinezlm · 2025-02-12T16:24:19Z

README.md

@@ -104,7 +104,10 @@ For a Rails application this would go in `config/initializers/webauthn.rb`.
 WebAuthn.configure do |config|
  # This value needs to match `window.location.origin` evaluated by
  # the User Agent during registration and authentication ceremonies.
-  config.origin = "https://auth.example.com"
+  # Multiple origins can be used when needed. Using more than one will imply you MUST configure rp_id explicitely. If you need your credentials to be bound to a single origin but you have more than one tenant, please see [our Advanced Configuration section](https://github.com/cedarcode/webauthn-ruby/blob/master/docs/advanced_configuration.md) instead of adding multiple origins.
+  config.allowed_origins = [


Very nit-picky, would you mind putting this single element array in a single line?

brauliomartinezlm · 2025-02-12T16:24:46Z

lib/webauthn/authenticator_response.rb

@@ -105,8 +114,9 @@ def valid_user_verified?
      authenticator_data.user_verified?
    end

+    # Extract RP ID from origin in case rp_id is not provided explicitly


Another nit-picky one, would you mind removing this comment?

* add a possibility to set `allowed_origins` configuration option that would be an alternative to `origin` * update Readme * add deprecation warning * adjust test suite * overwrite writer to consistently trigger deprecation warnings * fix origin extraction code * update according to CR

obroshnij · 2025-02-15T19:54:50Z

Thank you for the changes @obroshnij ! Last round, very picky from my side but straightforward I hope. After that I'm done, I promise 🙏 😄

Thanks @brauliomartinezlm , thanks for following up on this. I have addressed the CRs. Let me know if there is anything else I can help with

brauliomartinezlm

Thank you so much for all the work in this PR @obroshnij . Really appreciate the collaboration ❤️
I'll merge this now and we'll cut a release tomorrow.

brauliomartinezlm · 2025-02-17T17:29:53Z

Released in v3.4.0

obroshnij · 2025-02-17T20:48:54Z

Thank you @brauliomartinezlm . So happy to see that happening! Would immediately switch to newest version!

santiagorodriguez96 reviewed Sep 6, 2024

View reviewed changes

obroshnij force-pushed the feat-allow-multiple-origins branch from 04774a3 to e0261d0 Compare September 9, 2024 12:30

obroshnij requested a review from santiagorodriguez96 September 9, 2024 12:30

santiagorodriguez96 reviewed Sep 18, 2024

View reviewed changes

obroshnij force-pushed the feat-allow-multiple-origins branch from 458d738 to b090300 Compare October 24, 2024 00:09

santiagorodriguez96 reviewed Oct 24, 2024

View reviewed changes

obroshnij requested a review from santiagorodriguez96 November 5, 2024 19:58

obroshnij force-pushed the feat-allow-multiple-origins branch 3 times, most recently from 79501ae to 761516b Compare November 8, 2024 20:07

brauliomartinezlm reviewed Feb 5, 2025

View reviewed changes

obroshnij force-pushed the feat-allow-multiple-origins branch from 79c5605 to 4e1f5a7 Compare February 7, 2025 18:53

obroshnij requested a review from brauliomartinezlm February 7, 2025 19:00

brauliomartinezlm reviewed Feb 11, 2025

View reviewed changes

obroshnij force-pushed the feat-allow-multiple-origins branch from ade3bdc to ba324c7 Compare February 12, 2025 01:30

obroshnij requested a review from brauliomartinezlm February 12, 2025 01:55

brauliomartinezlm reviewed Feb 12, 2025

View reviewed changes

obroshnij force-pushed the feat-allow-multiple-origins branch from c1b3569 to 9405421 Compare February 15, 2025 19:52

obroshnij requested a review from brauliomartinezlm February 15, 2025 19:52

brauliomartinezlm approved these changes Feb 17, 2025

View reviewed changes

brauliomartinezlm merged commit a711a63 into cedarcode:master Feb 17, 2025
11 checks passed

brauliomartinezlm mentioned this pull request Feb 17, 2025

RelyingParty should support a list of origins #428

Closed

vmstan mentioned this pull request Feb 18, 2025

WebAuthn deprecation warning mastodon/mastodon#33952

Closed

feat: allow multiple origins set per RelyingParty #431

feat: allow multiple origins set per RelyingParty #431

Conversation

obroshnij commented Sep 6, 2024

santiagorodriguez96 left a comment

Choose a reason for hiding this comment

obroshnij commented Sep 9, 2024

obroshnij commented Sep 18, 2024

santiagorodriguez96 left a comment

Choose a reason for hiding this comment

obroshnij commented Nov 5, 2024

obroshnij commented Nov 8, 2024

santiagorodriguez96 commented Nov 8, 2024

obroshnij commented Nov 8, 2024

obroshnij commented Jan 10, 2025

alycit commented Jan 22, 2025

obroshnij commented Jan 22, 2025

anaclair commented Jan 23, 2025

obroshnij commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brauliomartinezlm commented Feb 5, 2025 • edited Loading

obroshnij commented Feb 7, 2025

brauliomartinezlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obroshnij commented Feb 12, 2025

brauliomartinezlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obroshnij commented Feb 15, 2025

brauliomartinezlm left a comment

Choose a reason for hiding this comment

brauliomartinezlm commented Feb 17, 2025

obroshnij commented Feb 17, 2025

brauliomartinezlm commented Feb 5, 2025 •

edited

Loading