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

[Multiple Datasources] Add TLS configuration for multiple data sources #6171

Merged
merged 14 commits into from
Mar 22, 2024

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Mar 16, 2024

Description

This PR adds 2 settings to multiple datasources:

data_source.ssl.verificationMode
data_source.ssl.certificateAuthorities

Example multi-datasource config:

data_source.enabled: true
data_source.ssl.verificationMode: full
data_source.ssl.certificateAuthorities: [ "/Users/cwperx/Projects/opensearch/OpenSearch-Dashboards/config/root-ca.pem" ]

These settings are common across all configured datasources so setting verificationMode to none will disable ssl verification for all datasources. Similarly, the list of all certificateAuthorities will be used to verify across all configured datasources.

These configuration values are separate then their equivalents when OSD is configured with one OpenSearch cluster where these are used to configure the legacy client to connect with the default OpenSearch cluster:

opensearch.ssl.verificationMode
opensearch.ssl.certificateAuthorities

Issues Resolved

Testing the changes

I have been testing by running 2 clusters locally, both configured with the demo certificates. One cluster is the default OpenSearch and the other cluster I add as another data source.

Tested following scenarios:

  1. Both clusters have the security plugin installed and https enabled

    • Able to connect to Cluster 2 when verificationMode is set to none
    • Able to connect to Cluster 2 when verificationMode is set to certificate and certificateAuthorities are set with correct CAs
    • Able to connect to Cluster 2 when verificationMode is set to full and certificateAuthorities are set with correct CAs
    • Not able to connect to Cluster 2 when verificationMode is set to certificate and certificateAuthorities are set with incorrect CAs
    • Not able to connect to Cluster 2 when verificationMode is set to full and certificateAuthorities are set with incorrect CAs
  2. Disable HTTPS on cluster 2

    • Able to connect to cluster 2 with https disabled. When entering hostname make sure the protocol is http instead of https

I am setting this PR to draft while tests are added in the security plugin to verify all of these scenarios.

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

Attention: Patch coverage is 83.72093% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 67.36%. Comparing base (0dce00a) to head (a2b7f61).

Files Patch % Lines
...plugins/data_source/server/client/client_config.ts 78.57% 1 Missing and 2 partials ⚠️
...plugins/data_source/server/legacy/client_config.ts 78.57% 1 Missing and 2 partials ⚠️
...s/data_source/server/util/tls_settings_provider.ts 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6171   +/-   ##
=======================================
  Coverage   67.35%   67.36%           
=======================================
  Files        3351     3352    +1     
  Lines       65037    65080   +43     
  Branches    10475    10485   +10     
=======================================
+ Hits        43805    43839   +34     
- Misses      18685    18688    +3     
- Partials     2547     2553    +6     
Flag Coverage Δ
Linux_1 31.82% <ø> (ø)
Linux_2 55.57% <ø> (ø)
Linux_3 44.73% <83.72%> (+0.04%) ⬆️
Linux_4 35.03% <ø> (ø)
Windows_1 31.87% <ø> (ø)
Windows_2 55.53% <ø> (ø)
Windows_3 44.74% <83.72%> (+0.05%) ⬆️
Windows_4 35.03% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seraphjiang
Copy link
Member

We might add the new config into default opensearch_dashboards.yml and comment out with recommended default value

@cwperks
Copy link
Member Author

cwperks commented Mar 18, 2024

We might add the new config into default opensearch_dashboards.yml and comment out with recommended default value

@seraphjiang Added commented out examples with reasonable defaults to opensearch_dashboards.yml

I also added unit tests. This is ready for review now.

@cwperks
Copy link
Member Author

cwperks commented Mar 18, 2024

@seraphjiang In what circumstances is the legacy_client used? I'm looking through the data-source plugin now and it looks like this PR may also need to be extended to the legacy client. Is the legacy client used to connect to ES clusters?

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Mar 18, 2024

Is the legacy client used to connect to ES clusters?

I see an open issue around version decoupling. I added the legacy client changes into this PR as well.

@BionIT @ZilongX Could you take a look at this PR when you have a chance?

@ZilongX
Copy link
Collaborator

ZilongX commented Mar 19, 2024

also triggered rerunning of failed checks, there were some reverted changes in the FT repo yesterday and now all CI should pass

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Mar 22, 2024

@BionIT @ZilongX I have replied to all PR feedback, can you take another look when you have a chance?

@ZilongX ZilongX self-requested a review March 22, 2024 17:46
@ZilongX ZilongX merged commit a9b400e into opensearch-project:main Mar 22, 2024
69 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-6171-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a9b400e42cf9949e7e647c0da5c37bbcd6d0f6d7
# Push it to GitHub
git push --set-upstream origin backport/backport-6171-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6171-to-2.x.

@cwperks
Copy link
Member Author

cwperks commented Mar 22, 2024

I will open a manual backport

cwperks added a commit to cwperks/OpenSearch-Dashboards that referenced this pull request Mar 22, 2024
opensearch-project#6171)

* Add TLS configuration for multiple data sources

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG and add examples commented out in opensearch_dashboards.yml

Signed-off-by: Craig Perkins <[email protected]>

* Add tests and replace instance of any

Signed-off-by: Craig Perkins <[email protected]>

* Add tls config to legacy client

Signed-off-by: Craig Perkins <[email protected]>

* Add test for certificate mode

Signed-off-by: Craig Perkins <[email protected]>

* Respond to PR feedback

Signed-off-by: Craig Perkins <[email protected]>

* Extract readCertificateAuthorities to util file and add more tests

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit a9b400e)
@cwperks
Copy link
Member Author

cwperks commented Mar 22, 2024

@ZilongX @BionIT Created a manual backport to 2.x for this here: #6244

ZilongX pushed a commit that referenced this pull request Mar 29, 2024
#6171) (#6244)

* Add TLS configuration for multiple data sources

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG and add examples commented out in opensearch_dashboards.yml

Signed-off-by: Craig Perkins <[email protected]>

* Add tests and replace instance of any

Signed-off-by: Craig Perkins <[email protected]>

* Add tls config to legacy client

Signed-off-by: Craig Perkins <[email protected]>

* Add test for certificate mode

Signed-off-by: Craig Perkins <[email protected]>

* Respond to PR feedback

Signed-off-by: Craig Perkins <[email protected]>

* Extract readCertificateAuthorities to util file and add more tests

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit a9b400e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants