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

Improve Nitrokey Start upgrade process #255

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions pynitrokey/cli/start.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
# http://opensource.org/licenses/MIT>, at your option. This file may not be
# copied, modified, or distributed except according to those terms.


import fnmatch
import os
import os.path
from subprocess import check_output
from sys import stderr, stdout
from time import sleep
Expand All @@ -16,7 +18,7 @@
from tqdm import tqdm
from usb.core import USBError

from pynitrokey.helpers import local_critical, local_print
from pynitrokey.helpers import confirm_keyboard_interrupt, local_critical, local_print
from pynitrokey.start.gnuk_token import get_gnuk_device
from pynitrokey.start.threaded_log import ThreadLog
from pynitrokey.start.upgrade_by_passwd import (
Expand Down Expand Up @@ -164,6 +166,12 @@ def set_identity(identity):
default=False,
help="Use firmware for early 'Nitrokey Start' key hardware revisions",
)
@click.option(
"--force",
is_flag=True,
default=False,
help="Execute the firmware update even if environment sanity checks fail",
)
def update(
regnual,
gnuk,
Expand All @@ -175,9 +183,24 @@ def update(
yes,
skip_bootloader,
green_led,
force,
):
"""update device's firmware"""

if not find_udev_rules():

Choose a reason for hiding this comment

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

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}"

if force:
local_print(
"Warning: Could not find Nitrokey udev rules but will continue anyway as --force is set."
)
else:
local_critical(
"Failed to find Nitrokey udev rules. These udev rules are required for the update.",
"Please see the nitropy documentation for information on installing these rules:",
" https://docs.nitrokey.com/software/nitropy/linux/udev.html",
"If you want to continue anyway, you can use the --force option.",
support_hint=False,
)

args = (
regnual,
gnuk,
Expand All @@ -198,11 +221,28 @@ def update(
"use one from: https://github.com/Nitrokey/nitrokey-start-firmware)",
)

if IS_LINUX:
with ThreadLog(logger.getChild("dmesg"), "dmesg -w"):
with confirm_keyboard_interrupt("Cancelling the update may brick your device."):
if IS_LINUX:
with ThreadLog(logger.getChild("dmesg"), "dmesg -w"):
start_update(*args)
else:
start_update(*args)
else:
start_update(*args)


def find_udev_rules() -> bool:
dirs = [
"/usr/lib/udev/rules.d",
"/usr/local/lib/udev/rules.d",
"/run/udev/rules.d",
"/etc/udev/rules.d",
]
for d in dirs:
if os.path.isdir(d):
for name in os.listdir(d):
if fnmatch.fnmatch(name, "??-nitrokey.rules"):
Copy link
Member

Choose a reason for hiding this comment

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

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.

logger.info(f"Found matching udev file at {os.path.join(d, name)}")
return True
return False


@click.command()
Expand Down
31 changes: 30 additions & 1 deletion pynitrokey/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@
import logging
import os
import platform
import signal
import sys
import time
from contextlib import contextmanager
from getpass import getpass
from numbers import Number
from threading import Event, Timer
from typing import List, Optional
from typing import Any, Iterator, List, Optional

import click
from tqdm import tqdm
Expand Down Expand Up @@ -351,6 +353,33 @@ def ask(self):
prompt = functools.partial(click.prompt, err=True)


@contextmanager
def confirm_keyboard_interrupt(msg: Optional[str] = None) -> Iterator[None]:
"""
Registers a signal handler for SIGINT (i. e. Ctrl+C) that asks the user to confirm before
raising a KeyboardInterrupt.

If used as a context manager, it resets the signal handler after the execution. The given
message is appended to the confirmation prompt.
"""

def handle_sigint(signum: int, frame: Any) -> None:
text = "Do you really want to stop nitropy?"
if msg:
text += " "
text += msg
if confirm(text):
raise KeyboardInterrupt

handler = signal.signal(signal.SIGINT, handle_sigint)
try:
yield
except Exception:
raise
finally:
signal.signal(signal.SIGINT, handler)


def require_windows_admin() -> None:
if os.name == "nt":
if ctypes.windll.shell32.IsUserAnAdmin() == 0: # type: ignore
Expand Down
17 changes: 11 additions & 6 deletions pynitrokey/start/upgrade_by_passwd.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,15 +279,15 @@ def get_latest_release_data():
try:
# @todo: move to confconsts.py
r = requests.get(
"https://api.github.com/repos/Nitrokey/nitrokey-start-firmware/releases"
"https://api.github.com/repos/Nitrokey/nitrokey-start-firmware/releases/latest"
)
json = r.json()
if r.status_code == 403:
local_critical(
f"JSON raw data: {json}",
f"No Github API access, status code: {r.status_code}",
)
latest_tag = json[0]
latest_tag = json

except Exception as e:
local_critical("Failed getting release data", e)
Expand Down Expand Up @@ -590,6 +590,7 @@ def start_update(
"- all data will be removed from the device!",
"- do not interrupt update process - the device may not run properly!",
"- the process should not take more than 1 minute",
"- if the update fails, do not remove the device! Repeat the update instead.",
)
if yes:
local_print("Accepted automatically")
Expand Down Expand Up @@ -651,9 +652,9 @@ def start_update(
"",
"Could not proceed with the update",
"Please execute one or all of the following and try again:",
"- re-insert device to the USB slot",
"- run factory-reset on the device",
"- close other applications, which could use it (e.g., scdaemon, pcscd)",
"- repeat the update",
)

dev_strings_upgraded = None
Expand All @@ -662,7 +663,10 @@ def start_update(
for i in range(TIME_DETECT_DEVICE_AFTER_UPDATE_S):
if i > TIME_DETECT_DEVICE_AFTER_UPDATE_LONG_S:
if not takes_long_time:
local_print("", "Please reinsert device to the USB slot")
local_print(
"",
"If you have removed the device, please reinsert it to the USB slot",
)
takes_long_time = True
time.sleep(1)
dev_strings_upgraded = get_devices()
Expand All @@ -673,11 +677,12 @@ def start_update(
local_print(".", end="", flush=True)

if not dev_strings_upgraded:
local_print(
local_critical(
"",
"could not connect to the device - might be due to a failed update",
"please re-insert the device, check version using:",
"please check the device version with:",
"$ nitropy start list",
"and repeat the update if necessary",
)

local_print(
Expand Down