search

d-e-s-o / nitrocli

A command line tool for interacting with Nitrokey devices.
30 stars 10 forks source link

Discussion: Validation of user input #45

Open robinkrahl opened 5 years ago

robinkrahl commented 5 years ago

Most of the user input provided via arguments or options is subject to restrictions. These restrictions can be fixed (e. g. a hexadecimal string must always have an even number of hex digits) or variable (e. g. the current Nitrokey Pro has 3 HOTP slots, but the next version might have 5). To which degree should we validate such input in nitrocli (that is also validated by libnitrokey or the firmware)?

Advantages of validation in nitrocli are better error messages and failing early. Disadvantages are that we might be less compatible or might make wrong assumptions.

Currently we perform no validation. My suggestion would be to perform basic validation on fixed restrictions (hex string), but no validation for variable restrictions (slot count) unless libnitrokey gives us the required information (e. g. a TOTP_SLOT_COUNT constant).

Examples for the current error messages:

Invalid hex string:

$ nitrocli otp set 0 test test
Could not write OTP slot: Unknown

Invalid slot:

$ nitrocli otp set 30 test test
Could not write OTP slot: InvalidSlot
d-e-s-o commented 5 years ago

Perhaps it's best to decide that on a case-by-case basis. Off hand I am tempted to say that it would be best to keep the current model. For cases where the error message emitted by nitrokey or libnitrokey is not detailed enough, perhaps it would be most beneficial to adjust the upstream code instead? After all, nitrocli is not the only consumer of at least libnitrokey and non-descriptive error messages have plagued other users (again, case-by-case, that would just be my preference). Plus, the call stack is not deep and the command the user executed corresponds pretty much 1:1 to what may go wrong, so there should be sufficient context.

I am not opposed to basic checks as you suggested, but ultimately even those may mean a maintenance burden in the long term. E.g., if (hypothetically) we were to validate the slot count and a new hardware model would provide more slots.

d-e-s-o commented 5 years ago

Does that sound reasonable? Should we compile a list of commands that accept user input that has restrictions on it and see whether the error codes are detailed enough and, if not, engage with the Nitrokey team?

robinkrahl commented 5 years ago

Yes, we can try to fix that upstream. I’ll start a wiki page for the evaluation.

robinkrahl commented 5 years ago

→ libnitrokey error handling wiki page