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

[FEATURE] Implement SecureAuxTransportSettingsProvider for auxiliary transports #5104

Open
finnegancarroll opened this issue Feb 11, 2025 · 5 comments
Labels
enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized

Comments

@finnegancarroll
Copy link

finnegancarroll commented Feb 11, 2025

Is your feature request related to a problem?
Recently added to core, auxiliary transports are client/server transports which run in parallel to the existing rest api. To implement TLS for these transports they need access to a SecureTransportSettingsProvider as provided by the security plugin.

What solution would you like?
I would propose the security plugin provide a SecureAuxTransportSettingsProvider to be consumed by NetworkPlugins which implement getAuxTransports() and configured with settings distinct from its http counterpart (plugins.security.ssl.aux.pemkey_filepath, ...).

What alternatives have you considered?
Have AuxTransports consume the SecureHttpTransportSettingsProvider. I think it makes sense to create an entirely new SettingsProvider because:

  • Users may want to have separate configurations for their rest and aux client/server transports
  • Future AuxTransports may not be http based
  • SecureHttpTransportSettingsProvider is implicitly a rest based implementation as HttpServerTransport contains some rest specific objects.
    *This could probably be solved with a small refactor?

Additional context
opensearch-project/OpenSearch#16905
opensearch-project/OpenSearch#16787

@finnegancarroll finnegancarroll added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Feb 11, 2025
@finnegancarroll
Copy link
Author

Hi @rishabhmaurya,
Can you check and make sure I'm not conflicting with your implementation in #5011? Does this issue only relate to the transport layer of your arrow flight bootstrap pr?

@rishabhmaurya
Copy link

@finnegancarroll it depends on how we are planning to change SecureTransportSettingsProvider to build Netty's SslContext for aux transports.
In #5011, I have added optional parameters (which will be provided by security plugin eventually), which can be used to build SslContext.

Do you see any concerns?

@finnegancarroll
Copy link
Author

finnegancarroll commented Feb 11, 2025

It seems to me there are two distinct problems:

  1. The SecureTransportSettingsProvider interface only provides a javax.net.ssl SSLEngine. Transport implementations which abstract away creation of SSLEngines (gRPC/flight) might find it more convenient to consume from the SecureTransportParameters you've laid out and build the ssl context/engines themselves.
  2. Each supported transport type currently consumes from its own single settings provider. As such, aux transports currently must re-use the certs/keys configured under plugins.security.ssl.transport or plugins.security.ssl.http.

I like your solution to problem 1 and would like to build on it to address problem 2 by adding a new SecureAuxTransportSettingsProvider which also includes the SecureTransportParameters interface. (and seperate configuration settings, plugins.security.ssl.aux...)

I think these two solutions are complimentary but let me know your thoughts are about this path forward.

@rishabhmaurya
Copy link

@finnegancarroll sounds good to me. Do we currently have any additional settings which aux transport might need? If not, we can defer 2.

@finnegancarroll
Copy link
Author

finnegancarroll commented Feb 12, 2025

@finnegancarroll sounds good to me. Do we currently have any additional settings which aux transport might need? If not, we can defer 2.

Nothing comes to mind. Sounds good.
I think it will be necessary for 3.0 to support protobuf/gRPC release but not immediate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized
Projects
None yet
Development

No branches or pull requests

2 participants