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

[Backport 1.3] Allow TransportConfigUpdateAction when security config initialization has completed #4115

Merged
merged 12 commits into from
Mar 13, 2024

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Mar 12, 2024

Backport #3810 to 1.3

Testing performed with the configuration below:

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}

… 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)
Signed-off-by: Craig Perkins <[email protected]>
…testDelayInSecurityIndexInitialization

Signed-off-by: Craig Perkins <[email protected]>
@cwperks cwperks changed the title [Backport 1.3] [Backport 1.3] Allow TransportConfigUpdateAction when security config initialization has completed Mar 12, 2024
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]>
Signed-off-by: Craig Perkins <[email protected]>
@cwperks cwperks merged commit 780b4f0 into opensearch-project:1.3 Mar 13, 2024
19 checks passed
@peternied
Copy link
Member

@cwperks Why couldn't the integration tests be backported?

@cwperks
Copy link
Member Author

cwperks commented Mar 19, 2024

@cwperks Why couldn't the integration tests be backported?

I ran into issues backporting to 1.3 with the integrationTest framework. I may have misremembered which integration tests are running on the 1.3 line and thought it was only the ResourceFocusedTests of the integrationTest framework.

I'm taking another look at it now and currently running into an issue with the TransportClient in SecurityAdmin.

@cwperks
Copy link
Member Author

cwperks commented Mar 19, 2024

@peternied I raised a PR for adding the integ tests to 1.3: #4134

The biggest issue was the port for the security admin tests. In 1.3 its the transport port, but in 2.x its the http port.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants