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

Support SSL Sync Configuration #5507

Merged
merged 17 commits into from
Apr 19, 2023
Merged

Support SSL Sync Configuration #5507

merged 17 commits into from
Apr 19, 2023

Conversation

elle-j
Copy link
Contributor

@elle-j elle-j commented Feb 27, 2023

What, How & Why?

When opening a synced realm, users can provide an SSL configuration containing either:

  • (A) A callback function used to accept or reject the server's "SSL" (TLS) certificate.
  • (B) A path to a trust certificate to hand off the validation.
  • (C) Choose not to validate the server's certificate (intended for non-production use).

Examples:

// (A) Custom validation.
const ssl: SSLConfiguration = {
  validate: true, // (Default)
  validateCertificates: (verifyObject: SSLVerifyObject) => {
    
    // Inspect the SSLVerifyObject and validate certificate..

    // Accept server's certificate:
    return true;

    // Or reject it:
    // return false;
  },
};

// (B) Hand off the validation.
const ssl: SSLConfiguration = {
  validate: true,
  certificatePath: "path/to/trust/certificate",
};

// (C) Do not validate.
const ssl: SSLConfiguration = {
  validate: false,
};

const realm = await Realm.open({
  schema: [MySchema],
  sync: {
    ssl,
    // ...
  },
});

This closes #5485.

☑️ ToDos

  • 📝 Changelog entry
  • 🚦 Tests (Located in PR 5694)

@cla-bot cla-bot bot added the cla: yes label Feb 27, 2023
Comment on lines 258 to 278
static auto make_ssl_verify_callback(std::function<bool(const std::string& server_address, int server_port,
std::string_view pem_data, int preverify_ok, int depth)> callback) {
return [callback = std::move(callback)](const std::string& server_address, uint16_t server_port,
const char* pem_data, size_t pem_size, int preverify_ok, int depth) {
return callback(server_address, server_port, { pem_data, pem_size }, preverify_ok, depth);
};
}
Copy link
Contributor Author

@elle-j elle-j Feb 27, 2023

Choose a reason for hiding this comment

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

  1. [line 269] Take the JS-wrapped version of the callback as the argument. This callback will (when invoked) call the user-provided callback for validating certificates.
  2. [line 271] Return the CPP (Core-friendly) version of the callback (stored in the binding).
  3. [line 273] When the CPP callback is invoked, invoke the JS-wrapped callback and return its result.

...parseClientResetConfig(clientReset, onError),
};
}

/** @internal */
Copy link
Contributor Author

@elle-j elle-j Feb 27, 2023

Choose a reason for hiding this comment

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

On second thought, don't think this annotation is necessary since the function is not exported. (Ps. there are several non-exported functions in this file using this annotation already.)

Base automatically changed from bindgen to master March 3, 2023 14:05
@elle-j elle-j force-pushed the lj/bindgen/ssl-sync-config branch from dc2da79 to 9ca326b Compare March 17, 2023 15:05
@elle-j elle-j mentioned this pull request Apr 3, 2023
1 task
@elle-j elle-j marked this pull request as ready for review April 3, 2023 19:15
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM. Would be great to see some tests, but I think that's not so straightforward. I assume you have tested this manually?

@elle-j
Copy link
Contributor Author

elle-j commented Apr 4, 2023

@takameyer, you can look at the separate PR for the tests. We decided to split up the PRs as with the Jira issues (v12 and final v12). All tests pass in my Docker container.

@elle-j elle-j force-pushed the lj/bindgen/ssl-sync-config branch from 893101c to a15ad76 Compare April 5, 2023 16:27
Comment on lines +270 to +278
static auto make_ssl_verify_callback(std::function<bool(const std::string& server_address, int server_port,
std::string_view pem_data, int preverify_ok, int depth)>
callback)
{
return [callback = std::move(callback)](const std::string& server_address, uint16_t server_port,
const char* pem_data, size_t pem_size, int preverify_ok, int depth) {
return callback(server_address, server_port, std::string_view(pem_data, pem_size), preverify_ok, depth);
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. [signature]: Take the JS-wrapped version of the callback as the argument. This callback will (when invoked) call the user-provided callback for validating certificates.
  2. [1st return]: Return the CPP (Core-friendly) version of the callback (stored in the binding).
  3. [2nd return]: When the CPP callback is invoked, invoke the JS-wrapped callback and return its result.

@takameyer
Copy link
Contributor

takameyer commented Apr 6, 2023

Getting the following error when building locally and on the darwin arm64 build job in GitHub Actions:

clang: error: unable to execute command: Segmentation fault: 11
clang: error: clang frontend command failed due to signal (use -v to see invocation)
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.3.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
clang: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /var/folders/r7/nrrb917s7859md1_13pxplwm0000gp/T/node_init-5555d2.cpp
clang: note: diagnostic msg: /var/folders/r7/nrrb917s7859md1_13pxplwm0000gp/T/node_init-5555d2.sh
clang: note: diagnostic msg: Crash backtrace is located in
clang: note: diagnostic msg: /Users/andrew.meyer/Library/Logs/DiagnosticReports/clang_<YYYY-MM-DD-HHMMSS>_<hostname>.crash
clang: note: diagnostic msg: (choose the .crash file that corresponds to your crash)
clang: note: diagnostic msg: 

********************

This is not appearing on main. Unsure what could be causing that 🤷

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

We need to fix the build issues before this is merged.

type: bool
default: true
ssl_trust_certificate_path: util::Optional<std::string>
ssl_verify_callback: Nullable<SSLVerifyCallback>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be off_thread?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is. See line 642.

Copy link
Contributor Author

@elle-j elle-j Apr 12, 2023

Choose a reason for hiding this comment

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

Not sure if it can be marked as off_thread here.

As this is the SSLVerifyCallback:

SSLVerifyCallback:
  cppName: std::function<SyncConfig::SSLVerifyCallback>

And having ssl_verify_callback as:

SyncConfig:
  fields:
    # ...
    ssl_verify_callback: Nullable<SSLVerifyCallback>

It's unsure how/if it should be applied. These are 2 examples of other how other methods use it:

should_compact_on_launch_function: 'Nullable<std::function<(total_bytes: uint64_t, used_bytes: uint64_t) off_thread -> bool>>'
error_handler: 'Nullable<std::function<(session: SharedSyncSession, error: SyncError) off_thread -> void>>'

We do however need it for our helper make_ssl_verify_callback:

classes:
  Helpers:
    staticMethods:
      # ...
      make_ssl_verify_callback: '(callback: (server_address: const std::string&, server_port: int, pem_data: std::string_view, preverify_ok: int, depth: int) off_thread -> bool) -> SSLVerifyCallback'

But @RedBeard0531 could chime in here 🙂

@@ -286,11 +286,22 @@ export class SyncSession {
// TODO: Figure out a way to avoid passing a mocked app instance when constructing the User.
get config(): SyncConfiguration {
const user = new User(this.internal.user, mockApp);
const { partitionValue, flxSyncRequested, customHttpHeaders } = this.internal.config;
const { partitionValue, flxSyncRequested, customHttpHeaders, clientValidateSsl, sslTrustCertificatePath } =
Copy link
Member

Choose a reason for hiding this comment

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

Does the clientValidateSsl and sslTrustCertificatePath have sane defaults when coming from core? (i.e. not just an empty string or something).

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 will be whatever the user has provided as their validate (binding: clientValidateSsl) and certificatePath (binding: sslTrustCertificatePath).

export type SSLConfiguration = {
  validate?: boolean;
  certificatePath?: string;

  // ...
};

If you don't provide a value for certificatePath, the value can be undefined as in this test. validate defaults to true.

@elle-j elle-j merged commit 667d729 into main Apr 19, 2023
@elle-j elle-j deleted the lj/bindgen/ssl-sync-config branch April 19, 2023 09:12
papafe added a commit that referenced this pull request Apr 21, 2023
* main:
  Added note in docs for change listeners (#5749)
  Update CHANGELOG entry (#5751)
  Use `SchemaParseError` for invalid schemas (#5722)
  Improving Electron tests (#5738)
  Support SSL Sync Configuration (#5507)

# Conflicts:
#	CHANGELOG.md
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 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.

Implement SSL Sync config
5 participants