pulsar-rs icon indicating copy to clipboard operation
pulsar-rs copied to clipboard

Support for disabling TLS entirely

Open cstrahan-blueshift opened this issue 3 years ago • 2 comments

I have to connect to a Pulsar cluster which advertises broker_service_url_tls: Some("pulsar+ssl://pulsar-brokerN.HOST_REDACTED:6651"), but doesn't actually have a TLS listener on that port, and thus connection fails like so:

ServiceDiscovery(Connection(Tls(Ssl(Error { code: ErrorCode(5), cause: None }, X509VerifyResult { code: 0, error: "ok" }))))

If I neuter the code that chooses the TLS address over the other, then everything works just fine.

Would you welcome an option somewhere along the lines of use_tls: boolean? It looks like there's precedent for this, from some searching around:

See clientBuilder.enableTls(authConfig.isUseTls()):

    private static PulsarClient createPulsarClient(String pulsarServiceUrl, AuthenticationConfig authConfig)
            throws PulsarClientException {
        ClientBuilder clientBuilder = null;
        if (isNotBlank(pulsarServiceUrl)) {
            clientBuilder = PulsarClient.builder().serviceUrl(pulsarServiceUrl);
            if (authConfig != null) {
                if (isNotBlank(authConfig.getClientAuthenticationPlugin())
                        && isNotBlank(authConfig.getClientAuthenticationParameters())) {
                    clientBuilder.authentication(authConfig.getClientAuthenticationPlugin(),
                            authConfig.getClientAuthenticationParameters());
                }
                clientBuilder.enableTls(authConfig.isUseTls());
                clientBuilder.allowTlsInsecureConnection(authConfig.isTlsAllowInsecureConnection());
                clientBuilder.enableTlsHostnameVerification(authConfig.isTlsHostnameVerificationEnable());
                clientBuilder.tlsTrustCertsFilePath(authConfig.getTlsTrustCertsFilePath());
            }
            return clientBuilder.build();
        }
        return null;
    }

or see .useTls(useTls):

  @Override
  void runCmd() throws Exception {
    // merge deprecated args with new args
    mergeArgs();
    CmdFunctions.startLocalRun(FunctionConfigUtils.convert(functionConfig, classLoader), functionConfig.getParallelism(),
        instanceIdOffset, brokerServiceUrl, stateStorageServiceUrl,
        AuthenticationConfig.builder().clientAuthenticationPlugin(clientAuthPlugin)
            .clientAuthenticationParameters(clientAuthParams).useTls(useTls)
            .tlsAllowInsecureConnection(tlsAllowInsecureConnection)
            .tlsHostnameVerificationEnable(tlsHostNameVerificationEnabled)
            .tlsTrustCertsFilePath(tlsTrustCertFilePath).build(),
        userCodeFile, admin);
  }
}

cstrahan-blueshift avatar Oct 29 '21 23:10 cstrahan-blueshift

That could be an option, but I'm not a fan of making TLS easy to disable. Why aren't you directly using a pulsar:// URL for connection? Or is it the proxy or one of the brokers that gives you the invalid URL? It makes sense for the client to choose the TLS version if both URLs are provided

Geal avatar Nov 06 '21 12:11 Geal

Why aren't you directly using a pulsar:// URL for connection?

I am using a pulsar:// URL, but the the broker is giving us the invalid TLS address. We have producers/consumers using the official C++ library (libpulsar) (and the Java lib as well, IIUC) that are working fine; maybe that's because TLS requires explicit enabling? I've asked our Ops to look into it, but we're not yet sure what's going on precisely.

It makes sense for the client to choose the TLS version if both URLs are provided

I agree, in principle.

[..] I'm not a fan of making TLS easy to disable.

Ditto. I do wonder, though, if other users might run into precisely the same problem we're having; keeping TLS enabled by default (when an address is advertised by the proxy/broker), with some way to explicitly opt-out might be a reasonable compromise?

FWIW, I believe we're on Pulsar 2.7.1, and upgrading to 2.8.1 shortly.

cstrahan-blueshift avatar Nov 12 '21 22:11 cstrahan-blueshift