-
Notifications
You must be signed in to change notification settings - Fork 1
feat: yubikey mock and touch prompting #63
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
base: master
Are you sure you want to change the base?
Conversation
coreyleavitt
left a comment
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 putting this together. The motivation is spot on. The current YubiKey implementation was really just a proof of concept, and your PR highlighted that it's time to do this properly.
You also surfaced a couple things I've opened issues to address separately:
- #65: Release workflow not updating
uv.lock/__init__.pyversion - #66: Documenting KDF customization (re: the
high_security()iterations param)
After thinking through the interface design, I want to take a different approach. Looking ahead to supporting FIDO2 hmac-secret (which covers YubiKey, Trezor, Ledger, and many other hardware security devices), I think we need:
- A Protocol rather than ABC. There's no shared implementation to inherit; it's purely a contract. This also means test mocks and third-party implementations don't need to import/inherit from library code. They just implement the method.
- The interface itself should just be
challenge_response(bytes) -> SecureBytes. The Database doesn't need to know anything about what device produced it. - Device-specific concerns (touch, PIN, confirmation) are configuration on the provider class, not part of the Protocol interface. The library provides classes for known devices; users implementing custom providers just need the method signature.
Worth noting: we'll want to support both the current YubiKey HMAC-SHA1 approach (KeePassXC-compatible) and FIDO2 hmac-secret (device-agnostic, broader hardware support). Users will need to consider portability when choosing, since KeePassXC and KeePassDX don't support FIDO2 hmac-secret yet.
I also want to rework the touch workflow. I have some ideas for how to handle that more cleanly.
On MockYubiKey: I see it's intended for downstream users' tests. The core mock is straightforward (just HMAC the challenge), but I see the value in the predefined test secrets and factory methods. If we ship one, it probably belongs in kdbxtool.testing rather than kdbxtool.security.yubikey. Thoughts?
Rather than ask you to rework the whole thing to match what's in my head, I'm going to take a pass at the implementation myself. Planning to work on this later this week. I'll cherry-pick directly from your branch where possible so you get credit in the git history. Hope that's okay.
|
Yes, that approach makes sense to me and FIDO2 sounds like a nice addition. Protocol sounds correct as well. I considered this, but wanted to provide a PR that was digestible for you to actually consider merging, so I tried to not disturb too many of the preexisting APIs from 1.5. If you're considering at more complete implementation that rethinks the API in return for more functionality, all the better as far as I'm concerned! Although I originally added the MockYubi mostly for testing (critical to prevent regressions IMO), I actually found it is better to think of it as a "soft" implementation. Since it doesn't require hardware or programming, it's also useful in the user-facing APIs since it allows you to implement a soft key on the fly. You have to be willing to risk the secret loss that you'd traditionally get from true hardware, but I used this "soft implementation" quite a bit in a test environment until migrating to hardware for a final cut. Great for testing in a server container, for example. Even as a pure soft implementation, it still has some advantages over the more common TOTP. In short, I hope to continue to use it for more than just testing. Feel free to take from either of my fork branches. PS: I could not find a way to query if a Yubi requires touch or not, so my approach was just to print the message after a long pause. You might have better luck searching their APIs, this approach did bother me as well. Also agree this shouldn't include changes to One minor request: while I was making these changes I did notice that there was a force push to the |
I had pushed an unsigned commit before adding branch protection rules requiring signed commits. For a security library I wanted a complete signed history for auditability, so I rewrote rather than leaving a gap in the chain of trust. I didn't realize at the time that you had forked - sorry for the inconvenience. This shouldn't happen again now that branch protections are in place. I'll tag you on my PR for the challenge-response rework once it's ready - would appreciate your input on the approach. |
Summary:
Key Changes:
Minor:
0.1.5to match github release version.