Skip to content
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

Warning and confirmation strategy #65

Open
robinkrahl opened this issue Jan 14, 2019 · 6 comments
Open

Warning and confirmation strategy #65

robinkrahl opened this issue Jan 14, 2019 · 6 comments
Labels

Comments

@robinkrahl
Copy link
Collaborator

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.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jan 15, 2019

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
Copy link
Owner

d-e-s-o commented Jan 15, 2019

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.

@robinkrahl
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jan 16, 2019

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

@robinkrahl
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants