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

Add support for the TLS registry to the (reactive) REST client extension #41005

Closed
cescoffier opened this issue Jun 6, 2024 · 12 comments · Fixed by #41153
Closed

Add support for the TLS registry to the (reactive) REST client extension #41005

cescoffier opened this issue Jun 6, 2024 · 12 comments · Fixed by #41153
Assignees
Milestone

Comments

@cescoffier
Copy link
Member

Description

With the integrated TLS registry, it should be possible to configure the reactive REST client (Quarkus Rest client) using the TLS registry instead of the specific configuration.

Implementation ideas

This is the code used for the mailer:

 private void configureTLS(String name, MailerRuntimeConfig config, TlsConfigurationRegistry tlsRegistry, MailConfig cfg,
            boolean globalTrustAll) {
        TlsConfiguration configuration = null;

        // Check if we have a named TLS configuration or a default configuration:
        if (config.tlsConfigurationName.isPresent()) {
            Optional<TlsConfiguration> maybeConfiguration = tlsRegistry.get(config.tlsConfigurationName.get());
            if (!maybeConfiguration.isPresent()) {
                throw new IllegalStateException("Unable to find the TLS configuration "
                        + config.tlsConfigurationName.get() + " for the mailer " + name + ".");
            }
            configuration = maybeConfiguration.get();
        } else if (tlsRegistry.getDefault().isPresent() && tlsRegistry.getDefault().get().isTlsEnabled()) {
            configuration = tlsRegistry.getDefault().get();
        }

       // Apply the configuration
        if (configuration != null) {
            // This part is often the same (or close) for every Vert.x client:
            cfg.setSsl(true);

            if (configuration.getTrustStoreOptions() != null) {
                cfg.setTrustOptions(configuration.getTrustStoreOptions());
            }

           // For mTLS:
            if (configuration.getKeyStoreOptions() != null) {
                cfg.setKeyCertOptions(configuration.getKeyStoreOptions());
            }

            if (configuration.isTrustAll()) {
                cfg.setTrustAll(true);
            }
            if (configuration.getHostnameVerificationAlgorithm().isPresent()) {
               // ACHTUNG HERE - this is protocol specific. The HTTP-based protocols should use HTTPS by default. 
                cfg.setHostnameVerificationAlgorithm(configuration.getHostnameVerificationAlgorithm().get());
            }

            SSLOptions sslOptions = configuration.getSSLOptions();
            if (sslOptions != null) {
                cfg.setSslHandshakeTimeout(sslOptions.getSslHandshakeTimeout());
                cfg.setSslHandshakeTimeoutUnit(sslOptions.getSslHandshakeTimeoutUnit());
                for (String suite : sslOptions.getEnabledCipherSuites()) {
                    cfg.addEnabledCipherSuite(suite);
                }
                for (Buffer buffer : sslOptions.getCrlValues()) {
                    cfg.addCrlValue(buffer);
                }
                cfg.setEnabledSecureTransportProtocols(sslOptions.getEnabledSecureTransportProtocols());

            }

        } else {
           // Mailer specific configuration (very incomplete as you can see:
            boolean trustAll = config.trustAll.isPresent() ? config.trustAll.get() : globalTrustAll;
            cfg.setSsl(config.ssl);
            cfg.setTrustAll(trustAll);
            applyTruststore(config, cfg);
        }
    }
@cescoffier cescoffier added the area/housekeeping Issue type for generalized tasks not related to bugs or enhancements label Jun 6, 2024
Copy link

quarkus-bot bot commented Jun 6, 2024

/cc @geoand (rest-client)

@geoand
Copy link
Contributor

geoand commented Jun 6, 2024

@cescoffier if you are not already working on this, I'll take it

@cescoffier cescoffier removed the area/housekeeping Issue type for generalized tasks not related to bugs or enhancements label Jun 6, 2024
@cescoffier cescoffier moved this from Todo to In Progress in WG - Enhanced TLS support Jun 6, 2024
@cescoffier
Copy link
Member Author

Go ahead @geoand !

@geoand
Copy link
Contributor

geoand commented Jun 6, 2024

👍🏼

@geoand
Copy link
Contributor

geoand commented Jun 6, 2024

I think I need to wait for #40990 to be merged

@geoand
Copy link
Contributor

geoand commented Jun 6, 2024

Furthermore, I don't see how I can access the configured keystore and trustore password. I need those due to how the programmatic API works

@cescoffier
Copy link
Member Author

That's a good point. Can't you pass the Keystore objects directly? The problem passing password is about work duplication.

The registry already opened the keystore, check the passwords (plural), validity, aliases... If we pass the password, there is a good chance you will redo part of that expensive work.

@geoand
Copy link
Contributor

geoand commented Jun 12, 2024

I'll have to take another look.

@cescoffier
Copy link
Member Author

BTW, what did you need from the mailer PR? I removed all the "additions" I made to the TLS registry from that PR as they were not required.

We can imagine keeping the config objects around. I'm only worried about the cost of maintaining references to these objects.

@geoand
Copy link
Contributor

geoand commented Jun 12, 2024

I don't remember what I needed really, I'll have to get back to this and see

@geoand
Copy link
Contributor

geoand commented Jun 12, 2024

I don't remember what I needed really, I'll have to get back to this and see

Looks like since we decided to not use the default configuration, I don't need anything else.

Can't you pass the Keystore objects directly? The problem passing password is about work duplication

It seems that I'll have to add a few things, but it should be doable.

@geoand
Copy link
Contributor

geoand commented Jun 12, 2024

#41153 add the necessary support

geoand added a commit to geoand/quarkus that referenced this issue Jun 12, 2024
geoand added a commit that referenced this issue Jun 12, 2024
Introduce support for the TLS Registry in the REST Client
@github-project-automation github-project-automation bot moved this from In Progress to Done in WG - Enhanced TLS support Jun 12, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 12, 2024
@gsmet gsmet modified the milestones: 3.13 - main, 3.12.0 Jun 18, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jun 18, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants