-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
ARROW-5643: [FlightRPC] Add ability to override SSL hostname checking #4608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rebase? The TLS tests in test_flight.py
were fixed recently on master.
cpp/src/arrow/flight/client.h
Outdated
std::string tls_root_certs; | ||
/// \brief Override the hostname checked by TLS. Insecure, use with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to mark it insecure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can shorten the name, but I'd at least like to stress that it's potentially a footgun in production.
CertKeyPair root_cert; | ||
ASSERT_OK(ExampleTlsCertificateRoot(&root_cert)); | ||
client_options.tls_root_certs = root_cert.pem_cert; | ||
ASSERT_OK(FlightClient::Connect(server_->location(), client_options, &client)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... interesting. The certificate error is not raised at connect time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gRPC doesn't actually try to make a connection until an RPC is issued, generally.
action.type = "test"; | ||
action.body = Buffer::FromString(""); | ||
std::unique_ptr<ResultStream> results; | ||
ASSERT_RAISES(IOError, client->DoAction(options, action, &results)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an error message we can check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is, but I've been bitten elsewhere by messages that changed across gRPC versions...once #4484 lands, I'd rather we wrap and expose the gRPC error code + error message instead of just a message (and maybe map error codes to a set of Flight codes).
Rebased, shortened name to "override_hostname". |
python/pyarrow/tests/test_flight.py
Outdated
certs = example_tls_certs() | ||
|
||
with flight_server( | ||
ConstantFlightServer, tls_certificates=certs["certificates"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... you have to pass the connect_args
as above, otherwise flight_server
may be stuck in a loop trying to reconnect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.> Right, thanks.
hey @pitrou this probably isn't the place for this question but I can't find an issue that matches my problem more closely. I was wondering if there was good example of deploying a python flight server/service into kubernetes. I'm using the pyarrow cookbook for the flight server and am getting hung up on some gotcha in the service or deployment config, or maybe something with the URL. any help or guidance would be much appreciated |
@patcollis34 I don't have an answer, I suggest you ask on the user mailing-list. |
Adds the ability to override hostname checks, so you can connect to localhost over TLS but still verify that the certificate is for some other domain.
Example: when deploying on Kubernetes with headless services, clients connect directly to backend services and do load balancing themselves. Thus all instances of an application must present a certificate for the same hostname. To do health checks in such an environment, you can't connect to the TLS hostname (which may resolve to a different instance); you need to connect to localhost, and override the hostname check.
Also needs apache/arrow-testing#5