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 support for yubikey-manager #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NgoHuy
Copy link

@NgoHuy NgoHuy commented Dec 8, 2023

Because yubikey-personalization is not under active development, I add support for yubikey-manager. It works fine on my machine for format, enroll and open.
I did not test NFC because I do not have NFC machine.
For hooks, I added ykman and od, it should work.
README should be modified if this commit is approved.

@Vincent43
Copy link
Collaborator

As discussed in the issue ykman isn't drop-in replacement for yubikey-personalization so this is incompatible change

@NgoHuy
Copy link
Author

NgoHuy commented Dec 8, 2023

My patch is compatible with previous version. You can use it with your old challenge. You should read my patch again.

Copy link
Collaborator

@Vincent43 Vincent43 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, it's interesting.

As for now I spotted few issues with quoting detected by shellcheck, see instructions how to check it yourself.

Also let's keep using printf % instead of echo -n for consistency with rest of the code.

It seems this reintroduces #44 (challenge leaks in process list as argument to ykman otp calculate command). It needs to be addressed.

It should be doable - we need to pipe challenge as before, for example:
printf %s "$YKFDE_REAL_CHALLENGE" | ykman otp calculate "$YKFDE_CHALLENGE_SLOT"

src/hooks/ykfde Outdated Show resolved Hide resolved
src/hooks/ykfde Outdated Show resolved Hide resolved
src/ykfde-enroll Outdated Show resolved Hide resolved
src/ykfde-enroll Outdated Show resolved Hide resolved
src/ykfde-enroll Outdated Show resolved Hide resolved
src/ykfde-open Outdated Show resolved Hide resolved
src/ykfde-open Outdated Show resolved Hide resolved
testrun.sh Outdated Show resolved Hide resolved
testrun.sh Outdated Show resolved Hide resolved
testrun.sh Outdated Show resolved Hide resolved
@NgoHuy
Copy link
Author

NgoHuy commented Dec 9, 2023

I added new commit to address those issue. Please test it

@NgoHuy
Copy link
Author

NgoHuy commented Dec 9, 2023

There is problem on hooks, I will look at tomorrow. Python is dificult to add library.

@Vincent43
Copy link
Collaborator

Yes yubikey-manager has a lot of python deps. Adding them to initramfs would be a challenge.

@NgoHuy
Copy link
Author

NgoHuy commented Dec 10, 2023

We have few options:

  • pack ykmanager as ELF binary, it would increase initramfs's size. I don't prefer this way.
  • write custom script to find deps and add it to initramfs, but it's hard to maintain because we're not sure deps change in the future and break our package. It needs to regenerate every time when yubikey-manager has been released.
    I will read otp code and find package it used, not all packages.

@NgoHuy
Copy link
Author

NgoHuy commented Dec 11, 2023

I tested with pyinstaller successfully, but the initramfs's zise is around 50Mb
if we find all python's libraries, we need some .so files. I have an idea, unpack pyinstaller's output and add those files which in plain text.

@NgoHuy
Copy link
Author

NgoHuy commented Dec 12, 2023

I added new commit, please check it, it works with hooks on my machine.

src/install/ykfde Outdated Show resolved Hide resolved
@agherzan
Copy link
Owner

Can you please squash the commits that are iterative and update the git log to help review?

Because yubikey-personalization is not under active development, I add support for yubikey-manager. It works fine on my machine for format, enroll and open.
I did not test NFC because I do not have NFC machine. For hooks, I added ykman and od, it should work.
README should be modified if this commit is approved.
@NgoHuy
Copy link
Author

NgoHuy commented Dec 13, 2023

I squashed commit, please review it.

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.

3 participants