opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
183 stars 266 forks source link

`transport_enabled` setting on an auth domain and authorizer may be unnecessary after transport client removal #3191

Open cwperks opened 11 months ago

cwperks commented 11 months ago

Since OpenSearch 2.0, support for the transport client has been removed. The transport client was removed from the security plugin in this PR: https://github.com/opensearch-project/security/pull/1701

My understanding of transport_enabled is that it is used to enable an auth domain for authentication on the transport layer. There is a separate setting called http_enabled to enable the auth domain on the REST layer. This setting also appears to be applicable to authorizers in the authz section.

Below is an example of a basic auth entry in the authc: section of the security plugin's config.yml file from the demo configuration:

basic_internal_auth_domain:
    description: "Authenticate via HTTP Basic against internal users database"
    http_enabled: true
    transport_enabled: true
    order: 4
    http_authenticator:
      type: basic
      challenge: true
    authentication_backend:
      type: intern

The transport_enabled setting may not be needed here after the TransportClient's removal.

First: Determine if it is safe to remove values from settings. Second: If safe to remove, remove. If not, update documentation.

scrawfor99 commented 10 months ago

[Triage] Hi @cwperks, thank you for filing this issue. The tasks at the bottom are the required completion criteria so marking as triaged.

prabhask5 commented 8 months ago

@cwperks @scrawfor99 I'm interested in working on this issue. How would one begin to determine if this change is safe? Does this just mean the tests work correctly? Also is the config.yml file the only file where changes need to be made?

scrawfor99 commented 8 months ago

Hi @prabhask5, thanks for checking in on this issue. As Craig mentioned, this setting is a pretty old one based on enabling/disabling authentication on the transport layer. That being said, we don't have the Transport Client anymore. The two settings Craig mentioned are defined here:

| `http_enabled` | Enables or disables authentication on the REST layer. Default is `true` (enabled). |
| `transport_enabled` | Enables or disables authentication on the transport layer. Default is `true` (enabled). |

To test whether you can remove the transport setting, there are two options. First, you can try swapping it back and forth and on the config.yml settings and see what happens. If you get the same behavior with it on as with it off it is a good indicator it may not matter.

The second way to determine whether it is required is do a code search for where the setting is used and determine the extent to which it is called or checked. This should either be done instead of the first option or after having completed the first option. By checking the code use, you can get the best idea of whether the code is actually necessary.

You are right that the only changes needed here should be in the config.yml file.

cwperks commented 8 months ago

These values are also displayed on the UI. Looks like they can be deleted from there as well.

Screenshot 2023-11-13 at 4 40 54 PM