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

Add support for custom certificate #16481

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

voczi
Copy link
Contributor

@voczi voczi commented May 26, 2024

Copied from #15996

Purpose / Description

Since the new backend is forced on the newer Ankidroid releases, Android users cannot connect to their self-hosted sync server if it uses a self-signed certificate. The problem should be fixed with this PR. More than willing to accept any changes made to the UI part of this feature, it ain't that pretty.

Fixes

Approach

Allow users to specify a custom root certificate to trust (in PEM format) and feed this through the backend.

How Has This Been Tested?

Took the PEM-formatted root certificate I use for my local servers, and then copied the contents of it into the text field in the custom sync server section. After doing this, I don't get a error message telling me that the sync server certificate has an unknown issuer.

Learning

PEM, DER, CRT, and CER: X.509 Encodings and Conversions

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Screenshot

Screenshot_20240325_223253
(sorry for the flashbang)

Copy link
Contributor

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

@voczi voczi marked this pull request as draft May 26, 2024 13:51
@david-allison david-allison added the Blocked by dependency Currently blocked by some other dependent / related change label May 26, 2024
@voczi voczi marked this pull request as ready for review June 18, 2024 17:42
@david-allison david-allison added Needs Review and removed Blocked by dependency Currently blocked by some other dependent / related change labels Jun 19, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust that the change works, as mentioned in Discord, I won't have the capacity to verify

Required

  • Documentation + testing of how to remove the certificate
    • If this does work, a comment on the method (un)setting the preference is sufficient
  • Docs on whether un-setting the preference uses the empty string, or uses null
  • ankitects/anki@9e3a34f and/or the Anki Desktop PR in the commit message

Implementer's choice

  • I don't know what the 'scope' of the variable is within the backend: is it reset when the user changes collection, or would this setting persist until the backend is explicitly closed and opened (for example, if a user changes profile, is this reset?)
    • I would prefer if var currentSyncCertificate were moved further down the stack, possibly CollectionManager even if it's not exactly the right place. Possibly with logic inside CollectionManager.setCustomCertificate (if current cert == null, then no-op), which would reduce the code inside Sync.kt to

Strongly encouraged to disagree here

Subject: [PATCH] 
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt	(revision 4537497645db0522d6ee31901e0e5b74851ba8b2)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt	(date 1718818143347)
@@ -66,6 +66,8 @@
 
     private val testMutex = ReentrantLock()
 
+    private var currentSyncCertificate: String? = null
+
     /**
      * Execute the provided block on a serial background queue, to ensure
      * concurrent access does not happen.
@@ -400,4 +402,13 @@
         // as it does not seem to be compatible with the test scheduler
         queue = dispatcher
     }
+    /** TODO: Docs */
+    fun updateCustomCertificate(cert: String?): Boolean {
+        if (cert.isNullOrEmpty() && currentSyncCertificate.isNullOrEmpty()) return true
+
+        // TODO: explain ""
+        return getBackend().setCustomCertificate(cert ?: "").also {
+            currentSyncCertificate = cert
+        }
+    }
 }
Index: AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt	(revision 4537497645db0522d6ee31901e0e5b74851ba8b2)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt	(date 1718818143350)
@@ -32,7 +32,6 @@
 import com.ichi2.anki.CollectionManager.withCol
 import com.ichi2.anki.dialogs.DialogHandlerMessage
 import com.ichi2.anki.dialogs.SyncErrorDialog
-import com.ichi2.anki.preferences.get
 import com.ichi2.anki.preferences.sharedPrefs
 import com.ichi2.anki.servicelayer.ScopedStorageService
 import com.ichi2.anki.snackbar.showSnackbar
@@ -64,7 +63,6 @@
     const val CUSTOM_SYNC_URI = "syncBaseUrl"
     const val CUSTOM_SYNC_ENABLED = CUSTOM_SYNC_URI + VersatileTextWithASwitchPreference.SWITCH_SUFFIX
     const val CUSTOM_SYNC_CERTIFICATE = "customSyncCertificate"
-    var currentSyncCertificate = ""
 
     // Used in the legacy schema path
     const val HOSTNUM = "hostNum"
@@ -84,10 +82,7 @@
     val preferences = this.sharedPrefs()
 
     val currentSyncCertificate = preferences.getString(SyncPreferences.CUSTOM_SYNC_CERTIFICATE, null)
-    if (currentSyncCertificate != null && SyncPreferences.currentSyncCertificate != currentSyncCertificate) {
-        CollectionManager.getBackend().setCustomCertificate(currentSyncCertificate)
-        SyncPreferences.currentSyncCertificate = currentSyncCertificate
-    }
+    CollectionManager.updateCustomCertificate(currentSyncCertificate)
 
     val hkey = preferences.getString(SyncPreferences.HKEY, null)
     val resolvedEndpoint = getEndpoint(this)

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author squash-merge The pull request currently requires maintainers to "Squash Merge" and removed Needs Author Reply Waiting for a reply from the original author labels Jun 19, 2024
@voczi voczi force-pushed the custom-certificate branch from 1fd51bd to 4537497 Compare June 20, 2024 08:21
@voczi voczi requested a review from david-allison June 20, 2024 08:32
@voczi voczi force-pushed the custom-certificate branch from 508c605 to 265564d Compare June 20, 2024 08:54
@voczi
Copy link
Contributor Author

voczi commented Jun 20, 2024

side note: i did think of keeping track of the current cert in the backend code. although, this would mean that we would have to make calls to the backend every single time we have a cert stored. (even if it isn't changed)
not sure how fast calls to the backend are, so i chose to keep track of this in the app itself instead.

@voczi voczi force-pushed the custom-certificate branch from 265564d to 6c42f05 Compare June 20, 2024 12:47
@voczi voczi requested a review from david-allison June 20, 2024 12:48
@voczi voczi force-pushed the custom-certificate branch from 398360a to 6c42f05 Compare June 20, 2024 14:17
@voczi voczi removed Needs Author Reply Waiting for a reply from the original author squash-merge The pull request currently requires maintainers to "Squash Merge" labels Jun 22, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good (tested only the dialog, not a custom server accepting the cert)

  • The error message for an invalid PEM is truncated due to it being set as the title of the AlertDialog
  • Show some form of confirmation when the certificate is set correctly
  • In the comments to this issue, provide a valid PEM which can be used for testing

AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt Outdated Show resolved Hide resolved
@voczi voczi force-pushed the custom-certificate branch from 6c42f05 to a2f0564 Compare June 24, 2024 19:33
@voczi
Copy link
Contributor Author

voczi commented Jun 24, 2024

@voczi voczi requested a review from david-allison June 24, 2024 19:34
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more round, sorry for being unclear

AnkiDroid/src/main/res/values/03-dialogs.xml Outdated Show resolved Hide resolved
@voczi voczi force-pushed the custom-certificate branch from a2f0564 to 06a02e6 Compare June 25, 2024 10:33
@voczi
Copy link
Contributor Author

voczi commented Jun 25, 2024

One more round, sorry for being unclear

No worries, thank you for the suggestions! Should look better now.

@voczi voczi requested a review from david-allison June 25, 2024 10:34
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, cheers!

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jun 25, 2024
@voczi voczi requested a review from BrayanDSO June 26, 2024 08:57
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM - after all the previous rounds I don't see anything to polish
Strings just syncing now, putting this in the merge queue so it goes after strings, then re-sync

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Jun 27, 2024
@mikehardy
Copy link
Member

Another epic yak-shave (next to the release mode instrumented test) BTW, here going all the way upstream in anki then back through ankidroid to expose it, nice

@mikehardy mikehardy added this pull request to the merge queue Jun 27, 2024
Merged via the queue into ankidroid:main with commit edd15ee Jun 27, 2024
8 checks passed
@github-actions github-actions bot added this to the 2.19 release milestone Jun 27, 2024
Copy link
Contributor

Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR.

Read more about updating strings on the wiki,

@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jun 27, 2024
Copy link
Contributor

Hi there @voczi! This is the OpenCollective Notice for PRs merged from 2024-06-01 through 2024-06-30

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

Important

PLEASE NOTE: The process was updated in August 2024. Re-read the Payment Process page if you have not already.

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

@voczi voczi deleted the custom-certificate branch November 9, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants