Enable Certificate Transparency with OkHTTP Update #2681

avazirna · 2023-07-18T08:33:42Z

Summary

This PR addresses a vulnerability flagged by the ABDM auditors, according to which an attacker is able to use open source tools to intercepts requests and perform MITM attacks. The initial recommendation was to adopt Certificate Pinning but after some investigation we opted for Certificate Transparency (CT), considering the restrictions and maintenance burden related with pinning certificates. Certificate Transparency verifies the validity of the host's SSL certificate by checking the existence of Signed Certificate Timestamps (SCTs). SCTs are basically a confirmation that the certificate was submitted to a CT log by the Certificate Authorities (CA) when a new certificate is issued.

Feature Flag

This feature is behind a CommCare custom property called cc-enable-certificate-transparency

Product Description

When the verification fails, meaning there are no SCTs that accompany the Certificate, the connection is rejected and an exception such as the one below is raised.

 www.commcarehq.org -> Failure: This certificate does not have any Signed Certificate Timestamps in it.
23:38:44.175  W  logger> warning-network: Detecting captive portal failed with exceptionCLEARTEXT communication to www.commcarehq.org not permitted by network security policy
23:38:44.186  W  javax.net.ssl.SSLPeerUnverifiedException: Certificate transparency failed
23:38:44.186  W  	at com.appmattus.certificatetransparency.internal.verifier.CertificateTransparencyInterceptor.intercept(CertificateTransparencyInterceptor.kt:78)
23:38:44.186  W  	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
23:38:44.186  W  	at org.commcare.core.network.CommCareNetworkServiceGenerator.lambda$static$0(CommCareNetworkServiceGenerator.java:48)
23:38:44.186  W  	at org.commcare.core.network.CommCareNetworkServiceGenerator$$ExternalSyntheticLambda0.intercept(Unknown Source:0)

Considering that this feature is supposed to be enabled at the CommCare App level, a new Advanced menu option called Certificate Transparency has been added to allow the users to disable the feature without having to update the app.

Safety Assurance

I have confidence that this PR will not introduce a regression for the reasons below

Cross-request: dimagi/commcare-core#1311

avazirna · 2023-07-18T09:03:18Z

@damagatchi retest this please

ctsims

If I'm interpreting the current behavior correctly there are some changes we should make on these, but also satisfied with clarity in how it was determined that the behavior is different from what I'm interpreting.

ctsims · 2023-07-18T17:10:19Z

app/src/org/commcare/network/CTInterceptorConfig.kt

+    }
+
+    private val getInterceptor = certificateTransparencyInterceptor {
+        logger = object : CTLogger {


What's the frequency of this request pattern? The example lists a failed log, but it looks like at a minimum we would also expect getting a log record (or multiple) for each channel negotiation, which I don't think makes Logger the right channel unless we're filtering for failure more specifically

Based on the noisiness of logging I've been trying to push us back from using the CommCare logs for any events which are "unexceptional" and which would occur with higher frequency than user sessions. IE: If something normal / expected happens once per login it's fine to log, but if it happens on "machine frequency" (IE: a background process is retrying over and over) we should push to LogCat rather than CommCare's logs. It's also good to log blocking failures (Since eventually a user recovers from them), we just don't want to be constantly filling our sumo allocations with that data indefinitely.

Completely, indeed this would create a lot of noise as it's supposed to log the result of every negotiation. I will change it to only log failures (only once), which will allow us to know when someone gets blocked. However, I don't think in that situation we would receive anything from the device.

Cool, thanks for confirming. It's totally possible we could get the logged data from a request later if, for example, someone is trying to trick your WIFI with a MITM temporarily but you switch to mobile internet because it's not working.

ctsims · 2023-07-18T17:18:14Z

app/src/org/commcare/network/CTInterceptorConfig.kt

+        }
+
+        // In case there are CT Interceptors already attached
+        removeCTInterceptors(client)


I don't think this is possible, since the builder starts fresh each time in customizeRetrofitSetup(), so I'd prefer to avoid the redundancy, and especially the class name reflection check. Did you test specifically to see if things were getting added multiple times without this?

Yes, that's what I thought too but everything in the CommCareNetworkServiceGenerator is static and so we get the same OkHttpClient.Builder object with all the interceptors added previously. So this is just to remove the CT interceptors before the build() method is called. Unless I'm missing something.

Ah, got it.

That does seem tricky to deal with. I am a bit concerned about the structure of this approach being a straight string comparison, though.

Could we move this comparison to an instanceof check instead? That would prevent future drift issues that require a tight coupling of that string and the name

We can't because the CertificateTransparencyInterceptor class is internal, but we can switch to an object comparison.

ctsims · 2023-07-18T17:23:41Z

app/src/org/commcare/CommCareApplication.java

@@ -1183,4 +1182,8 @@ public AndroidPackageUtils getAndroidPackageUtils() {
    public boolean isNsdServicesEnabled() {
        return true;
    }
+
+    public void initCertificateTransparency() {
+        CommCareNetworkServiceGenerator.customizeRetrofitSetup(new CTInterceptorConfig());


Hm, are we sure this chains in this way? my read from the current code is that this may be overwriting this config rather than chaining it, and we may need a cleaner way to compose these two HttpBuilderConfig classes. It should be easy too, since they both act on the builder, we should just be able to make a composite that runs multiple.

@ctsims I had the opportunity to retest this and confirm my understanding. So, this is not supposed to impact any of the existing configuration of the OkHttpClient object such as the SSLSocketFactory, it just adds a new network interceptor and it sort of operates at a higher level. For instance, we can set a TrustManager for the Network Interceptor and it shouldn't override the TrustManager of the OkHttpClient. But I do like the idea of having a single entry point for these config classes.

shubham1g5

+1 to Clayton's comments.

a new Developers option called Certificate Transparency has been added to allow the users to disable the feature without having to update the app.

I would not expect users to interact with Developers Option at all and if we expect users to be able to turn this off - should we have it somewhere in Advanced Settings sections instead ? But I am also curious why do we want the user to have the ability to turn this off if an app specfically enforces it for it's users.

This is a like a fail-safe mechanism, to be used if, for some reason, CT stops validating the certificate. I imagine that this can happen when the certificate expires, or it's renewed with a different CA and it takes some time to propagate, or even, due to a failure on CT infra side, but I'm just guessing. I don't know for sure in which situations this is applicable but I think it's important to have a local mechanism to disabled it. Also because in that situation the device will not be able to communicate with HQ and so, updating the app to switch off the feature won't be possible. As per where this should live, I'm open to suggestions, I think Advanced Settings is a good option, but wouldn't it be too exposed? But I guess it wouldn't be a problem for a user to accidentally enable it.

shubham1g5 · 2023-07-18T17:17:34Z

app/src/org/commcare/network/CTInterceptorConfig.kt

+        return client
+    }
+
+    private val getInterceptor = certificateTransparencyInterceptor {


maybe lazy init this to make sure we are not initialising the interceptor when the certificate transparency is disabled.

avazirna · 2023-07-24T20:00:02Z

@damagatchi retest this please

avazirna · 2023-07-24T22:31:47Z

@damagatchi retest this please

ctsims · 2023-07-24T23:01:36Z

This is a like a fail-safe mechanism, to be used if, for some reason, CT stops validating the certificate. I imagine that this can happen when the certificate expires, or it's renewed with a different CA and it takes some time to propagate, or even, due to a failure on CT infra side, but I'm just guessing

There are a few scenarios to consider, but the three most relevant are

Customer is using their own cert upfront on a server where CT doesn't work
Customer is already on a server that will fail CT and can add the setting ahead of time after they catch the need with the announced pre-release
Google's Certificate Transparency service check disappears in the future

The first is the main one that requires a local option, since otherwise they wouldn't be able to download the app which would configure their device to not use it. I would mildly prefer to hide it behind developer options, simply because the need to use it is hypothetical and I don't want to introduce it unless there's a real demonstrated need or use case. If we do determine that for some reason CT works less well than we expect I could see moving it to a normal user setting instead.

avazirna · 2023-07-25T17:06:02Z

@damagatchi retest this please

avazirna · 2023-07-25T20:58:37Z

Details

I will take it back to Developer Options

avazirna · 2023-07-26T08:57:46Z

@damagatchi retest this please

avazirna · 2023-07-26T14:46:47Z

@damagatchi retest this please

avazirna · 2023-09-07T10:22:35Z

@damagatchi retest this please

shubham1g5

Some minor code feedback but looks good to go as it is.

shubham1g5 · 2023-09-07T15:45:45Z

app/src/org/commcare/network/CTInterceptorConfig.kt

+                logger = object : CTLogger {
+                    override fun log(host: String, result: VerificationResult) {
+                        if (result is VerificationResult.Failure && !previousRequestFailed) {
+                            Logger.log(LogTypes.TYPE_NETWORK, "$host -> $result")


can we add an additional message to the log - Logger.log(LogTypes.TYPE_NETWORK, "Certification verification failed: $host -> $result")

shubham1g5 · 2023-09-07T15:47:09Z

app/src/org/commcare/network/CTInterceptorConfig.kt

+                            Logger.log(LogTypes.TYPE_NETWORK, "$host -> $result")
+                            previousRequestFailed = true
+                        } else if (result is VerificationResult.Success && previousRequestFailed) {
+                            previousRequestFailed = false


This assignment can be moved out of if else block - previousRequestFailed = result is VerificationResult.Success

I think the idea is to compare it against VerificationResult.Failure

shubham1g5 · 2023-09-07T15:49:19Z

app/src/org/commcare/network/OkHttpBuilderCustomConfig.java

+         ctInterceptorConfig = new CTInterceptorConfig();
+         isrgCertConfig = new ISRGCertConfig();


Lets initialise them in performCustomConfig itself. That way we can make sure they are only initialised if needed. For ex. we don't need to init ISRGCertConfig if if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.N_MR1) { is not true.

on another thought, we actually don't need to instantiate these

can you say more to what you are thinking ?

Considering also that all the CommCareNetworkServiceGenerator content is static we don't actually need instances of CTInterceptorConfig and ISRGCertConfig to apply these custom configurations, right?

@shubham1g5 in concrete terms, this is what I'm thinking.

avazirna · 2023-09-18T12:02:01Z

app/src/org/commcare/CommCareApplication.java

+
+        // This is part of the CommCare app initialization because it needs to be applied during
+        // app initialization, update and when switching the seated app
+        customiseOkHttp();


@shubham1g5 I thought I had included in the initial PR. Basically, because the Certificate Transparency is a CommCare app level configuration, we need to be able to apply it in case it changes between app updates and when the user switches the seated app.

The ISRGCert needs to be attached on application start up as before otherwise users on some devices might not be able to download CC app itself.

@shubham1g5 true, when CommCare is installed there is not CC app to initialize and the configuration won't applied. I think consolidating configurations of different cycles/types is probably a stretch, but we can continue applying the configuration during app startup, just to cover all the scenarios.

avazirna · 2023-09-18T13:36:00Z

@damagatchi retest this please

shubham1g5

Couple questions -

How have you tested this change ? Can we specify it in the Safety story for PR
This setting needs to be QA'ed as part of regular releases and therefore we should have a QA note with QA note label set.

shubham1g5 · 2023-09-18T15:35:52Z

app/src/org/commcare/CommCareApplication.java

+
+        // This is part of the CommCare app initialization because it needs to be applied during
+        // app initialization, update and when switching the seated app
+        customiseOkHttp();


The ISRGCert needs to be attached on application start up as before otherwise users on some devices might not be able to download CC app itself.

avazirna · 2023-09-18T17:35:36Z

Couple questions -

How have you tested this change ? Can we specify it in the Safety story for PR

Yes, I have tested this a few time and in different use cases.

This setting needs to be QA'ed as part of regular releases and therefore we should have a QA note with QA note label set.

The label has been added and agree, this needs to be QA'ed.

avazirna · 2023-10-04T13:07:02Z

@damagatchi retest this please

avazirna · 2023-10-04T15:14:50Z

@damagatchi retest this please

avazirna · 2023-10-04T15:50:05Z

@damagatchi retest this please

avazirna added 6 commits July 17, 2023 23:20

Add Certificate Tranparency dependency

f7cb357

Add CT app preference

0ffb029

Add CT network interceptor

967cca0

Init CT network interceptor

cc8a513

Add CT developer preference

bf70f04

Improve logging

68c4193

avazirna mentioned this pull request Jul 18, 2023

Enable Certificate Transparency dimagi/commcare-core#1311

Merged

Check app preference in a null safe way

0ac900e

avazirna requested review from shubham1g5 and ctsims July 18, 2023 16:54

ctsims requested changes Jul 18, 2023

View reviewed changes

shubham1g5 requested changes Jul 18, 2023

View reviewed changes

avazirna added 5 commits July 24, 2023 10:28

Lazy init CT interceptor

4ea5627

Log only rejections and without repetions

9fb557c

Detach CT Interceptor

bee2ebf

Bump OkHttp to 4.11.0

7eb9987

Enable support for API desugaring

a5bd7d3

Whitelist CT Developer Option

d851810

avazirna force-pushed the enable_certificate_transparency branch from d18f18f to d851810 Compare July 26, 2023 11:27

Set Desugaring to minimal

caa0b4b

avazirna added 2 commits July 26, 2023 20:52

Update Proguard rules

e604805

Merge branch 'master' into enable_certificate_transparency

26dffec

Merge branch 'master' into enable_certificate_transparency

20ef053

avazirna requested a review from shubham1g5 September 7, 2023 15:12

shubham1g5 previously approved these changes Sep 7, 2023

View reviewed changes

Improve logging

f3abf1e

avazirna added this to the 2.54 milestone Sep 14, 2023

avazirna added 4 commits September 18, 2023 12:20

Lint

94efc1c

Refactor

279f859

Apply custom configuration statically

6a720db

Apply custom HTTP coniguration during CC app initialization

8131c5c

avazirna dismissed shubham1g5’s stale review via 8131c5c September 18, 2023 11:48

avazirna commented Sep 18, 2023

View reviewed changes

avazirna requested a review from shubham1g5 September 18, 2023 13:35

avazirna added Release Note QA Note labels Sep 18, 2023

shubham1g5 requested changes Sep 18, 2023

View reviewed changes

Apply custom configuration on app startup

0e53d88

shubham1g5 changed the title ~~Enable Certificate Transparency~~ Enable Certificate Transparency with OkHTTP Update Sep 18, 2023

shubham1g5 previously approved these changes Sep 18, 2023

View reviewed changes

avazirna requested a review from ctsims October 2, 2023 12:49

Merge branch 'master' into enable_certificate_transparency

c950a73

avazirna dismissed shubham1g5’s stale review via c950a73 October 3, 2023 23:41

shubham1g5 approved these changes Oct 4, 2023

View reviewed changes

avazirna merged commit 8407441 into master Oct 9, 2023
5 of 10 checks passed

avazirna deleted the enable_certificate_transparency branch October 9, 2023 13:03

		ctInterceptorConfig = new CTInterceptorConfig();
		isrgCertConfig = new ISRGCertConfig();

Enable Certificate Transparency with OkHTTP Update #2681

Enable Certificate Transparency with OkHTTP Update #2681

Conversation

avazirna commented Jul 18, 2023 • edited Loading

Summary

Feature Flag

Product Description

Safety Assurance

avazirna commented Jul 18, 2023

ctsims left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 left a comment • edited by avazirna Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Jul 24, 2023

avazirna commented Jul 24, 2023

ctsims commented Jul 24, 2023 • edited Loading

avazirna commented Jul 25, 2023

avazirna commented Jul 25, 2023 • edited Loading

avazirna commented Jul 26, 2023

avazirna commented Jul 26, 2023

avazirna commented Sep 7, 2023

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Sep 18, 2023

shubham1g5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Sep 18, 2023

avazirna commented Oct 4, 2023

avazirna commented Oct 4, 2023

avazirna commented Oct 4, 2023

avazirna commented Jul 18, 2023 •

edited

Loading

shubham1g5 left a comment •

edited by avazirna

Loading

ctsims commented Jul 24, 2023 •

edited

Loading

avazirna commented Jul 25, 2023 •

edited

Loading

shubham1g5 left a comment •

edited

Loading