-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: master
Are you sure you want to change the base?
Conversation
Ignore smartcard.pcsc types Ignore this particular mypy error
logger = logging.getLogger(__name__) | ||
stream_handler = logging.StreamHandler() | ||
stream_handler.setLevel(VERBOSE) | ||
stream_handler.setFormatter(logging.Formatter(LOG_FORMAT_STDOUT)) | ||
logger.addHandler(stream_handler) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
def gpg_agent_set_identity(identity: int): | ||
from pexpect import run | ||
|
||
cmd = f"gpg-connect-agent 'SCD APDU 00 85 00 0{identity}' /bye" |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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
.
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("|") |
There was a problem hiding this comment.
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.
To do: review, re-test and release as-is, with a further improvement plan if necessary |
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:
Changes
Checklist
make check
ormake fix
for the formatting checkFurther work
To be done in the future / next PRs:
Test Environment and Execution
Setup snippet
Relevant Output Example
Example output with logs:
Output for not connected device:
Fixes #295