Retrieving password fails when "Login name" field is left empty

[aardbol]

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:

name:     nologinpw
Command error: The given slot is not programmed

Device: Nitrokey Pro 2

[d-e-s-o]

The issue seems to be in nitrokey-rs.

--- 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 get_pws_result which maps the empty string to SlotNotProgrammed:

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.

[robinkrahl]

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.

[aardbol]

The login name field is the only one that can be left empty. Slot name and password are required entries.

[d-e-s-o]

This has to be fixed in nitrokey-rs, but we could work around it in nitrocli by ignoring the SlotNotProgrammed error in pws get.

I'd think that given the low severity of the issue, there is no need to add a workaround in nitrocli. We can probably wait until a bug fix release of nitrokey-rs is cut.

[robinkrahl]

Sorry for the delay! I’ve just had a closer look at this issue.

The login name field is the only one that can be left empty. Slot name and password are required entries.

@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:

This method also returns a SlotNotProgrammed error if the name is empty.

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.

[robinkrahl]

After a night of sleep, I suggest the following changes instead:

• The methods to access the slot data are moved to a new struct, PasswordSlot, that is only available for programmed slots (or if the user explicitly does not want to perform the check).
• In PasswordSafe, we introduce the get_programmed_slots method that queries the programmed slots from the device and returns a vector with a PasswordSlot instance for each programmed slot.
• We also introduce the get_slot method that returns a specific slot after verifying that it is programmed. It should probably have a flag that allows the user to skip that check.
• The old methods get_slot_status, get_slot_name, get_slot_login, get_slot_password are deprecated and eventually removed.

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.)

[d-e-s-o]

After a night of sleep, I suggest the following changes instead:

That sounds like the right step forward to me. Thanks!

[d-e-s-o]

Thanks to Robin's work, this issue should be fixed in the devel branch.