confluentinc / librdkafka

The Apache Kafka C/C++ library
Other
284 stars 3.15k forks source link

Allow TLS hostname overrides #3321

Open crivera-fastly opened 3 years ago

crivera-fastly commented 3 years ago

Description

It would be useful to have a way to override the hostname used for TLS hostname verification. For reference, the Go TLS stack provides a ServerName field for this purpose: tls - The Go Programming Language. This would allow clients to specify a trusted name for scenarios that would otherwise require modifications to the certificates (DNS SANs, IP SANs, etc.). If I’m reading the code correctly this would require exposing a new configuration value (maybe ssl.verification.hostname?) and using it in rd_kafka_transport_ssl_set_endpoint_id() if it’s set. I’m skipping the checklist since this is an enhancement request, but I’m happy to provide more context for my use case if necessary.

Checklist

Please provide the following information:

edenhill commented 3 years ago

If it was a configuration property it would be applied to all broker connections, is that useful?

crivera-fastly commented 3 years ago

Yes. My use case involves a set of brokers running in the cloud with some clients that connect from within the vpc and some clients that connect from outside of the vpc. The internal and external clients use different broker hostnames. Getting both cases to work currently requires adding SANs to the certs. The brokers (foo1.bar.com, foo2.bar.com, …) serve a cert for foo.bar.com. This setting would let me configure all clients to accept the foo.bar.com cert.

edenhill commented 3 years ago

Okay, thanks for explaining your use-case.

I think there are two approaches here; the simple one is adding the proposed config property, which works well for use-cases similar to yours. The other approach is adding a callback to query the application for a per-broker-connection name, which is similar to what is being worked on in https://github.com/edenhill/librdkafka/pull/3180 .

It would be fine with an incremental approach, starting with the config property and then if there is demand we could do the other, but it would be good to understand if SNI should be part of the #3180 API. cc @KJTsanaktsidis

KJTsanaktsidis commented 3 years ago

Its awkward, but the rd_kafka_conf_set_ssl_cert_verify_cb API might already do what you need -this lets you pass a callback that lets you delegate SSL certificate verification to your code instead of what OpenSSL would normally do.

Here's an example of using rd_kafka_conf_set_ssl_cert_verify_cb to delegate the TLS cert verification to the Golang TLS stack instead of OpenSSL's, for example:

https://github.com/zendesk/confluent-kafka-go/blob/zendesk_release/kafka/tlscb_thunk.c https://github.com/zendesk/confluent-kafka-go/blob/zendesk_release/kafka/tlscb.go

(sidebar - the above implemention as far as I can tell isn't verifying the peer name at all - I should fix this...)

Unfortunately, using this callback correctly to override OpenSSL's behaviour for a cert it would ordinarily reject is kind of dangerous; if you provide it, you take responsibility for all aspects of certificate validation. As far as I can tell, a correct implementation for your use case would be something like...

Having written all this out, this probably isn't a great API for applications to use (it's probably more useful for writing language bindings, because the host language probably already has a set of TLS APIs for doing this that can be used to implement the above properly).

As for whether it should be part of the #3180 API - my read is no, that's only used for supplying client certificates, not for verifying the server certificates on the other side of the connection. My vote would be a separate callback which is called from rd_kafka_transport_ssl_set_endpoint_id to choose an application defined value to pass into SSL_set1_host.

crivera-fastly commented 3 years ago

Thanks for the detailed response @KJTsanaktsidis. I saw the existing verify_cb, but I definitely don’t want to be on the hook for validating the entire cert chain. A callback that would allow the client to specify a per-host verification override would work for my case and it's definitely more flexible than my initial suggestion.

edenhill commented 3 years ago

Thanks @KJTsanaktsidis.

@crivera-fastly I think I'm mostly in favour of the configuration property approach at this point before there is a required use-case for a callback. A property makes the SNI override functionality immediately available in all derrived clients and tools without any extra work needed, while a callback interface falls under the ABI guarantees and needs to be future proof and also requires work to be supported in all the sibling clients, et.al.

crivera-fastly commented 3 years ago

@edenhill That makes sense.