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

Conversation

szszszsz
Copy link
Member

@szszszsz szszszsz commented Dec 17, 2022

This PR supports switching multiple identities for Nitrokey Start without changing the underlying services state, but using whatever is connected to the device at the time. 3 attempts are made, each using different method:

  1. Calling gpg-connect-agent
  2. Sending data through pcscd (requires pyscard; optional)
  3. Direct connection

Changes

  • Add pyscard support
  • Add gpg-agent support
  • Extract direct connection
  • Rewrite CLI call to use each method

Checklist

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

Further work

To be done in the future / next PRs:

  • Select device to connect to by serial number
  • Test more the behavior without pyscard installed

Test Environment and Execution

  • OS: Linux Fedora 35
  • device's model: Nitrokey Start
  • device's firmware version: RTM.13

Setup snippet

$ cd /tmp
$ python -m venv env
$ env/bin/pip install pynitrokey[pcsc]
$ env/bin/pip install git+https://github.com/Nitrokey/pynitrokey.git@295-switching-id-without-sudo
$ env/bin/nitropy start set-identity 1

Relevant Output Example

Example output with logs:

(venv) sz@stumpy ~/w/nitropy (295-switching-id-without-sudo)> env PYNK_DEBUG=" " ./venv/bin/nitropy start set-identity 2
Found PYNK_DEBUG=' '. Setting VERBOSE=10
Command line tool to interact with Nitrokey devices 0.4.31
Setting identity to 2
2022-12-17 17:31:25,140   INFO pynitrokey.confconsts Trying changing identity through gpg-agent
2022-12-17 17:31:25,144  DEBUG pynitrokey.confconsts ['gpg-connect-agent', 'SCD APDU 00 85 00 02', '/bye']
2022-12-17 17:31:25,144  DEBUG pynitrokey.confconsts b'ERR 65547 Bad passphrase <Unspecified source>\n'
2022-12-17 17:31:25,144   INFO pynitrokey.confconsts Trying changing identity through gpg-agent
2022-12-17 17:31:25,146  DEBUG pynitrokey.confconsts ['gpg-connect-agent', 'SCD APDU 00 85 00 02', '/bye']
2022-12-17 17:31:25,146  DEBUG pynitrokey.confconsts b'ERR 100663406 Card removed <SCD>\n'
2022-12-17 17:31:25,149   INFO pynitrokey.confconsts Trying changing identity through pcscd
2022-12-17 17:31:25,182  DEBUG pynitrokey.confconsts ['Nitrokey Nitrokey Start (FSIJ-1.2.19-87053532) 00 00', <smartcard.CardConnectionDecorator.CardConnectionDecorator object at 0x7fa9aaca9570>]
2022-12-17 17:31:25,182  DEBUG pynitrokey.confconsts Expected error. PCSC reports Failed to transmit with protocol T1. Transaction failed.
Reset done - now active identity: 2
(venv) sz@stumpy ~/w/nitropy (295-switching-id-without-sudo)>

Output for not connected device:


(venv) sz@stumpy ~/w/nitropy (295-switching-id-without-sudo) [1]> ./venv/bin/nitropy start set-identity 2
Command line tool to interact with Nitrokey devices 0.4.31
Setting identity to 2
Critical error:
Device is not connected or occupied by some other service and cannot be connected to. Identity not changed.

--------------------------------------------------------------------------------
Critical error occurred, exiting now
Unexpected? Is this a bug? Would you like to get support/help?
- You can report issues at: https://support.nitrokey.com/
- Writing an e-mail to [email protected] is also possible
- Please attach the log: '/tmp/nitropy.log.aneroxwq' with any support/help request!
- Please check if you have udev rules installed: https://docs.nitrokey.com/nitrokey3/linux/firmware-update.html#troubleshooting

Fixes #295

Comment on lines +54 to +58
logger = logging.getLogger(__name__)
stream_handler = logging.StreamHandler()
stream_handler.setLevel(VERBOSE)
stream_handler.setFormatter(logging.Formatter(LOG_FORMAT_STDOUT))
logger.addHandler(stream_handler)
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.

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

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

Looks good, just some thoughts regarding robustness:

  • We could handle the case where the gpg-connect-agent binary is not available and just ignore that execution path.
  • Similarly, System.readers would fail if pcsc is not running. We should handle that exception and ignore the pcsc path.

pynitrokey/cli/start.py Outdated Show resolved Hide resolved
def gpg_agent_set_identity(identity: int):
from pexpect import run

cmd = f"gpg-connect-agent 'SCD APDU 00 85 00 0{identity}' /bye"
Copy link
Member

Choose a reason for hiding this comment

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

It could make sense to run SERIALNO before this one:

This command should be used to check for the presence of a card. It is special in that it can be used to reset the card. Most other commands will return an error when a card change has been detected and the use of this function is therefore required.

pynitrokey/cli/start.py Outdated Show resolved Hide resolved
@robin-nitrokey
Copy link
Member

Ah, sorry, I commented on an outdated version. Will review again.



def gpg_agent_set_identity(identity: int):
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.

@szszszsz szszszsz marked this pull request as draft December 29, 2022 09:52
@szszszsz szszszsz self-assigned this Dec 29, 2022
def gpg_agent_set_identity(identity: int):
from subprocess import check_output as run

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.

@szszszsz
Copy link
Member Author

To do: review, re-test and release as-is, with a further improvement plan if necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
device/Nitrokey Start Concerns Nitrokey Start
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.4.31: Switching of identies often fails
3 participants