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 for moving RSA keys to Yubikey or NFC smart card #1278

Merged
merged 9 commits into from May 15, 2015

Conversation

Projects
None yet
3 participants
@josecastillo
Contributor

josecastillo commented May 12, 2015

This pull request implements an enhancement mentioned in #875: the ability to move a secret key from OpenKeychain to a Yubikey or NFC smart card. As discussed in #1256, on-card key generation is unacceptably slow, but the ability to generate keys in OpenKeychain and move them to a smart card is still desirable.

Subkeys in the Edit Key activity now support a "Move Subkey to Yubikey / Smart Card" action, which will check that the key is smart card compatible, and launch the NFC Operation activity to move the key to the smart card. The command automatically determines the appropriate slot on the card based on the key flags: SIGN or CERTIFY will go in the signing slot; ENCRYPT will go in the decryption slot; and AUTHENTICATE will go in the authentication slot (unless the subkey also has flags for SIGN or CERTIFY, in which case it will go in the signing slot instead).

Two new methods in the Base NFC Activity support the keytocard operation: nfcPutData, which is responsible for setting the fingerprints and key generation timestamps, and nfcPutKey, which is responsible for extracting the raw secret key parameters and sending them to the card in an appropriate format. These commands require verification of the Admin PIN, which I've hard-coded to the default "12345678" for now. This makes it simpler to put keys on a new card, but a future enhancement might involve prompting for the Admin PIN as well. I've also added a command, nfcModifyPIN, to change the PIN on a smart card; I didn't add UI for this, but it will probably be necessary for a user to fully customize their smart card or Yubikey with OpenKeychain.

I also introduce one additional UI tweak in this pull request. In the key creation activity, if the user taps a completely blank smart card (one with no key fingerprints), a dialog is displayed asking the following: "Would you like to generate a smart card compatible key?" If the user says "Yes", a flag is set, and the key generation parameters are modified to generate the following:

  1. an RSA-2048 master key for signing and key certification,
  2. an RSA-2048 subkey for encryption, and
  3. an RSA-2048 subkey for authentication.

I welcome code review and any suggestions for changes or additions. There are a few areas that I have specific questions on, which I'll call out with comments on the pull request.

@josecastillo

This comment has been minimized.

This feels hacky: we don't yet know the divert-to-card serial number, so we're passing an empty byte array for dummyDivert. When the keytocard operation is complete, the cryptoOperation() method is called, and if it finds a non-null dummyDivert, it replaces it with the card serial number, which by then is known.

This comment has been minimized.

Valodim replied May 12, 2015

I'm not a 100% on the data flow yet (it's complex, even if most of the stuff it's based on is mine =P), but doesn't this part fit more naturally after the upload anyways? just put everything into the operation, let it return an inputpendingresult which will perform the nfc operation then loop back to the operation just like in other places, and the operation can then modify the key.

@josecastillo

This comment has been minimized.

This also feels like a hack. We need to communicate the card's serial number back to the process that initiated the keytocard operation; this is using the CryptoInputParcel's cryptoData hashmap, even though it has nothing to do with signing hashes or decrypting keys. Not sure if there needs to be a more general-purpose object for communicating the results of NFC operations, since not all will be cryptographic in nature.

@Valodim

This comment has been minimized.

bouncycastle import in ui code (see below)

@Valodim

This comment has been minimized.

this sort of code should be in the operation, not a fragment

@Valodim

This comment has been minimized.

Member

Valodim commented May 12, 2015

awesome work so far! 👍

@josecastillo josecastillo force-pushed the josecastillo:nfckeytocard branch from 558c0b2 to b09696f May 13, 2015

@josecastillo

This comment has been minimized.

Contributor

josecastillo commented May 13, 2015

Thanks for the suggestions! Input pending results ended up being ideal for this; I was able to move the keytocard process entirely into the PgpKeyOperation, which simplifies the data flow and allows for multiple keys to be moved to the card in a single operation. The edit key UI sets a boolean flag mMoveKeyToCard in each SubkeyChange to indicate that a given subkey should be moved to a card. Once the NFC activity is complete, the operation grabs the serial number from the returned data, sets mDummyDivert, and completes.

I still have the concern that I'm using the RequiredInputParcel and CryptoInputParcel to map subkey ID's to card serial numbers, when they were intended to map input hashes to output hashes, but I wonder whether that's more of an issue of how they're named than how they're used; in the end, this works to send a list of data into the NFC operation and get a map back out.

algorithm != PublicKeyAlgorithmTags.RSA_SIGN &&
algorithm != PublicKeyAlgorithmTags.RSA_GENERAL) {
log.add(OperationResult.LogType.MSG_K2C_ERROR_BAD_ALGO, indent + 1);
return true;

This comment has been minimized.

@Valodim

Valodim May 13, 2015

Member

true?

@@ -736,6 +739,11 @@ public void onAction() {
MSG_EXPORT_LOG_EXPORT_ERROR_FOPEN(LogLevel.ERROR,R.string.msg_export_log_error_fopen),
MSG_EXPORT_LOG_EXPORT_ERROR_WRITING(LogLevel.ERROR,R.string.msg_export_log_error_writing),
MSG_EXPORT_LOG_EXPORT_SUCCESS (LogLevel.OK, R.string.msg_export_log_success),
// NFC keytocard
MSG_K2C_ERROR_BAD_ALGO(LogLevel.ERROR, R.string.edit_key_error_bad_nfc_algo),

This comment has been minimized.

@Valodim

Valodim May 13, 2015

Member

these are still part of the modify operation, should be MF

mSubkeysAdapter.getSecretKeyType(position);
if (secretKeyType == CanonicalizedSecretKey.SecretKeyType.DIVERT_TO_CARD ||
secretKeyType == CanonicalizedSecretKey.SecretKeyType.GNU_DUMMY) {
Toast.makeText(EditKeyFragment.this.getActivity(),

This comment has been minimized.

@Valodim

Valodim May 13, 2015

Member

use Snackbar available from our Notify class instead of Toast

CryptoInputParcel cryptoInput,
SaveKeyringParcel saveParcel) {
SaveKeyringParcel saveParcel,
OperationLog log) {

This comment has been minimized.

@Valodim

Valodim May 13, 2015

Member

log should always be passed together with indent, if it's supposed to be logged as part of another action

@Valodim

This comment has been minimized.

Member

Valodim commented May 13, 2015

I think it's fine, since the data in the RequiredInputParcel is explicitly typed by RequiredInputType. Since that data always looks the same, it's easier to use an enum there rather than subclasses, especially since parcelables require a lot of boilerplate to marshall around. It would probably make sense to change mInputHashes to something like mNfcInputData at this point though.

I added some minor comments here and there but looks great otherwise. I would be very grateful if you could add unit tests to the PgpKeyOperationTest class for the mMoveKeyToCard attribute.

} else {
AlertDialog.Builder builder = new AlertDialog.Builder(this);
builder.setTitle("Blank Smart Card / Yubikey Detected")
.setMessage("Would you like to generate a smart card compatible key?")

This comment has been minimized.

@dschuermann

dschuermann May 13, 2015

Member

These strings should be added to strings.xml
Maybe, it fits better into the wizard-like process to ask this as part of a fragment instead of a dialog, but this is not important for this pull request. I can also do this later for you.

@josecastillo josecastillo force-pushed the josecastillo:nfckeytocard branch from b09696f to 2d3f745 May 13, 2015

@josecastillo

This comment has been minimized.

Contributor

josecastillo commented May 13, 2015

Fixed the issues mentioned above, and added a test case for the keytocard functionality. Note that in the PgpKeyOperationTest class, I changed the third test subkey from an ELG-1024 key to an RSA-2048 key, so that I would have a smart card compatible key for the test. This change does not affect any of the other test cases.

I also changed one of the SubkeyChange constructors to take a moveKeyToCard boolean instead of a dummyDivert byte array, since mDummyDivert only ever gets set during the edit key operation. The constructor was no longer used anywhere except in test cases, so I changed it there. The actual functionality of changing a key to divert-to-card is still tested, in the new keytocard test case.

Thanks for your help and let me know if there's anything else!

@josecastillo

This comment has been minimized.

Contributor

josecastillo commented May 14, 2015

I can also squash at least the first five commits to clean up the history; let me know if you'd like me to do that.

@Valodim

This comment has been minimized.

Member

Valodim commented May 15, 2015

great! there's some open questions on how particular states of yubikey and secret key are handled which we should think about, for example when uploading a secret key to a non-empty yubikey, and how to connect the create-key flow to the upload one.

but the bulk is there and looks stable enough for testing, I think the rest can all be handled in issues. no need to squash, the history looks fine as it is. merged.

Valodim added a commit that referenced this pull request May 15, 2015

Merge pull request #1278 from josecastillo/nfckeytocard
Support for moving RSA keys to Yubikey or NFC smart card

@Valodim Valodim merged commit 5f6421e into open-keychain:development May 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Valodim Valodim referenced this pull request May 15, 2015

Closed

yubikey workflows #1282

@josecastillo josecastillo deleted the josecastillo:nfckeytocard branch May 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment