Skip to content

Conversation

imthaghost
Copy link
Contributor

@imthaghost imthaghost commented Apr 21, 2022

Overview

Some users didn't find it necessary for Vault to enforce the CA cert or PEM keys. Examples can be found in the related issues.

Design of Change

We add false booleans to the kubernetes_ca_cert and pem_keys fields. While the required field on the FieldSchema struct are deprecated we added this just for future reference and documentation. We also remove checks that enforced kubernetes_ca_cert to be present instead we just default to the local CA cert if kubernetes_ca_cert is not set which will return an error of x509: certificate signed by unknown authority if the user did supply an appropriate CA cert.

Related Issues

Issue #62
Issue #88

Docs

I don't believe docs need to be added since this is implied in the example below.

Screen Shot 2022-04-21 at 4 11 55 PM

@imthaghost imthaghost requested review from calvn, tomhjp and tvoran April 21, 2022 23:16
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can we get some unit test coverage? It's harder to test in an automated way, but I'd also like to ensure we at least manually test using the OS trust store. LMK if you'd like any help with that and I can go into more detail.

@imthaghost imthaghost requested a review from tomhjp April 26, 2022 19:25
Comment on lines +544 to +561
"no CA default to system": {
config: map[string]interface{}{
"kubernetes_host": "host",
"disable_local_ca_jwt": false,
"kubernetes_ca_cert": "",
},
setupInClusterFiles: true,

expected: &kubeConfig{
PublicKeys: []interface{}{},
PEMKeys: []string{},
Host: "host",
CACert: testLocalCACert,
TokenReviewerJWT: testLocalJWT,
DisableISSValidation: true,
DisableLocalCAJwt: false,
},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test different than "no CA or JWT, default to local" in TestConfig_LocalCaJWT()? IIRC disable_local_ca_jwt defaults to false, and kubernetes_ca_cert defaults to "".

"no CA or JWT, default to local": {
config: map[string]interface{}{
"kubernetes_host": "host",
},
setupInClusterFiles: true,
expected: &kubeConfig{
PublicKeys: []interface{}{},
PEMKeys: []string{},
Host: "host",
CACert: testLocalCACert,
TokenReviewerJWT: testLocalJWT,
DisableISSValidation: true,
DisableLocalCAJwt: false,
},
},

tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
}
if disableLocalJWT || len(caCert) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we don't use system certs if disableLocalJWT is true?

tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
}
if disableLocalJWT || len(caCert) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a more idiomatic check for empty strings:

Suggested change
if disableLocalJWT || len(caCert) > 0 {
if disableLocalJWT || caCert != "" {

@rinormaloku
Copy link

@imthaghost and @tomhjp can we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants