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

Introduce BWC tests in security plugin #1685

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

cliu123
Copy link
Member

@cliu123 cliu123 commented Mar 16, 2022

Signed-off-by: cliu123 lc12251109@gmail.com

Description

[Describe what this change achieves]

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Enhancement
  • Why these changes are required? Automate backward compatibility tests
  • What is the old behavior before changes and new behavior after changes?

Issues Resolved

#1417

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

Testing

BWC tests run in CI.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • 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.

@cliu123 cliu123 requested a review from a team March 16, 2022 19:23
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022

Codecov Report

Merging #1685 (d899401) into main (f275fb9) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1685      +/-   ##
============================================
+ Coverage     62.92%   62.94%   +0.02%     
- Complexity     3259     3261       +2     
============================================
  Files           253      253              
  Lines         18126    18126              
  Branches       3258     3258              
============================================
+ Hits          11406    11410       +4     
+ Misses         5071     5067       -4     
  Partials       1649     1649              
Impacted Files Coverage Δ
...ecurity/configuration/ConfigurationRepository.java 75.27% <0.00%> (+2.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f275fb9...d899401. Read the comment docs.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Can we generate the cert files at runtime instead of including them as binaries?

Signed-off-by: cliu123 <lc12251109@gmail.com>
@cliu123
Copy link
Member Author

cliu123 commented Mar 17, 2022

Can we generate the cert files at runtime instead of including them as binaries?

@peternied Great question! There's no way to generate these certs in runtime. They must be provided as test resources. Same case in other plugins, for example index-management-plugin. And these are just demo certs, so they are safe to provided as test resources. There're other similar certs provided as test resource in security plugin, for example, these and these . BTW, there's no need to block this PR with this comment. PRs won't be merged until all comments are resolved.

@peternied
Copy link
Member

I don't mean the OpenSearch service runtime, but during a setup phase before the bwc test are actually run? The heart of my goal is avoiding following the anti-pattern of checking in certificates

@cliu123
Copy link
Member Author

cliu123 commented Mar 17, 2022

I don't mean the OpenSearch service runtime, but during a setup phase before the bwc test are actually run? The heart of my goal is avoiding following the anti-pattern of checking in certificates

Yes, that's not ideal, but this is the best we can do since there's no way to generate those demo certs in runtime or setup phase. But this doesn't introduce any security vulnerability because those are just demo certs like the certs that have already existed in security plugin and other plugins such as Index Management Plugin as I mentioned above.

@cliu123 cliu123 requested review from peternied and a team March 17, 2022 22:24
@peternied
Copy link
Member

no way to generate those demo certs in runtime or setup phase

Why not, could you explain why this is not possible?

@cliu123
Copy link
Member Author

cliu123 commented Mar 17, 2022

no way to generate those demo certs in runtime or setup phase

Why not, could you explain why this is not possible?

These are all existing hardcoded demo certs that nobody uses in Prod env. I don't see any new risk being introduced by this PR as I mentioned above. This is a common way to provide demo certs as test resources across the OpenSearch community plugins as I mentioned above.
And I don't see a way to generate those certs in runtime or prepare steps. If there's a way, please post the guide. Otherwise, can we move on from this point and review the rest of this PR, and in the meantime, create an issue to explore a better way?

@peternied
Copy link
Member

We should be able to use command line tools such as openssl, how to, I am happy to make a PR onto this change to keep the security posture of this new code high.

@cliu123
Copy link
Member Author

cliu123 commented Mar 17, 2022

We should be able to use command line tools such as openssl, how to, I am happy to make a PR onto this change to keep the security posture of this new code high.

Cool! That is something unrelated to BWC tests. Let's include those changes in a separate PR. We can either merge this PR first if we haven't had a clear timeline on when the separate PR will be ready, or I can rebase this branch after you merge the separate PR into main branch. But please eliminate all the hardcoded certs in the security plugin repo in the separate PR.
In fact, this PR provides the demo certs as test resource in the same way as security plugin and other plugins, and the concern on this is a different topic and not in the scope of BWC tests, so I highly recommand to park on this for now and to review the rest of this PR.

@cliu123 cliu123 added the v2.0.0 label Mar 18, 2022
@davidlago
Copy link

Agree with @cliu123 as this removal of hardcoded certs is probably best as a follower PR/issue. For example, if we decide not to merge this one and we don't work on the removal of hardcoded certs, 2.0 will still have them. I suggest we go ahead and merge this one and then track the other work perhaps linking it to the overarching work of improving the getting started experience that @setiah is leading.

@cliu123 cliu123 dismissed peternied’s stale review March 18, 2022 23:24

@peternied Thanks for the comment! The comment brings up a valid community-wide area to improve. Let's track this effort in a separate track to eliminate all the hardcoded demo certificates in security plugin and other downstream plugins if you don't mind.

@cliu123 cliu123 merged commit e638c76 into opensearch-project:main Mar 18, 2022
@cliu123 cliu123 deleted the bwc-test-backup branch March 18, 2022 23:24
@cliu123 cliu123 added backport opendistro-1.3 Backport to opendistro-1.3 branch backport 1.x backport to 1.x branch backport 1.3 backport to 1.3 branch and removed backport opendistro-1.3 Backport to opendistro-1.3 branch labels Mar 18, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.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-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-1685-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e638c760cbc4a56d6eda1221e97747f29326c194
# Push it to GitHub
git push --set-upstream origin backport/backport-1685-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

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

@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.3 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-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-1685-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e638c760cbc4a56d6eda1221e97747f29326c194
# Push it to GitHub
git push --set-upstream origin backport/backport-1685-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-1685-to-1.3.

@cliu123 cliu123 removed backport 1.x backport to 1.x branch backport 1.3 backport to 1.3 branch labels Mar 18, 2022
@peternied
Copy link
Member

@opensearch-project/security Thanks for the feedback, we can work on improving the security posture separately with this change in place.

@cliu123 cliu123 restored the bwc-test-backup branch March 21, 2022 14:34
@cliu123 cliu123 deleted the bwc-test-backup branch March 25, 2022 23:21
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
Signed-off-by: cliu123 <lc12251109@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants