Skip to content

Conversation

@millerjason
Copy link

Summary:

  • Adds challenge-response provider abstraction to enable pluggable authentication backends (hardware YubiKey, mock implementations, future smart cards, etc.) via the new ChallengeResponseProvider ABC
  • Implements MockYubiKey for testing YubiKey functionality without physical hardware, supporting both zero-secret (for basic testing) and custom secret configurations
  • Adds touch prompting for YubiKey operations that require physical touch, improving user experience during challenge-response authentication (library would previously hang without notice)
  • Improves error hierarchy by introducing ChallengeResponseError as base class with YubiKeyError as implementation-specific subclass
  • Enhances test coverage with comprehensive tests for both hardware (when available) and mock YubiKey scenarios (always)

Key Changes:

  • src/kdbxtool/security/challenge_response.py (new): Abstract base class for challenge-response authentication
  • src/kdbxtool/security/yubikey.py: Split into HardwareYubiKey and MockYubiKey classes
  • src/kdbxtool/database.py: Refactored to use provider interface instead of direct YubiKey parameters
  • src/kdbxtool/exceptions.py: Added ChallengeResponseError base class in hierarchy
  • tests/test_yubikey.py: Updated with extensive mock-based tests (allows testing without hardware)
  • tests/test_yubikey_hardware.py: Separated hardware-specific tests

Minor:

  • Update init.py and uv versions to 0.1.5 to match github release version.

Copy link
Owner

@coreyleavitt coreyleavitt left a 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__.py version
  • #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.

@millerjason
Copy link
Author

millerjason commented Jan 20, 2026

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 highsecurity() given you have custom support.

One minor request: while I was making these changes I did notice that there was a force push to the main branch which invalidated most of my fork (I think it was done for a CI change?). If you can avoid that on main in the future, I'd appreciate it. It requires careful review (and confirmation there was no source tampering since all commit hashes change).

@coreyleavitt
Copy link
Owner

One minor request: while I was making these changes I did notice that there was a force push to the main branch which invalidated most of my fork (I think it was done for a CI change?). If you can avoid that on main in the future, I'd appreciate it.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants