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

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

Open
cwperks opened this issue Aug 16, 2023 · 4 comments
Labels
bug Something isn't working good first issue These are recommended starting points for newcomers looking to make their first contributions. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@cwperks
Copy link
Member

cwperks commented Aug 16, 2023

Since OpenSearch 2.0, support for the transport client has been removed. The transport client was removed from the security plugin in this PR: #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.

@cwperks cwperks added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 16, 2023
@stephen-crawford
Copy link
Contributor

stephen-crawford commented Aug 21, 2023

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

@stephen-crawford stephen-crawford added good first issue These are recommended starting points for newcomers looking to make their first contributions. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 21, 2023
@prabhask5
Copy link
Contributor

@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?

@stephen-crawford
Copy link
Contributor

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
Copy link
Member Author

cwperks commented Nov 13, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue These are recommended starting points for newcomers looking to make their first contributions. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants