-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
When RequestOptions.Rp.Id is null, we should default to Origin/effectiveDomain #370
Comments
To nuance this, RpID is not allowed to be null. If is is missing the value, we should default to using the Origin or effectiveDomain: https://www.w3.org/TR/webauthn-2/#GetAssn-DetermineRpId and https://www.w3.org/TR/webauthn-2/#CreateCred-DetermineRpId A credential will always be bound to a RpID/Origin. |
So, verify that the RpId in the attestation matches the origin of the request, or fill the options.Rp.Id from the initial request? The API could be used from multiple frontends so we can't really use a single configured domain here. |
...which actually breeds a whole new question I might actually try to google: What goes in the RpId when a desktop or mobile app (i.e. not a browser, not via website, no domain) makes a credential? |
Turns out, apps may use whatever as RP ID, like package name or hash of the signing key for android, although google seems to prefer people linking their app to a website and using that domain. But yeah, gotta think about clients not using the same domain as the server, or not using a domain at all. I therefore suggest to just not verify the RP ID if it doesn't exist in the creation options. Mostly because it's easier than adding an optional RP ID parameter to the API. The client can take care of filling out or deciding on the actual RP ID used for the authenticator, if any, as leaving it blank means the authenticator's implementation chooses. |
Ok with #372 maybe the extra parameter for creating the options looks better. |
@Regenhardt Can you highlight what you mean with the extra parameter for creating the options and why you think it is better? I'm not sure I follow |
Say a company offers three services, A, B, and C, and an identity provider Ident. You wanna login to A, so you go to a.myCo.com and login. Now to add a fingerprint because you don't want to enter your password every time, the website gets new creation options from Ident and puts them into navigator.credentials.create - at which point, the way it currently works, the browser cancels, because the RpId (ident.myCo.com) doesn't match the current domain (a.myCo.com). So before I'd've said just leave the RpId blank and let the client figure it out. As the spec says, the implementation of the authenticator will fill the RpId with the current domain anyway and it's not something cryptographically secure we absolutely need to verify. But since it's just nice to actually do verify it, we need a way of putting a.myCo.com into the RpId when it's created, because after that it's gonna be immutable (once #372 goes live). So whether the controller reads it from the request or requests a parameter, the options need a way of setting a specific RpId that is not the server and can be one of multiple services that use Ident. So in my opinion, Fido2.RequestNewCredential should take an additional parameter, either the RpId or a whole Edit, demonstration via the included demo: Edit2: Different error actually, since the RpId actually is ok here (localhost) but I forgot to add the client's full domain to the allowed origins. Can't actually locally show this problem as I only have localhost and not two domains. |
What you are describing is already possible by creating a new Fido2-class with the appropriate config? |
Oh I see. I must admit I totally forgot you can actually work without DI and construct the thing directly. Yes that should indeed completely remove this problem, will check it out. |
Ok I inject a factory now with default Fido2Config, with a method constructing a Fido2 service with the HttpContext.Request.Host.Host as |
I'm guessing your sending a request from the mobile app to your webservice? Then you could just pass along the RPID you wish as part of the request and instantiate the FIDO2 service with the data that the client send. This could be done using DI or without -- albeit, not using DI will be more straight forward.
I'm sorry, I'm not completely understanding what you want to accomplish here. Could you explain it in a different way? |
I'd like to improve the documentation for someone implementing Fido2. I'd put something like |
I agree that documentation is in need for improvement. Would welcome a addition in either code comments or adding a small section in the readme (or creating a docs.md and linking from the readme). If modifying existing code comments. We try to keep to the WebAuthn Spec for describing fields and reference the spec where it makes sense, but adding an additional "Friendly description" (without removing existing comments) is appreciated. |
On that note, at CredentialCreateOptions.cs line 162, for the
To me, that is very misleading and confusing and is nowhere close to what the specification says. I think it would be nice to update the XML doc of that property so that it includes a link to the official documentation and can also just duplicate the documentation right there as well. You'll notice that the specification also briefly mentions what other non-web platforms, such as mobile applications MAY do, which I don't know if it would still be of interest to @Regenhardt . However, it still says that the RP ID syntax MUST conform to either a domain name or URI.
|
I agree it could have more information that is already in the spec, although I'm hesitant to just put the whole spec there as it's way too big for inline docs in my opinion. After my current PR is finished, I'll gladly make some documentation improvements and take this into consideration, summarizing the most relevant information from the spec here and linking the actual spec for further reading. |
Currently the lib compares the rp id in the original
PublicKeyRequestOptions
to the rp id in the created credential. If the id was however not set in the request options, as is specified as valid in the spec (see https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredentialRequestOptions), it just crashes because it just passes the id to UTF8.GetBytes, which (rightly so) doesn't accept null values.There should be a
if(originalOptions.Rp.Id != null)
guard before that line to guard either that one at 7) computing the hash (here) or also the one at 9) comparing said hash.The text was updated successfully, but these errors were encountered: