fix #8859, add NitroKey3 HOTP support

[UndeadDevel]

This PR:

• fixes qvm-run of yubikey solution not returning anything when screen locked qubes-issues#8859
• adds support for NitroKey3 (HOTP mode)
• updates comments to reflect NK3 support
• changes some yk-* file names to remove yk- as they are now for both YK and NK3
• adds new dependencies (coreutils for base32, util-linux for hexdump, xxd or vim-common for xxd, all available by default in dom0) to rpm file
• bumps version to 1.2.0

I have tested the new NK3 functionality, but don't have a YubiKey to test with; since the YK functionality is not modified aside from @marmarek 's tip in the above linked issue, it should still work / work again, since that issue broke YK support.

Should this PR be accepted, I will provide another one to update the docs.

[UndeadDevel]

Hang on, it looks like there's a bug with the HOTP code for a small number of outputs...fixing...

[UndeadDevel]

Fixed; ready for review.

[marmarek]

changes some yk-* file names to remove yk- as they are now for both YK and NK3

This part isn't that easy - it still need to handle old names for compatibility. Otherwise, the update will break login for some users, which is not acceptable.

[marmarek]

How is nitropy installed? I take there is no proper distribution package, right? Unfortunately, pip (or pipx for that matter) has rather poor integrity validation and is something we do not recommend using. I see there is also an option to download binary (binary? isn't that python?) directly from https://github.com/Nitrokey/pynitrokey/releases/tag/v0.4.44, but it isn't signed either.
Lets see, maybe this can be fixed: Nitrokey/pynitrokey#495

[UndeadDevel]

Yes, pipx seems to be the main way for the typical end user.

The "binary" is actually an executable Linux binary; unpacking it and doing sudo ln -s /path/to/unpacked/binary /usr/local/bin/nitropy allows easy execution without calling python3; it may still use python3 "under the hood", but who knows; when I uninstall python3 in a dvm (as far as possible; the process crashes because qubes packages need python3) and via qvm-console-dispvm unpack and execute the binary, it works.
Edit: any crypto ops currently don't work with the binary, however, due to this issue

Signed artifacts or even better official distro packaging would be nice, yes.

[UndeadDevel]

@marmarek

This part isn't that easy - it still need to handle old names for compatibility. Otherwise, the update will break login for some users, which is not acceptable.

Added checks for old filenames in the script. Edit: or does the update create the new files additionally? In that case I'd need to modify the checks. improved the check for old filenames such that even if the update adds those files, the old ones will still work.

All other suggestions also implemented and I've expanded the command to also work with a proper nitropy installation (currently not possible due to lacking distro package and the available binary not supporting crypto-ops due to this issue).

[marmarek]

Ok, now it looks mostly fine. Please run shellcheck on the script, there are few pre-existing issues, but your change add several more (especially missing quotes in quite a few places).

[marmarek]

PipelineRetry

[UndeadDevel]

Implemented / fixed all shellcheck suggestions / warnings except for the ones on line 86 and 88.
For line 86:

hmac=$(printf "$(printf "%016X" "$counter" | sed -e 's/\(..\)/\\x\1/g')" | 
              ^-- SC2059 (info): Don't use variables in the printf format string. Use printf '..%s..' "$foo".

If I put '..%s..' or '%s' or the same with double quotes there, it will produce a different hmac and thus a different (wrong) OTP...no idea why. In the original script there is also no format argument used.

For line 88:

otp=$(printf "%08d" "$(( ( (0x${hmac:$((2 * 0x${hmac: -1})):8}) & 0x7FFFFFFF) % (10**8) ))" )
                              ^-- SC3057 (warning): In POSIX sh, string indexing is undefined.
                                              ^---------^ SC3057 (warning): In POSIX sh, string indexing is undefined.
                                                                                   ^-- SC3019 (warning): In POSIX sh, exponentials are undefined.

These are "just" warnings, but it's beyond my skill to fix...the code is the basically the same as in the above mentioned script; I've refactored it a bit, but my changes are not the cause of the warnings.

[marmarek]

hmac=$(printf "$(printf "%016X" "$counter" | sed -e 's/(..)/\x\1/g')" |

What about using xxd instead of printf to decode hex? Something like:

hmac=$(printf "%016X" "$counter" | xxd -r -ps | openssl ...

(sed is not necessary then)

^-- SC3057 (warning): In POSIX sh, string indexing is undefined.

This looks to be bash-specific feature, maybe simply change shebang at the top to #!/bin/bash

[marmarek]

BTW should this work with Nitrokey Pro? I think it does support HOTP too.

[UndeadDevel]

It worked, shellcheck is clean now!
Regarding the failed pipeline, it seems to be due to

09:09:56,781 [executor:docker:5a19419cfb15] output:   Running scriptlet: qubes-release-4.2-2.fc37.noarch                      60/60
09:09:56,781 [executor:docker:5a19419cfb15] output: System has not been booted with systemd as init system (PID 1). Can't operate.
09:09:56,782 [executor:docker:5a19419cfb15] output: Failed to connect to bus: Host is down
09:09:56,782 [executor:docker:5a19419cfb15] output: warning: %posttrans(qubes-release-4.2-2.fc37.noarch) scriptlet failed, exit status 1
09:09:56,782 [executor:docker:5a19419cfb15] output: 
09:09:56,783 [executor:docker:5a19419cfb15] output: Error in POSTTRANS scriptlet in rpm package qubes-release

...so not caused by this PR I think.

BTW should this work with Nitrokey Pro? I think it does support HOTP too.

Theoretically NK3, NKPro and even NK Storage should be compatible, but I don't have the latter two to test with and I'm not sure how one would tell them via command line (qvm-run) to release their OTP...in nitropy pro or nitropy storage there is no option to set it up, only in nitropy nk3. However, with my NK3 I can do FIDO2 stuff in nitropy fido2, even though that's supposed to be for NitroKey FIDO2 devices, so maybe the same nitropy nk3 commands would also work with NKPro and NK Storage, but again, I can't test it; there is a forum topic on the NK forums about this, however, maybe it will generate an answer.

[UndeadDevel]

Hmm, looks like xxd is not available on Fedora 32 for Qubes 4.1 (Edit: or it's part of another package, e.g. vim-common?), but then again I haven't tested qubes-dom0-yubikey on 4.1, maybe it worked there; in any case, if this PR is eventually accepted I guess it should only be published as an update for 4.2...as for the pipeline for 4.2 I don't know why it keeps failing, but it could be dependency issues regarding the libvirt-daemon packages...doesn't seem related to this PR but a problem with the testing environment.

[marmarek]

Hmm, looks like xxd is not available on Fedora 32 for Qubes 4.1 (Edit: or it's part of another package, e.g. vim-common?),

You can put something like:

%if 0%{?fedora} < 36
Requires: vim-common
%else
Requires: xxd
%endif

(I checked - it's f36 where the package was split)

as for the other failure, unrelated CI issue, should be fixed in the meantime

[UndeadDevel]

Well, still no answer about NitroKey Pro or Storage on their forum and the latest nitropy release (released after you opened the issue about detached signatures) still doesn't have a detached signature. It seems this is as good as it gets for now. In any case, at least for the detached signature issue, the change would be in the documentation, not this PR.

All other problems are resolved.

[UndeadDevel]

Okay so someone who seems pretty knowledgeable on the NitroKey forum (a member) says nitropy doesn't support HOTP on the NK Pro and NK Storage; the official support is only via the GUI-only NitroKey App. There is, however, a third party CLI tool that could be used, but it's not from NitroKey and, again, I don't have the Pro or Storage to test with, so I'm reluctant to try to "wing it" by somehow including support for it and those two devices.

[marmarek]

Okay, thanks for checking

[marmarek]

I can confirm the YubiKey support still works :)

[UndeadDevel]

Thanks for merging! I'll whip up another PR for the docs in the coming days.

Edit: I'll wait ~2 weeks with the doc PR as it's too complicated right now to setup nitropy with gpg verification what with the mentioned issue with oscrypto, lacking release asset signatures and lacking signature on the pypi release.
Hopefully the situation will improve in the meantime.

[UndeadDevel]

Update: none of the above mentioned issues have been resolved and I can't get pip or pipx to install the local, gpg-verified source package because for some reason it won't accept the presence of the gpg-verified system package flit_core dependency (Fedora 39 flit package).
I'll wait two more weeks, but if there is still no resolution I'll make the doc PR with just a regular pipx install without gpg verification, so that it's at least documented (though the code of this here PR also self-documents the main points); I can put a big warning about lacking verification that can hopefully be removed in the future once nitropy is either properly packaged or at least a signed release asset exists.