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

Add status output for Nitrokey Pro #29

Closed
wants to merge 3 commits into from

Conversation

robinkrahl
Copy link
Collaborator

No description provided.

The print_status function only prints the Storage-specific status
struct.  Therefore it is renamed to print_storage_status.
This patch extracts the print_status function that prints the status
fields common to all supported Nitrokey devices from the
print_storage_status function.
@d-e-s-o d-e-s-o self-assigned this Dec 26, 2018
Copy link
Owner

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

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

Thanks for enabling this command for the Pro device, Robin! I believe you should also adjust the help message printed for the status command, which still talks about the storage command only.

Print the status of the connected Nitrokey Storage

Also, do you plan on adjusting the man page later on?

println!(
r#"Status:
model: {model}
smart card ID: {id}
smart card ID: 0x{id}
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Can this addition be moved into the previous commit where you factored out this function? Or am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. This change is needed as we replace the value from the StorageStatus struct (with leading 0x) with the value from Device::get_serial_number (no leading 0x) in this commit.

fwv1 = firmware_minor,
urc = user_retries,
arc = admin_retries,
id = serial_number,
Copy link
Owner

Choose a reason for hiding this comment

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

Does it make sense to differentiate between the smart card ID (for the storage device) and the serial number for the Pro? The string smart card ID seems to be misleading now, and I am trying to understand whether to move this part into the device specific print function or keep it here and change the string to serial number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’d prefer to have this as a common field. It is the same ID that we will show in the pinentry dialog (see #24) and that is reported by gpg --card-status in the same way for both models. I’ll rename it to serial number.

That said, for my Nitrokey Storage, the serial number returned by NK_get_status_storage and NK_device_serial_number do not match the number reported by gpg --card-status. I’ll have to investigate that. Could you please check the result of NK_device_serial_number for your Storage stick?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I did not get to that today. I will get back to you tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I opened Nitrokey/nitrokey-storage-firmware#76 for this issue.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for filing this issue. For me the serial number reported by NK_device_serial_number (as per your example) is always zero. Haven't investigated.

Btw., this has bugged me for ages: does your nitrokey "hang" frequently? E.g., when I am using gpg and then nitrocli the red light just stays on and the key won't do anything or take ages to respond (sometimes it just responds with garbage after a long time). I have to disconnect and reconnect (physically) to get it into a working state again. I am still on firmware version 0.47 (but plan to upgrade soon). I think later today I can check whether my testing stick has the same problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I’ve never experienced such behavior. But I’ve never used GnuPG on my development stick, and I only occasionally run nitrocli with my production stick. And I do not use the Storage volumes at all. So there might be interaction problems that I did not trigger with my usage pattern. (That reminds me that we have to implement a --debug option that enables the debugging output from libnitrokey.)

@robinkrahl
Copy link
Collaborator Author

I believe you should also adjust the help message printed for the status command, which still talks about the storage command only.

Thanks! I missed that.

Currently, the status command fails for a Nitrokey Pro.  This patch
changes the command to also print basic status information for Pro
devices.  For the sake of consistency, the common status is always
queried using the common `Device` functions, even if the Storage status
includes the same information.
@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 27, 2018

Merged. Thanks Robin!

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

Successfully merging this pull request may close these issues.

None yet

2 participants