Improve Nitrokey Start upgrade process #255

robin-nitrokey · 2022-07-19T17:17:43Z

This PR improves the Nitrokey Start upgrade process with the goal to avoid bricked devices after failed updates.

Changes

Show confirmation prompt on SIGINT signals, e. g. caused by Ctrl+C (Improve update tool nitrokey-start-firmware#25).
Remove messages recommending device reinsertion, recommend repeating the update instead.

Checklist

tested with Python3.9
run make check or make fix for the formatting check
signed commits
updated documentation (e.g. parameter description, inline doc, docs.nitrokey)
added labels

With this patch, we register a custom signal handler for the start update subcommand that asks the user for confirmation if they trigger a SIGINT signal, for example by pressing Ctrl+C.

This patch improves the error messages for the Nitrokey Start update process so that it no longer recommends device reinsertion. Instead, users should try to repeat the update if it fails. If they remove the device after an unsuccessful update, they risk bricking the device.

With this patch, we replace the /releases API endpoint with /releases/latest. This endpoint only returns proper releases, not pre-releases, so users are no longer offered firmware release candidates.

robin-nitrokey · 2022-07-21T17:59:14Z

I’ve added checks for the udev rules (#232) and changed the release lookup to ignore pre-releases.

szszszsz

This looks good!
Please check the note regarding the udev rules.

szszszsz · 2022-07-22T15:09:13Z

pynitrokey/cli/start.py

+    for d in dirs:
+        if os.path.isdir(d):
+            for name in os.listdir(d):
+                if fnmatch.fnmatch(name, "??-nitrokey.rules"):


AFAIR using anything starting with number over 70- will probably not work. At some point we had a rule starting with 99-, which brought a couple of issues.
I would change this to accept only the official name, which starts with 41-, to avoid potential problems.
Ideally it would be really nice to check for the updates of the udev file and compare hashes to determine, whether it is up to date.

szszszsz · 2022-07-22T15:14:10Z

A couple of ideas more:

It would be nice to check pynitrokey version against Pypi to see if it is the latest one before starting the update process.
One final thing I am considering is to make sure that other smart card services like gnupg and pcscd are not running during the update.

If that would not take significant effort, it would be great to have these two. What do you think @robin-nitrokey ?

robin-nitrokey · 2022-07-22T15:20:35Z

One final thing I am considering is to make sure that other smart card services like gnupg and pcscd are not running during the update.

There’s already kill_smartcard_services that is called if the card is not found.

szszszsz · 2022-07-22T15:29:49Z

Ok, but there might be one problem with it. Once bootloader shows up, it can be still attached by pcscd due to automatic rerun by systemd. Is this rechecked during connection attempt with the bootloader?

robin-nitrokey · 2022-07-22T15:38:30Z

As far as I see, the exception handler that calls kill_smartcard_services wraps the main invocation so it should also catch bootloader issues. Also, kill_smartcard_services executes sudo systemctl stop pcscd pcscd.socket which should also stop automatic restarting via the socket.

szszszsz · 2022-07-23T07:14:04Z

Alright. This is what I wanted to know. Looks good!

szszszsz · 2022-08-08T10:59:02Z

bump

robin-nitrokey · 2022-08-08T11:34:11Z

From my point of view, this is ready to merge as soon it has been tested with an actual device.

szszszsz · 2022-11-08T11:58:08Z

I have merged in the latest master changes. Will merge to master if you do not mind @robin-nitrokey .

szszszsz · 2022-11-21T15:23:59Z

@robin-nitrokey This looks OK for merge. Anything against?
I can fix the conflicts of course.

robin-nitrokey · 2022-11-21T15:36:44Z

I think this should be tested at least once due to consequences of an update failure, otherwise it is good to merge.

daringer · 2023-09-12T11:19:21Z

needs a rebase
tried it, some work, no real issues
nitropy start list works fine
nitropy start update leads to a:

Do you want to continue? [yes/no]: yes
...
Starting bootloader upload procedure
error while running update
	Exception encountered: OnlyBusyICCError('Found only busy ICC: [None]')
...

tried killing gpg-agent, factory-resetting, restart pcscd, run as root - all of them lead to the same output/error

alt3r-3go · 2024-02-18T17:01:40Z

Just tested this one (after rebasing onto current master), works fine for me for list, start list and start update commands (~~I do not observe those "ICC" errors reported by @daringer earlier~~ EDIT: I actually do, after factory-resetting the token, see below). Also [accidentally :)] tested the udev rules check - worked as intended.

I'd suggest merging this one, these seem to be worthwhile updates and there are some other ones that could be added later (e.g., #298, #91, to name but a few). I also plan for proposing one for the issue I'm personally facing (Nitrokey/nitrokey-start-firmware#23).

EDIT: noticed one small item about the udev message, please see a line comment below.

alt3r-3go · 2024-02-18T17:34:18Z

pynitrokey/cli/start.py

 ):
    """update device's firmware"""

+    if not find_udev_rules():


Noticed when testing my (unrelated) changes on top this PR - there is an udev-related advice in local_critical(), which seems to be redundant now, so probably should be removed.

GH review functionality doesn't allow me to attach a comment to an unchanged line, so putting it here. The line I'm talking about is:

f"- Please check if you have udev rules installed: {UDEV_URL}"

alt3r-3go · 2024-02-18T18:34:15Z

Looks like I actually do observe the "ICC" error. I had an already-provisioned Start and with that the start update only gave me the "6581" error described in Nitrokey/nitrokey-start-firmware#23. That's what I reported above. Now, after factory-resetting the token and trying the update again, I see the same ICC error. Apologies for any confusion caused. I'll dig into that and will report back if/when I find the root cause.

alt3r-3go · 2024-02-18T20:20:53Z

That error is also present on the master for me, so doesn't seem to be related to this PR.

At the end of the day, it looks like something is holding on to the token, and after some iteration of killing gpg-agent, pcscd, gnome-keyring-agent (just in case), I'm now seeing a firm and repeatable USBTimeoutError(110, 'Operation timed out'), on both the rebased PR, master and PyPi version of pynitrokey, on two different hosts with two different VMs, but no ICC error.

The list works fine, and so does factory-reset via gpg, i.e., doesn't look like a bricked token.

Maybe this deserves a dedicated issue at this point. I'll continue debugging this later next week as this blocks me from updating my token.

robin-nitrokey added 2 commits July 19, 2022 17:26

start update: Require confirmation for SIGINT

dab337a

With this patch, we register a custom signal handler for the start update subcommand that asks the user for confirmation if they trigger a SIGINT signal, for example by pressing Ctrl+C.

robin-nitrokey added the device/Nitrokey Start Concerns Nitrokey Start label Jul 19, 2022

robin-nitrokey requested a review from szszszsz July 19, 2022 17:17

robin-nitrokey added 2 commits July 21, 2022 19:39

start: Don’t use pre-releases

e4ac7a3

With this patch, we replace the /releases API endpoint with /releases/latest. This endpoint only returns proper releases, not pre-releases, so users are no longer offered firmware release candidates.

start: Check for udev rules before update

a489cb4

szszszsz approved these changes Jul 22, 2022

View reviewed changes

Merge branch 'master' into nk-start-update

88bbbf4

szszszsz self-assigned this Dec 29, 2022

alt3r-3go reviewed Feb 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Nitrokey Start upgrade process #255

Improve Nitrokey Start upgrade process #255

robin-nitrokey commented Jul 19, 2022

robin-nitrokey commented Jul 21, 2022

szszszsz left a comment

szszszsz Jul 22, 2022

szszszsz commented Jul 22, 2022

robin-nitrokey commented Jul 22, 2022

szszszsz commented Jul 22, 2022

robin-nitrokey commented Jul 22, 2022

szszszsz commented Jul 23, 2022

szszszsz commented Aug 8, 2022

robin-nitrokey commented Aug 8, 2022

szszszsz commented Nov 8, 2022 •

edited

Loading

szszszsz commented Nov 21, 2022

robin-nitrokey commented Nov 21, 2022

daringer commented Sep 12, 2023

alt3r-3go commented Feb 18, 2024 •

edited

Loading

alt3r-3go Feb 18, 2024

alt3r-3go commented Feb 18, 2024

alt3r-3go commented Feb 18, 2024 •

edited

Loading

Improve Nitrokey Start upgrade process #255

Are you sure you want to change the base?

Improve Nitrokey Start upgrade process #255

Conversation

robin-nitrokey commented Jul 19, 2022

Changes

Checklist

robin-nitrokey commented Jul 21, 2022

szszszsz left a comment

Choose a reason for hiding this comment

szszszsz Jul 22, 2022

Choose a reason for hiding this comment

szszszsz commented Jul 22, 2022

robin-nitrokey commented Jul 22, 2022

szszszsz commented Jul 22, 2022

robin-nitrokey commented Jul 22, 2022

szszszsz commented Jul 23, 2022

szszszsz commented Aug 8, 2022

robin-nitrokey commented Aug 8, 2022

szszszsz commented Nov 8, 2022 • edited Loading

szszszsz commented Nov 21, 2022

robin-nitrokey commented Nov 21, 2022

daringer commented Sep 12, 2023

alt3r-3go commented Feb 18, 2024 • edited Loading

alt3r-3go Feb 18, 2024

Choose a reason for hiding this comment

alt3r-3go commented Feb 18, 2024

alt3r-3go commented Feb 18, 2024 • edited Loading

szszszsz commented Nov 8, 2022 •

edited

Loading

alt3r-3go commented Feb 18, 2024 •

edited

Loading

alt3r-3go commented Feb 18, 2024 •

edited

Loading