Fix #15: Add SetPIN and ChangePIN #48
Conversation
|
Thank you for working on this @msirringhaus! I added a few comments, and cc'd @iinuwa in case he has further thoughts from the perspective on the API client. This already provides good functionality, and we don't yet have a stable API. So if you want to return to any improvements at a later time, just let me know and I'll merge this as is. |
libwebauthn/src/webauthn.rs
Outdated
| Ok(fido_protocol) | ||
| } | ||
|
|
||
| async fn webauthn_change_pin( |
There was a problem hiding this comment.
I'm wondering what's the best way to represent this, as PIN management is not actually part of WebAuthn, and this method should not be available to applications. Rather, it'd be used by @iinuwa's linux-webauthn-platform-api.
What would you think of have separate traits, ie. WebAuthn for browser APIs (GetAssertion, MakeCredential), and PinManagement (or similar) for these operations?
There was a problem hiding this comment.
I felt indeed weird writing the webauthn_-prefix to that function. 😅
Let's make a new trait for it.
libwebauthn/src/webauthn.rs
Outdated
| if new_pin.as_bytes().len() < get_info_response.min_pin_length.unwrap_or(4) as usize { | ||
| // If platformCollectedPinLengthInCodePoints is less than platformMinPINLengthInCodePoints then the platform SHOULD display a "PIN too short" error message to the user. | ||
| // TODO: New error for "PIN too short" vs. "PIN too long"? | ||
| return Err(Error::Ctap(CtapError::PINPolicyViolation)); |
There was a problem hiding this comment.
I've been thinking about your question on what to do for invalid new PINs. Reading the spec, I'm not sure the platform needs to return a CTAP error - rather CTAP2_ERR_PIN_POLICY_VIOLATION should only occur if the invalid PIN is somehow sent to the authenticator.
IMO, ideally we would want a callback like the existing PinProvider#provide_pin to retrieve the new PIN from the client. However, we can't really re-use this method, because (a) it obtains the retries-left count from the authenticator, and provides it as a hint to the client, which doesn't necessarily make sense in this case as these platform validation failures can be retried safely without incrementing the authenticator's attempts count, and (b) there would be no way for the client to know if provide_pin is being invoked to provide the old PIN, or new PIN.
Could we extend the PinProvider trait to have an additional provide_new_pin method, and use this to retrieve the new PIN for PIN change operations? We could:
- Share the PIN length requirements with the client as parameters to
provide_new_pin, so that the client may communicate these to the user and/or do some validation (e.g. setting max length on the input field); - Allow retries, by invoking the callback again if the PIN fails validation;
- Upon retrying, add an optional hint to the callback. This lets the user know which specific validation failure occurred (too long, or too short).
As clients may only be interested in implementing GetAssertion and MakeCredential operations, they may not be interested in providing a valid implementation of provide_new_pin. We could make this optional, eg. having a separate trait for PIN providers that support PIN-change operations, or having a default implementation that cancels the operation.
Cc @iinuwa, in case he has further thoughts on this.
There was a problem hiding this comment.
There are IMHO two things to consider, which makes the collection of new PINs differ from the collection of the current PIN:
-
Necessity: The current PIN provider callback is needed, because the protocol basically forces an interactive collection of the PIN (we don't know for certain if we need it at all, it could be mistyped and we need to collect one again, fingerprint-scans could run out and force us to fall back to PINs, etc.). Whereas for new PINs, we know we need one, when we want to set/change the PIN. So there is no need for interactiveness. Yes, the new PIN could fail the PIN policy, but in that case we could just call
change_pin()again, as that is virtually identical to repeating the request internally with interactive collection of new PINs. -
UI-paradigm: This interactive way of collecting PINs forces applications to a certain UI. The classic 3-input fields form, where you enter the current PIN and the new PIN twice would only be possible with some hacky application-level caching-mechanism (at least I find it somewhat hacky). On the other hand, if an application wants to use a popup-style PIN-collection, it can do so easily enough and then hand in the result non-interactively (and repeat the process, if the function errors out). In short, I think it's far easier to achive 'all' UI-modes with a non-interactive "new PIN"-collection.
For those reasons I also implemented a non-interactive interface in authenticator-rs. I opted for the possibility to even optionally hand in the current PIN non-interactively, and skip interactive callbacks all together. Only if the current PIN was wrong would the interactive callback be initiated.
However, I'm not strongly opposed to collect both PINs interactively, if you want me to. I just fear, it may be more annoying to application writers.
Either way, you are right, that the platform probably shouldn't return a CTAP-error, but have a platform-error variant as well (if we go for non-interactive collection).
There was a problem hiding this comment.
You raise good points, especially around forcing an interactive collection method.
This could be somewhat mitigated by providing a StaticPinProvider implementation for those applications to use, but it's still not ideal - eg. we wouldn't want a StaticPinProvider to automatically retry with the same PIN in case of an error.
The only big downside I see is that we will lose some of the benefit of minPINLength hint. Its inclusion in the authenticatorGetInfo response would normally allow platforms to know this constraint before presenting the dialog to the user, but currently we don't expose this authenticator information to the client application. Right now, the best the client can do is display "PIN too short" and allow a new attempt, hoping the new PIN will be long enough.
Let's merge this as it is for now, as this is already a great improvement and we have no stable API.
I look forward to hearing @iinuwa's thoughts as his portal will be my client of reference. We can change this later on to either become interactive, or alternatively export authenticator information, including minPINLength, to PIN management clients.
|
@AlfioEmanueleFresta, thanks for the ping; I'll make some time to take a look this coming weekend. |
msirringhaus
force-pushed
the
pin_management
branch
from
d873ca2 to
0a342b2
Compare
AlfioEmanueleFresta
merged commit 90fc502
into
linux-credentials:master
3 participants
To get a feel for the project, I figured, implementing something smaller might be a good start.
I have 3 TODOs in there: 2 are related to PinTooShort/PinTooLong errors. The platform "should" display this to the user. I currently only return a PinPolicyViolation in both cases, as otherwise I'd need to have platform-specific error-codes that are not specified in the spec. Should be simple enough, if you want me to do it. Otherwise we ignore the "should" and only tell the user something is wrong with the PIN-length.
The third TODO is what to do if the device doesn't even support Pins. Maybe I have overlooked it, but the spec doesn't seem to mention what exact error should be returned in that case. We can abort early, as I'm doing right now, or simply forward a SetPIN-request to the device at let it error out for us.