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

Re-add custom certificate support #7739

Merged
merged 18 commits into from
Dec 4, 2022
Merged

Conversation

cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Nov 11, 2022

This PR re-adds custom certificate code that was assumed to not be needed anymore. We where wrong. It was mostly the public API's and tests that had been removed. Most of the internal plumbing was still present in the code base.

It has not been possible to test the success case, but since the code has just been re-added it is assumed to work. The error case is tested by the tests and these also verify that we correctly load the certificate from assets and call the validate functions.

TODO

  • Verify all tests have been ported
  • Verify custom certificates actually work

@cla-bot cla-bot bot added the cla: yes label Nov 11, 2022
@cmelchior cmelchior marked this pull request as ready for review November 21, 2022 12:06
Copy link
Collaborator

@nhachicha nhachicha left a comment

Choose a reason for hiding this comment

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

Thanks for reviving this!

.modules(DefaultSyncSchema())
.trustedRootCA("untrusted_ca.pem")
.build()
// waitForInitialRemoteData will throw an Internal error (125): Operation Canceled
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still throwing?

Copy link
Collaborator

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

I am a bit uncomfortable with amount of ignored tests due to expired test certificates and no test for the happy path ... and I am not even able to see how the asset file is copied to the internal location (but it might just be me that can't find it 🤪)

There are also numerous references to Realm Object Server in the docs

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -1283,6 +1381,19 @@ public SyncConfiguration build() {
String absolutePathForRealm = user.getApp().getSync().getAbsolutePathForRealm(user.getId(), partitionValue, filename);
File realmFile = new File(absolutePathForRealm);

if (!Util.isEmptyString(serverCertificateAssetName)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the certificate copied from the asset location to the internal location? ... can't seem to find 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.

The code was left in RealmCache, so isn't part of this PR. But i discovered a bug in it while testing 🙈 (it failed if you had both an certificate and disableSsl enabled).

Dockerfile Outdated Show resolved Hide resolved
val syncConfigSSL: SyncConfiguration = configFactory.createSyncConfigurationBuilder(user, partition)
.modules(DefaultSyncSchema())
.waitForInitialRemoteData()
.disableSSLVerification()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this test? Is there anything else in the setup that would make this fail if you didn't include this option? Maybe if you added an otherwise failling certificate you would show that we are bypassing that 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a manual test to this class that runs against an prod Atlas app. It requires that you run it manually, but does verify that things work.

@cmelchior cmelchior merged commit b6d1c94 into master Dec 4, 2022
@cmelchior cmelchior deleted the cm/custom-certificate-support branch December 4, 2022 17:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants