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

Fix branding issue with HOTP USB Security Dongles #761

Merged
merged 21 commits into from Jul 23, 2020

Conversation

alex-nitrokey
Copy link
Contributor

@alex-nitrokey alex-nitrokey commented Jun 24, 2020

This PR fixes the issue of having the terminology Librem Key everywhere in the code while there are more possible keys. On the same time we want to make it easy for users to know which key to insert. Thus, it is ensured that heads asks for the correct branding after initialization by storing the info in /boot/kexec_hotp_key.

This PR already includes #748 and thus #756 because otherwise it would not function correctly.

Things still needed:

In best case I can add these tomorrow already.

Please note that I intentionally used other terminology for the variables than discussed in #746 as they felt more suitable for me. Please consider my suggestion and let me know if shall change CONFIG_HOTPKEY to CONFIG_HOTP_USB_SECURITY_DONGLE instead.

@alex-nitrokey alex-nitrokey changed the title WIP: Fix branding issue with HOTP USB Security Dongles Fix branding issue with HOTP USB Security Dongles Jun 25, 2020
@alex-nitrokey
Copy link
Contributor Author

Ready and tested. Reviews are welcome :)

I am sorry that it took so long and I hope you like this solution.

@tlaurion
Copy link
Collaborator

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 28, 2020

NitroKey CircleCI:
31e0a25e4dcf2efb2e4f541050db894709c376750bc224b8bc884df74a7e5fcd ./bin/hotp_verification
Local build on debian-10:
ec8c73bbfd7380b7262164d50d8bf25e430b37f71b61543974a7d576d95c1127 ./bin/hotp_verification

`

@tlaurion
Copy link
Collaborator

@szszszsz we still have reproducibility issues?

@tlaurion
Copy link
Collaborator

Randomly chosen binary:

Nitrokey CI:
9ffac1032dc6805d0a45200d2d9e9543928ecc6555a6c47c7f6a174cb24bfe77 ./lib/libqrencode.so.3

Local:
9ffac1032dc6805d0a45200d2d9e9543928ecc6555a6c47c7f6a174cb24bfe77 ./lib/libqrencode.so.3

@tlaurion
Copy link
Collaborator

Building and functionally working, though. :)

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 28, 2020

Note that testing with a Librem Key still asks for user to insert his Nitrokey. Not sure Purism will be happy with that merge.

How is that going be be dealt with finally? Are we expected to duplicate all boards config to have their _Nitrokey _LibremKey derivatives so that it is specified specifically in board configs?

@kylerankin @alex-nitrokey @szszszsz

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 28, 2020

@alex-nitrokey : Im comfortable with CONFIG_HOTPKEY

@alex-nitrokey
Copy link
Contributor Author

Note that testing with a Librem Key still asks for user to insert his Nitrokey. Not sure Purism will be happy with that merge.

Erm... yes, I guess it is the version with the older firmware were there was a vid:pid error, right? As this PR is checking the vid to decide which device is used, it is difficult to do anything about this issue with the older firmware without breaking the idea of an automatic discovery :(

we still have reproducibility issues?

Will have a look at it with @szszszsz

@alex-nitrokey
Copy link
Contributor Author

besides hotp_verfication there is a reproducibility problem with

  • ./lib/libgcrypt.so.20 and
  • ./lib/libassuan.so.0

hotp_initialize buils correctly though.

Calculate the uncompressed used cache space
Decrease retry count
@szszszsz
Copy link
Contributor

@tlaurion @alex-nitrokey
We should have no repro issues anymore - investigating. Queued job on Gitlab CI.

@alex-nitrokey
Copy link
Contributor Author

alex-nitrokey commented Jun 30, 2020

I tested the new circleci hashes with a local build (debian) and they matches for all hotp stuff (though not for the above mentioned).

quite strange...

@szszszsz
Copy link
Contributor

szszszsz commented Jun 30, 2020

@szszszsz
Copy link
Contributor

@tlaurion @alex-nitrokey
Looks like this change helped. Both builds have deb80848041f503 hashes now for ./bin/hotp_verification. Confirmed as well with local build following Gitlab CI config file.

Regarding #761 (comment) nothing comes to my mind.

@szszszsz
Copy link
Contributor

szszszsz commented Jul 6, 2020

Hi @alex-nitrokey @tlaurion !

@alex-nitrokey
Copy link
Contributor Author

I believe this PR is ready for merge. If not, what's left here to finish?

I considered it done, yes.

I think the repro build issue mentioned in #761 (comment) is not introduced by this PR. Could you confirm or deny @alex-nitrokey ?

Exactly, it consists independently. Probably after updating gpg2, but I am just guessing here.

initrd/bin/seal-hotpkey Outdated Show resolved Hide resolved
# Set HOTP USB Security Dongle branding based on VID
if [ ! $(lsusb | grep -q "20a0:") ]; then
HOTPKEY_BRANDING="Nitrokey"
elif [ ! $(lsusb | grep -q "316d:") ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why, but this ends up showing my Librem Key as a Nitrokey.
using:
elif lsusb | grep -q "316d:" ; then
works correctly however

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The negation is used because grep returns 0 if it found something.

Older Librem Keys had the same VID as the Nitrokeys because of a mistake done when building the firmware for it, see #761 (comment)

I could not think of a way to prevent that :(

So I am wondering, what is the return value of lsusb | grep -q "316d:" for you? If it is 0 the patch should work as intended. Does it?

Copy link
Contributor

@szszszsz szszszsz Jul 24, 2020

Choose a reason for hiding this comment

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

In other words first batches of the Librem Key had incorrectly set VID:PID to the same as Nitrokey Pro has, hence the confusion.
Perhaps they have changed the USB names only back then (might not show up in the lsusb, but maybe dmesg) - that would already help to differentiate between models, otherwise I do not see how to make it possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I am wondering, what is the return value of lsusb | grep -q "316d:" for you? If it is 0 the patch should work as intended. Does it?

0, and no the patch does not work correctly with my LK. It identifies as a NK without the change I mentioned above

edit: this LK is one of our new US-built ones with VID/DID 316d:4c4b. Another older one is 20a0:4108 and so identifies as a Nitrokey

Copy link
Contributor

Choose a reason for hiding this comment

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

pushed #781 to fix

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.

None yet

5 participants