encryption of notebooks in sync repos

[heinzelotto]

Here is an initial implementation of encrypted notebook sync! Only encrypted files are sent to the remote repository so that the unencrypted plaintext of the notebook never leaves the phone. See the discussion in #43.

Current state of the feature:

• Notebooks are encrypted in the PGP format using the AES-256 symmetric encryption algorithm.
• Encryption and decryption is done using the bouncycastle OpenPGP API. API usage Code is taken from OpenKeychain (also GPLv3) since at the moment they don't offer an API for symmetric encryption (there is an open bug openpgp-api actions for symmetric encryption and decryption open-keychain/open-keychain#2536).
• Encryption is enabled and password is configured individually for each sync repo.
• Currently there is a configuration UI for Dropbox repos.
• Encrypted repo files are identified by the file extension .org.gpg.

There are some issues with tricky sync situations where I would be grateful for guidance. Confusing behavior happens during loading of notebooks if the repo contains encrypted .org.pgp files and encryption is disabled the app; if the repo contains unencrypted .org files and encryption is enabled; if both x.org and x.org.gpg are present. Currently it is possible to toggle encryption or change the password for an existing repo, allowing for these cases to happen. It also happens when adding a new repo pointing to an existing Dropbox path with these such present. Clearing of the repo and possibly re-encryption should never automatically be performed IMO (also not at the next sync).

From what I gather, in the save directly after toggling encryption, if at least one sync has taken place before, in saveBookToRepo() we get the wrong filename (for instance the old x.org instead of x.org.gpg when encryption was just toggled on), so I made sure the target filename suffix is corrected based on the encryption setting. In loadBookFromRepo() I think the filenames are originally from the SyncRepo.getBooks(), so it would maybe make sense to reject loading a file x.org.gpg (exception?) if encryption is disabled, and vice versa x.org if encryption is enabled. I will also have to do further investigation for the case where both x.org and x.org.gpg exist. @nevenz do you have any ideas how and where to best handle these cases?

Other issues (I am new to java/android):

• Should I intersperse more BufferedInput/OutputStream in the encryption routines?
• UI: In the dropbox repo layout, I suspect the android:imeOptions="actionNext" do not have the desired effect. When inputting into the directory textedit and then hitting the next (enter) button on the keyboard, the settings window closes, even though I think it should skip to the encryption textedit.
• which strings should be string resources?
• needs tests

[zekooooo]

Thanks for working on this, I was just about to open an issue asking for OpenKeychain encryption support as I don't really want to back up my notes so that my other apps can read them :) I hope your work gets accepted.

[heinzelotto]

In order to more robustly handle switching between encrypted/unencrypted state of a repo, I changed DropboxClient::getBooks() to only return ".org" or ".org.gpg" notebooks, respectively. Now after you switch encryption on, having both "book.org" and "book.org.gpg" in the repo will not cause problems.

Currently in save-/loadBookFromRepo() it is still necessary to force the bookname file extension to either ".org" or ".org.gpg" because the fileName might still have the wrong, old file extension. As a result of the function, this value will be updated and in future calls it will be correct.

It might be better to globally update all relevant filenames once, right after toggling of encryption, rather than locally as a side effect of the first sync after toggling. However this would require more refactoring work. I would need more input on whether the general approach is the right one before attempting to go forward with such a refactoring. I would be glad if you could give me some pointers :)

[nevenz]

Sorry for the delay.

I think we need to be able to set the password per notebook.

Default password can be supported, but it looks like it's much more work for that to be per repository and I'm not sure that that's even useful. Perhaps a single property in Settings would be enough.

I would suggest a new table like books_encryption with fields like book_id, is_enabled, password.

Users should be able to set the password and flag per notebook. If we detect that remote notebook is encrypted (by extension), is_enabled would be set automatically. The default password would be tried for decryption.

If both *.org and *.org.gpg are present as remote books, perhaps both should be synced and treated separately. I don't think app needs to be clever there.

But if you change is_enabled flag for the existing notebook from the app, user should be able to choose if he wants the other version of the notebook deleted in the repository (similar to the checkbox in the dialog for deleting the notebook).

Should I intersperse more BufferedInput/OutputStream in the encryption routines?

If we can avoid creating a temporary file it's probably a good idea.

UI: In the dropbox repo layout, I suspect the android:imeOptions="actionNext" do not have the desired effect.

The listener is set in the code. It could be removed if needed, as repos are not created often enough for this to be that useful. Although as I mentioned, I don't think it's worth adding this to each repo's UI.

which strings should be string resources?

Any that need to be translated.

Let me know if I haven't answered everything. And thanks for working on this!

[spolakh]

First, thanks so much @heinzelotto for taking this up! It always required hacky and painful workflow setups to sync orgzly with encrypted org stores!

@nevenz - May I suggest that it might be a good first iteration to support only the per-repo encryption (as it is already implemented in this PR) and adding a per-file option later on? It should cover a sizeable chunk of people's requests from #43 and #54 and it seems like adding more complexity to this PR might make it too heavy.

[heinzelotto]

Hi @spolakh, sorry for no updates in a long time. I became a little frustrated and stuck and then let it rest. However I gained some experience with kotlin recently and will take it back up within the next month where I will have some time.

I concur with @nevenz that per file is probably the way to go.

[harningt]

I think that if we switched to PGP keys vs a symmetric passphrase, we might be able to get some more security out of this and be handled more transparently by Emacs.

Workflow experienced when handling using a key was that when opening an org file that was encrypted, it asked for the password to the PGP key (since I hadn't used it in a while). Closed it out, opened it again and it didn't ask because the system saved it.

On Android - a similar workflow exists for OpenKeychain.

It does require you setup keys usable by all machines, but for the most part OpenKeychain handles that, if I remember right. You might just have to ask OpenKeychain what keys to encrypt to and store that in a configuration file.

[nevenz]

May I suggest that it might be a good first iteration to support only the per-repo encryption (as it is already implemented in this PR) and adding a per-file option later on?

I'm fine with that.

Trying this now, with:

Repository	Device
Getting Started with Orgzly.org	Getting Started with Orgzly
secret-test.org.gpg

After syncing, I get:

Repository	Device
$fileName.gpg
Getting Started with Orgzly.org	Getting Started with Orgzly
secret-test.org.gpg	secret-test

Filename issue aside, I don't think we should be uploading gpg version of the file if the non-encrypted version already exists in the repository?

There are quite a lot different scenarios and things to check, as @heinzelotto wrote in the first comment. So I agree we should add tests to cover them all and also serve to clear things up.

[nevenz]

BTW, due to changes in org-java (which I should be releasing more often), a specific version that works with this PR:

diff --git a/build.gradle b/build.gradle
index d7105ce7..13033f1e 100644
--- a/build.gradle
+++ b/build.gradle
@@ -39,7 +39,7 @@ buildscript {
 
     versions.android_test_uiautomator = '2.2.0'
 
-    versions.org_java = '1.3-SNAPSHOT'
+    versions.org_java = '1.3-20200414.084711-8'
 
     versions.loremipsum = '1.0'
 

[bhankas]

Is there any update on this PR? It is a useful feature, and if original author is occupied with $LIFE, perhaps someone else might want to pick it up?

[heinzelotto]

@bhankas check #855 where I did a preliminary implementation of per-book encryption, as suggested by @nevenz. Beside that I am unfortunately busy currently and can not pursue the PR further, everyone feel free to pursue further.