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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Jan 6, 2023 •
edited
Loading

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

@cla-bot cla-bot bot added the cla: yes label Jan 6, 2023
@coveralls
Copy link

coveralls commented Jan 6, 2023 •
edited
Loading

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 Coverage Status
Change from base Build 3871753423: -0.2%
Covered Lines: 2819
Relevant Lines: 3161
💛 - Coveralls

@nielsenko nielsenko marked this pull request as ready for review January 6, 2023 12:16
nirinchev
nirinchev reviewed Jan 6, 2023
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

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

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

final _pinnedClient = () {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. otherwise I should have done:

  final context = SecurityContext(withTrustedRoots: true);

Copy link
Contributor Author

@nielsenko nielsenko Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But only for the HttpClient we use.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@nielsenko
Copy link
Contributor Author

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
blagoev reviewed Jan 6, 2023
View reviewed changes
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

nielsenko commented Jan 6, 2023 •
edited
Loading

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.

@nielsenko 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
desistefanova
desistefanova approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

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 nielsenko requested review from blagoev and nirinchev January 6, 2023 14:34
@nielsenko
Copy link
Contributor Author

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.

@nielsenko nielsenko mentioned this pull request Jan 6, 2023
Closed
@Navil
Copy link

Navil commented Jan 6, 2023

@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
Copy link
Contributor Author

@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.

@nielsenko nielsenko marked this pull request as draft January 6, 2023 16:21
@nielsenko nielsenko mentioned this pull request Aug 9, 2023
Closed
@nirinchev nirinchev closed this Feb 23, 2024
@nirinchev nirinchev deleted the kn/pin-certificate branch February 23, 2024 14:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@desistefanova desistefanova desistefanova approved these changes

@blagoev blagoev Awaiting requested review from blagoev

@nirinchev nirinchev Awaiting requested review from nirinchev

Assignees

@nielsenko nielsenko

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: non-zero custom status code considered fatal
6 participants
@nielsenko @coveralls @Navil @nirinchev @blagoev @desistefanova