-
Notifications
You must be signed in to change notification settings - Fork 10
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
Retrieving password fails when "Login name" field is left empty #133
Comments
The issue seems to be in --- src/util.rs
+++ src/util.rs
@@ -56,7 +56,9 @@ pub fn result_from_string(ptr: *const c_char) -> Result<String, Error> {
run_with_string(ptr, |s| {
// An empty string can both indicate an error or be a valid return value. In this case, we
// have to check the last command status to decide what to return.
+ println!("STRING: '{}'", s);
if s.is_empty() {
+ println!("LAST RESULT: {:?}", get_last_result());
get_last_result().map(|_| s.to_owned())
} else {
Ok(s.to_owned()) We see: $ cargo run --bin=nitrocli -- --usb-path 0001:000c:02 pws get 1 --login
STRING: ''
LAST RESULT: Ok(())
Command error: The given slot is not programmed So at this layer everything is fine. But then we have fn get_pws_result(s: String) -> Result<String, Error> {
if s.is_empty() {
Err(CommandError::SlotNotProgrammed.into())
} else {
Ok(s)
}
} I don't have all the background as to why that is being done. @robinkrahl should know. |
Thanks for the report! I thought that the Nitrokey firmware would
reject empty values, but apparently, that’s wrong. This has to be fixed
in nitrokey-rs, but we could work around it in nitrocli by ignoring the
SlotNotProgrammed error in pws get.
|
The login name field is the only one that can be left empty. Slot name and password are required entries. |
I'd think that given the low severity of the issue, there is no need to add a workaround in |
Sorry for the delay! I’ve just had a closer look at this issue.
@aardbol Are you sure about that? In my tests, I was able to clear all fields. The slot is still listed as programmed even if all fields are empty, and libnitrokey returns empty strings for them. Even if the slot is not programmed, libnitrokey returns empty strings for the name, login and password. Actually, this isn’t even a bug in nitrokey-rs because the API documentation says:
I agree that this is not really intuitive (in fact, I already forgot about it when I implemented the PWS function in nitrocli …). But on the other hand, I’m not really happy with returning an empty string for unprogrammed slots either. Still, this is probably the better option, so I’ll include that change in the next regular release if you don’t have any other suggestions. |
After a night of sleep, I suggest the following changes instead:
This would mean that (unless the user skips the check), empty strings are no longer ambiguous. It would not increase the number of commands required to list all slots. It would increase the number of commands for querying a single slot from 3 to 4 (unless the user skips the check), but I think that’s okay. (In nitrocli, we already perform that check manually anyway.) |
That sounds like the right step forward to me. Thanks! |
Thanks to Robin's work, this issue should be fixed in the |
I have a password for which I do not have a "login" and nitrocli fails to retrieve the values of that slot.
nitrocli pws get 1
results in:Device: Nitrokey Pro 2
The text was updated successfully, but these errors were encountered: