[fix][admin] Add SNI header when tlsHostnameVerification is not enabled

[fantapsody]

Fixes #16416

Motivation

The pulsar admin client and the HTTP lookup service don't add an SNI header when tlsHostnameVerification is not enabled.

Modifications

The async-http-client doesn't split the flag for SNI header and hostname verification, so I added a new SSL engine factory to set the SNI header.

Verifying this change

• Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

Documentation

Check the box below or label this PR directly.

Need to update docs?

• doc-required
  (Your PR needs to update docs and you will update later)

• doc-not-needed
  (Please explain why)

• doc
  (Your PR contains doc changes)

• doc-complete
  (Docs have been already added)

[fantapsody]

@michaeljmarshall PTAL

[fantapsody]

/pulsarbot run-failure-checks

[michaeljmarshall]

The solution looks good to me, but it needs one minor fix (see comment).

The underlying issue is here:

  public SSLEngine newSslEngine(AsyncHttpClientConfig config, String peerHost, int peerPort) {
    SSLEngine sslEngine =
      config.isDisableHttpsEndpointIdentificationAlgorithm() ?
        sslContext.newEngine(ByteBufAllocator.DEFAULT) :
        sslContext.newEngine(ByteBufAllocator.DEFAULT, domain(peerHost), peerPort);
    configureSslEngine(sslEngine, config);
    return sslEngine;
  }

https://github.com/AsyncHttpClient/async-http-client/blob/c959fa0483adb4a71fbccc5be94d8b6faa4f74be/client/src/main/java/org/asynchttpclient/netty/ssl/DefaultSslEngineFactory.java#L63-L70

Specifically, when the engine is created without the hostname, the engine does not have the hostname to then pass as a header, which breaks the SNI routing.

An alternative solution could have overridden the newSslEngine method, but that seems equivalent to this one, so no need to change this PR.

Additionally, I was concerned that this would affect the case where hostname verification is not enabled and SNI is not used, but I think we should be fine, since this is just a header and we pass these headers when creating the client ssl engines for the Pulsar Protocol connections.

Thanks for opening the PR @fantapsody, I didn't understand the issue originally, but the PR helped me see it.

[nodece]

I wonder can we use the allowTlsInsecureConnection() instead of the enableTlsHostnameVerification()?

[fantapsody]

@michaeljmarshall Thanks for the feedback, please take another look.

[fantapsody]

I wonder can we use the allowTlsInsecureConnection() instead of the enableTlsHostnameVerification()?

@nodece I think they do different validations.

[nodece]

LGTM

[michaeljmarshall]

LGTM. Thanks @fantapsody!

[michaeljmarshall]

@fantapsody - can you please rebase this PR so that it picks up the latest CI fixes? There were recent changes that we need to capture before we can merge this PR. Thanks.

[fantapsody]

@fantapsody - can you please rebase this PR so that it picks up the latest CI fixes? There were recent changes that we need to capture before we can merge this PR. Thanks.

Done. @michaeljmarshall