nitrocli icon indicating copy to clipboard operation
nitrocli copied to clipboard

Warning and confirmation strategy

Open robinkrahl opened this issue 6 years ago • 6 comments

nitrokey-app warns the user about insecure configurations. We should consider whether we also want to have these warnings.

  • default PINs: I don’t think we should to check that. Changing the default PINs should be common sense.
  • missing AES key: Might make sense when accessing features that need an AES key.
  • SD card not filled with random data: Would make sense to check that as it’s non-intuitive and as the user can clear that warning using the NK_clear_new_sd_card_warning function if they don’t want to clear the SD card.

These are all I could think of, but there might be more. On a related note, we have commands that may easily cause significant damage – namely the factory reset. If the admin PIN is cached, the user does not have to confirm the reset. Possible strategies:

  • Clear the admin PIN cache before dangerous operations.
  • Explicitly ask for confirmation.
  • No confirmation required.

I’d prefer the first option.

robinkrahl avatar Jan 14 '19 16:01 robinkrahl

nitrokey-app warns the user about insecure configurations. We should consider whether we also want to have these warnings.

Where do you think we should display warnings?

  • default PINs: I don’t think we should to check that. Changing the default PINs should be common sense.

Agreed.

  • missing AES key: Might make sense when accessing features that need an AES key.

Sounds to me more as if the functions using those keys should be adjusted to return proper error codes. Do you know if that is something the Nitrokey team is working to report properly by any chance?

  • SD card not filled with random data: Would make sense to check that as it’s non-intuitive and as the user can clear that warning using the NK_clear_new_sd_card_warning function if they don’t want to clear the SD card.

These are all I could think of, but there might be more. On a related note, we have commands that may easily cause significant damage – namely the factory reset. If the admin PIN is cached, the user does not have to confirm the reset. Possible strategies:

  • Clear the admin PIN cache before dangerous operations.
  • Explicitly ask for confirmation.
  • No confirmation required.

I’d prefer the first option.

Yeah, sounds fine.

d-e-s-o avatar Jan 15 '19 02:01 d-e-s-o

How about instead of a warning we just fail the operation for a sufficiently severe problem (# 3 seems like a candidate)? We can add a --force parameter that overrides the check.

I think that would also play better with the extension related functionality we have been discussing. We could not really embed a warning into the output format, and emitting it to stderr is just bound to make it appear somewhere where nobody cares, I'd say.

d-e-s-o avatar Jan 15 '19 06:01 d-e-s-o

Where do you think we should display warnings?

My first thought was printing to stderr, but I haven’t thought about it that much.

Do you know if that is something the Nitrokey team is working to report properly by any chance?

Not that I’m aware of. I intend to file an issue for that in the context of #45. But as it should be fixed in the firmware, not in libnitrokey, I don’t think there will be a change soon.

How about instead of a warning we just fail the operation for a sufficiently severe problem (# 3 seems like a candidate)? We can add a --force parameter that overrides the check.

Yeah, we can do that. Even for 2), we don’t have to perform the command just to get a nice error message.

robinkrahl avatar Jan 15 '19 12:01 robinkrahl

Another possible warning would be outdated firmware. We could warn a) if we know that there is a newer firmware version, b) if we detect an incompatible firmware version (currently < 0.52 due to the access mode changes), or c) if a firmware with a security update has been published (0.51).

While I’m not to keen on checking the firmware, an average user probably won’t notice firmware updates, so b) might make sense.

robinkrahl avatar Jan 16 '19 02:01 robinkrahl

Yeah, b) may be okay. But I think we should probably error out there as well (only for the command in question). Regardless, this is something that has very low priority, at least on my list :D

d-e-s-o avatar Jan 16 '19 02:01 d-e-s-o

I agree, I’m just currently transforming my hand-written notes into issues and todos before I forget about them. :)

robinkrahl avatar Jan 16 '19 02:01 robinkrahl