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

Support factory reset #69

Closed
robinkrahl opened this issue Jan 16, 2019 · 7 comments
Closed

Support factory reset #69

robinkrahl opened this issue Jan 16, 2019 · 7 comments

Comments

@robinkrahl
Copy link
Collaborator

I’m unsure how to name the factory reset command: Based on the existing commands, reset is a fitting choice. On the other hand, it might be beneficial to have a name that is more explicit, like factory-reset. Given that we also have to name the command to build the AES key, we probably have to start using more complex command names anyway. What do you think?

@d-e-s-o
Copy link
Owner

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

Hm, that's a tough one. I prefer reset. I don't see much ambiguity that would really require the additional "factory" part. And so I'd say let's keep it short.

Regarding the build AES key issue: could we just auto-trigger that silently? I.e., no error, no warning, just do it. Is there anything that we could be destroying by doing so? Intuitively I'd say no, as everything that requires those keys would have failed to begin with, but I may very well be missing something.

I guess the only use case for an explicit rebuild command would be to render the data unretrievable. And for that we could go with an explicit destroy (or whatever).

Not sure, just a thought.

@robinkrahl
Copy link
Collaborator Author

Actually, I thought about that too, but decided not to bring up the issue as I’m not quite sure about the consequences.

First of all, it would significantly increase the time needed for the command. Secondly, people who don’t use the PWS or the encrypted storage don’t need the key. But I think that’s both acceptable.

But what came to my mind is whether it is a problem to generate the AES key while the default PINs are set. As far as I know, the AES key is stored on the smart card, is not derived from the PINs and cannot be retrieved even if the PINs are known, so it should not be a problem. But I’m not really sure about that.

@szszszsz May I ask for your opinion on that? Does it make sense to always build an AES key after performing a factory reset?

@szszszsz
Copy link
Contributor

Hi!
We actually do AES generation in the Nitrokey App, as part of the factory-reset. Otherwise unlocking the PWS or EV will fail, and device would appear to confused user to be "broken". I think in general the outcome of the reset is expected to have full working device after the process is completed. It should be easier to handle as well, than scattered checks and calls for AES generation over the PWS and EV unlocks.
As for the time of the operation - this is done so rarely, that a period shorter than 30 seconds for the whole process could be safely left out of consideration IMO.
In case only PWS or EV is to be erased, it is actually faster to call just the AES generation. Then the OTP slots should stay intact.

What the AES generation does (AFAIR), is it gets the random data from the smartcard, and sets both smartcard AES key and device's secret, hence the long execution time.
Factory reset clears the flash page of the configuration and OTP data (also AFAIR).

Final AES key, used for EV/PWS, is created in the following way:

  • Device's key (stored in flash) --> PSO:Decipher (using smart card's internal AES key) --> final AES key

PSO:Decipher is an OpenPGP v2.1 command, which does AES decryption using the internally stored value.

cc @jans23

@robinkrahl
Copy link
Collaborator Author

Thanks for the explanation, @szszszsz! So combining factory reset and building the AES key at the same time seems to be the best solution. We just have to work around this bug.

@d-e-s-o
Copy link
Owner

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

Thanks everybody for getting us to a proper solution! :) The corresponding pull request has been merged. I take it there is nothing else to discuss on this front. Closing the issue. If I missed something feel free to reopen.

@d-e-s-o d-e-s-o closed this as completed Jan 26, 2019
@robinkrahl
Copy link
Collaborator Author

I think we may have neglected the experience for new users when deciding to not support building AES keys without a factory reset.

For many users – at least that was the case for me –, the first priority when buying a Nitrokey Pro is the OpenPGP smart card. So for me, the user experience with nitrocli would have been:

  1. setup an OpenGPG key with gnupg
  2. setup TOTP with nitrocli
  3. setup PWS with nitrocli – doesn’t work because of the missing AES key. Now I either have to nitrocli reset and do steps 1 + 2 again, or I have to use nitrokey-app to build the AES key.

That’s not really satisfying. So my suggestion is to add a new option to the reset command, maybe --only-aes-key, that does not perform the factory reset and only generates a new AES key.

@d-e-s-o d-e-s-o reopened this Oct 8, 2020
@d-e-s-o
Copy link
Owner

d-e-s-o commented Oct 8, 2020

That’s not really satisfying. So my suggestion is to add a new option to the reset command, maybe --only-aes-key, that does not perform the factory reset and only generates a new AES key.

I am fine with such an option.

robinkrahl added a commit to robinkrahl/nitrocli that referenced this issue Apr 14, 2021
This patch adds an --only-aes-key option to the reset command to only
build a new AES key without performing a full factory reset.

Fixes d-e-s-o#69.
d-e-s-o pushed a commit that referenced this issue Apr 17, 2021
This patch adds an --only-aes-key option to the reset command to only
build a new AES key without performing a full factory reset.

Fixes #69
d-e-s-o pushed a commit that referenced this issue May 9, 2021
This patch adds an --only-aes-key option to the reset command to only
build a new AES key without performing a full factory reset.

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

Successfully merging a pull request may close this issue.

3 participants