WIP: Migration to symmetrically encoded key blocks for secret keys #1884

alexfjw · 2016-06-02T07:52:13Z

Migration away from s2k to symmetric secret key blocks

TODO:

Modify consolidate to write secret key related data directly to db, instead of importing secret keys*
Refactoring of provider helper (remove remnants from api level 11)
Ask for passphrases when importing
Modify secret key related operations (sign, decrypt, new id, etc)
Modify new key creation
Modify secret key export
Migrate old secret keys to symmetric key blocks

*Current consolidation requires reimporting secret keys. This would not work with symmetric key blocks as they would have to be decrypted, which means we have to ask for passphrases on each consolidate.

Refactoring
Rebasing on Andrea's work to add in the change in key import
Using the key import interface for migration

Targets for refactoring:

PassphraseDialogActivity (is in a somewhat disorderly state, as it has to handle key s2k keys, sym encrypted keyrings, both types of backup codes)
ProviderHelper (has grown quite greatly in size)

Both the single & multi-passphrase workflows have been added,
along with an applock which works with the master passphrase.

There is probably a need to refine the UI for both migration and a fresh install of the application.
However, it is definitely usable in its current state, with support for all pre-existing features before this PR.

adithyaphilip · 2016-06-12T11:06:42Z

...hain/src/main/java/org/sufficientlysecure/keychain/keyimport/ParcelableEncryptedKeyRing.java

+ * Parcelable representation of an encrypted keyring block as raw data,
+ * with related fields not obtainable without decrypting the block
+ */
+public class ParcelableEncryptedKeyRing implements Parcelable{


code style ;)

Woops I should fix that. Sounds pretty confusing now that I read that comment ><

alexfjw · 2016-06-23T09:11:26Z

Regarding failing tests:
2 tests relating to keyring retrieval currently fail as I'm still working on it.

Regarding encoding of keyring blocks:
A 256 bit encryption key is generated using pbkdf2 with HMAC SHA1, and the keyring is encrypted with AES-CBC. I'm not quite used to working with crypto algorithms directly so do pardon my mistakes if any.

Code for routines can be found at OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ByteArrayEncryptor.java

The salt and IV are stored together with the encrypted keyring data through serialization, and the entire serialized object is saved into the db as a blob. We could also store the salt and IV in the database directly in their own columns but I thought that serialization might make the code easier to read.

I do feel that the encryption routines I wrote are kinda flaky, but I'm gonna leave this for later.

adithyaphilip · 2016-08-17T19:34:19Z

In the migration system's current state, if the user has forgotten the password for one of his keys, the app becomes unusable until he clears data. Would it be possible to make the migration optional, or allow it to be skipped for some keys (with the skipped keys being rendered unusable until migrated)?

alexfjw · 2016-08-17T23:59:18Z

I was planning on using an interface similar to what we have for key
import, where the user is able to select which key he wants, if not all of them. Will be
working on that part on a later date, perhaps when Andrea is done with the
import selection UI.

On Thu, Aug 18, 2016 at 3:34 AM, Adithya Abraham Philip <
notifications@github.com> wrote:

In the migration system's current state, if the user has forgotten the
password for one of his keys, the app becomes unusable until he clears
data. Would it be possible to make the migration optional, or allow it to
be skipped for some keys (with the skipped keys being rendered unusable
until migrated)?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1884 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AK016zuNJVVeKAQk1wv_4PSQ1iwsNSNNks5qg2I-gaJpZM4IsSnR
.

adithyaphilip · 2016-08-18T07:52:22Z

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java

-                    PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder().setProvider(
-                            Constants.BOUNCY_CASTLE_PROVIDER_NAME).build(cryptoInput.getPassphrase().getCharArray());
-                    masterPrivateKey = masterSecretKey.extractPrivateKey(keyDecryptor);
+                    masterPrivateKey = masterSecretKey.extractPrivateKey(null);


Could you put in a comment explaining where the unlocking is happening now?

alexfjw · 2016-08-18T13:43:26Z

Just a note, I have not amended the 'no password' indicator which shows up when a user tries to examine his subkeys. Was using it as an indication that stripping s2k was working.

Will be hiding that in a future commit.

dschuermann · 2016-08-18T14:25:23Z

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ByteArrayEncryptor.java

+import java.security.SecureRandom;
+import java.security.spec.KeySpec;
+
+public final class ByteArrayEncryptor {


Is there a reason why you implement your own encryption method for byte arrays? Why not use our PGP methods for this? If there is no specific reason, I would propose switching to PGP encryption.

dschuermann · 2016-08-18T14:41:57Z

And please merge/rebase master. Some changes shown in your PR by @runnerway are now on master and shouldn't be displayed here as changes.

alexfjw · 2016-08-18T15:06:37Z

I think something went wrong when I was rebasing. Will fix that in a bit.

On Thu, Aug 18, 2016 at 10:42 PM, Dominik Schürmann <
notifications@github.com> wrote:

And please merge/rebase master. Some changes shown in your PR by
@runnerway https://github.com/runnerway are now on master and shouldn't
be displayed here as changes.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1884 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AK0161AV2d3R1u_O1EPThqXELXtfFL-Nks5qhG83gaJpZM4IsSnR
.

adithyaphilip · 2016-08-18T22:19:44Z

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderWriter.java

+        try {
+
+            mProviderHelper.log(LogType.MSG_IP_PREPARE);
+            result += 1;


I think this used to be mIndent+=1 before it was copied over. This also highlights an issue with logging in ProviderWriter and ProviderReader in general - I'm not very sure whether accessing the ProviderHelper's operation log this way is ideal (getting it to work would involve changing ProviderHelper.mIndent too).

Any suggestions on how I should go about fixing the logging issue?

adithyaphilip · 2016-08-18T22:55:48Z

Just got a crash while trying to update a private key, which was imported from a file.

08-19 03:55:41.365 2600-2848/org.sufficientlysecure.keychain.debug E/Keychain D: A key could not be imported during multi-threaded import
                                                                                 java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: passphrase is null
                                                                                     at java.util.concurrent.FutureTask.report(FutureTask.java:93)
                                                                                     at java.util.concurrent.FutureTask.get(FutureTask.java:163)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation.multiThreadedKeyImport(ImportOperation.java:506)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation.execute(ImportOperation.java:458)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation.execute(ImportOperation.java:86)
                                                                                     at org.sufficientlysecure.keychain.service.KeychainService$1.run(KeychainService.java:155)
                                                                                     at java.lang.Thread.run(Thread.java:818)
                                                                                  Caused by: java.lang.IllegalArgumentException: passphrase is null
                                                                                     at org.sufficientlysecure.keychain.provider.ProviderReader.getCanonicalizedSecretKeyRingHelper(ProviderReader.java:194)
                                                                                     at org.sufficientlysecure.keychain.provider.ProviderReader.getCanonicalizedSecretKeyRing(ProviderReader.java:184)
                                                                                     at org.sufficientlysecure.keychain.provider.ProviderWriter.savePublicKeyRing(ProviderWriter.java:792)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation.serialKeyRingImport(ImportOperation.java:328)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation.access$000(ImportOperation.java:86)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation$1.call(ImportOperation.java:497)
                                                                                     at org.sufficientlysecure.keychain.operations.ImportOperation$1.call(ImportOperation.java:485)
                                                                                     at java.util.concurrent.FutureTask.run(FutureTask.java:237)
                                                                                     at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:422)
                                                                                     at java.util.concurrent.FutureTask.run(FutureTask.java:237)
                                                                                     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)

alexfjw · 2016-08-18T23:00:47Z

Sure, I'll look into that.

On Fri, Aug 19, 2016 at 6:55 AM, Adithya Abraham Philip <
notifications@github.com> wrote:

Just got a crash while trying to update a private key, which was imported
from a file.

08-19 03:55:41.365 2600-2848/org.sufficientlysecure.keychain.debug E/Keychain D: A key could not be imported during multi-threaded import
java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: passphrase is null
at java.util.concurrent.FutureTask.report(FutureTask.java:93)
at java.util.concurrent.FutureTask.get(FutureTask.java:163)
at org.sufficientlysecure.keychain.operations.ImportOperation.multiThreadedKeyImport(ImportOperation.java:506)
at org.sufficientlysecure.keychain.operations.ImportOperation.execute(ImportOperation.java:458)
at org.sufficientlysecure.keychain.operations.ImportOperation.execute(ImportOperation.java:86)
at org.sufficientlysecure.keychain.service.KeychainService$1.run(KeychainService.java:155)
at java.lang.Thread.run(Thread.java:818)
Caused by: java.lang.IllegalArgumentException: passphrase is null
at org.sufficientlysecure.keychain.provider.ProviderReader.getCanonicalizedSecretKeyRingHelper(ProviderReader.java:194)
at org.sufficientlysecure.keychain.provider.ProviderReader.getCanonicalizedSecretKeyRing(ProviderReader.java:184)
at org.sufficientlysecure.keychain.provider.ProviderWriter.savePublicKeyRing(ProviderWriter.java:792)
at org.sufficientlysecure.keychain.operations.ImportOperation.serialKeyRingImport(ImportOperation.java:328)
at org.sufficientlysecure.keychain.operations.ImportOperation.access$000(ImportOperation.java:86)
at org.sufficientlysecure.keychain.operations.ImportOperation$1.call(ImportOperation.java:497)
at org.sufficientlysecure.keychain.operations.ImportOperation$1.call(ImportOperation.java:485)
at java.util.concurrent.FutureTask.run(FutureTask.java:237)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:422)
at java.util.concurrent.FutureTask.run(FutureTask.java:237)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1884 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AK0164dSggMjU9rWCWD4dL5YabvzIakoks5qhOL2gaJpZM4IsSnR
.

adithyaphilip · 2016-08-18T23:05:47Z

With reference to the aforemention crash - looks like line 792 in ProviderWriter is missing a passphrase == null check (the comment before it described what action to take if there is no passphrase available).

…o db

adithyaphilip · 2016-08-26T13:40:23Z

@alexfjw Just posting the stacktrace to the issue you had mentioned. App crashes on start right after consolidate is completed, without asking for passphrases for any keys.

08-26 18:58:05.376 9492-9492/org.sufficientlysecure.keychain.debug E/AndroidRuntime: FATAL EXCEPTION: main
                                                                                     Process: org.sufficientlysecure.keychain.debug, PID: 9492
                                                                                     java.lang.AssertionError: bad return class (CreateSecretKeyRingCacheResult), this is a programming error!
                                                                                         at org.sufficientlysecure.keychain.ui.base.CryptoOperationHelper.onHandleResult(CryptoOperationHelper.java:358)
                                                                                         at org.sufficientlysecure.keychain.ui.base.CryptoOperationHelper$1.handleMessage(CryptoOperationHelper.java:304)
                                                                                         at android.os.Handler.dispatchMessage(Handler.java:102)
                                                                                         at android.os.Looper.loop(Looper.java:135)
                                                                                         at android.app.ActivityThread.main(ActivityThread.java:5289)
                                                                                         at java.lang.reflect.Method.invoke(Native Method)
                                                                                         at java.lang.reflect.Method.invoke(Method.java:372)
                                                                                         at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
                                                                                         at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)

Does not happen on a fresh install or after clearing data.

adithyaphilip · 2016-08-26T13:47:59Z

Also, opting out of master passphrase in preferences has a few issues:

Secret keys retain master password as their respective individual passwords. Should we should ask the user for new passwords for each of his keys?
[Bug] Opting in again still requires the old master password to be remembered and entered.
Choosing not to use the master passphrase workflow on the first time screen still requires you to enter the master password. This password needs to be remembered and entered when opting in to the master passphrase (as seen in the above point)

Considering these issues, it might be better to not have the preference for now and make the master passphrase compulsory. Any thoughts, @dschuermann @Valodim?

alexfjw · 2016-08-26T14:32:08Z

Thanks Adithya, I"ll look into the bugs in a bit.

adithyaphilip · 2016-08-26T14:36:03Z

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/Preferences.java

@@ -131,6 +125,76 @@ public void setFirstTime(boolean value) {
        editor.commit();
    }

+    public boolean isUsingEncryptedKeyRings() {


Might be nice to add a comment explaining what this means.

alexfjw · 2016-08-26T14:53:02Z

About "[Bug] Opting in again still requires the old master password to be remembered and entered."

We would still require the master passphrase if want to provide the user the option of using the applock no matter what workflow he's on. That was the reason I coded it as such. I suppose the alternative would be to prevent applock usage if he is on the multi-passphrase workflow.

adithyaphilip · 2016-08-26T15:40:56Z

When importing a secret key from file after opting into master passphrase, the associated public key is imported and appears in the public key list, but the secret key is not.
Possibly relevant stacktrace:

08-26 20:58:06.453 30196-31503/org.sufficientlysecure.keychain.debug E/Keychain D: Fatal bug in code, the master passphrase we tried is incorrect
                                                                                   org.sufficientlysecure.keychain.provider.ByteArrayEncryptor$IncorrectPassphraseException
                                                                                       at org.sufficientlysecure.keychain.provider.ByteArrayEncryptor.decryptByteArray(ByteArrayEncryptor.java:105)
                                                                                       at org.sufficientlysecure.keychain.provider.ProviderReader.getMasterSecretKey(ProviderReader.java:164)
                                                                                       at org.sufficientlysecure.keychain.provider.ProviderWriter.saveCanonicalizedSecretKeyRing(ProviderWriter.java:665)
                                                                                       at org.sufficientlysecure.keychain.provider.ProviderWriter.saveSecretKeyRingHelper(ProviderWriter.java:1030)
                                                                                       at org.sufficientlysecure.keychain.provider.ProviderWriter.saveSecretKeyRing(ProviderWriter.java:969)
                                                                                       at org.sufficientlysecure.keychain.operations.ImportOperation.serialKeyRingImport(ImportOperation.java:324)
                                                                                       at org.sufficientlysecure.keychain.operations.ImportOperation.serialKeyRingImport(ImportOperation.java:117)
                                                                                       at org.sufficientlysecure.keychain.operations.ImportOperation.execute(ImportOperation.java:443)
                                                                                       at org.sufficientlysecure.keychain.operations.ImportOperation.execute(ImportOperation.java:86)
                                                                                       at org.sufficientlysecure.keychain.service.KeychainService$1.run(KeychainService.java:161)
                                                                                       at java.lang.Thread.run(Thread.java:818)

Debugging Line 103, ByteArrayEncryptor.java will show the cause to be an Exception creating cipher resulting from an EOFException. Hope that helps.
P.S. If the master passphrase has been opted out of, there is no issue.
UPDATE: Okay, looks like the problem is that the passphrase I entered to import my key is the same one being used in getMasterSecretKey in ProviderReader whereas it expects the master passphrase (verified this in debug mode - works if the master passphrase is used). Should help pinpoint the issue.

adithyaphilip · 2016-09-05T09:13:32Z

@alexfjw Great work fixing the import bug. Works flawlessly now. Just to nitpick, the passphrase dialog asks for the master passphrase while importing keys but does not cache it. Could you fix that?

adithyaphilip · 2016-09-05T09:40:41Z

Upgrading from master also works great. The only potential issue that remains is if a user has a secret key which he has forgotten the password to, he will have to clear data to proceed. Maybe the option to select which keys will be migrated (with the rest deleted), you had mentioned earlier would be a good solution.

alexfjw · 2016-09-05T10:01:16Z

I'll look into the caching issue this weekend.

As for the secret key selection for migration, was thinking of waiting till
Andrea's changes are merged into master.

Could try and do it now with the current import key ui but having to fix it
up when the changes come in doesn't feel very appealing.

On Sep 5, 2016 5:40 PM, "Adithya Abraham Philip" notifications@github.com
wrote:

Upgrading from master also works great. The only potential issue that
remains is if a user has a secret key which he has forgotten the password
to, he will have to clear data to proceed. Maybe the option to select which
keys will be migrated (with the rest deleted), you had mentioned earlier
would be a good solution.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1884 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AK016_dQpu5pNSoZfIxN421iKD-tay7Uks5qm-OcgaJpZM4IsSnR
.

…ge_workflow ops

Valodim · 2018-07-04T21:02:16Z

closing stale PR

Valodim added the in progress label Jun 2, 2016

adithyaphilip modified the milestones: OpenKeychain 3.x, GSoC 2016 Jun 4, 2016

adithyaphilip reviewed Jun 12, 2016
View reviewed changes

alexfjw changed the title ~~WIP: Migration to symmetric key blocks for secret keys~~ WIP: Migration to symmetrically encoded key blocks for secret keys Jun 20, 2016

alexfjw force-pushed the symmetric-key-blocks branch from 5f7ad75 to e938cc0 Compare August 5, 2016 04:15

alexfjw force-pushed the symmetric-key-blocks branch from 47ef4d3 to 959afcd Compare August 16, 2016 09:21

adithyaphilip reviewed Aug 18, 2016
View reviewed changes

dschuermann reviewed Aug 18, 2016
View reviewed changes

alexfjw force-pushed the symmetric-key-blocks branch from 959afcd to 9bf9c92 Compare August 18, 2016 15:20

adithyaphilip reviewed Aug 18, 2016
View reviewed changes

alexfjw added 4 commits August 19, 2016 17:49

Added s2k check for openpgpservice

76ecbbf

Fixed rogue indentation in ChangeUnlockOperation

191967e

Modified consolidate to write secret keys and related info directly t…

623b962

…o db

Removed legacy code from ProviderHelper

fa5ee3b

alexfjw added 3 commits August 23, 2016 17:18

Cleaned up unused imports

bbbd15e

Added license headers

47d5b74

Added comment to explain passphrase workflows

1c9d784

alexfjw force-pushed the symmetric-key-blocks branch from f3dec3e to 1c9d784 Compare August 23, 2016 12:13

alexfjw added 3 commits August 23, 2016 21:18

Added revert operation which triggers when change workflow fails

887de44

Performance fix for applock, should not lag as bad now

6138e51

Fixed applock bug from force-shutdown

cb85395

adithyaphilip reviewed Aug 26, 2016
View reviewed changes

Added comments to preferences

0689bb7

alexfjw added 5 commits August 26, 2016 23:44

cleaned up residual comment for applock

e88c0c0

Fixed import bug for single passphrase workflow

6bec066

Fixed migrate secret keys bug

4b147eb

Fixed bug with collecting master passphrase

a905605

Enabled failsafe for migrate activity showing twice

d48cfd0

Added master passphrase caching for backup, import, create_key & chan…

b34a591

…ge_workflow ops

dschuermann added on hold and removed in progress labels Nov 30, 2016

Valodim closed this Jul 4, 2018

Valodim deleted the symmetric-key-blocks branch July 4, 2018 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Migration to symmetrically encoded key blocks for secret keys #1884

WIP: Migration to symmetrically encoded key blocks for secret keys #1884

alexfjw commented Jun 2, 2016 •

edited

adithyaphilip Jun 12, 2016

alexfjw Jun 12, 2016

alexfjw commented Jun 23, 2016

adithyaphilip commented Aug 17, 2016

alexfjw commented Aug 17, 2016 •

edited

adithyaphilip Aug 18, 2016

alexfjw commented Aug 18, 2016

dschuermann Aug 18, 2016

dschuermann commented Aug 18, 2016

alexfjw commented Aug 18, 2016

adithyaphilip Aug 18, 2016

alexfjw Aug 21, 2016

adithyaphilip commented Aug 18, 2016

alexfjw commented Aug 18, 2016

adithyaphilip commented Aug 18, 2016 •

edited

adithyaphilip commented Aug 26, 2016 •

edited

adithyaphilip commented Aug 26, 2016 •

edited

alexfjw commented Aug 26, 2016

adithyaphilip Aug 26, 2016

alexfjw commented Aug 26, 2016 •

edited

adithyaphilip commented Aug 26, 2016 •

edited

adithyaphilip commented Sep 5, 2016

adithyaphilip commented Sep 5, 2016

alexfjw commented Sep 5, 2016

Valodim commented Jul 4, 2018

WIP: Migration to symmetrically encoded key blocks for secret keys #1884

WIP: Migration to symmetrically encoded key blocks for secret keys #1884

Conversation

alexfjw commented Jun 2, 2016 • edited

adithyaphilip Jun 12, 2016

Choose a reason for hiding this comment

alexfjw Jun 12, 2016

Choose a reason for hiding this comment

alexfjw commented Jun 23, 2016

adithyaphilip commented Aug 17, 2016

alexfjw commented Aug 17, 2016 • edited

adithyaphilip Aug 18, 2016

Choose a reason for hiding this comment

alexfjw commented Aug 18, 2016

dschuermann Aug 18, 2016

Choose a reason for hiding this comment

dschuermann commented Aug 18, 2016

alexfjw commented Aug 18, 2016

adithyaphilip Aug 18, 2016

Choose a reason for hiding this comment

alexfjw Aug 21, 2016

Choose a reason for hiding this comment

adithyaphilip commented Aug 18, 2016

alexfjw commented Aug 18, 2016

adithyaphilip commented Aug 18, 2016 • edited

adithyaphilip commented Aug 26, 2016 • edited

adithyaphilip commented Aug 26, 2016 • edited

alexfjw commented Aug 26, 2016

adithyaphilip Aug 26, 2016

Choose a reason for hiding this comment

alexfjw commented Aug 26, 2016 • edited

adithyaphilip commented Aug 26, 2016 • edited

adithyaphilip commented Sep 5, 2016

adithyaphilip commented Sep 5, 2016

alexfjw commented Sep 5, 2016

Valodim commented Jul 4, 2018

alexfjw commented Jun 2, 2016 •

edited

alexfjw commented Aug 17, 2016 •

edited

adithyaphilip commented Aug 18, 2016 •

edited

adithyaphilip commented Aug 26, 2016 •

edited

adithyaphilip commented Aug 26, 2016 •

edited

alexfjw commented Aug 26, 2016 •

edited

adithyaphilip commented Aug 26, 2016 •

edited