Add ISRG X1 Root certificate (needed for Android 7.0 and earlier) #1088

nielsenko · 2023-01-06T11:44:30Z

The certificates from https://realm.mongodb.com/ are signed by lets-encrypt, which is dependent on the root certificate ISRG Root X1.

The list of platforms that support the ISRG Root X1 root certificate can be seen here: https://letsencrypt.org/docs/certificate-compatibility/. In particular you need Android >= 7.1.1

There is a dirty work-around to make these certificates work with old Android apps (>= 2.3.6), that you can read about here: https://letsencrypt.org/docs/dst-root-ca-x3-expiration-september-2021/. Unfortunately I don't think this will work with BoringSSL that is used by Dart / Flutter.

Fixes: #1087

coveralls · 2023-01-06T11:54:38Z

Pull Request Test Coverage Report for Build 3874151541

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
12 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 89.181%

Files with Coverage Reduction	New Missed Lines	%
lib/src/subscription.dart	1	90.48%
lib/src/app.dart	4	89.47%
lib/src/configuration.dart	7	63.11%

Totals
Change from base Build 3871753423:	-0.2%
Covered Lines:	2819
Relevant Lines:	3161

💛 - Coveralls

nirinchev

Looks good, assuming you verified it works. I have comments about the pinning wording, but those don't affect the actual implementation.

nirinchev · 2023-01-06T12:22:31Z

lib/src/app.dart

@@ -26,6 +27,48 @@ import 'credentials.dart';
 import 'native/realm_core.dart';
 import 'user.dart';

+final _pinnedClient = () {


This is not pinning the certificate, rather it's adding it to the set of trusted certificates. This doesn't prevent other certificates from being trusted by the app.

It pins the root certificate. No other root certificates are allowed

.. otherwise I should have done:

final context = SecurityContext(withTrustedRoots: true);

But only for the HttpClient we use.

No other root certificates are allowed this doesn't sound as the best approach imo.

https://security.stackexchange.com/questions/224246/certficate-pinning-should-i-pin-the-leaf-or-intermediate#:~:text=By%20pinning%20against%20the%20root,not%20to%20mis%2Dissue%20certificates.

.. but it is trivial to change

We restrict to only trust certificates signed (transitively) by ISRG X1 Root, which is less than what we trust today, but still a whole lot.. basically anything signed by lets-encrypt.

.. and different from what is trusted on Android prior to 7.1.1

After a long slack discussion I have added the root certificate instead of pinning it.

This turns out to break it for @Navil. I was a bit quick to accept this change. Will revert to draft.

nielsenko · 2023-01-06T12:41:05Z

Looks good, assuming you verified it works. I have comments about the pinning wording, but those don't affect the actual implementation.

It has been verified to work. Should we configure CI to run Android tests on an older image as well?

blagoev

Should we check the android version and do this only on platforms that are problematic. Also isn't this a double edged sword Conveniently it also ensure we are not dependent on the installed root certificates on the device.
If for some reasons server and client certificates are not working or updated. This means we will not work by default on newer Android versions. Then people will need to manually fix it for newer platforms by default. And this is just for supporting a low number of users using outdated Android versions.

nielsenko · 2023-01-06T12:59:17Z

Should we check the android version and do this only on platforms that are problematic. Also isn't this a double edged sword Conveniently it also ensure we are not dependent on the installed root certificates on the device. If for some reasons server and client certificates are not working or updated. This means we will not work by default on newer Android versions. Then people will need to manually fix it for newer platforms by default. And this is just for supporting a low number of users using outdated Android versions.

Yes we will work everywhere, as long as realm.mongodb.com signs with a certificate that is in turn signed by ISRG X1 Root. Also on any newer Android versions, even if they decide not to trust ISRG X1 Root.

desistefanova

It is not a good practice to hardcode the certificate that will expire. But still it will work for some time. Probably we will have to update that at some point.

nielsenko · 2023-01-06T14:35:07Z

It is not a good practice to hardcode the certificate that will expire. But still it will work for some time. Probably we will have to update that at some point.

Root certificates are always "hardcoded". They are updated with system updates, if not added explicitly by the user.

Navil · 2023-01-06T15:25:12Z

@nielsenko When adding
withTrustedRoots: true
it does not work anymore on my old testing device. Anything else to keep in mind when adding that flag?

nielsenko · 2023-01-06T16:19:58Z

@nielsenko When adding withTrustedRoots: true it does not work anymore on my old testing device. Anything else to keep in mind when adding that flag?

Interesting .. I will need to check up on why. You don't need that flag. I only added it in the PR, due to a discussion on slack.

cla-bot bot added the cla: yes label Jan 6, 2023

github-actions bot assigned nielsenko Jan 6, 2023

nielsenko force-pushed the kn/pin-certificate branch from ae8ace5 to 341f042 Compare January 6, 2023 11:58

nielsenko requested review from blagoev, desistefanova and nirinchev January 6, 2023 12:16

nielsenko marked this pull request as ready for review January 6, 2023 12:16

nirinchev reviewed Jan 6, 2023

View reviewed changes

blagoev reviewed Jan 6, 2023

View reviewed changes

nielsenko changed the title ~~Pin root certificate to ISRG X1 Root~~ Add ISRG X1 Root certificate (needed for Android 7.0 and earlier) Jan 6, 2023

nielsenko force-pushed the kn/pin-certificate branch from e022076 to 95d278d Compare January 6, 2023 14:00

desistefanova approved these changes Jan 6, 2023

View reviewed changes

nielsenko requested review from blagoev and nirinchev January 6, 2023 14:34

nielsenko mentioned this pull request Jan 6, 2023

[Bug]: non-zero custom status code considered fatal #1087

Closed

nielsenko marked this pull request as draft January 6, 2023 16:21

nielsenko added 4 commits January 9, 2023 14:17

Pin root certificate to ISRG X1 Root

b5d0db7

Update CHANGELOG

216429c

Add instead of pin the root certificate

184bb86

Update CHANGELOG

38622db

nielsenko force-pushed the kn/pin-certificate branch from be6f09d to 38622db Compare January 9, 2023 13:17

nielsenko mentioned this pull request Aug 9, 2023

Login on windows is not working: non-zero custom status code considered fatal #1370

Closed

nirinchev closed this Feb 23, 2024

nirinchev deleted the kn/pin-certificate branch February 23, 2024 14:23

github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ISRG X1 Root certificate (needed for Android 7.0 and earlier) #1088

Add ISRG X1 Root certificate (needed for Android 7.0 and earlier) #1088

nielsenko commented Jan 6, 2023 •

edited

coveralls commented Jan 6, 2023 •

edited

nirinchev left a comment

nirinchev Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko Jan 6, 2023 •

edited

blagoev Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko Jan 6, 2023

nielsenko commented Jan 6, 2023

blagoev left a comment

nielsenko commented Jan 6, 2023 •

edited

desistefanova left a comment

nielsenko commented Jan 6, 2023

Navil commented Jan 6, 2023

nielsenko commented Jan 6, 2023

Add ISRG X1 Root certificate (needed for Android 7.0 and earlier) #1088

Add ISRG X1 Root certificate (needed for Android 7.0 and earlier) #1088

Conversation

nielsenko commented Jan 6, 2023 • edited

coveralls commented Jan 6, 2023 • edited

Pull Request Test Coverage Report for Build 3874151541

💛 - Coveralls

nirinchev 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

nielsenko Jan 6, 2023 • edited

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

nielsenko commented Jan 6, 2023

blagoev left a comment

Choose a reason for hiding this comment

nielsenko commented Jan 6, 2023 • edited

desistefanova left a comment

Choose a reason for hiding this comment

nielsenko commented Jan 6, 2023

Navil commented Jan 6, 2023

nielsenko commented Jan 6, 2023

nielsenko commented Jan 6, 2023 •

edited

coveralls commented Jan 6, 2023 •

edited

nielsenko Jan 6, 2023 •

edited

nielsenko commented Jan 6, 2023 •

edited