Added secure settings for ssl related passwords: #2296

chriswhite199 · 2022-12-04T07:18:12Z

Description

Migration of SSL certificate / keystore passwords to use secure configuration (opensearch keystore). Existing settings are marked as insecure and will require current deployments to set the opensearch.allow_insecure_setting: false setting to allow the use of insecure settings in opensearch.yml

Secure settings added for existing ssl settings, using a _secure suffix, to allow older settings to be marked as deprecated:

plugins.security.ssl.http.pemkey_password_secure
plugins.security.ssl.http.keystore_password_secure
plugins.security.ssl.http.keystore_keypassword_secure
plugins.security.ssl.http.truststore_password_secure
plugins.security.ssl.transport.pemkey_password_secure
plugins.security.ssl.transport.server.pemkey_password_secure
plugins.security.ssl.transport.client.pemkey_password_secure
plugins.security.ssl.transport.keystore_password_secure
plugins.security.ssl.transport.keystore_keypassword_secure
plugins.security.ssl.transport.server.keystore_keypassword_secure
plugins.security.ssl.transport.client.keystore_keypassword_secure
plugins.security.ssl.transport.truststore_password_secure

Issues Resolved

Resolves #1549

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Updated unit tests, added new test for asserting the case where old insecure settings are used, and opensearch.allow_insecure_setting hasn't be set.

Check List

New functionality includes testing
New functionality has been documented
- To be completed in another PR to the opensearch-website repo
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov-commenter · 2022-12-04T21:43:56Z

Codecov Report

Merging #2296 (e40d273) into main (cad7c72) will increase coverage by 0.06%.
The diff coverage is 95.65%.

@@             Coverage Diff              @@
##               main    #2296      +/-   ##
============================================
+ Coverage     61.04%   61.10%   +0.06%     
- Complexity     3268     3274       +6     
============================================
  Files           259      260       +1     
  Lines         18337    18363      +26     
  Branches       3248     3250       +2     
============================================
+ Hits          11193    11221      +28     
+ Misses         5559     5557       -2     
  Partials       1585     1585

Impacted Files	Coverage Δ
...security/auditlog/sink/ExternalOpenSearchSink.java	`59.25% <0.00%> (ø)`
...ensearch/security/ssl/util/SSLConfigConstants.java	`77.77% <ø> (ø)`
...ic/auth/ldap/backend/LDAPAuthorizationBackend.java	`57.81% <100.00%> (+0.07%)`	⬆️
...amazon/dlic/util/SettingsBasedSSLConfigurator.java	`62.30% <100.00%> (ø)`
...azon/dlic/util/SettingsBasedSSLConfiguratorV4.java	`59.06% <100.00%> (+0.21%)`	⬆️
...opensearch/security/auditlog/sink/WebhookSink.java	`76.74% <100.00%> (ø)`
...ensearch/security/ssl/DefaultSecurityKeyStore.java	`67.28% <100.00%> (-0.55%)`	⬇️
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java	`78.82% <100.00%> (-1.29%)`	⬇️
...org/opensearch/security/ssl/SecureSSLSettings.java	`100.00% <100.00%> (ø)`
...opensearch/security/ssl/util/SSLRequestHelper.java	`65.71% <100.00%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

peternied

Thanks for the contribution! Great to see a clear improvement in the security posture. Had some ideas about how we could make it a more concise change, would love to know what you think.

src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPluginSecureSettings.java

src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java

src/main/java/org/opensearch/security/ssl/util/SSLConfigConstants.java

cwperks

This is a great contribution @chriswhite199! Left a couple of comments around tests, but overall this looks good to me.

src/test/java/org/opensearch/security/ssl/SSLTest.java

src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java

src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java

peternied

Looks great, I can go either way on the test cases for the enum class. Also added a nitpicky comment to use static imports, thanks for the great contribution!

src/main/java/org/opensearch/security/auditlog/sink/WebhookSink.java

src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java

* plugins.security.ssl.http.pemkey_password_secure * plugins.security.ssl.http.keystore_password_secure * plugins.security.ssl.http.keystore_keypassword_secure * plugins.security.ssl.http.truststore_password_secure * plugins.security.ssl.transport.pemkey_password_secure * plugins.security.ssl.transport.server.pemkey_password_secure * plugins.security.ssl.transport.client.pemkey_password_secure * plugins.security.ssl.transport.keystore_password_secure * plugins.security.ssl.transport.keystore_keypassword_secure * plugins.security.ssl.transport.server.keystore_keypassword_secure * plugins.security.ssl.transport.client.keystore_keypassword_secure * plugins.security.ssl.transport.truststore_password_secure resolves opensearch-project#1549 Signed-off-by: Chris White <[email protected]>

chriswhite199 · 2022-12-06T22:33:39Z

Still up for discussion is the use of SecureRestClientBuilder from commons-utils in other plugins, and how to best handle when the secure variants are used on a cluster hosting the plugin

peternied · 2022-12-06T23:03:09Z

Still up for discussion is the use of SecureRestClientBuilder from commons-utils in other plugins, and how to best handle when the secure variants are used on a cluster hosting the plugin

Is there an issue for this one, if not could you create one?

peternied · 2022-12-08T16:35:35Z

Looks like a commit missed the DCO signoff:

Commit sha: 6daf73e, Author: Chris White, Committer: Chris White; The sign-off is missing.

DarshitChanpura

@chriswhite199 thank you for adding the tests! Would you mind signing the commits?
Also you can you use ./gradlew spotlessApply to fix any formatting errors.

src/test/java/org/opensearch/security/ssl/SSLTest.java

src/test/java/org/opensearch/security/ssl/SecureSSLSettingsTest.java

Signed-off-by: Chris White <[email protected]>

DarshitChanpura · 2022-12-08T18:05:12Z

@chriswhite199 Can you add a new-line at the end of SecureSSLSettingsTest.java to fix CodeHygiene check?

Signed-off-by: Chris White <[email protected]>

DarshitChanpura

LGTM! Thanks @chriswhite199 !

cwperks

Thank you for the great contribution!

security plugin PR opensearch-project/security#2296 added secure variants of the keystore and pemfile passwords resolves opensearch-project#334 Signed-off-by: Chris White <[email protected]>

security plugin PR opensearch-project/security#2296 added secure variants of the keystore and pemfile passwords resolves #334 Signed-off-by: Chris White <[email protected]>

cwperks · 2023-07-11T16:54:23Z

@chriswhite199 I just realized that this change was not backported into the 2.x release branch. I will add a backport 2.x label so that this change gets released.

opensearch-trigger-bot · 2023-07-11T16:54:47Z

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2296-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2c20be065775a02ef376abd273c1d5fc8976e8c7
# Push it to GitHub
git push --set-upstream origin backport/backport-2296-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

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

…2296) Added secure settings for ssl related passwords: * plugins.security.ssl.http.pemkey_password_secure * plugins.security.ssl.http.keystore_password_secure * plugins.security.ssl.http.keystore_keypassword_secure * plugins.security.ssl.http.truststore_password_secure * plugins.security.ssl.transport.pemkey_password_secure * plugins.security.ssl.transport.server.pemkey_password_secure * plugins.security.ssl.transport.client.pemkey_password_secure * plugins.security.ssl.transport.keystore_password_secure * plugins.security.ssl.transport.keystore_keypassword_secure * plugins.security.ssl.transport.server.keystore_keypassword_secure * plugins.security.ssl.transport.client.keystore_keypassword_secure * plugins.security.ssl.transport.truststore_password_secure Signed-off-by: Chris White <[email protected]> (cherry picked from commit 2c20be0)

… (#3037) ### Description Backports 2c20be0 from #2296 ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [x] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https:/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Darshit Chanpura <[email protected]> Co-authored-by: Chris White <[email protected]> Co-authored-by: Craig Perkins <[email protected]>

chinawushuai · 2023-09-06T08:40:39Z

Which version currently implements this feature？ @chriswhite199

willyborankin · 2023-09-06T10:48:52Z

Hi @chinawushuai, it looks like that documentation have not been updated yet. I found the property starting from 2.7.0.0 release.

chinawushuai · 2023-09-07T05:14:39Z

I have checked the release notes, (#2296) is not included in version 2.7.0 @willyborankin

chriswhite199 requested a review from a team December 4, 2022 07:18

chriswhite199 force-pushed the main branch from aa4eef6 to 30b6bb4 Compare December 5, 2022 19:57

peternied requested changes Dec 5, 2022

View reviewed changes

chriswhite199 force-pushed the main branch from 30b6bb4 to b9e1833 Compare December 6, 2022 06:33

cwperks reviewed Dec 6, 2022

View reviewed changes

src/test/java/org/opensearch/security/ssl/SSLTest.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java Show resolved Hide resolved

peternied previously approved these changes Dec 6, 2022

View reviewed changes

src/main/java/org/opensearch/security/auditlog/sink/WebhookSink.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java Show resolved Hide resolved

chriswhite199 dismissed peternied’s stale review via 5da044c December 6, 2022 22:25

chriswhite199 force-pushed the main branch from 5da044c to ccc2328 Compare December 6, 2022 22:30

chriswhite199 mentioned this pull request Dec 6, 2022

[FEATURE] Update password settings to utilize secure variants opensearch-project/common-utils#334

Closed

peternied previously approved these changes Dec 8, 2022

View reviewed changes

DarshitChanpura requested changes Dec 8, 2022

View reviewed changes

src/test/java/org/opensearch/security/ssl/SSLTest.java Outdated Show resolved Hide resolved

src/test/java/org/opensearch/security/ssl/SecureSSLSettingsTest.java Show resolved Hide resolved

src/test/java/org/opensearch/security/ssl/SecureSSLSettingsTest.java Show resolved Hide resolved

Added unit tests

a684386

Signed-off-by: Chris White <[email protected]>

chriswhite199 dismissed peternied’s stale review via a684386 December 8, 2022 16:54

chriswhite199 force-pushed the main branch from 6daf73e to a684386 Compare December 8, 2022 16:54

Fixed code hygiene issue

e40d273

Signed-off-by: Chris White <[email protected]>

DarshitChanpura approved these changes Dec 8, 2022

View reviewed changes

cwperks approved these changes Dec 8, 2022

View reviewed changes

peternied merged commit 2c20be0 into opensearch-project:main Dec 8, 2022

chriswhite199 mentioned this pull request Dec 9, 2022

Added insecureString options to allow to easier migration to secure setting variants opensearch-project/OpenSearch#5496

Merged

chriswhite199 mentioned this pull request Dec 9, 2022

Update keystore password to utilize secure variants opensearch-project/common-utils#335

Merged

cwperks added the backport 2.x backport to 2.x branch label Jul 11, 2023

cwperks mentioned this pull request Jul 20, 2023

[Backport 2.x] Update imports for files refactored in core PR #8157 (#3003) #3036

Merged

3 tasks

DarshitChanpura mentioned this pull request Jul 20, 2023

[Backport 2.x] Added secure settings for ssl related passwords: (#2296) #3037

Merged

3 tasks

peternied mentioned this pull request Sep 6, 2023

[Documentation] Encrypted passwords in settings files #3320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added secure settings for ssl related passwords: #2296

Added secure settings for ssl related passwords: #2296

chriswhite199 commented Dec 4, 2022

codecov-commenter commented Dec 4, 2022 •

edited

Loading

peternied left a comment

cwperks left a comment

peternied left a comment

chriswhite199 commented Dec 6, 2022

peternied commented Dec 6, 2022

peternied commented Dec 8, 2022

DarshitChanpura left a comment

DarshitChanpura commented Dec 8, 2022 •

edited

Loading

DarshitChanpura left a comment

cwperks left a comment

cwperks commented Jul 11, 2023

opensearch-trigger-bot bot commented Jul 11, 2023

chinawushuai commented Sep 6, 2023 •

edited

Loading

willyborankin commented Sep 6, 2023

chinawushuai commented Sep 7, 2023

Added secure settings for ssl related passwords: #2296

Added secure settings for ssl related passwords: #2296

Conversation

chriswhite199 commented Dec 4, 2022

Description

Issues Resolved

Testing

Check List

codecov-commenter commented Dec 4, 2022 • edited Loading

Codecov Report

peternied left a comment

Choose a reason for hiding this comment

cwperks left a comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

chriswhite199 commented Dec 6, 2022

peternied commented Dec 6, 2022

peternied commented Dec 8, 2022

DarshitChanpura left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Dec 8, 2022 • edited Loading

DarshitChanpura left a comment

Choose a reason for hiding this comment

cwperks left a comment

Choose a reason for hiding this comment

cwperks commented Jul 11, 2023

opensearch-trigger-bot bot commented Jul 11, 2023

chinawushuai commented Sep 6, 2023 • edited Loading

willyborankin commented Sep 6, 2023

chinawushuai commented Sep 7, 2023

codecov-commenter commented Dec 4, 2022 •

edited

Loading

DarshitChanpura commented Dec 8, 2022 •

edited

Loading

chinawushuai commented Sep 6, 2023 •

edited

Loading