adding client certificate and key #90
Conversation
…nfiguration if parameter is given
…nfiguration if parameter is given
|
|
|
Hi @raoel, Can you sign the CLA, please? We need it to accept the contribution. |
had to get permission from my employer :-) should be fixed. |
|
Hi @raoel, Are you using a self-signed certificate or an internal CA? You should not share the pair with the client but just provide the CA file. https://prometheus.io/docs/guides/tls-encryption/#testing |
|
hello (sorry for the delay... things go slow - it's been christmas here too ;-) ) We have mutual TLS, so instead of providing the client with a username/password we need to provide the client with it's key and certificate to be able to authenticate. So, as far as I know, the client really need the pair. |
There was a problem hiding this comment.
Hey @raoel, sorry for the late reply 🙏 Thanks again for your contribution. I took a quick look and I've added some comments.
I didn't test yet it on a real setup, I will be on PTO for the rest of the week. I will do more tests on Monday.
| K6_PROMETHEUS_RW_CLIENT_CERTIFICATE_KEY=client.key \ | ||
| ./k6 run -o xk6-prometheus-rw script.js | ||
| ``` | ||
|
|
There was a problem hiding this comment.
We should document that in the case a different CA is required then the relative operating systems' storage shouid be used for providing the custom CA.
| InsecureSkipVerify: conf.InsecureSkipTLSVerify.Bool, //nolint:gosec | ||
| } | ||
|
|
||
| if conf.ClientCertificate.Valid && conf.ClientCertificateKey.Valid { |
There was a problem hiding this comment.
I would prefer if we skip the assignment if Insecure is set to true.
EDIT: this a wrong suggestion
| if conf.ClientCertificate.Valid && conf.ClientCertificateKey.Valid { | ||
| cert, _ := tls.LoadX509KeyPair(conf.ClientCertificate.String, conf.ClientCertificateKey.String) | ||
| hc.TLSConfig.Certificates = []tls.Certificate{cert} | ||
| } |
There was a problem hiding this comment.
Are these duplicated lines?
| if applied.ClientCertificate.Valid { | ||
| conf.ClientCertificate = applied.ClientCertificate | ||
| } | ||
|
|
||
| if applied.ClientCertificateKey.Valid { | ||
| conf.ClientCertificateKey = applied.ClientCertificateKey | ||
| } |
| }{ | ||
| "JSON": {jsonRaw: json.RawMessage(`{"clientCertificate":"client.crt","clientCertificateKey":"client.key"}`)}, | ||
| "Env": {env: map[string]string{"K6_PROMETHEUS_RW_CLIENT_CERTIFICATE": "client.crt", "K6_PROMETHEUS_RW_CLIENT_CERTIFICATE_KEY": "client.key"}}, | ||
| //"Arg": {arg: "username=user1,password=pass1"}, |
There was a problem hiding this comment.
| //"Arg": {arg: "username=user1,password=pass1"}, |
| } | ||
| } | ||
|
|
||
| func TestOptionClientCertificate(t *testing.T) { |
There was a problem hiding this comment.
Hey @raoel, thanks again for the contribution. I merge this PR on a development branch where we will do some polish of the feature then we will merge on the main branch. I will make sure to preserve you as the author/contributor of your commits.
|
Unfortunately, this branch contains conflicts and we can't push here because the PR is generated from a fork. Going to continue the work on #143 preserving the commit history and the original author. |
hey there,
for our case we needed to add a client certificate and key to the configuration - I'd like to upstream my changes.
I hope this passes your standards - I've modified existing unit tests for configuration (I didn't do a full integration test with certificates because that would make the testing setup a lot more complex)