-
Notifications
You must be signed in to change notification settings - Fork 274
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
Added secure settings for ssl related passwords: #2296
Conversation
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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/util/SSLConfigConstants.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great contribution @chriswhite199! Left a couple of comments around tests, but overall this looks good to me.
src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
* 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 <chriswhite199@gmail.com>
Still up for discussion is the use of |
Is there an issue for this one, if not could you create one? |
Looks like a commit missed the DCO signoff:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Signed-off-by: Chris White <chriswhite199@gmail.com>
@chriswhite199 Can you add a new-line at the end of SecureSSLSettingsTest.java to fix CodeHygiene check? |
Signed-off-by: Chris White <chriswhite199@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @chriswhite199 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <chriswhite199@gmail.com>
security plugin PR opensearch-project/security#2296 added secure variants of the keystore and pemfile passwords resolves #334 Signed-off-by: Chris White <chriswhite199@gmail.com>
@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. |
The backport to
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 |
…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 <chriswhite199@gmail.com> (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://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: Chris White <chriswhite199@gmail.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>
Which version currently implements this feature? @chriswhite199 |
Hi @chinawushuai, it looks like that documentation have not been updated yet. I found the property starting from 2.7.0.0 release. |
I have checked the release notes, (#2296) is not included in version 2.7.0 @willyborankin |
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 inopensearch.yml
Secure settings added for existing ssl settings, using a
_secure
suffix, to allow older settings to be marked as deprecated: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
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.