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 reset command for nitrokey 3 #46

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Add reset command for nitrokey 3 #46

merged 4 commits into from
Dec 11, 2024

Conversation

sosthene-nitrokey
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Nov 25, 2024

Close #42

./hotp_verification reset

Is a noop on a device other than NK3. On an NK3, reset the secrets app and thus the HOTP code.

@nestire
Copy link

nestire commented Nov 27, 2024

I get this with a nitrokey pro 2 or nitrokey storage attached, same binary works with an nk3

-rwxr-xr-x 1 user user 55K Nov 27 10:54 hotp_verification
sha256sum hotp_verification
86c94ed1bc0cfb43b3dc2b423b74a56ef80f356fc0aa83915a82ef04651c2862  hotp_verification
[user@work nitrokey-hotp-verification (nk3-reset)]$ ./hotp_verification reset
HOTP code verification application, version 1.6

Segmentation fault (core dumped)
[user@work nitrokey-hotp-verification (nk3-reset)]$ 

@tlaurion
Copy link
Contributor

tlaurion commented Nov 28, 2024

@tlaurion
Copy link
Contributor

Ideally, hotp_verification reset would accept a parameter here, which would be the secret app PIN.

Otherwise as can be seen under linuxboot/heads@07f3710, it requires an additional step which would be PIN change, but we don't have a PIN here, since we just reset :)

I still think, as said under #36 (comment) that changing PIN is not really important to Heads use case since we reset. What is important is setting a secret app PIN at moment of oem factory reset/re-ownership.

Need:

  • hotp_verification reset DESIRED_SECRET_APP_PIN

Originally posted by @tlaurion in #42 (comment)

@sosthene-nitrokey
Copy link
Contributor Author

Yes, I'm on it, just waiting for the devices I need to reproduce and debug the segfaults, they're on their way.

@tlaurion
Copy link
Contributor

Ideally, hotp_verification reset would accept a parameter here, which would be the secret app PIN.

Otherwise as can be seen under linuxboot/heads@07f3710, it requires an additional step which would be PIN change, but we don't have a PIN here, since we just reset :)

I still think, as said under #36 (comment) that changing PIN is not really important to Heads use case since we reset. What is important is setting a secret app PIN at moment of oem factory reset/re-ownership.

Need:

  • hotp_verification reset DESIRED_SECRET_APP_PIN

Originally posted by @tlaurion in #42 (comment)

@sosthene-nitrokey ack please.

@tlaurion
Copy link
Contributor

tlaurion commented Nov 30, 2024

Reasoning exerpt:

TLDR...... hotp-verification should

  • Either set a default secret app pin and offer pin change so re-ownership can change it just like before as part of re-ownership for <nk3 (GPG Admin PIN for <nk3 is nk3 secret app PIN for nk3. Regression as of today, best practices not followed, reinventing the wheel with weaker process was chosen as of today)
  • have hotp_verification reset SECRET_APP_PIN requiring a pin if none set by default
  • Have hotp_verification reset set a default PIN, which if we don't plan to reinvent the wheel should be equivalent to gpg Admin PIN which is 12345678.

Originally posted by @tlaurion in linuxboot/heads#1866 (comment)

Its implementation choices. Second option chosen in present PR, either is good, but one needed to be implemented.
Note that physical presence still needed, but once instead of twice (changing PIN would also require physical presence, right? until #41 implemented in firmware version bump).

As soon as a commit provided without regression, I can easily adapt WiP code under linuxboot/heads#1850, specifically part related to present PR change under nk3 related secret app reset needed under oem-factory-reset as self-review comment at linuxboot/heads#1850 (comment)

@@ -38,9 +38,10 @@ void print_help(char *app_name) {
"\t%s info\n"
"\t%s version\n"
"\t%s check <HOTP CODE>\n"
"\t%s regenerate <ADMIN PIN>\n"
"\t%s reset [ADMIN PIN]\n"
Copy link
Contributor

@tlaurion tlaurion Dec 2, 2024

Choose a reason for hiding this comment

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

Can this be SECRET APP PIN or this should be readdressed in separate issues and PRs in another round?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean [SECRET APP PIN] ?

Copy link
Contributor

@tlaurion tlaurion Dec 2, 2024

Choose a reason for hiding this comment

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

You mean [SECRET APP PIN] ?

I restate that UX with many Admin PIN if no User PIN counterpart is misleading.

Yes, Secret App PIN. At least for everything exposed to user and Heads as output.

Admin /User PIN makes sense when one have power over the other which is not the case here and users continuously lost with which PIN is asked here for security dongle, where Heads can only reuse PIN to simplify things.

Secret app pin has counter of 6 for that reason as opposed to user/Admin PIN of gpg if I understood correctly.

Do you understand what I'm talking about for sake of less support requests and UX consistency and less user confusion?

Yes, secret app PIN here, for secure element's secret app PIN!= gpg OpenPGP smartcard's Admin PIN.

#46 (comment) referring to linuxboot/heads#1866 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I repeat

You proposed :

See also issue #36
Change this:


HOTP code verification application, version 1.6
Connected device status:
	Card serial: 0x7BE66C6C
	Firmware: v4.13
	Card counters: Admin 6, User 6
Operation success

To

HOTP code verification application, version 1.6
Connected device status:
	Card serial: 0x7BE66C6C
        Firmware Nitrokey 3: v1.7.1
	Firmware Secrets app: v4.13
	Secret app pin counters : Admin 6, User 6
Operation success

I proposed and restated:

Even more sensical: no secret app even named anywhere because there is none on non nk3(regression), so no version of non existing secret app, no secret app pin, just real information :

HOTP code verification application, version 1.7
Connected device status:
Card serial: 0x7BE66C6C
         Firmware Nitrokey 2: v1.7.1
OpenPGP smartcard PIN counters : Admin: 3, User: 3
Operation success

For nk3:

HOTP code verification application, version 1.7
Connected device status:
Card serial: 0x7BE66C6C
         Firmware Nitrokey 3: v1.7.1
         Firmware Secrets app: v4.13
Secret app PIN counter : 6
OpenPGP smartcard PIN counters : Admin: 3, User: 3
Operation success

Originally posted by @tlaurion in #38 (comment)

@tlaurion
Copy link
Contributor

tlaurion commented Dec 5, 2024

I tested this commit and can say that it works in PoC at linuxboot/heads#1850 (comment)

seal-hotp prompts for either Secure App PIN/GPG Admin PIN as per referred commit.

reset works with SECURE_APP_PIN per re-ownership/oem factory reset.

Comments welcome over there. All tests can be done under qemu.

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 6, 2024
…instead of Nitrokey/nitrokey-hotp-verification#46 for hotp-verification info parsing and validation of oem-factory-reset and seal-hotp

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 6, 2024
…fef5d1c82a014e0e2bf79346 directory: waiting for Nitrokey/nitrokey-hotp-verification#43 and Nitrokey/nitrokey-hotp-verification#46 to be merged to change modules/hotp-verification commit

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
@tlaurion
Copy link
Contributor

tlaurion commented Dec 6, 2024

@sosthene-nitrokey Adapted linuxboot/heads#1850 (comment) and tested this successfully.

Please tell me when #43 merged alongside #46

Copy link
Contributor

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

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

LGTM tested functionally under linuxboot/heads#1850

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 6, 2024
…instead of Nitrokey/nitrokey-hotp-verification#46 for hotp-verification info parsing and validation of oem-factory-reset and seal-hotp

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 6, 2024
…fef5d1c82a014e0e2bf79346 directory: waiting for Nitrokey/nitrokey-hotp-verification#43 and Nitrokey/nitrokey-hotp-verification#46 to be merged to change modules/hotp-verification commit

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 7, 2024
…rification#46 so that this PR can be tested and reviewed from OEM Factory Reset/User Re-Ownership perspective

(PR 43 not in which fixes hotp_verification info, needed to reuse default PINs under seal-hotp if pubkey age <1 month and if Secret app PIN/GPG Admin PIN count >=3 )

Repro:
mkdir patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346
wget https://patch-diff.githubusercontent.com/raw/Nitrokey/nitrokey-hotp-verification/pull/46.patch -O patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/46.patch
sudo rm -rf  build/x86/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/
./docker_repro.sh make BOARD=qemu-coreboot-whiptail-tpm2-hotp USB_TOKEN=Nitrokey3NFC PUBKEY_ASC=pubkey.asc inject_gpg run

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 7, 2024
…erification#46 so that this PR can be tested and reviewed from OEM Factory Reset/User Re-Ownership perspective

(PR 43 not in which fixes hotp_verification info, needed to reuse default PINs under seal-hotp if pubkey age <1 month and if Secret app PIN/GPG Admin PIN count >=3 )

Repro:
mkdir patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346
wget https://patch-diff.githubusercontent.com/raw/Nitrokey/nitrokey-hotp-verification/pull/46.patch -O patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/46.patch
sudo rm -rf  build/x86/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/
./docker_repro.sh make BOARD=qemu-coreboot-whiptail-tpm2-hotp USB_TOKEN=Nitrokey3NFC PUBKEY_ASC=pubkey.asc inject_gpg run

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 9, 2024
@tlaurion
Copy link
Contributor

tlaurion commented Dec 9, 2024

@sosthene-nitrokey tlaurion/heads@94565dc tested with #43 and #46

Some notes though:

  • "touched received" is always obtained even if no touched happened in output. It's kinda hard to help user troubleshoot what happens if the output is lying. I guess you should open an issue after replicating.
  • oem-factory-reset was modified to loop 3 times when error code 3 is given by hotp-verification reset $PIN, even though the error code 1 is outputted on screen (inconsistence). You should probably open another issue after replicating, but code takes for granted error 3 is abensce of touch/physical presence as of now, errors and push user/oem to be attentive to lack of Allow factory reset/pin change without user presence #41 implementation

Otherwise, oem-factory-reset and seal-hotp were modified under Heads and i'm cleaning commits. Secrets App PIN is attempted automatically based on hotp-verification info output under seal-hotp and user is warned to do re-ownership since a default PIN is used for remote attestation, defeating remote attestation purpose.

TODO:

  • merge this PR
  • create additional issues/pr to fix inconsistencies of output (Heads says to contact Nitrokey support if they can't make sense of output incosistence in code if you want to review)
  • version bump hotp-verification
  • communicate with me asap so Heads upstream contains hotp-verification version bump Nitrokey is ok dealing with support requests coming their way if prior issues unfixed.

Thanks @sosthene-nitrokey

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 9, 2024
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 10, 2024
@tlaurion
Copy link
Contributor

tlaurion commented Dec 11, 2024

@sosthene-nitrokey tlaurion/heads@94565dc tested with #43 and #46

Some notes though:

  • "touched received" is always obtained even if no touched happened in output. It's kinda hard to help user troubleshoot what happens if the output is lying. I guess you should open an issue after replicating.
  • oem-factory-reset was modified to loop 3 times when error code 3 is given by hotp-verification reset $PIN, even though the error code 1 is outputted on screen (inconsistence). You should probably open another issue after replicating, but code takes for granted error 3 is abensce of touch/physical presence as of now, errors and push user/oem to be attentive to lack of Allow factory reset/pin change without user presence #41 implementation

Otherwise, oem-factory-reset and seal-hotp were modified under Heads and i'm cleaning commits. Secrets App PIN is attempted automatically based on hotp-verification info output under seal-hotp and user is warned to do re-ownership since a default PIN is used for remote attestation, defeating remote attestation purpose.

TODO:

  • merge this PR
  • create additional issues/pr to fix inconsistencies of output (Heads says to contact Nitrokey support if they can't make sense of output incosistence in code if you want to review)
  • version bump hotp-verification
  • communicate with me asap so Heads upstream contains hotp-verification version bump Nitrokey is ok dealing with support requests coming their way if prior issues unfixed.

Thanks @sosthene-nitrokey

@sosthene-nitrokey this PR was approved. What is preventing to hit the merge button and version bump hotp-verification version, as well as opening other issues from cited points here? A reminder that this PR was the priority for testing and moving forward with merging things under Heads so that Heads feature freeze happens before Christmas.

@sosthene-nitrokey sosthene-nitrokey merged commit a15c1b1 into master Dec 11, 2024
1 check passed
@sosthene-nitrokey
Copy link
Contributor Author

We could just remove the touch received display. It's not trivial to distinguish errors that come from touch from other errors since there is no standard CCID status code for this, so devices use their own status codes, that are sometimes indistinguishable from other errors. @tlaurion

@tlaurion
Copy link
Contributor

tlaurion commented Dec 11, 2024

We could just remove the touch received display. It's not trivial to distinguish errors that come from touch from other errors since there is no standard CCID status code for this, so devices use their own status codes, that are sometimes indistinguishable from other errors. @tlaurion

@sosthene-nitrokey : I will use error 3 returned as said. Problem I explained is that inside of hotp-verification, its showed error within hotp-verification 1, not 3 (which is exit status on which I will rely on). There is inconsistencies.
Anyway, Will redirect support requests to nitrokey if I receive any on that, as written in code you can review and test yourself at linuxboot/heads#1875. I'm running out of time, this is taking way too much time.

Will wait for hotp-verification version bump to revisit this. I was told that the 30h I worked on the testing/fixing Heads was not going to be paid separately of profit sharing agreement since it was not scoped prior of work done.
A reminder that I didn't scope this work either and didn't had time for it, but still made it because Heads users rely on nk3 and librem keys for remote attestation and oem-factory-reset and seal-hotp was broken per nitrokey changes, not mine.

Once known, it was impossible to let Heads set a PIN on first use nor leave end user to be said to use a separate machine to reset their dongle, or permit regressions that were worked here.

A reminder that not so long ago, I also worked countless hours leading to security disclosure blog post at https://www.nitrokey.com/blog/2024/heads-v25-and-nitrokey-3-firmware-v171-security-update (which was the nk3 accepting any PIN and lying to users that is was verifying authentication, only relying on physical presence) permitting to attest any tampered firmware desired from an evil-made and rendering void any security promises Heads provided when using a nk3, which was also unpaid work.

Work trace leading to current 30h of work on my side at linuxboot/heads#1866

CC: @jans23 donations expected.

@tlaurion
Copy link
Contributor

tlaurion commented Dec 12, 2024

@sosthene-nitrokey screenshot says more then words
2024-12-12-150242

  • First touch received: is a lie. I haven't touched the device, which results in output error 1 and exit code 3. Heads catches error code 3 and opens whiptail window telling user 1/3 attempt consumed to bring his attention on physical presence needed (that is error 3 returned, here showed per hotp-verification: 1: unknown error. Interesting, to say the least, no? I see support requests flowing, but as said, will redirect them to nitrokey, no way I deal with this for free because not fixed upstream for feature freeze.Warned. Will act upon.
  • Then nk3 says two touch received, each of one saying the truth but no clue why sometimes it goes to threee captions like this before succeeding. Also weird, implementation ddetails are not my problem, but is condusing to say the least again.

I leave you with opening distinct issues or not, I will use hotp-verification version 1.7 that was just bumped and will let you do your own things until I receive a call from @jans23 for proper compensation on past work done: my job is not to fix Nitrokey stuff but to integrate it for the best sake of its ecosystem, resellers, users and partners (also, my job description doesn't include constantly begging for money compensations either). It should be Nitrokey's job to do proacticely and recognise work done for proper ecosystem collaboration and respect the ecosystem depending on them, more efficiently.


Context:

Unfortunately i'm the one seeing those design/implementation/regression issues, and collaboration was once again long, uneasy, requiring way too much interaction from my side, still unpaid. And the unfortunate outcome of this, as of today, is that nk3 is still requiring physical presence because no synced nk3 firmware version released, so I will need to reiterate on that subsequently, which should not be covered by my own fees to fix this again, because ecosystem doesn't want physical presence nor expected it under Heads which gets in the way of unattended workflow which is needed by all Nitrokey partners distributing their keys to customers. There is a total lack of pro-active collaboration nor scoping of work done in a proper manner and every single past collaboration needed to happen in some kind of emergency which is not my fault but became my problem since partners depend on Heads being robust for downstream releases and feature freeze. I would prefer a different way of working and collaboration, but I have an history with Nitrokey selling x230/t430 based on my free work, and only small and sporadic compensations, where no dasharo subscriptions were sold in the past, which is the only way i'm compensated for heads work and contributing to Nitrokey. Working like this is not sustainable, and rush rush rush is not a way to work if the goal is to create a sustainable ecosystem with more and more non-technical users and going mass market.

So, even if i'm told to not work for free, I actually am currently forced to do so, and I intend to put a stop on this unless i'm paid for work actually done in the past to entice proper and sane collaboration for the future. Otherwise, there will be none, and it needs to be understood that up to now, both for nk3 1.7.1 and hotp-verification 1.6 6 months ago which was around 30h of work and coordination with QubesOS team ofr coordinated disclosure which was pure chaos and damage control, as well as now another 30h, I do not intend to repeat this in the future in the mindset of knowing not paid for it because ecosystem says it should be nitrokey problem to pay and fix, which in my experience is always hard, limited and required a lot of work to get donations, as can be seen publicly (for that reason) at https://opencollective.com/nitrokey/transactions

tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
…instead of Nitrokey/nitrokey-hotp-verification#46 for hotp-verification info parsing and validation of oem-factory-reset and seal-hotp

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
…fef5d1c82a014e0e2bf79346 directory: waiting for Nitrokey/nitrokey-hotp-verification#43 and Nitrokey/nitrokey-hotp-verification#46 to be merged to change modules/hotp-verification commit

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
…erification#46 so that this PR can be tested and reviewed from OEM Factory Reset/User Re-Ownership perspective

(PR 43 not in which fixes hotp_verification info, needed to reuse default PINs under seal-hotp if pubkey age <1 month and if Secret app PIN/GPG Admin PIN count >=3 )

Repro:
mkdir patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346
wget https://patch-diff.githubusercontent.com/raw/Nitrokey/nitrokey-hotp-verification/pull/46.patch -O patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/46.patch
sudo rm -rf  build/x86/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/
./docker_repro.sh make BOARD=qemu-coreboot-whiptail-tpm2-hotp USB_TOKEN=Nitrokey3NFC PUBKEY_ASC=pubkey.asc inject_gpg run

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
…instead of Nitrokey/nitrokey-hotp-verification#46 for hotp-verification info parsing and validation of oem-factory-reset and seal-hotp

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
…fef5d1c82a014e0e2bf79346 directory: waiting for Nitrokey/nitrokey-hotp-verification#43 and Nitrokey/nitrokey-hotp-verification#46 to be merged to change modules/hotp-verification commit

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
…erification#46 so that this PR can be tested and reviewed from OEM Factory Reset/User Re-Ownership perspective

(PR 43 not in which fixes hotp_verification info, needed to reuse default PINs under seal-hotp if pubkey age <1 month and if Secret app PIN/GPG Admin PIN count >=3 )

Repro:
mkdir patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346
wget https://patch-diff.githubusercontent.com/raw/Nitrokey/nitrokey-hotp-verification/pull/46.patch -O patches/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/46.patch
sudo rm -rf  build/x86/hotp-verification-e9050e0c914e7a8ffef5d1c82a014e0e2bf79346/
./docker_repro.sh make BOARD=qemu-coreboot-whiptail-tpm2-hotp USB_TOKEN=Nitrokey3NFC PUBKEY_ASC=pubkey.asc inject_gpg run

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Dec 21, 2024
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.

Add option to do a secrets apps reset for the nk3 similar to: nitropy nk3 secret reset
4 participants