-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: add capability of handling appid extension #319
Conversation
This commits adds the capability of specifying which App ID was used previously for registering U2F credentials.
appid_domain = WebAuthn.configuration.appid || WebAuthn.configuration.rp_id || | ||
WebAuthn.configuration.origin || raise("Unspecified expected origin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should find a way of unifying the identification of where the rp_id
comes from in one method available for all, cause we are doing something similar in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered adding the extension automatically to the output of WebAuthn::Credential.options_for_get
when the user set the config?
@@ -20,7 +20,7 @@ def initialize(type:, id:, raw_id:, client_extension_outputs: {}, response:) | |||
@type = type | |||
@id = id | |||
@raw_id = raw_id | |||
@client_extension_outputs = client_extension_outputs | |||
@client_extension_outputs = client_extension_outputs || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this here, this is the default in the argument definition.
Is it for some specific case in which the keyword argument is passed explicitly with nil
?
If so, why we still want to override with {}
? Shouldn't the caller be the one who needs to specify {}
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it for some specific case in which the keyword argument is passed explicitly with nil?
No, it's just an attempt to normalize the output, related to the talk that we discuss here.
Besides, this helped me to avoid doing something like client_extension_outputs&.[](:appid)
in this line.
But after we agreed on not normalize the output yet, I will go ahead an change it 🙂
end | ||
end | ||
|
||
context "when verifying a migrated U2F credential" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I think it would be great to also have it
s testing when the verification correctly fails if response is not valid.
These test cases are as important as the ones that test it works when all valid, we don't want false positives :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
@@ -16,7 +16,8 @@ def verify(challenge, public_key:, sign_count:, user_verification: nil) | |||
encoder.decode(challenge), | |||
public_key: encoder.decode(public_key), | |||
sign_count: sign_count, | |||
user_verification: user_verification | |||
user_verification: user_verification, | |||
rp_id: client_extension_outputs[:appid] ? appid : nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also need to check the new config is present here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!! That's what we are doing here 🙂
That's actually a good idea! I'll give it a try 👀 |
This commits adds the `appid` extension automatically to the output of `WebAuthn::Credential.options_for_get` when the user set the `legacy_u2f_appid` config, if the said extension wasn't already requested in the call to that method.
020b8f2
to
fe644dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the testing efforts!
… that receives the argument
Split off this ☝️ into it's own separate commit (f787a2f) and pushed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great contribution @santiagorodriguez96 !
Left you a couple of comments/questions.
Also, do you know why this was not included in the RelyingParty API in 3.0.0.alpha1 ?
@@ -12,7 +12,7 @@ class Options | |||
|
|||
def initialize(timeout: default_timeout, extensions: nil) | |||
@timeout = timeout | |||
@extensions = extensions | |||
@extensions = default_extensions.merge(extensions || {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was analyzing #319 (comment) and I'm not sure if you processed with what you concluded or. This looks to me as if you're still trying to cover the case when extensions
is still pass in as nil
. Else I would have the default in the method definition itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that this changes came after the comment that you mentioned – take a look at this comment: #319 (comment). I can change it so we do not support the case when extensions
is still pass in as nil
but I also don't think it hurts to have it 🤔
I don't really know, but the PR now has some conflicts to resolve that are related to the RelayingParty API. I'll try to fix them so we can get this back on track and hopefully merge it 🙂 |
def appid | ||
appid_domain = relying_party.legacy_u2f_appid || relying_party.id || | ||
relying_party.origin || raise("Unspecified expected origin") | ||
|
||
URI.parse(appid_domain).to_s | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong but I suspect this is incorrect – we should only check for the legacy_u2f_appid
if the appid
extension is present given that if it is not set, even if we pass in the rp_id
, the verification will fail.
I would also consider raising an exception if the appid
extension is present but the legacy_u2f_appid
was not set 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 4058bea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should only check for the
legacy_u2f_appid
if theappid
extension is present given that if it is not set, even if we pass in therp_id
, the verification will fail.
I confirmed that this is the case: if I leave PublicKeyCredentialWithAssertion#appid
to be:
def appid
appid_domain = relying_party.legacy_u2f_appid || relying_party.id ||
relying_party.origin || raise("Unspecified expected origin")
URI.parse(appid_domain).to_s
end
then this spec that I created will fail with a WebAuthn::RpIdVerificationError
appid_domain = relying_party.legacy_u2f_appid || relying_party.id || | ||
relying_party.origin || raise("Unspecified expected origin") | ||
|
||
URI.parse(appid_domain).to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed either: AppID should be a web origin – like https://login.example.com
– so doing URI.parse(appid).to_s
should always return appid
if I'm not mistaken
If the appid extension was requested and the output was `true` but the `legacy_u2f_appid` config was not set, raise an error. We were previously were passing the `rp_id` instead if the mentioned config was not present, but that is needlessly given that it will fail with a rp verification error.
c820cd1
to
c572bcb
Compare
c572bcb
to
4beef45
Compare
@@ -233,5 +235,111 @@ | |||
end | |||
end | |||
end | |||
|
|||
context "when verifying a migrated U2F credential" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just noticed that we have very similar test cases here...
Maybe it's a good idea to change spec/webauthn/public_key_credential_with_assertion_spec.rb
to mock WebAuthn::AuthenticatorAssertionResponse#verify
so that we can just test that we are sending the right parameters to it 🤔
it "raises an error" do | ||
expect do | ||
public_key_credential.verify( | ||
challenge, | ||
public_key: credential_public_key, | ||
sign_count: credential_sign_count | ||
) | ||
end.to raise_error("Unspecified legacy U2F AppID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a tricky one tbh...
PublicKeyCredentialWithAssertion #verify when verifying a migrated U2F credential and appid is not set in configuration file raises an error
We currently support that you can call AuthenticatorAssertionResponse.from_get(extensions: { appid: { 'my_appid' }
– without the need of setting the appid in the config – and that will correctly send the extension. But doing it this way will raise an error if you use PublicKeyCredentialWithAssertion#verify
– in the case that the extension output returning from the client was true
.
In the U2F migration docs it states that you have to call AuthenticatorAssertionResponse#verify
instead and pass the appid
as the rp_id
, but I feel that we could also accept the rp_id
param in PublicKeyCredentialWithAssertion#verify
so we don't force users to use the former – which is not that "smart" and does not perform all the verifications.
I've updated the docs – both the U2F migration doc and the Configuration section in the README. @bdewater it would be great if you could take a quick look at them and check that they still make sense 🙂 |
README.md
Outdated
# Otherwise you would have to re-register those credentials to scope them to the RP ID. | ||
# See https://github.com/cedarcode/webauthn-ruby/blob/master/docs/u2f_migration.md. | ||
# | ||
# config.legacy_u2f_appid = "https://login.example.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep this out of the readme - by now the U2F API is well on its way out (even removed from Chrome) so this will confuse newcomers looking to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Updated in 1e6564f 🙂
The build is failing because there's a fixup commit in the branch. I tried rebasing to squash it but there were too much conflicts at this point. I think I'll just go ahead and Squash and Merge it instead. |
What
Add capability of internally handling the
appid
extension.Now the user only has to add the legacy FIDO AppID in the configuration –
legacy_u2f_appid
– and the extension will be automatically requested and the method PublicKeyCredentialWithAssertion#verify will be smart enough to determine if it should use the AppID or the RP ID to verify the WebAuthn credential, depending on the output of the appid client extension.