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

[3x] TlsManager backport #7650

Merged

Conversation

trentjeff
Copy link
Member

@trentjeff trentjeff commented Sep 21, 2023

This addresses #7544 (base issue #7279) - TlsManager w/ an OCI Certificates service integration for 3.x.

There are some notable changes between this version and the 4.x version:

  • The TlsManager contract has a few variations - the additional methods are marked deprecated.
  • ServiceLoader instead of InjectionServices.
  • No example module here in 3.x.

@trentjeff trentjeff added the 3.x Issues for 3.x version branch label Sep 21, 2023
@trentjeff trentjeff added this to the 3.x milestone Sep 21, 2023
@trentjeff trentjeff self-assigned this Sep 21, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 21, 2023
@trentjeff trentjeff marked this pull request as ready for review September 22, 2023 03:21
Copy link
Member

@Verdent Verdent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implementation currently does not work. It would not reload TLS right now, if I am not mistaken.

I will also take a closer look into whether we actually need to have those init methods at all (for example on TlsManager) or whether it could be done without it. This will come in my next review.

* @return a new trust manager factory trusting all
*/
protected TrustManagerFactory trustAllTmf() {
return new TrustAllManagerFactory();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not need to return new instance every time, since if I am not mistaken, it does not keep any internal state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, sure, but it only is called once per config - so it really doesn't matter. And besides, this is the same code in v4.

* @param tlsConfig TLS configuration
* @return secure random
*/
protected SecureRandom secureRandom(WebServerTls tlsConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameter is never used

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same iface from v4 backported (except the type in v4 is TlsConfig and not WebServerTls. I kept it this way to be uniform, and also in the anticipation we do want to make it configurable as it is in v4.

Comment on lines +130 to +132
if (explicitSslContext != null) {
return explicitSslContext;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Since sslContext is now moved to the TlsManager, everything regarding SSLContext should be taken from there. The reason why I am saying that is, that if SSL gets reloaded, but we had some explicit SSLContext at the beginning, this method will not be returning updated SSLContext. On the other hand, it might be what you want, but it would be strange since sslContext method on TlsManager would be returning updated version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone explicitly sets the SSLContext programmatically, we want to take it as-is. The ConfiguredTlsManager should not be redefined from config as that would give a higher weight to config over the programmatic approach. What I can do is log a warning when programmatic is set and the OciTlsManager is being used - because that is nonsensical.

@@ -163,6 +163,9 @@ class NettyWebServer implements WebServer {
.childHandler(childHandler);

bootstraps.put(name, bootstrap);

// subscribe to updates
soConfig.tls().ifPresent(tlsConfig -> updateTls(tlsConfig, name));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not subscribe to updates. This actually registers yet another tls and overwrites the one created few lines above :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See last commit.

@trentjeff
Copy link
Member Author

I think this implementation currently does not work. It would not reload TLS right now, if I am not mistaken.

I will also take a closer look into whether we actually need to have those init methods at all (for example on TlsManager) or whether it could be done without it. This will come in my next review.

Should be all addressed now in the last couple of commits.

Copy link
Member

@Verdent Verdent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I found right now, are more less minor changes.

However, I would definitely recommend to check whether this new approach with manager will work correctly also when updateTls method is used. This method was designed to work with completely new WebServerTls.
Basically what I am asking about is:

  1. to check whether this method is compatible with this new approach
  2. try to use it with combination of this new way of reloading

@trentjeff
Copy link
Member Author

What I found right now, are more less minor changes.

However, I would definitely recommend to check whether this new approach with manager will work correctly also when updateTls method is used. This method was designed to work with completely new WebServerTls. Basically what I am asking about is:

  1. to check whether this method is compatible with this new approach
  2. try to use it with combination of this new way of reloading

Here is a breakdown of the two ways to update today:

The updateTls() flow:

summary: `NettyWebServer::updateTls` -> `NettyWebServer::createSslContext` -> `TlsManager:sslContext()` -> "update httpInitializer"

code snippet details: (below)
updateTls(WebServerTls tls, String socketName) {
    SslContext context = createSslContext(tls);
    httpInitializer.updateSslContext(context);
}

SslContext createSslContext(WebServerTls webServerTls) {
    SSLContext context = webServerTls.sslContext();
    return createSslContext(webServerTls, context);
}

SslContext createSslContext(WebServerTls webServerTls, SSLContext context) {
    Collection<String> enabledProtocols = webServerTls.enabledTlsProtocols();
    Set<String> cipherSuite = webServerTls.cipherSuite();
    return new JdkSslContext(
            context, false, cipherSuite.isEmpty() ? null : cipherSuite,
            IdentityCipherSuiteFilter.INSTANCE, UpgradeManager.alpnConfig(),
            webServerTls.clientAuth().nettyClientAuth(), protocols, false);
}

The manager::reload() flow:

summary: `TlsManager::reload` -> `NettyWebServer::createSslContext` -> `TlsManager::sslContext()` -> "update httpInitializer"

code snippet details: (below)
Optional<WebServerTls> webServerTls = soConfig.tls();
webServerTls
        .map(WebServerTls::manager)
        .ifPresent(manager -> manager.subscribe(newCtx -> {
            SslContext newSslCtx = createSslContext(webServerTls.get(), newCtx);
            childHandler.updateSslContext(newSslCtx);
        }));

Analysis

  • Both ways ultimately rely on TlsManager#sslContext() to render the new SSLContext instance.
  • Where there may be a discrepancy here lies in whether the rest of the configuration behind WebServerTls can mutate - if that is assumed to be able to mutate, then the TlsManager will need to be "in the loop" to know about that config change as well since it relies on it in both V3 and V4. In V3 this is sessionCacheSize and sessionTimeoutSecs, or perhaps other configuration based upon the details of the implementation of TlsManager. In V4, it is these two plus protocol, provider, internalKeystoreType, internalKeyStoreProvider, and secureRandom. Here is a snipped coming from the manager code in V3 that uses the rest of WebServerTls for these two configurations:
    protected void configureAndSet(WebServerTls tlsConfig,
                                   SSLContext sslContext) {
        SSLSessionContext serverSessionContext = sslContext.getServerSessionContext();
        if (serverSessionContext != null) {
            int sessionCacheSize = tlsConfig.sessionCacheSize();
            if (sessionCacheSize > 0) {
                serverSessionContext.setSessionCacheSize(sessionCacheSize);
            }
            int sessionTimeoutSecs = tlsConfig.sessionTimeoutSeconds();
            if (sessionTimeoutSecs > 0) {
                serverSessionContext.setSessionTimeout(sessionTimeoutSecs);
            }
        }
        this.sslContext = sslContext;
    }

However, in V4 we have "reloadable key stores and trust stores", and that is what tls managers are mutating while preserving the SSLContext instance to be steady. Its in V3 where we don't do it this way (i.e., no single SSLContext with any notion of reloadable components, etc.).

So the net summary here is that we need to either assume the configuration for these two configuration elements (session cache size and timeout) must remain steady an immutable, or else we will need to do more in this PR to allow for a new WebServerTls instance to come into the manager post-startup.

My advice is to assume these configuration attributes are assumed to be immutable and everything is fine as it is now.

@trentjeff trentjeff merged commit 58a859e into helidon-io:helidon-3.x Sep 27, 2023
12 checks passed
@barchetta barchetta removed this from the 3.x milestone Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants