-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Nitrokey 3 support #1483
Nitrokey 3 support #1483
Conversation
* add Nitrokey 3 support * corrected UI issues, when PIN is not set * add serial number getter * improve HID calls speed * Full changelogs to be found here: https://github.com/Nitrokey/nitrokey-hotp-verification/releases
The goal should be to have a runtime mechanism for switching to another algorithm during oem-factory-reset so that the Nitrokey 3 can be used with any HEADS board and there is no need for a compile-time configuration.
@tlaurion is this ok for you ? |
Further I like to address this comment: #1417 (comment) (@saper) Generally, I would also prefer ed25519, but for now this is obviously not a real option, the main issue which is addressed is this one:
From what I understand, this does not apply here. We generate the key on the Nitrokey 3, thus also signing is done on the Nitrokey 3 through GnuGPG, right? Then the host-rng is not used, but the Nitrokey 3 rng - means this indeed is a valid point for host-only gpg keys and signing, but not for this situation using a token like the Nitrokey 3. |
This seems to be the way, yes. Some notes from past discussions and current Purism implementation that was upstreamed from Pureboot #1419 : The re-ownership now proposes to the user to use defaults and then goes into prompts for advanced choices. I think #1417 was not "wrong" and having a default algo either, but we need to have Heads pick the right one if some USB Security dongles cannot deal with the default. Past discussions made us decide for RSA3076 since RSA4096 was taking 5 minutes for subkeys for a total of 15 minutes of key gen and was considered unacceptable. RSA3076 was chosen for tradeoff as default to keygen in USB Security dongle in 3 minutes. Since NK3 cannot do RSA3076 now, I think the code to select default should warn user of the limitation as of now, and welcome the user to track documented issue (temporary measure) and then select the proper algo and continue its business with
@daringer : We have a plan? |
@saper @daringer I have experimented on that matter under #1476, which discussion should continue there. Some get home messages at time of writing these lines:
So TLDR:
@saper @daringer : opinions on that? I think the above will is going to be the state of #1478 prior of merging. |
@daringer my hunch from the root of the problem in code is that RSA3096 is enforced as default without forcing usb stack enablement (. /etc/functions enable_usb) to make sure product supports what we otherwise would force to be supported. I think its wrong for many reasons, most of which not supported right now just like gnuk and everything else https://gitlab.com/openpgp-card/virtual-cards which would be nice eventually to be supported since OpenGPG smartcards afterall. For example, gnuk only support RSA2048, which would also fail oem-factory-reset as of now. But to stay on point with current issue, I think the solution is:
@daringer @JonathonHall-Purism ? thoughts? |
I think this is a good plan @tlaurion . I don't see a downside, we can enable more hardware to work without user intervention. I don't see any problem calling enable_usb earlier in oem-factory-reset. |
pushed an initial draft:
canceled the circle-ci build and did the change on top of the current implementation, once we define that this is the correct solution I can squash and run CircleCI builds. Also not extensively tested yet, for us it would reduce efforts for testing once #1485 is merged... |
I generally like what I see under 5435131 Some quick comments as shared through direct comunication channels
Would be ideal. We do not want oem-factory-reset to become spaghetti code more then it is right now, calling usb_security_token_capabilities_check or whatever you see fit should be clear to anyone running Heads in debug to understand what is happening. The reasoning behind this is that if clean enough, gnuk keys, hsm keys or whatever token/usb security dongle used will be easy to support. |
I would also love to see NK3 usb security dongles supported under qemu boards for more general testing (which is what I will use to test this PR) under
Which block can be found under all qemu-coreboot-* boards under boards |
|
@JonathonHall-Purism @daringer: outside of the comments here, I am ok to merge |
CircleCI run not canceled (don't expect any issues here) - once this is done I will squash things down to 2 commits and switch to a "Ready for Review"-PR so it can be merged... Generally about formatting: Is there any reason HEADS is not using |
I have absolutely no problem doing the cleaning iteratively, I started doing in my other PRs recently (files touched were identation unified while I adapted code on those files). There is an issue opened to do that in the final goal of having shellcheck added in CI as an early failing step prior of building boards. Nobody did it yet. We would need to move all scripts including functions scripts to sh suffixes filenames and adapt codebase to call those new names. Tldr: no reason to not use shfmt on top of changes as of today. In my opinion that will reduce the review cost of doing everything said earlier to make sure nothing broke. |
* use GPG_ALGO as gpg key generation algorithm * determine GPG_ALGO during runtime like this: * if CONFIG_GPG_ALGO is set, use as preference * adapt based on usb-token capabilities (currently only Nitrokey 3)
d19addb
to
b47da0b
Compare
|
@daringer I took liberty to updated OP content |
Originally first introduced in: #1417