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] Built-in secure transports support #12256

Closed
reta opened this issue Feb 8, 2024 · 8 comments · Fixed by #12435
Closed

[FEATURE] Built-in secure transports support #12256

reta opened this issue Feb 8, 2024 · 8 comments · Fixed by #12435
Assignees
Labels
enhancement Enhancement or improvement to existing feature or request Plugins v2.13.0 Issues and PRs related to version 2.13.0 v3.0.0 Issues and PRs related to version 3.0.0

Comments

@reta
Copy link
Collaborator

reta commented Feb 8, 2024

Is your feature request related to a problem? Please describe

OpenSearch core provides the mechanism for a plugins to supply alternative transport protocols (TCP + HTTP) using NetworkPlugin extension point. However, it was designed to support only plain transports without security in mind.

We have the following transport plugins (as of today):

  • netty4 (default, TCP + HTTP)
  • nio (TCP + HTTP)
  • reactor-netty4 (HTTP)

None of them could be configured with secure communication out of the box. The security plugin is trying to close this gap by "hijacking" the netty4 transport and providing own version of it with SSL/TLS configured. Not only nio nor reactor-netty4 are not supported, the general problem is that any other transport needs custom, ad-hoc work in security plugin to function as expected (the effect of that: switching transport from netty4 to any other with security plugin enabled results in degradation to plain transport).

Describe the solution you'd like

Ideally, the secure transport should be provided by the transport plugins, if and when those support secure communication. The role of the security plugin would be in providing the settings (certificates, keystores, etc) so the respective transport engine could be configured using those. Ideally, the change should be non-breaking and portable to 2.x (1.x?) release lines.

The proposal is to extend NetworkPlugin with 2 additional extension points:

public interface NetworkPlugin {
    // ...

    default Map<String, Supplier<Transport>> getSecureTransports(Collection<SecureSettingProvider> ssps, ...) {
        return Collections.emptyMap();
    }

    default Map<String, Supplier<HttpServerTransport>> getSecureHttpTransports(Collection<SecureSettingProvider> ssps, ...)
        return Collections.emptyMap();
    }
}

The existing Plugin could be extended with 1 additional extension point:

public interface Plugin {
    // ...

    public Collection<SecureSettingProvider> getSecureSettingProviders() {
        return Collections.emptyList();
    }
}

The SecureSettingProvider by itself is not limited to secure transport settings only (however initially it will primarily include those) but could go beyond that.

interface SecureSettingProvider {
    public Optional<SSLEngine> buildSecureHttpEngine(HttpServerSettings settings) throws SSLException;
    public Optional<SSLEngine> buildSecureServerTransportEngine(ServerTransportSettings settings) throws SSLException;
    public Optional<SSLEngine> buildSecureClientTransportEngine(ClientTransportSettings settings) throws SSLException;
}

NOTE: the APIs are roughly sketched to highlight the idea and are subject to change.

Related component

Plugins

Describe alternatives you've considered

Keep things as is

Additional context

@reta reta added enhancement Enhancement or improvement to existing feature or request untriaged labels Feb 8, 2024
@reta reta self-assigned this Feb 8, 2024
@reta reta changed the title [Feature Request] Built-in secure transports support [FEATURE] Built-in secure transports support Feb 8, 2024
@reta
Copy link
Collaborator Author

reta commented Feb 8, 2024

@peternied @dblock @andrross would love your thoughts on the matter folks, thank you

@peternied
Copy link
Member

peternied commented Feb 8, 2024

Thanks for writing this up as security in near and dear to my heart - and the way the security plugin engages with the transport layers is less than idea.

I'm not as up to speed as I'd like on the transport system, but if I were to imagine how I'd like the system to operate, there would be no such thing as an insecure transport. OpenSearch would always use secure-able channels - the level of security for these channels would be modifiable via an extension point - ideally we ship with a minimal functional https system for all of the three transports.

@reta Is that kind of approach feasible? I believe we know how to secure netty4, but I don't know how much effort wrapping the other layers would require.

Aside: I know SSL termination is a consideration for folks running OpenSearch - but if we can I'd like to invert our practices to be secure by default and carefully open only what is needed for this use case.

FYI - @davidlago

@andrross
Copy link
Member

andrross commented Feb 8, 2024

Thanks @reta! From your "hijacking" and @peternied's "less than ideal" phrasing it sounds like we have consensus that there is room for improvement in the security plugin integration with the transport layer!

@reta
Copy link
Collaborator Author

reta commented Feb 8, 2024

@reta Is that kind of approach feasible? I believe we know how to secure netty4, but I don't know how much effort wrapping the other layers would require.

In JVM, we do have common denominator - JCE / SSL, so whatever transport it is going to be, it will be using those in some form or another (I think). So I do think it is feasible.

Aside: I know SSL termination is a consideration for folks running OpenSearch - but if we can I'd like to invert our practices to be secure by default and carefully open only what is needed for this use case.

That's the beauty of it, we could offload the transport defaults (aka secure by default) to OpenSearch core at zero cost, the initial plan (as I see it in my head) would be to: move code, keep configs. That's would be non-disruptive to our users.

@dblock
Copy link
Member

dblock commented Feb 9, 2024

I think users do not want an OpenSearch with security turned off by default, they want the opposite. So I am with @peternied and would work backwards from "all transports must be secure", aka evolve the transport interface to one with security enabled by default, then implement passthrough/dummy security as needed. I would also deprecate any non-secure transport and require security in 3.0.

@reta
Copy link
Collaborator Author

reta commented Feb 9, 2024

I think users do not want an OpenSearch with security turned off by default, they want the opposite. So I am with @peternied and would work backwards from "all transports must be secure", aka evolve the transport interface to one with security enabled by default, then implement passthrough/dummy security as needed. I would also deprecate any non-secure transport and require security in 3.0.

Thanks @dblock , I would argue whatever transport is the default and do we want it or not is not the issue I am trying in scope of this feature request. The problem I am trying to address is that there is no way to bundle secure transport (default or not) with core - any attempt to do so would require dedicated lockstep and ad-hoc support in security plugin - this is not sustainable I think (and the current state of the transports confirms that).

We could change the default any time, but we need to fill the gap with providing secure transports in principle.

@dblock
Copy link
Member

dblock commented Feb 9, 2024

We could change the default any time, but we need to fill the gap with providing secure transports in principle.

Agreed. I am just saying that we should do it as a breaking change in 3.0 and require a secure transport.

@peternied
Copy link
Member

[Triage - attendees 1 2 3 4 5 6]
@reta Thanks for filing, looking forward to seeing how this develops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request Plugins v2.13.0 Issues and PRs related to version 2.13.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants