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 OIDC Common HTTP client #41001

Closed
cescoffier opened this issue Jun 6, 2024 · 8 comments · Fixed by #43148
Closed

Add support for the TLS registry to the OIDC Common HTTP client #41001

cescoffier opened this issue Jun 6, 2024 · 8 comments · Fixed by #43148
Assignees
Milestone

Comments

@cescoffier
Copy link
Member

Description

With the integrated TLS registry, it should be possible to configure TLS using the TLS registry instead of the specific OIDC common 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
@quarkus-bot quarkus-bot bot added the area/oidc label Jun 6, 2024
Copy link

quarkus-bot bot commented Jun 6, 2024

/cc @pedroigor (oidc), @sberyozkin (oidc)

@cescoffier cescoffier removed the area/housekeeping Issue type for generalized tasks not related to bugs or enhancements label Jun 6, 2024
@sberyozkin
Copy link
Member

sberyozkin commented Jun 27, 2024

@cescoffier Thank you for driving it, it is an awesome feature. I noticed this request, I'll try to get to it soon.
FYI, the OIDC common code related to setting up TLS support is covering 4 cases:

  • OIDC server MTLS authentication against OIDC providers
  • OIDC client MTLS authentication against OIDC providers
  • OIDC server Private Key (extracted from the keystore) JWT authentication against OIDC providers
  • OIDC client Private Key (extracted from the keystore) JWT authentication against OIDC providers

If more than one OIDC tenant is used, then each tenant can have its own specific combination, i.e, keystore properties from for ex keycloak TLS registry bucket won't be applicable to other OIDC tenants.

I'll investigate a bit later how to bind it with TLS registry, thanks

@michalvavrik
Copy link
Member

@sberyozkin I just spend some time doing QE verification of TLS registry. Would you like me to implement this? If so, I will look later next week and may come with few questions. Thanks

@michalvavrik
Copy link
Member

michalvavrik commented Sep 2, 2024

I take it as yes @sberyozkin , I'll draft something this week and we can iterate together to get it where you think. Thanks

@michalvavrik michalvavrik self-assigned this Sep 2, 2024
@sberyozkin
Copy link
Member

Thanks @michalvavrik

@michalvavrik
Copy link
Member

Ad tlsRegistry.getDefault().isPresent() && tlsRegistry.getDefault().get().isTlsEnabled() from the issue description. If we were using default TLS registry everywhere if it is enabled, for 4 cases mentioned by Sergey above., it would be confusing users. Probably unintentionally configuring TLS for cases they don't want to. I think we need to use only named TLS config and advise users to explicitly declare io.quarkus.tls.runtime.config.TlsConfig#DEFAULT_NAME if they want to use the default registry. Otherwise we will need a flag everywhere.

Anyway, we don't need to solve this here, I'll write it so that we can make change in one place so we can change this logic easily.

@cescoffier
Copy link
Member Author

@michalvavrik yes, that's what we do for clients - we do not look at the default TLS configuration which is only for "servers".
See https://github.com/quarkusio/quarkus/blob/main/adr/0004-using-the-tls-registry-for-clients.adoc

@michalvavrik
Copy link
Member

Missed that, thank you.

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