iqlusioninc
Add curve 25519 support
Yubico added support for curve 25519 in the PIV applet since firmware '5.7.X'. This support has already been added to yubico-piv-tool, but it is lacking in this Rust library. This PR is an attempt to keep the libraries aligned on functionality. Changes made are as described:
curve25519-dalekcrates added as dependenciesX25519andEd25519added as available algorithms- Updated the integration tests to include an ED25519 signature check
- Tests did not seem to work well for me (constant poisoned mutex issue) so I made some modifications but can revert these if necessary
- Wise to double check these work as intended
I'm open to any feedback. This is my first real go at production Rust code so criticism is welcome/needed for me to learn.
Additional items to maybe consider:
- I'm not sure how this will impact anyone using firmware older than '5.7.X'
- Might want to add a check on that and throw
NotSupportederrors - Otherwise, take an approach where firmware version is tied to release version?
- Might want to add a check on that and throw
What is the status here? Doesn't look like a fundamental change, but it feels like it hasn't been reviewed in half a year :confused:.
I'm in the process of reviewing the PR, but can't test it effectively before #578 because the Yubikey firmware version that added Ed25519 support also changed management keys to use AES instead of DES.
You can test on a YubiKey with 5.7 firmware by first setting the management key to the default TDES key. Assuming the default AES management key:
ykman piv access change-management-key -m 010203040506070801020304050607080102030405060708 -a TDES -n 010203040506070801020304050607080102030405060708
I am currently trying to rebase this onto main, but the changes introduced in #583 make it hard to integrate ed25519-dalek, as they still use the older version of the signature crate. 😕
I would wait a week or so to do that. We're still in progress of bumping rand_core/edition/msrv everywhere in rustcrypto
I would wait a week or so to do that. We're still in progress of bumping rand_core/edition/msrv everywhere in rustcrypto
any updates on what is currently going on here in the Rust crypto ecosystem?
I took a look at this again recently, and it appears the curve 25519 libs (ed25519-dalek) are all still missing certain traits which need to get implemented for a proper rebase to work in the new KeyType implementation in src/certificate.rs. I've got a new branch rebase/curve_25519 on my fork which utilizes a wrapper around the types to get it compiled (haven't tested since I need a clean YubiKey to run the integration again). I haven't moved much more than that because I don't think a wrapper in this project makes sense vs getting proper support upstream.
So I'm basically going to sit on my hands with this until that work is done, or if the age-plugin-yubikey project gets updated to utilize the new KeyType changes in this lib. Otherwise, I don't have much personal use cases to force things to work in this lib at the moment.
No, ed25519 and ed25519-dalek have the required traits (https://github.com/dalek-cryptography/curve25519-dalek/pull/779 & https://github.com/RustCrypto/signatures/pull/889), that issue is not closed yet because x448 doesn't have them yet. (i don't recall the situation around ed448).
@baloo, I see why I didn't find it. It's in pre-release (3.0.0-pre.0) still. I didn't think to check that, my bad. That should make this easier if all the traits are implemented in ed25519. I'll pin to the pre-release and give this a shot once more and report back if we have everything to move this forward more.
Alright, I was able to get a rebase and sort out a bunch of the merge conflicts to get past changes integrated (it's basically a new branch). I also, made another new branch on my fork of age-plugin-yubikey to make use of this branch for testing. Everything seems to work like it had before the x509_cert updates so I'm feeling better about these changes. Would love a double-check by someone more familiar with Rust and the dalek libs. I definitely took a cudgel to the lockfile in trying to get the different deps working. I'm also not sure if there is a better direction to take with the x25519 algorithm so feedback is very much appreciated.
Once #589 is merged (it and #588 are now in review), I'll be resuming review of this.
I saw that #589 was merged and rebased this branch onto main to pick that up. Checked everything against my personal yubikey using my fork of age-plugin-yubikey that uses this branch as a dependency, and everything still works when using TDES and AES192 management keys set (with PIN protection). Again, I don't have a clean key I can use with the integration tests so can't exactly confirm things on that end directly. Would appreciate a proper review/test if anyone has the capacity to do so.
cloned @dlubawy repo branch rebase/combined_encryption, was able to build and run age-plugin-yubikey --generate for yubikey v5.7.1 using AES256 successful. thank you 🙏 🙇
