@akshajrawat Just wanted to update you with my list of considerations and further analysis.
Things already covered in the proposal without further explanation required
- is_locally_encrypted flag is added to notes and server items
- Option gated Note save / load is used for the note editor and viewer flows of the desktop and mobile apps, and the load / save note flows of the cli and api, in order to only apply local encryption / decryption for the paths which need it and to bypass sending unencrypted data via the sync during editing. Care is taken to cover the full flow, to avoid possible corruption
- An unlock note UI will be added to allow decrypting notes for viewing and editing
- A single encryption key will be used for managing encrypted notes, and a management UI will be implemented with change password and reset password options
- Encrypted notebooks will be implemented using a cascading lock / unlock all notes in notebook function (which should be protected against double encryption / decryption)
- For encrypted resources, they are decrypted to a temp-cache folder, and this is emptied on app start and exit on desktop, but just on app start on mobile, which is fine because the data dir for the app is secure. Decrypted resources should not get read completely into memory
- Encrypted notes will be marked with a padlock icon in the note list for visibility
Conflict resolution concerns
Regarding concerns around conflict resolution which I said I’d look into, my concern was that storing the encrypted note content in a separate field would mean that the value would be hidden from any kind of conflict resolution, making it impossible to resolve it in any meaningful way. As you moved to an approach which does not introduce a new field, this however is a non issue. While the content is still in encrypted form, it wont be legible to compare differences, but as long as the user is able to choose one version, or both, then the contents can at least be verified by unlocking the notes from the original notebook later (so no change required). Other considerations I think are out of scope for this project, but instead are factors to consider for the automatic conflict resolution project instead.
Just one change should be considered here, which is that the unlock note UI should not conflict with any UI which will be added by the automatic conflict resolution proposal, but the conflict UI should take precedence. If that proposal goes ahead, it might be worth considering just suppressing the unlock note UI completely for notes within the conflicts folder on the desktop and mobile app.
Search indexing (no change to the solution required)
Ideally encrypted note content should not be included in search indexing as it has no use, so opting out for notes with is_locally_encrypted true is a good idea. For old clients receiving encrypted notes from the sync, the opt out wont be possible. However, the note_normalized table is cleared on app start, so after fallback logic has migrated the note to have is_locally_encrypted set, this would no longer be an issue
Revision management
Initially I proposed that encrypted notes should opt out of the revision service. This however I think is risky, particularly when you consider the possibility of conflicts and a very poor experience to be able to resolve them. Additionally, I discovered there is an edge case whereby it is possible for an unencrypted revision to be uploaded by the sync, after a note has been been encrypted on another device, and without further handling, this unencrypted revision will be available on all devices until the the revision expires.
Aside from additional impact, the main concern with storing revisions for encrypted notes is that when content is updated, updated ciphers will create diffs which are as big as the entire note. Upon reflection, increased file size for revisions seems like an acceptable compromise, but the main issue is that in order to evaluate a revision for such notes, the diffs of the entire chain of revisions must be evaluated, which could perform poorly or crash the app, for long chains of revisions. My proposal would be for encrypted notes, instead of opting out of the revision service, the revision service should create revisions which have no parent, which means evaluation of those revisions is standalone.
Therefore a concept of encrypted revisions should be introduced, by adding an is_locally_encrypted flag which matches the value on the note at the time. In order to solve the sync edge case, revisions for an encrypted note where is_locally_encrypted is not true could be deleted upon opening that note in the UI of the desktop or mobile apps, or upon executing get note in the cli / api. This is not perfect, as unencrypted revisions can be persisted locally indefinitely, but as it is an edge case, this should be acceptable. Particularly for the UI routes, this would prevent ever being able to view such revisions, because the note must be opened first to get to the revision viewer. Regarding backwards compatibility, encrypted notes which were modified on old clients would have their history removed even if it was still encrypted, but this is an acceptable compromise because it is also edge case.
Additionally, to make the revisions for encrypted notes useful, it would be necessary to show the unlock note UI in the revision viewer where is_locally_encrypted is true. It wont be necessary to do any backwards compatibility checks here (see backwards compatibility section for migration scenario).
Backwards compatibility
Storing a JOPLIN_CIPHER: prefix in the note body for encrypted notes (stored in the server item and in the db, only removed via option gated Note load in order to decrypt the value), ensures that the encryption status cannot be lost when syncing encrypted notes with old Joplin clients. It also prevents possible double encryption via E2EE, as it makes it possible to determine a cipher created via local encryption specifically.
Regarding downloading server items which have passed through an older Joplin clients, the is_locally_encrypted flag will be stripped off, and it is necessary to update the is_locally_encrypted flag to true again for the purpose of search indexing and revision management (also the case when an old client containing encrypted notes synced from a newer client is upgraded). As it is not possible to determine the prefix on save when incoming from the sync, due to E2EE, the same solution for the revision management can be applied. Upon opening any note, where is_locally_encrypted is not defined, a migration will be performed which will set the value to true or false depending on whether the prefix is set
Resource management (additional questions / considerations)
- There is a resource management screen now on both the desktop and mobile apps (to be released on mobile). How would you handle when an encrypted resource is opened from that screen?
- How would you handle when a note with a resource is encrypted, and that resource is used in other notes which are not encrypted?
- If you remove the reference to the encrypted resource in an encrypted note, how would you manage detecting the need for decryption of that resource in an efficient way which doesn’t involve scanning notes? What if the resource is used in other encrypted notes still?
- Do we need to include an is_locally_encrypted flag for resources too, and have dedicated management to lock / unlock encrypted resources?