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

Nitrokey Start ID switching without sudo #305

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Cleanup implementation. Use logger instead of print.
szszszsz committed Dec 17, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 9306b896384374cfd3a52320cb72ad8b83e077b8
83 changes: 42 additions & 41 deletions pynitrokey/cli/start.py
Original file line number Diff line number Diff line change
@@ -14,16 +14,14 @@
from tqdm import tqdm
from usb.core import USBError

from pynitrokey.confconsts import logger
from pynitrokey.helpers import local_critical, local_print
from pynitrokey.start.gnuk_token import OnlyBusyICCError, get_gnuk_device
from pynitrokey.start.threaded_log import ThreadLog
from pynitrokey.start.upgrade_by_passwd import (
DEFAULT_PW3,
DEFAULT_WAIT_FOR_REENUMERATION,
IS_LINUX,
kill_smartcard_services,
logger,
restart_smartcard_services,
show_kdf_details,
start_update,
validate_gnuk,
@@ -95,14 +93,18 @@ class CardRemovedGPGAgentException(RuntimeWarning):
pass


class NoCardPCSCException(RuntimeWarning):
pass


def gpg_agent_set_identity(identity: int):
from pexpect import run
from subprocess import check_output as run
Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer removing that rename because there also is subprocess.run.


cmd = f"gpg-connect-agent 'SCD APDU 00 85 00 0{identity}' /bye"
app = run(cmd)
print(cmd)
print(app)
if b"ERR 100663406 Card removed" in app:
cmd = f"gpg-connect-agent" f"|SCD APDU 00 85 00 0{identity}" f"|/bye".split("|")
Copy link

Choose a reason for hiding this comment

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

You don't need an f-string for all of those strings.
Also, it might be worth providing a string list instead.

app_out = run(cmd)
logger.debug(cmd)
logger.debug(app_out)
if b"ERR 100663406 Card removed" in app_out:
raise CardRemovedGPGAgentException("Card removed")


@@ -112,25 +114,22 @@ def pcsc_set_identity(identity):
from smartcard.CardConnection import CardConnection
from smartcard.Exceptions import NoCardException

def find_smartcard(uuid: typing.Optional[int] = None) -> CardConnection:
def find_smartcard(name: typing.Optional[str] = None) -> CardConnection:
# use this for debug: sudo pcscd -f -a
for reader in System.readers():
if "Nitrokey Start" not in str(reader):
if "Nitrokey Start" not in str(reader) or (
name and name not in str(reader)
):
continue
conn: CardConnection
conn = reader.createConnection()
try:
conn.connect()
except NoCardException:
continue
# use this for debug
# sudo pcscd -f -a

# if not select(conn, AID_ADMIN):
# continue
# data, sw1, sw2 = conn.transmit([0x00, 0x62, 0x00, 0x00, 16])
print(reader)
print(conn)
logger.debug([reader, conn])
return conn
# raise Exception(f"No smartcard with UUID {uuid:X} found")
raise NoCardPCSCException(f"No smartcard with UUID {name} found")

def select(conn: CardConnection, aid: bytes) -> bool:
apdu = [0x00, 0xA4, 0x04, 0x00]
@@ -140,17 +139,15 @@ def select(conn: CardConnection, aid: bytes) -> bool:
return (sw1, sw2) == (0x90, 0x00)

def send_id_change(conn: CardConnection, identity: int) -> None:
# 00 85 00 02
out = [0x00, 0x85, 0x00, identity]
for i in range(5):
for i in range(2):
data, sw1, sw2 = conn.transmit(out)
print((bytes(out).hex(), data, hex(sw1), hex(sw2)))
logger.debug((bytes(out).hex(), data, hex(sw1), hex(sw2)))
res = bytes([sw1, sw2]).hex()
if res == "9000":
print("success")
break
if res == "6a88":
print(f"error: {res}")
logger.debug(f"error: {res}")
continue

conn = find_smartcard()
@@ -159,8 +156,7 @@ def send_id_change(conn: CardConnection, identity: int) -> None:
send_id_change(conn, identity)

except ImportError:
logger.debug("pcsc feature is deactivated, skipping firmware mode test")
pass
logger.debug("pcsc feature is deactivated, skipping this method")


@click.command()
@@ -187,15 +183,16 @@ def set_identity(identity):
# Note: the error in communication coming after changing the identity is caused by the immediate restart of
# the device, without responding to the call. The idea was to avoid operating with a potentially inconsistent state in
# the memory.
def inner():
def inner() -> None:
"""
Call all the methods in the order of the success chance
"""

# this works when gpg has opened connection, stops after changing identity with it
try:
logger.info("Trying changing identity through gpg-agent")
gpg_agent_set_identity(identity)
return True
return
except CardRemovedGPGAgentException:
# this error shows up when the identity was just changed with gpg, and the new state was not reloaded
pass
@@ -206,21 +203,25 @@ def inner():

# this works when gpg has no connection, but pcsc server is working
try:
logger.info("Trying changing identity through pcscd")
pcsc_set_identity(identity)
print(f"PCSC change works")
return True
logger.info(
f"Device returns success. Identity is already set to {identity}."
)
return
except CardConnectionException as e:
print(f"Expected error. PCSC reports {e}")
# this error is expected after sucessfully changing the identity
return True
except EstablishContextException:
# pcscd must not work, try another method
local_print("pcscd must not work, try another method")
logger.debug(f"Expected error. PCSC reports {e}")
return
except (EstablishContextException, NoCardPCSCException) as e:
logger.debug(
f"pcscd must not work, try another method. Reported error: {e}"
)
except ImportError:
local_print("pcsc feature is deactivated, skipping this switching method")

# this works, when neither gnupg nor opensc is claiming the smart card interface
try:
logger.info("Trying changing identity through direct connection")
set_identity_raw(identity)
except:
raise
@@ -233,7 +234,7 @@ def inner():


def set_identity_raw(identity):
for x in range(3):
for x in range(1):
try:
gnuk = get_gnuk_device()
with gnuk.release_on_exit() as gnuk:
@@ -242,7 +243,7 @@ def set_identity_raw(identity):
gnuk.cmd_set_identity(identity)
break
except USBError:
# local_print(f"Reset done - now active identity: {identity}")
logger.debug(f"Reset done - now active identity: {identity}")
break

except OnlyBusyICCError:
@@ -252,8 +253,8 @@ def set_identity_raw(identity):
break
except ValueError as e:
if "No ICC present" in str(e):
local_print(
"Device is occupied by some other service and cannot be connected to. Identity not changed."
local_critical(
"Device is not connected or occupied by some other service and cannot be connected to. Identity not changed."
)
else:
local_critical(e)
9 changes: 9 additions & 0 deletions pynitrokey/confconsts.py
Original file line number Diff line number Diff line change
@@ -36,6 +36,8 @@ class Verbosity(IntEnum):
f"environment variable: '{ENV_DEBUG_VAR}' invalid, "
f"setting default: {VERBOSE.name} = {VERBOSE.value}"
)
print(f"Found {ENV_DEBUG_VAR}='{_env_dbg_lvl}'. Setting VERBOSE={VERBOSE}")


LOG_FN = tempfile.NamedTemporaryFile(prefix="nitropy.log.").name
LOG_FORMAT_STDOUT = "%(asctime)-15s %(levelname)6s %(name)10s %(message)s"
@@ -47,3 +49,10 @@ class Verbosity(IntEnum):
UDEV_URL = (
"https://docs.nitrokey.com/nitrokey3/linux/firmware-update.html#troubleshooting"
)


logger = logging.getLogger(__name__)
stream_handler = logging.StreamHandler()
stream_handler.setLevel(VERBOSE)
stream_handler.setFormatter(logging.Formatter(LOG_FORMAT_STDOUT))
logger.addHandler(stream_handler)
Comment on lines +54 to +58
Copy link
Member Author

Choose a reason for hiding this comment

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

The default verbosity level should probably be higher than INFO to avoid printing to console.