TLS and MTLS enhancements to HTTPListener input plugin#3191
TLS and MTLS enhancements to HTTPListener input plugin#3191danielnelson merged 5 commits intoinfluxdata:masterfrom
Conversation
Signed-off-by: Jeff Nickoloff <jeff@allingeek.com>
danielnelson
left a comment
danielnelson
left a comment
There was a problem hiding this comment.
This will have to be a 1.5 addition.
| ssl_allowed_client_certificate_authorities = ["/etc/ca.crt"] | ||
|
|
||
| ## Add non-public root of trust certificate authorities | ||
| ssl_certificate_authorities = ["/etc/ca.crt"] |
There was a problem hiding this comment.
This is probably a limitation in my TLS understanding, but I thought this (the one assigned to RootCAs) was only needed if you were a client. For instance, when setting a basic nginx server without client authentication up, you only need the ssl_certificate and ssl_certificate_key. Why do we need both of these CA options?
Signed-off-by: Jeff Nickoloff <jeff@allingeek.com>
|
You're correct. I've removed that unnecessary field and validated in testing. |
danielnelson
left a comment
danielnelson
left a comment
There was a problem hiding this comment.
I think this change will end up being copied around to other inputs that could support it, so I'm being even more pedantic about naming that normal. Let me know what you think.
| write_timeout = "10s" | ||
|
|
||
| ## HTTPS | ||
| ssl_certificate = "/etc/service.crt" |
There was a problem hiding this comment.
I thought about calling this ssl_cert to match the normal client configuration, but since this our first server configuration maybe we have a chance to fix a few naming issues as I'm sure this will end up being copied to other listeners. What if we name the options:
- tls_cert
- tls_key
- tls_cacert (server or client is implicit based on how the plugin works)
I think all of the examples paths should end with .pem to indicate the formatting. The client paths are current like: "/etc/telegraf/cert.pem", but perhaps better would be "/etc/telegraf/server/cert.pem"?
At some point I would like to rename these in a backwards compatible way on the client side as well.
There was a problem hiding this comment.
Agreed on all points. Will update.
There was a problem hiding this comment.
Except CA... that needs to be something like "tls_allowed_cacerts" because service endpoints need to be able to authorize using a set of certs.
| ssl_key = "/etc/service.key" | ||
|
|
||
| ## MTLS | ||
| ssl_allowed_client_certificate_authorities = ["/etc/ca.crt"] |
There was a problem hiding this comment.
In the client config, we only support a single CA certificate, is there a strong argument for needing multiple CAs on the server side? I know it might be needed, but I don't want to make things complicated for limited payoff.
Also, should we have a way to use the hosts root CA set with client certificate validation? Is this what happens if tlsConfig.ClientCAs is nil but ClientAuth is set to RequireAndVerifyClientCert?
There was a problem hiding this comment.
The allowed client CAs are the only authorization mechanisms. I've worked in orgs and on products where multiple client root CAs are in use. Using a list of certificates for this purpose is common.
The current change will not use the system cert pool CAs. I'm not sure that CAs in the root trust chain vend client certificates. However, if we decide that is a feature we should pursue then we would need to add a flag - something like "allow_all_system_root_cas" - to indicate that the CertificatePool should originate from the SystemCertPool instead of an empty one. Personally, I'm not sure that I would ever authorize "all clients with certificates signed by a public root CA."
Currently, if ssl_allowed_client_certificate_authorities is unset then the service does not require client auth. At that point it is just an HTTPS listener which is still useful.
Signed-off-by: Jeff Nickoloff <jeff@allingeek.com>
|
Let me know if you have any other feedback. |
|
Looks like we just need to update the README with the new names. |
Signed-off-by: Jeff Nickoloff <jeff@allingeek.com>
danielnelson
added
the
feat
|
Updated... embarrassing. |
Signed-off-by: Jeff Nickoloff <jeff@allingeek.com>
|
Thanks! |
2 participants
This enhancement allows a user to enable TLS for the endpoint by providing certificate and private key files. They can use private PKI by specifying a list of certificate authority file names. Mutual TLS authentication can be enabled by providing a list of allowed client CA certificate files.
Signed-off-by: Jeff Nickoloff jeff@allingeek.com
Required for all PRs: