-
Notifications
You must be signed in to change notification settings - Fork 61
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
make kubernetes_ca_cert optional #238
Conversation
…ack to the system's trust store
…RootCAs has become nil or at the initial update
- check that the CA cert chain contains at least one valid certificate.
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.
Looking good. I had a few suggestions/nits for your consideration.
CHANGELOG.md
Outdated
@@ -10,6 +10,10 @@ | |||
* `k8s.io/api` v0.29.1 -> v0.29.2 | |||
* `k8s.io/apimachinery` v0.29.1 -> v0.29.2 | |||
|
|||
### Improvements | |||
|
|||
* Make kubernetes_ca_cert optional [GH-238](https://github.com/hashicorp/vault-plugin-auth-kubernetes/pull/238) |
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 think this is really about having "TLS uses the host's root CA set" if no CA chain is provided - which is a behavioural change.
We should also document the extra validation being done on the provided CA PEM bundle.
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Thank you for your feedback! |
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.
A few minor nits, otherwise 👍
path_config.go
Outdated
} | ||
|
||
if caCert != "" && !hasCerts(caCert) { | ||
return logical.ErrorResponse("The provided CA PEM data contains no valid certificates"), nil |
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.
nit: the line is bit long, may bring the message down one line, as it was before.
CHANGELOG.md
Outdated
@@ -10,6 +10,10 @@ | |||
* `k8s.io/api` v0.29.1 -> v0.29.2 | |||
* `k8s.io/apimachinery` v0.29.1 -> v0.29.2 | |||
|
|||
### Improvements | |||
|
|||
* Allow TLS client to use the host's root CA set when no CA certificates are provided and `disable_local_ca_jwt` is true if running Vault in a Kubernetes pod. Additionally, validate the configuration's provided CA PEM bundle. [GH-238](https://github.com/hashicorp/vault-plugin-auth-kubernetes/pull/238) |
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.
nit: line is a bit long, mind folding it?
Overview
Users want to make
kubernetes_ca_cert
optional. Since the CA cert is used only for establishing a TLS connection Kubernetes api, we can default to use the system's trust store with no harm as mentioned in #62.Design of Change
The config write handler removes non-nil
kubernetes_ca_cert
enforcement.When
kubernetes_ca_cert
is not given:disable_local_default_ca_jwt
is false, set the local CA tocaCertBytes
.caCertBytes
is empty, assign the default TLS config to transport.TLSClientConfig and return early.Related Issues/Pull Requests
Contributor Checklist
The self-signed kubernetes host CA is in the host’s trust store
The self-signed kubernetes host CA is not in the host’s trust store
Invalid
kubernetes_ca_cert
Correctly formatted
kubernetes_ca_cert
but not the kubernetes host's CAValid
kubernetes_ca_cert
Switching from system cert pool to
kubernetes_ca_cert
Switching back to system cert pool from
kubernetes_ca_cert