Closed frenchtoastbeer closed 4 years ago
Certificate validation (full, only CA, disable) is a feature that should be part of the client.
Rather than allowing a client to be passed to TransportBuilder
though, I think it would be preferable to have functions on the transport builder to be able to set the "level" of certificate validation (perhaps an enum?). My reasoning here is that Reqwest's client builder may expose options on it (now or in the future) that are not relevant to or could be broken Elasticsearch, so I think the API should expose only those that are valid.
For some weird reason, when I tried this, it compiles and runs. I don't understand how that's possible, but even so it still revokes self-signed certs.
That is strange, I would expect a compiler error too 😕
For my current problem, what you describe would work just fine. Is that how it is handled by ES clients in other languages?
It's possible the client negotiates TLS with a reverse proxy, rather than the ES servers directly. In that scenario, settings that would normally be breaking may become necessary.
The reason I thought passing a client might be preferable is that there are other settings that may be relevant for the transport client. A few I can think of right now:
It's up to you guys, passing a client adds more flexibility at the expense of less control over the transport. Maybe a documentation example / test case that builds a "proper" reqwest client is good enough?
I don't think I've ever submitted a PR before on GitHub, but I'd be happy to try that in this case for whichever direction you want to take on this issue.
@frenchtoastbeer From what I know of other clients, they have typically tended to encapsulate the language http library/client of choice and exposed only the relevant client behaviours.
The reason I thought passing a client might be preferable is that there are other settings that may be relevant for the transport client. A few I can think of right now:
- custom DNS resolvers
- server name indication
- custom ciphers list/tls version
You raise a good point; I haven't personally come across a user needing to be able to set these in other clients, but I will ask around. I think for now, adding functions to transport builder would be sufficient, and we can re-evaluate in future if it starts to get unwieldy or the topic comes up frequently.
I don't think I've ever submitted a PR before on GitHub, but I'd be happy to try that in this case for whichever direction you want to take on this issue.
A PR for this would be greatly appreciated! Take a look at the contributing guide to get started and reach out if you have questions.
I think the actual implementation will be quite straightforward but feel free to reach out on a draft PR if you'd like another set of eyes to take a look. The more challenging piece I think will be the integration tests for this as ideally, it should run against an instance of Elasticsearch. I'm about to embark on a better suite for integration tests in preparation for #19, so the testing part can probably be implemented at a later point, once a better suite is in place.
@frenchtoastbeer I've added custom certificate validation to the client and released a v7.6.1-alpha.1 version with them. Check out the CertificateValidation
docs for details
Is your feature request related to a problem? Please describe. I wanted to disable certificate validation for testing, but realized that there is no TransportBuilder method which allows me to pass in a custom reqwest client.
Describe the solution you'd like I'd like to first generate a client like so. (I made an edit to remove calling build on the reqwest client)
and then pass it to a TransportBuilder like so:
Describe alternatives you've considered My first thought was to make
client_builder
public, but it may not be obvious thatTransportBuilder.client_builder
was a reqwest client.Additional context For some weird reason, when I tried this, it compiles and runs. I don't understand how that's possible, but even so it still revokes self-signed certs.