cedarcode / webauthn-ruby

WebAuthn ruby server library ― Make your Ruby/Rails web server become a conformant WebAuthn Relying Party
https://rubygems.org/gems/webauthn
MIT License
658 stars 55 forks source link

Support multiple domains (instance-based) #285

Closed sandstrom closed 4 years ago

sandstrom commented 4 years ago

Awesome gem! 🏅

Just one question. Have you thought about making the library "instance-based"? Right now it seems to be a global, making it trickier to configure multiple webauthn-based instances in one ruby application.

For example, we're considering using webauthn for our internal admin-backend, plus for our users. They would be served by the same ruby app, but under different domains.

This structure can also make testing a bit easier, since there isn't any need to override global config for every test case.

# first instance
authentication_instance1 = WebAuthn.new(origin: "https://auth.example.com", rp_name: "Example Inc.")

user_id = authentication_instance1.generate_user_id
# …

# second instance
authentication_instance2 = WebAuthn.new(origin: "https://internal-system.example.com", rp_name: "Internal Sys.")

admin_id = authentication_instance2.generate_user_id
# …

Just wanted to throw the idea out there. This already seems to be an excellent library though, and I'm sure there are many other changes that may be more important than this.

brauliomartinezlm commented 4 years ago

Hi @sandstrom ! First of all, thank you so much for the feedback. I'm glad you find it useful :).

Personally, I don't remember we had any general conversation on making any of the configs more instance-based but as a gem user, I think you have a way to override/bypass that origin you configure by making sure that for each instance you call the verify method to you assertion/attestation response, you pass the correct expected_origin as the second param. Analogously you can do that with rp_id you pass to the verify method.

About the rp_name and all other options that we provide when calling credentials_request_options and credential_create_options, there's always the chance of overriding before the server sends them to the browser in the response or even in the actual browser right before calling the WebAuthn JS API.

Does this help you work through the problems you're facing? Was I able to explain myself correctly on how to do it? Maybe there's something that the design is not currently allowing to reconfigure and I'm missing it 🤔

Let me know and I'd be glad to followup or chat in Gitter if you need more help with that.

Thank you for bringing up your situation and creating this issue 🍻

sandstrom commented 4 years ago

@brauliomartinezlm Alright, sounds excellent!

So basically one can use the gem without ever calling configure, and instead pass in the required options to each method?

brauliomartinezlm commented 4 years ago

@sandstrom You'll need to use configure probably for other reason, like to generate the challenge and other options (unless you want to do it all manually, which you can, but will need to understand and construct one by one). You can set for that origin attribute a default one (same for rp_id), and make sure you override accordingly when calling verify.

Something that @padulafacundo brought to my attention today, is that calling verify on the AuthenticatorAttestationResponse and AuthenticatorAssertionResponse is part of the old interface and now the public documented way of doing this is to call verify to PublicKeyCredentialWithAttestation and PublicKeyCredentialWithAssertion respectively. Those don't support passing expected_origin and rp_id as I suggested earlier.

I'll file 2 new issues for those attributes to allow users to pass these options in the new API, but for now you can use the old one (as I suggested in my first comment), which is public and due to backwards compatibility reasons will be just as well maintained. Be aware you might need to handle some of the decoding yourself and for that purpose you might find handy to use WebAuthn.standard_encoder#decode which is also class for public use.

Clearly your situation arose the fact we need to support gem users to pass origin and rp_id to the verify method (or set them in the PublicKeyCredential object before calling verify) in the new API (gem version >2.0), so I appreciate you bringing this up. We lost some flexibility in the API redesign done lately and the best way of learning about our decision is the gem users claiming for that flexibility 👍

sandstrom commented 4 years ago

Thanks for letting me know! 😄

We'll still use webauthn in it's current form, and I'm sure it'll be very useful! That said, I do think an "instance-based" structure for the gem could be useful (my_instance = WebAuthn.new(…)), instead of a global config (can also be useful for testing).

But I'm sure you have other requests and ideas, so don't let this influence you too much. Maybe our situation is an edge case.

Either way, great gem! 🏅

grzuy commented 4 years ago

Awesome gem! medal_sports

Just one question. Have you thought about making the library "instance-based"? Right now it seems to be a global, making it trickier to configure multiple webauthn-based instances in one ruby application.

For example, we're considering using webauthn for our internal admin-backend, plus for our users. They would be served by the same ruby app, but under different domains.

Interesting perspective. We didn't considered this in previous design sessions.

This structure can also make testing a bit easier, since there isn't any need to override global config for every test case.

True.

# first instance
authentication_instance1 = WebAuthn.new(origin: "https://auth.example.com", rp_name: "Example Inc.")

user_id = authentication_instance1.generate_user_id
# …

# second instance
authentication_instance2 = WebAuthn.new(origin: "https://internal-system.example.com", rp_name: "Internal Sys.")

admin_id = authentication_instance2.generate_user_id
# …

Not sure we would change WebAuhtn from being a module to a class. Maybe this could be something like WebAuthn::RelyingParty.new(origin: "...", ...) or WebAuthn::RP.new(origin: "...", ...), or similar...

Just wanted to throw the idea out there. This already seems to be an excellent library though, and I'm sure there are many other changes that may be more important than this.

Thank you very much for opening this issue and sharing the idea.

Right now most of our focus is on adding support for https://github.com/cedarcode/webauthn-ruby/issues/66, which should land in v2.1.

That said, I don't think this is a bad idea. I think is worth coding a prototype to learn would this "instance-based" interface would look like and if it feels good or we found it has too many problems. Worth considering for v2.2 or, if found to be something with some minimal incompatibilities, then 3.0.

sandstrom commented 4 years ago

Thanks, happy you found it helpful.

Not sure we would change WebAuhtn from being a module to a class. Maybe this could be something like WebAuthn::RelyingParty.new(origin: "...", ...) or WebAuthn::RP.new(origin: "...", ...), or similar...

Yeah, something like that may make more sense (you know the library much better than I do, so you're in a better position to make that call).

grzuy commented 4 years ago

Planning on having this solved for one of the next two releases (v2.2 or v2.3).

grzuy commented 4 years ago

Fixed on v3.0.0.alpha1 with the introduction of the RelyingParty class (#296).

Give it a try and let us know if you have any feedback on how it works or anything. Thanks!

sandstrom commented 4 years ago

@grzuy Awesome, I'll try it out! I've added some feedback in the PR (#296).