[fix] Fix hostname verification

[izumo27]

Motivation

If ValidateHostName is set to true, handshake always fails.

INFO  [] ClientConnection:375 | [ -> ] Connected to broker
ERROR [] ClientConnection:463 | [ -> ] Handshake failed: certificate verify failed
INFO  [] ClientConnection:1560 | [ -> ] Connection closed

Modifications

• Verify serviceUrl.host(), not physicalAddress.
  • physicalAddress is serviceUrl, which contains protocol (e.g. pulsar+ssl) and port number.
• Use ssl::stream::set_verify_callback instead of ssl::context::set_verify_callback.
  • Verification should work with ssl::context::set_verify_callback, but somehow it doesn't work.

Verifying this change

• Make sure that the change passes the CI checks.

Documentation

• 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)

[izumo27]

Rebased master

[izumo27]

@merlimat PTAL, thanks

[BewareMyPower]

After I reverted your changes in ClientConnection.cc, these tests could still pass. Could you explain it?

$ ./tests/pulsar-tests --gtest_filter='AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile'
Note: Google Test filter = AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AuthPluginTest
[ RUN      ] AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile
2022-12-19 19:55:54.972 INFO  [140386200329920] ClientConnection:190 | [<none> -> pulsar+ssl://localhost:6651] Create ClientConnection, timeout=10000
2022-12-19 19:55:54.973 INFO  [140386200329920] ConnectionPool:97 | Created connection for pulsar+ssl://localhost:6651
2022-12-19 19:55:54.974 INFO  [140386197206784] ClientConnection:387 | [127.0.0.1:50090 -> 127.0.0.1:6651] Connected to broker
2022-12-19 19:55:54.982 ERROR [140386197206784] ClientConnection:487 | [127.0.0.1:50090 -> 127.0.0.1:6651] Handshake failed: certificate verify failed
2022-12-19 19:55:54.982 INFO  [140386197206784] ClientConnection:1599 | [127.0.0.1:50090 -> 127.0.0.1:6651] Connection closed with ConnectError
2022-12-19 19:55:54.982 ERROR [140386197206784] ClientImpl:183 | Error Checking/Getting Partition Metadata while creating producer on persistent://private/auth/testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile -- ConnectError
2022-12-19 19:55:54.982 INFO  [140386197206784] ClientConnection:268 | [127.0.0.1:50090 -> 127.0.0.1:6651] Destroyed connection
[       OK ] AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile (13 ms)

[izumo27]

After I reverted your changes in ClientConnection.cc, these tests could still pass.

testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile tests whether the result of createProducer is a connection error when certsFile is missing. Hostname verification of master always fails, so this test could still pass.

This PR fixes testTlsDetectPulsarSslWithHostNameValidation. When certsFile is set corretly, the result of createProducer should be ok, not a connection error.
https://github.com/apache/pulsar-client-cpp/blob/v3.1.0/tests/AuthPluginTest.cc#L152
https://github.com/apache/pulsar-client-cpp/pull/126/files#diff-43a1460bfe2216170e3d7a5f634e8d0422b78946b018f81118dc275cbba2d04fR152

[RobertIndie]

Could you also add a test case that demonstrates the failed case of hostname verification? Like here: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticationTlsHostnameVerificationTest.java#L140

[izumo27]

@RobertIndie

Could you also add a test case that demonstrates the failed case of hostname verification?

The test requires changing configurations of a broker, but mocking is difficult.
I don't think it is a good idea to set up another standalone cluster.
Do you have any ideas?

[BewareMyPower]

You can change the current config test-conf/standalone-ssl.conf directly. If it can be configured at namespace level, you can create a separated namespace for tests.

If it requires changing configurations at broker level and it will affect other tests, I think setting up another standalone is okay.

[izumo27]

I added tests for invalid broker using another standalone.

[BewareMyPower]

The fix LGTM, but could you describe how to generate this certificates? It would not block this PR but I think it could be improved to make tests clear. It seems that when hn-verification/cacert.pem is used, setValidateHostName must be set true.

[izumo27]

could you describe how to generate this certificates?

The certificates I added for man-in-the-middle attacks is the same as the certificates in pulsar repository.
https://github.com/apache/pulsar/tree/v2.11.0/pulsar-broker/src/test/resources/authentication/tls/hn-verification
This is also true for other existing certificates in pulsar-client-cpp repository.