Implement fido2/webauthn second-factor auth #116
Conversation
quexten
changed the title
|
@ambroisie For some reason I received your comments on this PR via email but don't see them in GitHub. Anyways, I removed the unecessary cloning code in the last commit, it was left over from when I tried a different Fido2 library, and I hadn't gotten around to cleaning it up yet. |
|
@quexten yes, upon closer inspection I realized that the code was just scaffolding, hence why I deleted my comments. |
quexten
force-pushed
the
feature/webauthn-token
branch
from
037a2e3 to
0ebb4cc
Compare
|
I cleaned up the PR a bit now. I had to make this an optional feature (compiled with cargo build --features=webauthn) because the webauthn-rs library requires openssl. Other than that I'm fairly happy with the state the PR is in right now for a first version. One more improvement that could be made would be to only read the pin when needed. I haven't yet debugged why, but for some reason webauthn-rs always wants the pin, even when Firefox does not require the pin at all for Bitwarden. That way we could remove the code handling the token pin. But maybe that's for a follow-up PR. |
|
|
||
| [features] | ||
| # Enables webauthn 2-FA authenticaton, but requires openssl | ||
| webauthn = ["dep:webauthn-rs", "dep:webauthn-rs-proto", "dep:webauthn-authenticator-rs"] |
There was a problem hiding this comment.
Missing newline at the end of the file.
| webauthn-authenticator-rs = { version="0.5.0-dev", git = "https://github.com/kanidm/webauthn-rs.git", url = "./webauthn-authenticator-rs/", features = ["usb"], optional = true } | ||
| webauthn-rs-proto = { version="0.5.0-dev", git = "https://github.com/kanidm/webauthn-rs.git", url = "./webauthn-rs-protocol/", optional = true } | ||
| webauthn-rs = { version="0.5.0-dev", git = "https://github.com/kanidm/webauthn-rs.git", optional = true } |
There was a problem hiding this comment.
Is there a big difference between this version and the one currently released on crates.io (0.4.8)?
There was a problem hiding this comment.
Yes, Fido2 (CTAP2) was only implemented after 0.4.8. I would like to switch this to the non-dev package as soon as it is released upstream, but without the dev package this PR does not work.
| let out = serde_json::to_string(&result.unwrap())? | ||
| .replace("\"appid\":null,\"hmac_get_secret\":null", "\"appid\":false") | ||
| .replace("clientDataJSON", "clientDataJson"); |
There was a problem hiding this comment.
This looks quite brittle.
An alternative approach would be to create a wrapper struct which serializes the fields in the way the API expects them.
There was a problem hiding this comment.
Yeah, agreed, your proposed solution makes sense. I'll change it to that.
|
thanks for this! i'll probably hold off on this until the webauthn crates are actually on crates.io to avoid having to deal with git revisions. i'd also like for this to be an optional feature, because this does seem to pull in a bunch of dependencies that are going to be annoying to build on some platforms. feel free to poke me once those things are in place and i'll look things over more closely (i've been a bit busy the past few weeks). |
|
Now that webauthn-rs is on crates.io, is there interest in resurrecting this PR? |
As far as I can see, the version on crates.rs is 0.4.8, which is still behind the required 0.5.0. Currently the pr uses 0.5.0-dev, which - while it is on crates.io - is a pre-release, so not sure if that should already be included. Regardless, due to time constraints, I can't currently work on the PR though might at a future date. Aside from that, anyone who needs it right now can of course take over the PR. |
|
|
|
|
This PR implements support for Yubikeys and other Fido2 (Webauthn) tokens as a second factor.
It is currently tested and working with Vaultwarden and a Yubikey 5C, but still needs cleanup / testing.
Fixes #7. Might also make #76 redundant, since newer Yubikeys support Fido2.