Skip to content
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

Add challenge-response method for KeepassXC support #64

Merged
merged 1 commit into from
May 17, 2023

Conversation

szszszsz
Copy link
Member

@szszszsz szszszsz commented May 6, 2023

Add support for KeepassXC through challenge-response method.

Tests: Nitrokey/pynitrokey#384
KeepassXC integration: keepassxreboot/keepassxc#9397
Does not contain PWS: #63 With PWS now.

Incompatible changes:

  • runner should provide now via options the device's serial number

To discuss:

  • Should returned version in the Status command be application or runner firmware version (to show up in KeepassXC, see linked PR)
  • Regarding used PKCS#7 unpadding, extract to separate crate or use known good implementation
  • Would unchecked get bring any performance improvement (in unpadding)

To do in the next PRs:

To test later:

  • Use touch protected HMAC secret slot

Fixes #61

@szszszsz szszszsz added the enhancement New feature or request label May 6, 2023
@szszszsz szszszsz marked this pull request as ready for review May 6, 2023 14:27
@szszszsz
Copy link
Member Author

@sosthene-nitrokey @robin-nitrokey ping

src/authenticator.rs Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
src/authenticator.rs Show resolved Hide resolved
src/authenticator.rs Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
src/calculate.rs Show resolved Hide resolved
@sosthene-nitrokey
Copy link
Collaborator

sosthene-nitrokey commented May 16, 2023

Regarding PKCS#7 padding:

I don't think the current implementation is correct.
PKCS#7 padding is dataxxxx where x is the remaining length. If the last byte of data is also x it will also be unpadded, when it should be left.

I think we should instead use the block_padding crate from RustCrypto.

@szszszsz
Copy link
Member Author

szszszsz commented May 16, 2023

Yup, that's correct indeed - this is Yubikey's "PKCS#7 padding" version. To do:

  • describe in detail in the code's comments the difference from the correct padding

Edit: I have noted that down in the Python tests, but here it would be nice to have it as well

@szszszsz
Copy link
Member Author

I don't think the current implementation is correct. PKCS#7 padding is dataxxxx where x is the remaining length. If the last byte of data is also x it will also be unpadded, when it should be left.

We might need to support it bug-for-bug. If there are any other applications, they probably have it implemented like this anyway. To discuss.

I think we should instead use the block_padding crate from RustCrypto.

I suppose we can use it, once we take into account the non-standard PKCS#7 input, and correct it beforehand.

@szszszsz
Copy link
Member Author

Review corrections were added in #71. Please verify briefly.
Merging this PR in.

@szszszsz szszszsz merged commit 16780f1 into main May 17, 2023
@szszszsz szszszsz deleted the 61-yk-keepassxc branch May 25, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add challenge-response support for KeepassXC
2 participants