Add authentication to Elasticsearch via client cert #191
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
tobio
force-pushed
the
feature/client-auth
branch
from
d466ade to
c2ecc3f
Compare
internal/clients/api_client.go
Outdated
| ensureTLSClientConfig(&config) | ||
| config.Transport.(*http.Transport).TLSClientConfig.Certificates = []tls.Certificate{cert} |
There was a problem hiding this comment.
IMO these blocks would likely read more nicely if ensureTLSClientConfig returned a *http.Transport so the subsequent code could avoid the casts. WDYT?
To be clear: "You're wrong because..." is totally fine.
There was a problem hiding this comment.
Good idea! I would even go one step further. What about returning *tls.Config?
I guess everyone calling ensureTLSClientConfig wants to customize TLSClientConfig afterwards.
| } | ||
|
|
||
| if certFile, ok := esConfig["cert_file"]; ok && certFile.(string) != "" { | ||
| if keyFile, ok := esConfig["key_file"]; ok && keyFile.(string) != "" { |
There was a problem hiding this comment.
I know this is enforced in the schema, but IMO we should return an error if cert_file is ok but key_file is not ok.
| } | ||
| } | ||
| if certData, ok := esConfig["cert_data"]; ok && certData.(string) != "" { | ||
| if keyData, ok := esConfig["key_data"]; ok && keyData.(string) != "" { |
There was a problem hiding this comment.
Similarly, I think this code should error out if cert_data is specified without key_data
| } | ||
|
|
||
| if certFile, ok := conn["cert_file"]; ok && certFile.(string) != "" { | ||
| if keyFile, ok := conn["key_file"]; ok && keyFile.(string) != "" { |
| } | ||
| } | ||
| if certData, ok := conn["cert_data"]; ok && certData.(string) != "" { | ||
| if keyData, ok := conn["key_data"]; ok && keyData.(string) != "" { |
|
Thanks for this @andreaskapfer we'll have a new version of the provider out with this change in the next few weeks. |
|
Great! Many thanks for the review @tobio |
|
This has been released with 0.5.0 |
3 participants
This PR adds the ability to authenticate to Elasticsearch via client certificate.
It also fixes some bugs related to the options
ca_fileandca_data.