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

Added insecureString options to allow to easier migration to secure setting variants #5496

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

chriswhite199
Copy link
Contributor

Description

To address discussion points in the following issues and PRs in common-utils and security plugin:

This change adds override options to SecureSetting.insecureString such that migrating from legacy insecure settings to secure variants can be achieved without breaking existing cluster deployments and having to set opensearch.allow_insecure_settings. Instead usage of deprecated insecure settings are logged (warn level) with a note as to the new secure variant of the setting too.

If approved, changes can be made to common-utils and security plugin to utilize this new code

@chriswhite199 chriswhite199 requested review from a team and reta as code owners December 9, 2022 06:29
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2022

Codecov Report

Merging #5496 (d39d86b) into main (606255f) will decrease coverage by 0.03%.
The diff coverage is 84.61%.

❗ Current head d39d86b differs from pull request most recent head c547046. Consider uploading reports for the commit c547046 to get more accurate results

@@             Coverage Diff              @@
##               main    #5496      +/-   ##
============================================
- Coverage     70.85%   70.82%   -0.03%     
- Complexity    56553    58693    +2140     
============================================
  Files          4722     4769      +47     
  Lines        267594   280696   +13102     
  Branches      39212    40535    +1323     
============================================
+ Hits         189600   198801    +9201     
- Misses        61983    65563    +3580     
- Partials      16011    16332     +321     
Impacted Files Coverage Δ
.../org/opensearch/common/settings/SecureSetting.java 74.62% <84.61%> (+9.71%) ⬆️

... and 1548 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Should we be deprecating insecure settings instead rather than just warn-usage?

@saratvemulapalli
Copy link
Member

We could put out the warning in 2.x line and probably deprecate it in 3.x?

@chriswhite199
Copy link
Contributor Author

Should we be deprecating insecure settings instead rather than just warn-usage?

InsecureStringSettings are already marked as deprecated and a log entry is added when used in settings, but that doesn't give developers to opportunity to flag in the log entry what they should be using instead.

The issue discussed over in security plugin was related to not breaking existing setups and plugins that might still expect the insecure setting to be used. Just using insecureString wouldn't suffice as unless they had allow_insecure_settings to be true, startup would fail.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Last question: are we going to flood logs with these? should the warning happen once per node/constructor?

@chriswhite199
Copy link
Contributor Author

Last question: are we going to flood logs with these? should the warning happen once per node/constructor?

I don't think the logs will be flooded, but any time the settings is queried it will log yes. Typically this only occurs at startup for a plugin, but I see the risk where a plugin might repeatedly read the setting (as say part of a custom mapping).

The issue with construction time is that we don't yet know whether the insecure setting is actually used in configuration. It should be trivial to add a volatile or sync block around the current log statement and only log it once.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Add a test that it's not logged twice in a separate test.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member

dreamer-89 commented Dec 14, 2022

Gradle Check (Jenkins) Run Completed with:

Failing due to version bump after 2.4.1 release, #5560. Fixed is merged into main branch. @chriswhite199: Can you please rebase your branch against latest main branch ?

* What went wrong:
Execution failed for task ':distribution:bwc:bugfix:buildBwcLinuxTar'.
> Building 2.4.1 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/bugfix/build/bwc/checkout-2.4/distribution/archives/linux-tar/build/distributions/opensearch-min-2.4.1-SNAPSHOT-linux-x64.tar.gz

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@kartg
Copy link
Member

kartg commented Jun 14, 2023

Looks like this has been ready to merge for a while but got missed out amongst our other PRs. Since the baseline is quite dated, i'll rebase from the current main and resolve the CHANGELOG conflict

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Chris White <chriswhite199@gmail.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testPitCreatedOnReplica

@kartg kartg merged commit 0b775e7 into opensearch-project:main Jun 16, 2023
@kartg kartg added the backport 2.x Backport to 2.x branch label Jun 16, 2023
@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:

# 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
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5496-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0b775e79f637c7225ae908e368f1a62ce9ece011
# Push it to GitHub
git push --set-upstream origin backport/backport-5496-to-2.x
# Go back to the original working tree
popd
# 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-5496-to-2.x.

kartg pushed a commit to kartg/OpenSearch that referenced this pull request Jun 16, 2023
…-project#5496)

Signed-off-by: Chris White <chriswhite199@gmail.com>
(cherry picked from commit 0b775e7)
kasundra07 pushed a commit to harishbhakuni/OpenSearch that referenced this pull request Jun 16, 2023
kartg added a commit that referenced this pull request Jun 16, 2023
)

Signed-off-by: Chris White <chriswhite199@gmail.com>
(cherry picked from commit 0b775e7)

Co-authored-by: Chris White <chriswhite199@gmail.com>
gaiksaya pushed a commit to gaiksaya/OpenSearch that referenced this pull request Jun 26, 2023
…-project#5496) (opensearch-project#8094)

Signed-off-by: Chris White <chriswhite199@gmail.com>
(cherry picked from commit 0b775e7)

Co-authored-by: Chris White <chriswhite199@gmail.com>
imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request Jun 27, 2023
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…-project#5496)

Signed-off-by: Chris White <chriswhite199@gmail.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants