bitfireAT/cert4android

Continuous request of certificate verification

Closed this issue · 6 comments

ArnyminerZ commented

I think the issue might come from the saving of the certificate in CustomCertStore. We are never saving the updates from the Keystore object to the file. The only time we really save the changes is in clearUserDecisions:

cert4android/lib/src/main/java/at/bitfire/cert4android/CustomCertStore.kt

Line 81 in d6fd798

saveUserKeyStore()

We should also save in

cert4android/lib/src/main/java/at/bitfire/cert4android/CustomCertStore.kt

Lines 142 to 149 in d6fd798

@Synchronized
fun setTrustedByUser(cert: X509Certificate) {
Cert4Android.log.info("Trusted by user: $cert")
userKeyStore.setCertificateEntry(CertUtils.getTag(cert), cert)
untrustedCerts -= cert
}

cert4android/lib/src/main/java/at/bitfire/cert4android/CustomCertStore.kt

Lines 149 to 156 in d6fd798

@Synchronized
fun setUntrustedByUser(cert: X509Certificate) {
Cert4Android.log.info("Distrusted by user: $cert")
userKeyStore.deleteEntry(CertUtils.getTag(cert))
untrustedCerts += cert
}

ArnyminerZ commented

To reproduce:

  1. Launch the sample app.
  2. Trust a certificate
  3. Try to trust again, it's already been trusted
  4. Close the app (also from recents just in case)
  5. Open the app again
  6. The certificate can be trusted again, changes are not persisted.
rfc2822 commented

Haha nice, thanks for the report. So: core functionality broken. Could be time for some tests ^^

ArnyminerZ commented

Yup, quite a serious bug 😅

rfc2822 commented

At least easy to fix :)

ArnyminerZ commented

Maybe we want to add a test for this, I've tested this and it detects the issue on the main branch:

    @Test
    fun testPersistingChanges() {
        addTrustedCertificate()

        // Dispose the keystore - load keystore again
        certManager.certStore.loadUserKeyStore()

        // Check it has been trusted
        certManager.checkServerTrusted(siteCerts!!.toTypedArray(), "RSA")
    }

However, it needs to expose certStore and loadUserKeyStore through the VisibleForTesting annotation.

See 3110633