Add support for NitroKey v3 #2842
Conversation
https://raw.githubusercontent.com/Nitrokey/libnitrokey/master/data/41-nitrokey.rules lists the following USB VID/PID: 20a0:42b2: Nitrokey 3A Mini/3A NFC/3C NFC
|
|
|
I managed to compile and install it on my phone. The previous error message is gone, but a new one appears: "Error: failed to get pw status bytes". If I try again, it fails with an error to read USB-CCID header, and then just random-looking USB errors. So I guess the token is now recognized as supported, but the actual support might be missing or incomplete. I suppose this is why it's a draft PR 😝 |
Very much so, many thanks for giving it a shot. I'm still getting toolchains set up here. |
|
I'm subscribed to this PR and I'm very interested in trying it out. From what I remember, GitHub actually pings by email on new commits to PRs one is subscribed to, but do not hesitate to ping me whenever you specifically want me to test your changes on real hardware! |
|
Yep… well, I had tried downloading Android Studio 2023.1.1.10 (in Gentoo; Downloaded 2020.3.1.24, and things seem much happier after telling it to use the bundled JDK. So, I might be able to see that error for myself soon. :-) The error message you mention seems to come from here: https://github.com/open-keychain/open-keychain/blob/master/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java#L441 -- now the question is, how do we wind up here? |
Some sort of low-level USB glitch? It definitely found the correct device, but it seems it barfed when it attempted to communicate. |
|
Regarding toolchain issues: I use NixOS on my work laptop, so I just wrote the following Nix expression to pull in just enough Android SDK components to make the app build: let
pkgs = (builtins.getFlake "nixpkgs").legacyPackages.${builtins.currentSystem};
sdkConfig = {
cmdLineToolsVersion = "8.0";
toolsVersion = "26.1.1";
platformToolsVersion = "34.0.1";
buildToolsVersions = [ "30.0.2" ];
includeEmulator = false;
emulatorVersion = "33.1.6";
platformVersions = [ "28" "33" ];
includeSources = false;
includeSystemImages = false;
systemImageTypes = [ "google_apis_playstore" ];
abiVersions = [ "armeabi-v7a" "arm64-v8a" ];
cmakeVersions = [ "3.10.2" ];
includeNDK = true;
ndkVersions = ["22.0.7026061"];
useGoogleAPIs = false;
useGoogleTVAddOns = false;
includeExtras = [
"extras;google;gcm"
];
};
androidComposition = pkgs.androidenv.composeAndroidPackages sdkConfig;
sdk = androidComposition.androidsdk;
in pkgs.mkShell rec {
buildInputs = [
pkgs.openjdk11
];
ANDROID_SDK_ROOT = "${sdk}/libexec/android-sdk";
GRADLE_OPTS = "-Dorg.gradle.project.android.aapt2FromMavenOverride=${ANDROID_SDK_ROOT}/build-tools/${builtins.elemAt sdkConfig.buildToolsVersions 0}/aapt2";
shellHook = ''
export PATH="$(echo "$ANDROID_SDK_ROOT/cmake/${builtins.elemAt sdkConfig.cmakeVersions 0}".*/bin):$PATH"
'';
} |
|
Also, sadly, I am not proficient in low-level USB debugging, so I hope you'll be able to trace where the error comes from... |
|
Ahh yeah, I hear lots of things about NixOS. Interesting packaging management concept. Anyway… part of my problem seems to be the USB-C dock I was using was confusing matters. If I use a plain USB-C→USB-A adaptor cable, we get a little closer: So |
|
In the interests of science, this is what happens if I plug in a YubiKey 5 Nano into the same cable: So something is definitely causing the USB transfer to drop early. |
|
Looking at the two tokens and their responses, my hunch is the bulk transfer for the OpenPGP capabilities is ending early in the NitroKey 3. NitroKey 3A Nano (v1.5.0 firmware): YubiKey 5 Nano: I don't know enough about the CCID protocol to fully comprehend what is being sent back, but the YubiKey response is far longer: 258 bytes vs 54. Maybe the NitroKey v3 has less capabilities to send? I don't know, but if the transfer stops short, that'd explain why things aren't working. I tried upping |
|
Both dongles use the same "protocol" ( |
|
Hi! Perhaps this one is connected: trussed-dev/apdu-dispatch#17 |
For OpenPGP, calls to `SELECT`` do not return any data. There is one thing that seems suspicious to me:
We are sending 54 bytes. 54 bytes is exactly 64 - 10. 64 bytes is the length of the USB packets we are sending in usb-ccid, and 10 bytes is the length of a CCID header. |
|
This is what I get over OpenSC: Edit:
Yup. I meant here chaining as well, not the SELECT command per se. |
|
Looking at the data returned in the log, we have the following data objects: The first ones ( Looking at @Override
public byte[] transceive(@NonNull final byte[] apdu) throws UsbTransportException {
CcidDataBlock response = ccidTransceiver.sendXfrBlock(apdu);
return response.getData();
}I think it should instead check the @Override
public byte[] transceive(@NonNull final byte[] apdu) throws UsbTransportException {
ByteArrayOutputStream output = new ByteArrayOutputStream();
CcidDataBlock response = ccidTransceiver.sendXfrBlock(apdu);
output.write(response.getData());
while(output.getChainParameter() == 1 || output.getChainParameter() == 3) {
response = ccidTransceiver.receiveDataBlock();
output.write(response.getData());
}
return output.toByteArray();
}The relevant part of the CCID Specification is in Table 6.1-7, the
|
|
@szszszsz and @sosthene-nitrokey -- Many thanks for the hint there. This is the first time I've done any sort of low-level USB code and also the first time I've tried tackling CCID. Nothing like throwing myself in at the deep end. :-) A big problem being not knowing where to look -- the documentation you've just pointed me to and the example code is a great push in the right direction. I'll certainly give that CCID chaining code a try and see how that goes. |
The `PC_TO_RDR_XFR_BLOCK` structure has two fields which are used in multi-block transfers: - `blockWaitingTime` (that's what the comment called it, in the CCID specs this is "reserved") - `levelParam`: Indicates if APDU begins or ends in this command Some constants for `levelParam` are defined.
|
I added some further debugging to show the raw frames being sent back and forth. So to start: Host sends: So far so good… I notice Host sends: Was I too quick? Or was my "please continue" frame malformed in some way? Full logs: |
|
Looking at the same transaction on a laptop: What you're sending matches what PCSCD is sending (except for the sequence number but that's expected). |
I suppose you might need to do a |
|
Well… zero-sized packet certainly got it talking a little longer. The wheels fall off shortly after that. Received -1 bytes, and I'm guessing the rest of the data in that buffer is junk from the previous read. I note the code here is sending immediately after getting a response. Is there some sort of timing limitation I'm hitting here? |
|
Am I reading the timestamps correctly that the read failed with "-1" after 20 seconds? |
That is correct, I extended the timeouts for debugging. |
|
Ohhh, worked that time! |
|
Couldn't get Password Store to work with it (it's probably talking to the "stable release" of OpenKeychain) but the debug version seems to be able to digitally sign and decrypt using this security token. |
|
openkeychain-nitrokey3-v5.8.2-7-g3404cd2f6.zip The .apk is inside that zip file, for brave souls that want to try it without building. |
|
I can verify that it indeed works with my NitroKey 3A mini 🎉 |
Cool! I had the same problem while I was connecting Nitrokey HOTP Verification tool to NK3's CCID, and just repeating the request has worked, regardless of the set timeout. |
|
Hey folks, just quickly chiming in to let you know I appreciate the work here, and will try to review & merge this PR when it's ready. From a quick glance: Please add some comments around introduced behavior that is otherwise inexplicable, e.g. the "retry three times" logic in that last commit. USB (and NFC) communication is a fickle beast and much of it is organically grown precisely from empirical "if I hold it like this it'll work" work like here, so it's important to retain at least some of that knowledge for future readers of this code. |
Include references of where the magic numbers came from.
There is room in the 100-char width to fit this. (Yeah, too used to coding in 80 columns!)
100 characters with exceptions made for CLI commands, URIs and imports. Some of this is existing code, but for the sake of consistency, we'll wrap all at 100 characters. Function arguments: when this happens, if we put the closing `)` on its own line (like `}` in code blocks), it visually stands out better for indicating where the function call ends.
No problems… yes well… this also being my first time doing any kind of significant USB code (bar messing with some LUFA stack examples). I greatly appreciate the insights provided by @szszszsz and @sosthene-nitrokey, those were a big help to getting this across the line. I've done a bit of tidy-up on the code submission this morning (spotted one tab that escaped my attention earlier -- I need to tell Where I've introduced constants, unless its bleeding obvious (e.g. the USB product IDs, they're grouped together) I've added a reference to what document the constant was taken from. I hope that takes care of the coding style, but if there's anything more, let me know and I'll try to sort it out. I've also tried this same code against a YubiKey 5C, and that too seems to work fine, so it appears no regressions that I can see. That said, users of non-NitroKey CCID devices, it'd be worth trying this branch out with your CCID device and make sure I didn't accidentally introduce regressions. :-) |
|
Ok I finally got the time to test it out! I used a 3A Mini, and it seems to work (took hours before I finally managed to make my own APK >~<). The only problem I have is that okc-agent when logging in to SSH it does recognize my key, but it doesn't ask to enter a pin number. If I encrypt a file directly in OpenKeyChain, it does ask for a pin number, so this might be a okc-agent issue which I will dive into further (see screenshot). Another issue is that it didn't work to import my key if the pgp key was on a keyserver, I had to load the public key to my phone and then import the file in OpenKeyChain, then import the NitroKey. Not a big deal, but something you have to be aware of to avoid spending ages figuring out why it doesn't work.
Edit: Idk why, but when I deleted my key (make sure to untick revoke!) on OpenKeyChain and re-loaded it, it was working! |
I recall trying to get
This is odd… maybe there was an issue with communicating with the key server at that time? I have the URI set for the public key and that seems to work reliably -- provided the key server exports the UIDs (not all do). |
There was a green checkmark when it found my public key from the keyserver. I have no idea if it had to do with the fact that my pgp key is imported and not generated on-device? |
Doubtful… as I understand it, what's stored on the token is the private keys. Specifically the sub-keys for encryption, digital signatures, and authentication. To make those useful, you need the public key certificate which also includes UID information, and the public keys of the sub-keys (that match the private keys on the token). How the private keys came to exist on the device is irrelevant: the one you're importing is the public "master key", which does not exist on the token (nor does its private counterpart -- if done properly, the master private key is stored offline somewhere safe). There are two ways that I know of:
|
|
What's blocking to merge this NitroKey support to the master branch? It would be really useful for Nitrokey users. |
|
So, this PR has sat here for some time now. I understand that most development on OpenKeyChain has ceased with only major bug fixes and security issues being addressed now, which is a problem as I do not know of any alternatives. As some may have gathered, I'm far from an "expert", and fumbled my way through this… so I'm likely not a good candidate for taking over maintainer-ship. Technical debt: it relies on a quite old version of Android Studio to build, we'd have to spend a bit of time coaxing it to a newer release to keep dependencies current. I did not do this here as I wanted to keep the PR "pure". Modernisation of the code base to me should be a separate PR, and possibly is most practically done after merging NitroKey 3 support. A possible option for the short-term, is we re-brand OKC (maybe we call it "NitroKeyChain"?) so that people can keep existing installations for other security tokens if needed, and we maintain a fork which adds NK3 support until such time as the final fate of this PR (and OKC in general) is known. Personally though, I'd like to see OpenKeyChain continue as it is today, being a vendor-agnostic OpenPGP security token and keypair manager for Android. It already has support for other tools like PasswordStore (which I use), ConnectBot and K9Mail which would need to be duplicated if we forked it to a new program. I guess the question is, who amongst us has prior experience with Android application development involving NFC and USB interfaces? I can pitch in and help where I can, but I think there are better qualified people than I to take the lead. |
|
Took me a while to get everything running with all new dependencies and build tools, see #2876. We should have this done shortly and get all this work into a release. Sorry for the delay :) |
|
@Valodim Great news! Thank you! |
|
This PR is merged (with some changes) and part of the 6.0.0 release already, I had just forgotten to close it. Cheers, and thanks for your work again @sjlongland |
|
Has anyone got 3A NFC to work on GrapheneOS already by any chance? |
Description
This branch adds support for the NitroKey v3 family USB VID/PID to OpenKeychain.
Closes #2840
Motivation and Context
Plug in a NitroKey 3A Mini, at present it says it's not supported. The NFC version is not in stock, and it'll never be possible to do NFC with a NFC-free 3A mini.
#2840
How Has This Been Tested?
It has now, although I'd appreciate others giving this a shot.
Screenshots (if appropriate):
#2842 (comment)
Types of changes