Validate PWS and OTP string length

[robinkrahl]

Currently, we leave the validation of PWS slot names, logins and passwords as well as OTP slot names and secrets to libnitrokey which returns an error if a string is too long. There are two problems with this:

1. Slot names with length > 15 and <= 30 are truncated, see OTP slot names with 15 < length <= 30 are truncated without warning Nitrokey/libnitrokey#196.
2. It is not possible to see which string is too long. This is especially bad for the PWS with three potential offenders (and three different length limits).

While I consider 1. to be a bug in libnitrokey and/or the firmware, 2. is a legitimate issue. I suggest to store the maximum lengths as constants in nitrocli and to validate the user input, providing helpful error messages like:

The name and the login for the PWS slot are too long (maximum length: x bytes for the name, y bytes for the login).

@d-e-s-o What do you think?

[d-e-s-o]

I am not super opposed to it. Are there any guarantees that these lengths stay the same between releases, though? Are they different between different models? I think these were my main worries in the past.

From my perspective there is a trade off between implementation complexity/ease of maintainability and user-experience and the gain in UX seems pretty low here (especially in the face of other gaping unaddressed issues such as this one Nitrokey/nitrokey-storage-firmware#77).

[robinkrahl]

Are there any guarantees that these lengths stay the same between releases, though? Are they different between different models?

No, there are no guarantees. As far as I know, the limits are currently the same over all devices. I do expect the limits to change for the next Nitrokey generation, but that will require some modifications anyway. Nevertheless, I intend to add getters for these limits to libnitrokey so it should be easy to migrate a hard-coded check to a more dynamic one. The check logic will stay the same though, so I think it’s fine to implement it now for the current limits.

From my perspective there is a trade off between implementation complexity/ease of maintainability and user-experience and the gain in UX seems pretty low here (especially in the face of other gaping unaddressed issues such as this one Nitrokey/nitrokey-storage-firmware#77).

I think it’s hard to compare these two issues. You mention a firmware bug that we can’t address in nitrocli, this is about a UI issue that is only in nitrocli’s responsibility. And while it’s not a critical issue, I still think it’s relevant. Consider this example from the wiki:

$ nitrocli -m pro pws set 1 pws-test-new login-test pass0rd
> Could not write PWS slot: The supplied string is too long

That’s really annoying! All three strings seem pretty short, so I have to start guessing how much characters I have to remove from which string.

[d-e-s-o]

Are there any guarantees that these lengths stay the same between releases, though? Are they different between different models?

No, there are no guarantees. As far as I know, the limits are currently the same over all devices. I do expect the limits to change for the next Nitrokey generation, but that will require some modifications anyway. Nevertheless, I intend to add getters for these limits to libnitrokey so it should be easy to migrate a hard-coded check to a more dynamic one. The check logic will stay the same though, so I think it’s fine to implement it now for the current limits.

Thanks! Sounds good. Yeah, I think with proper exposure from libnitrokey my main concerns vanish (and we can hard-code for the time being, as long as there is a credible path to getting there).

[d-e-s-o]

I suppose there is also an implementation variation in which we just "update" one field after the other, amounting to three write_slot calls. This way we could easily infer which string is the offending one. But yeah, probably won't be as nice as knowing the supported length in advance. On the bright side, though, I suppose that approach would make it easier to switch to a raw HID implementation that circumvents libnitrokey altogether.

[robinkrahl]

I’d rather validate the input. Even if we use a raw hidapi backend, it would be nitrokey-rs’s job to keep track of the limits. (And if there are major firmware updates that change the limits, nitrokey-rs would probably need an update anyway.)