-
Notifications
You must be signed in to change notification settings - Fork 285
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
Allow TransportConfigUpdateAction when security config initialization has completed #3810
Allow TransportConfigUpdateAction when security config initialization has completed #3810
Conversation
…tstrapped Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
…ctions can pass Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java
Outdated
Show resolved
Hide resolved
...nTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3810 +/- ##
==========================================
- Coverage 66.58% 65.17% -1.42%
==========================================
Files 298 298
Lines 21188 21218 +30
Branches 3453 3460 +7
==========================================
- Hits 14109 13828 -281
- Misses 5363 5674 +311
Partials 1716 1716
|
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Looking at the CI failures |
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
5a2ff86
to
4e32d0f
Compare
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.
I'm concerned these test cases have will be flaky due to system performance differences on github runners. Lets make sure the validation is really solid since we are fixing a timing issue based problem.
…arer setup and tearDown Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
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.
I've created a PR [1] that I think helps clean up the logic of this class so there is a set way that the background initialization is triggered - its always bugged me that we start a thread in the constructor but then 'restart it' after init is called. Let me know what you think of this approach.
src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
Show resolved
Hide resolved
…eep security manager state Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
…ternied/refactor-test' into HEAD Signed-off-by: Peter Nied <[email protected]>
@scrawfor99 Could you take another look? |
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.
Great work Craig!
045d4ef
into
opensearch-project:main
The backport to
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-3810-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 045d4ef8a5d9bb8470e03c1b4d7cc68847b986cb
# Push it to GitHub
git push --set-upstream origin backport/backport-3810-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 |
… initialization has completed (opensearch-project#3810) Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> (cherry picked from commit 045d4ef)
… initialization has completed (#3810) (#3927) ### Description - Backport of #3810 from 045d4ef ### Check List - [X] New functionality includes testing - [X] 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: Peter Nied <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Craig Perkins <[email protected]>
… has completed (opensearch-project#3810) Introduces another variable on DynamicConfigFactory called `bgThreadComplete` that behaves differently than the `initialized` variable. `bgThreadComplete` is a flag that signals to TransportConfigUpdateAction that it can start accepting updates. There are 2 ways the security index can be created from scratch: 1. If `plugins.security.allow_default_init_securityindex` is set to **true** it will create the security index and load all yaml files 2. If `plugins.security.allow_default_init_securityindex` is set to **false**, the security index is not created on bootstrap and requires a user to run securityadmin to initialize security. When securityadmin is utilized, the cluster does depend on [TransportConfigUpdateAction](https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/tools/SecurityAdmin.java#L975-L977) to initialize security so there still needs to be an avenue where this can update the config before `initialized` is set to **true** This PR sets `bgThreadComplete` to **false** on node startup and explicitly sets it to **true** once its ok for TransportConfigUpdateAction to start accepting transport actions. In case 2) up above, it can be set to **true** before DynamicConfigFactory is `initialized` so that it can accept requests from securityadmin. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Bug fix - Resolves opensearch-project#3204 - [X] 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: Craig Perkins <[email protected]> Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> (cherry picked from commit 045d4ef)
… initialization has completed (#4115) Backport #3810 to 1.3 Testing performed with the configuration below: [1.3-latest.zip](https://github.com/opensearch-project/security/files/14577043/1.3-latest.zip) This uses a configuration similar to the demo configuration, but with the following modifications that make a reproduction of this error more pronounced: ``` # plugins.security.allow_default_init_securityindex: true plugins.security.unsupported.restapi.allow_securityconfig_modification: true plugins.security.unsupported.load_static_resources: false ``` Steps to test: 1. Start the 3 node cluster 2. Exec into one of the nodes: ` docker exec -it 13-latest-opensearch-node2-1 /bin/bash` 3. Run securityadmin: `cd plugins/opensearch-security/tools && ./securityadmin.sh -cd ../securityconfig/ -icl -nhnv \ -cacert ../../../config/root-ca.pem \ -cert ../../../config/kirk.pem \ -key ../../../config/kirk-key.pem` 4. Exit the remote session: `exit` 5. Start infinite loop of config update in another terminal: `while true;do curl -ai -u "admin:admin" -k -X PATCH https://localhost:9200/_opendistro/_security/api/securityconfig -H 'Content-Type: application/json' -d'[{"op": "replace", "path": "/config/dynamic/authc/basic_internal_auth_domain/transport_enabled", "value": "true"}]'; done` 6. Restart one of the nodes Query the rebooted node and you will not get a response: ``` > curl -XGET https://admin:admin@localhost:9201 -k No response ``` If the cache is loaded correctly you will get a response, but access denied since static resource loading is disabled (this is expected and it means that internal users have been loaded into the cache): ``` > curl -XGET https://admin:admin@localhost:9200 -k {"error":{"root_cause":[{"type":"security_exception","reason":"no permissions for [cluster:monitor/main] and User [name=admin, backend_roles=[admin], requestedTenant=null]"}],"type":"security_exception","reason":"no permissions for [cluster:monitor/main] and User [name=admin, backend_roles=[admin], requestedTenant=null]"},"status":403} ``` --------- Signed-off-by: Craig Perkins <[email protected]>
…config initialization… (#4116) Backport #3810 to opendistro-1.13 Testing performed with the configuration below: [opendistro-1.13-latest.zip](https://github.com/opensearch-project/security/files/14578486/opendistro-1.13-latest.zip) This uses a configuration similar to the demo configuration, but with the following modifications that make a reproduction of this error more pronounced: ``` # opendistro_security.allow_default_init_securityindex: true opendistro_security.unsupported.restapi.allow_securityconfig_modification: true opendistro_security.unsupported.load_static_resources: false ``` Steps to test: 1. Start the 3 node cluster 2. Exec into one of the nodes: ` docker exec -it 13-latest-opensearch-node2-1 /bin/bash` 3. Run securityadmin: `cd plugins/opendistro_security/tools && chmod 777 * && ./securityadmin.sh -cd ../securityconfig/ -icl -nhnv \ -cacert ../../../config/root-ca.pem \ -cert ../../../config/kirk.pem \ -key ../../../config/kirk-key.pem` 4. Exit the remote session: `exit` 5. Start infinite loop of config update in another terminal: `while true;do curl -ai -u "admin:admin" -k -X PATCH https://localhost:9200/_opendistro/_security/api/securityconfig -H 'Content-Type: application/json' -d'[{"op": "replace", "path": "/config/dynamic/authc/basic_internal_auth_domain/transport_enabled", "value": "true"}]'; done` 6. Restart one of the nodes Query the rebooted node and you will not get a response: ``` > curl -XGET https://admin:admin@localhost:9201 -k Unauthorized ``` If the cache is loaded correctly you will get a response, but access denied since static resource loading is disabled (this is expected and it means that internal users have been loaded into the cache): ``` > curl -XGET https://admin:admin@localhost:9200 -k {"error":{"root_cause":[{"type":"security_exception","reason":"no permissions for [cluster:monitor/main] and User [name=admin, backend_roles=[admin], requestedTenant=null]"}],"type":"security_exception","reason":"no permissions for [cluster:monitor/main] and User [name=admin, backend_roles=[admin], requestedTenant=null]"},"status":403} ``` ----- When checking out this branch you can use `mvn clean install -DskipTests` to build a jar. The jar is built in the `target` folder. Signed-off-by: Craig Perkins <[email protected]>
… has completed (opensearch-project#3810) Introduces another variable on DynamicConfigFactory called `bgThreadComplete` that behaves differently than the `initialized` variable. `bgThreadComplete` is a flag that signals to TransportConfigUpdateAction that it can start accepting updates. There are 2 ways the security index can be created from scratch: 1. If `plugins.security.allow_default_init_securityindex` is set to **true** it will create the security index and load all yaml files 2. If `plugins.security.allow_default_init_securityindex` is set to **false**, the security index is not created on bootstrap and requires a user to run securityadmin to initialize security. When securityadmin is utilized, the cluster does depend on [TransportConfigUpdateAction](https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/tools/SecurityAdmin.java#L975-L977) to initialize security so there still needs to be an avenue where this can update the config before `initialized` is set to **true** This PR sets `bgThreadComplete` to **false** on node startup and explicitly sets it to **true** once its ok for TransportConfigUpdateAction to start accepting transport actions. In case 2) up above, it can be set to **true** before DynamicConfigFactory is `initialized` so that it can accept requests from securityadmin. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Bug fix - Resolves opensearch-project#3204 - [X] 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: Craig Perkins <[email protected]> Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]>
Description
Introduces another variable on DynamicConfigFactory called
bgThreadComplete
that behaves differently than theinitialized
variable.bgThreadComplete
is a flag that signals to TransportConfigUpdateAction that it can start accepting updates.There are 2 ways the security index can be created from scratch:
plugins.security.allow_default_init_securityindex
is set to true it will create the security index and load all yaml filesplugins.security.allow_default_init_securityindex
is set to false, the security index is not created on bootstrap and requires a user to run securityadmin to initialize security. When securityadmin is utilized, the cluster does depend on TransportConfigUpdateAction to initialize security so there still needs to be an avenue where this can update the config beforeinitialized
is set to trueThis PR sets
bgThreadComplete
to false on node startup and explicitly sets it to true once its ok for TransportConfigUpdateAction to start accepting transport actions. In case 2) up above, it can be set to true before DynamicConfigFactory isinitialized
so that it can accept requests from securityadmin.Bug fix
Issues Resolved
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.