agherzan / yubikey-full-disk-encryption

Use YubiKey to unlock a LUKS partition
Apache License 2.0
821 stars 51 forks source link

Use `ykman` instead of `yubikey-personalization` #101

Open EliasHolzmann opened 1 year ago

EliasHolzmann commented 1 year ago

Currently, this project uses the YubiKey Personalization Tool which is no longer under active development. It would be nice for yubikey-full-disk-encryption to instead use YubiKey Manager which has more features. I would be especially interested in this because the YubiKey Personalization Tool does not seem to support NFC, which I need.

Would you be willing to merge a pull request that implements this? I'm currently a bit busy, but I might be able to get this done in a few weeks.

agherzan commented 1 year ago

Yes, that is something we can/should do. ykman provides the top calculate/chalresp that could be used similarly. The only issue is that calculate takes a hex argument, so we will need to do the conversion (od or something). I would also vote for maintaining backward compatibility especially since everything works as expected with yubikey-personalization as of now.

EliasHolzmann commented 1 year ago

The only issue is that calculate takes a hex argument, so we will need to do the conversion (od or something).

I'm probably being a bit obtuse right now, but didn't ykchalresp require the challenge in hex as well, and we get it in hex from sha256sum anyway? Replacing printf %s "$YKFDE_CHALLENGE" | ykchalresp -"$YKFDE_CHALLENGE_SLOT" -i- with ykman otp calculate 2 "$(printf %s "$YKFDE_CHALLENGE")" seems to work correctly for me at least (though I couldn't verify that both results are the same as I don't own a security token with USB).

I would also vote for maintaining backward compatibility especially since everything works as expected with yubikey-personalization as of now.

Agreed, that would be nice. Although that begs the question: Which of the two yubikey packages should be a dependency, and which should be an optional dependency? Or should we specify both as optdepends?

Vincent43 commented 1 year ago

I'm probably being a bit obtuse right now, but didn't ykchalresp require the challenge in hex as well, and we get it in hex from sha256sum anyway?

No, ykchalresp can take anything up to 64 byte length, it doesn't need to be hex. I used sha256 only because this way it's always a valid challenge while user typed one longer than 64 byte won't be valid.

In my testing the responses for the same challenge are different though which makes this a no-go as it breaks all current users:

❯ ykman otp calculate 2 61e748be34d998d8c5a13db63c2b50c4f129d2828a46bae182e6ddec5ff1ff8a
Touch your YubiKey...
a47aa802aedcf28c6f0391d74028da887f006e8d
❯ ykchalresp -2 61e748be34d998d8c5a13db63c2b50c4f129d2828a46bae182e6ddec5ff1ff8a
3613fa5ed3f61a3f187d9db0fbdfc5012af5e2b9
Vincent43 commented 1 year ago

BTW: there is https://github.com/Frederick888/ykchalresp-nfc which we support to some degree.

agherzan commented 1 year ago

That output looks strange. What could produce that difference?

Vincent43 commented 1 year ago

I found the fact ykman encodes challenge as hex is the answer. If I call ykchalresp in hex mode then the output matches:

❯ ykchalresp -2 -x 61e748be34d998d8c5a13db63c2b50c4f129d2828a46bae182e6ddec5ff1ff8a
a47aa802aedcf28c6f0391d74028da887f006e8d

In default mode those tools aren't compatible with each other. I think backward compatibility is crucial so ykman support could be only introduced as opt-in.

agherzan commented 1 year ago

I agree - and thanks for looking into it. We would need to make it backwards compatible.

Vincent43 commented 1 year ago

Note that in Arch ykman (yubikey-manager) package depends on yubikey-personalization so all users always need them both installed.

agherzan commented 1 year ago

I don't see why that dependency is defined as such. These specific tools don't have a dependency per se.

EliasHolzmann commented 1 year ago

I don't see why that dependency is defined as such. These specific tools don't have a dependency per se.

I've looked into that – it was needed until a while ago, now the dependency is unused and will probably be removed sooner or later: https://bugs.archlinux.org/task/73290

NgoHuy commented 12 months ago

I added commit #103 please test it.

NgoHuy commented 12 months ago

My patch keeps old challenge working and therefor, it can replace yk-personalization.