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] Support additional params in SecureTransportSettingsProvider to enable building of SslContext outside of security plugin #5011

Open
rishabhmaurya opened this issue Jan 6, 2025 · 7 comments
Assignees
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@rishabhmaurya
Copy link

Is your feature request related to a problem?
The netty's io.netty.handler.ssl.SslContext is needed by Flight server being introduced in opensearch-project/OpenSearch#16962. Today, it cannot be built outside of security plugin as SecureTransportSettingsProvider doesn't expose it.

What solution would you like?
Provide a way for consumers of SecureTransportSettingsProvider to build SslContext.
Since its a netty dependency which we don't want to add to server module thus we cannot build it in security plugin and expose it using SecureTransportSettingsProvider. Instead, expose all parameters needed to build SslContext to its consumer (plugins & modules) to build it directly.

What alternatives have you considered?
A clear and concise description of any alternative solutions or features you've considered.

Do you have any additional context?
Add any other context or screenshots about the feature request here.

@rishabhmaurya rishabhmaurya added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jan 6, 2025
@rishabhmaurya rishabhmaurya self-assigned this Jan 6, 2025
@cwperks cwperks added 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 Jan 13, 2025
@cwperks
Copy link
Member

cwperks commented Jan 13, 2025

[Triage] Thank you for filing this issue @rishabhmaurya . Can you elaborate on the parameters you would like to add and configure in SecureTransportSettingsProvider?

@rishabhmaurya
Copy link
Author

rishabhmaurya commented Jan 20, 2025

@cwperks I will raise a PR for it, thank you. I will be adding following parameters to SecureTransportSettingsProvider.SecureTransportParameters. Please let me know if you have any concerns exposing them?

interface SecureTransportParameters {
        boolean dualModeEnabled();

        KeyManagerFactory keyManagerFactory();

        String sslProvider();

        String clientAuth();

        Iterable<String> protocols();

        Iterable<String> cipherSuites();

        TrustManagerFactory trustManagerFactory();
    }

@finnegancarroll
Copy link

One concern with using this SecureTransportParameters interface is hot swapping may become more difficult. Transports consuming these params would need to somehow detect changes to security settings so they know when to rebuild the ssl context. We could maybe provide them with some method/callback to detect changes but then we additionally need to worry about "draining" existing connections and reloading the new context (which may require a new instance of the client/server transport be built).

On the other hand hot swapping with SecureHttpTransportSettingsProvider is seamless since it provides a new SSLEngine for every connection so the ssl context can be swapped without the client/server transport ever having to know. The downside for this approach being we don't allow our transport to manage the ssl context directly which maybe gives us some benefits from caching and resource sharing.

Considering the above I think providing an SSLEngine directly will be more convenient. Please let me know what you think. @cwperks @rishabhmaurya

@rishabhmaurya
Copy link
Author

@finnegancarroll can we set SSLEngine directly while building from io.grpc.netty.NettyServerBuilder?

@rishabhmaurya
Copy link
Author

rishabhmaurya commented Feb 21, 2025

If i understand it right - for hot reload, we need to see how can we 1) build the SSLEngine from SslContext and create new SSLHandler 2) replace ChannelHandlerContext pipeline with this newly built SSLHandler. So we probably don't have to worry about draining the connections.

@finnegancarroll
Copy link

@finnegancarroll can we set SSLEngine directly while building from io.grpc.netty.NettyServerBuilder?

I think we can take this route but NettyServerBuilder doesn't expose any obvious api to insert an SSLEngine. We could possibly inject a custom channel handler which pulls a fresh SSLEngine from the security plugin on each connection. I'm unsure of the complexity of this approach.

To keep the benefits of NettyServerBuilder managing the ssl context for us we could have the security plugin return a "ReloadableSSLContext" which looks something like this:

public class ReloadableSSLContext extends SslContext {
    @Override
    public SSLEngine newEngine(ByteBufAllocator alloc) {
        return buildSecureServerEngine();
    }

... // rest of io.grpc.SslContext interface

}

@rishabhmaurya
Copy link
Author

@finnegancarroll I like the ReloadableSSLContext approach. Let's confirm if it works with hot reload. Just make sure we don't end up adding SslContext or netty dependency directly in server. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants