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

Support security config updates on the REST API using permission #3264

Merged

Conversation

willyborankin
Copy link
Collaborator

@willyborankin willyborankin commented Aug 29, 2023

Description

Instead of setting SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION settings to update security configuration using PATCH or PUT a new permission was added: restapi:admin/config/update.

So far I decided to keep this flag as it is due to a backward compatibility and log a deprecation message that these settings will be removed in the future. Maybe it is better to remove it completely.

Besides, added the missed test for SecurityConfigApiAction

Issues Resolved

#2577

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.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #3264 (8281a1b) into main (7924da1) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3264      +/-   ##
============================================
+ Coverage     64.56%   64.59%   +0.03%     
- Complexity     3545     3550       +5     
============================================
  Files           269      269              
  Lines         20363    20365       +2     
  Branches       3376     3377       +1     
============================================
+ Hits          13147    13155       +8     
+ Misses         5525     5522       -3     
+ Partials       1691     1688       -3     
Files Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java 84.65% <100.00%> (+0.04%) ⬆️
...dlic/rest/api/RestApiAdminPrivilegesEvaluator.java 73.58% <100.00%> (+2.15%) ⬆️
...ecurity/dlic/rest/api/SecurityConfigApiAction.java 93.54% <100.00%> (+0.44%) ⬆️
...security/dlic/rest/api/SecurityRestApiActions.java 80.00% <ø> (ø)
...urity/dlic/rest/api/SecuritySSLCertsApiAction.java 82.35% <100.00%> (ø)

@willyborankin willyborankin force-pushed the config-update-permission branch 3 times, most recently from b86635e to 55b2386 Compare August 29, 2023 20:27
@willyborankin willyborankin marked this pull request as ready for review August 29, 2023 20:52
@willyborankin willyborankin force-pushed the config-update-permission branch from 55b2386 to ea39ed6 Compare September 26, 2023 11:05
Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Instead of
SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION settings
which we use to update security configuration
a new permission was added: `restapi:admin/config/update`.

Signed-off-by: Andrey Pleskach <[email protected]>
@willyborankin willyborankin force-pushed the config-update-permission branch from ea39ed6 to ef0a327 Compare October 3, 2023 15:17
peternied
peternied previously approved these changes Oct 3, 2023
cwperks
cwperks previously approved these changes Oct 3, 2023
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

@willyborankin willyborankin added the backport 2.x backport to 2.x branch label Oct 4, 2023
@willyborankin willyborankin force-pushed the config-update-permission branch from 588814d to 18e7e9f Compare October 4, 2023 18:29
Signed-off-by: Andrey Pleskach <[email protected]>
@willyborankin willyborankin force-pushed the config-update-permission branch from 18e7e9f to 8281a1b Compare October 4, 2023 18:29
@willyborankin
Copy link
Collaborator Author

willyborankin commented Oct 4, 2023

Hmmm BWC tests failed with an exception:

  Caused by: org.opensearch.index.engine.RecoveryEngineException: Phase[1] prepare target for translog failed
»  	at org.opensearch.indices.recovery.RecoverySourceHandler.lambda$prepareTargetForTranslog$22(RecoverySourceHandler.java:629) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.core.action.ActionListener$1.onFailure(ActionListener.java:90) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.core.action.ActionListener$4.onFailure(ActionListener.java:192) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.core.action.ActionListener$6.onFailure(ActionListener.java:311) ~[opensearch-core-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.action.support.RetryableAction$RetryingListener.onFinalFailure(RetryableAction.java:218) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.action.support.RetryableAction$RetryingListener.onFailure(RetryableAction.java:210) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.action.ActionListenerResponseHandler.handleException(ActionListenerResponseHandler.java:75) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.security.transport.SecurityInterceptor$RestoringTransportResponseHandler.handleException(SecurityInterceptor.java:402) ~[?:?]
»  	at org.opensearch.transport.TransportService$ContextRestoreResponseHandler.handleException(TransportService.java:1526) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.transport.InboundHandler.lambda$handleException$3(InboundHandler.java:421) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:849) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
»  	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
»  	at java.lang.Thread.run(Thread.java:833) ~[?:?]

So failures are note related. Something with CCR

@willyborankin
Copy link
Collaborator Author

Created an issue #3456.

@peternied peternied merged commit e4a1b77 into opensearch-project:main Oct 4, 2023
54 of 58 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/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3264-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e4a1b77e9259c019a8ef4728495260468954d6af
# Push it to GitHub
git push --set-upstream origin backport/backport-3264-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

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

@peternied
Copy link
Member

Thanks for creating that issue @willyborankin - I've merged your change bypassing the BWC issues. I've got a PR that should merge shortly that will fix the issue in main.

@willyborankin
Copy link
Collaborator Author

will prepare manual backport

willyborankin added a commit to willyborankin/security that referenced this pull request Oct 4, 2023
…nsearch-project#3264)

Instead of setting
`SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION`
settings to update security configuration using `PATCH` or `PUT` a new
permission was added: `restapi:admin/config/update`.

So far I decided to keep this flag as it is due to a backward
compatibility and log a deprecation message that these settings will be
removed in the future. Maybe it is better to remove it completely.

Besides, added the missed test for `SecurityConfigApiAction`

Signed-off-by: Andrey Pleskach <[email protected]>
(cherry picked from commit e4a1b77)
Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin added a commit to willyborankin/security that referenced this pull request Oct 4, 2023
…nsearch-project#3264)

Instead of setting
`SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION`
settings to update security configuration using `PATCH` or `PUT` a new
permission was added: `restapi:admin/config/update`.

So far I decided to keep this flag as it is due to a backward
compatibility and log a deprecation message that these settings will be
removed in the future. Maybe it is better to remove it completely.

Besides, added the missed test for `SecurityConfigApiAction`

Signed-off-by: Andrey Pleskach <[email protected]>
(cherry picked from commit e4a1b77)
Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin added a commit to willyborankin/security that referenced this pull request Oct 4, 2023
…nsearch-project#3264)

Instead of setting
`SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION`
settings to update security configuration using `PATCH` or `PUT` a new
permission was added: `restapi:admin/config/update`.

So far I decided to keep this flag as it is due to a backward
compatibility and log a deprecation message that these settings will be
removed in the future. Maybe it is better to remove it completely.

Besides, added the missed test for `SecurityConfigApiAction`

Signed-off-by: Andrey Pleskach <[email protected]>
(cherry picked from commit e4a1b77)
Signed-off-by: Andrey Pleskach <[email protected]>
peternied pushed a commit that referenced this pull request Oct 5, 2023
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.

5 participants