make kubernetes_ca_cert optional and allow TLS client to use the host…

…'s root CA set on no CA certificates provided (#238) - 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 - validate the configuration's provided CA PEM bundle --------- Co-authored-by: Ben Ash <[email protected]> Co-authored-by: Ben Ash <[email protected]>
hashicorp · Mar 27, 2024 · da08a6a · da08a6a
1 parent 26978d2
commit da08a6a
Show file tree

Hide file tree

Showing 6 changed files with 159 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,12 @@
   * `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)
+
 ## 0.18.0 (Feb 2, 2024)
 
 ### Changes

diff --git a/backend.go b/backend.go
@@ -128,7 +128,16 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend,
 	return b, nil
 }
 
-var getDefaultHTTPClient = cleanhttp.DefaultPooledClient
+// getDefaultTLSConfig returns a http.Client with the supported, default
+// tls.Config from getDefaultTLSConfig() set on the
+// cleanhttp.DefaultPooledTransport() http.Transport.
+func getDefaultHTTPClient() *http.Client {
+	transport := cleanhttp.DefaultPooledTransport()
+	transport.TLSClientConfig = getDefaultTLSConfig()
+	return &http.Client{
+		Transport: transport,
+	}
+}
 
 func getDefaultTLSConfig() *tls.Config {
 	return &tls.Config{
@@ -433,21 +442,24 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error {
 		caCertBytes = []byte(data)
 	}
 
-	certPool := x509.NewCertPool()
-	if len(caCertBytes) > 0 {
+	var certPool *x509.CertPool
+	if len(caCertBytes) == 0 {
+		// since the CA chain is not configured, we use the system's cert pool.
+		var err error
+		certPool, err = x509.SystemCertPool()
+		if err != nil {
+			return err
+		}
+	} else {
+		// since we have a CA chain configured, we create a new x509.CertPool with its
+		// contents.
+		certPool = x509.NewCertPool()
 		if ok := certPool.AppendCertsFromPEM(caCertBytes); !ok {
 			b.Logger().Warn("Configured CA PEM data contains no valid certificates, TLS verification will fail")
 		}
-	} else {
-		// provide an empty certPool
-		b.Logger().Warn("No CA certificates configured, TLS verification will fail")
-		// TODO: think about supporting host root CA certificates via a configuration toggle,
-		// in which case RootCAs should be set to nil
 	}
 
-	// only refresh the Root CAs if they have changed since the last full update.
-	if !b.tlsConfig.RootCAs.Equal(certPool) {
-		b.Logger().Trace("Root CA certificate pool has changed, updating the client's transport")
+	setTLSClientConfig := func() error {
 		transport, ok := b.httpClient.Transport.(*http.Transport)
 		if !ok {
 			// should never happen
@@ -456,6 +468,15 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error {
 
 		b.tlsConfig.RootCAs = certPool
 		transport.TLSClientConfig = b.tlsConfig
+		return nil
+	}
+
+	// only refresh the Root CAs if they have changed since the last full update.
+	if b.tlsConfig.RootCAs == nil {
+		return setTLSClientConfig()
+	} else if !b.tlsConfig.RootCAs.Equal(certPool) {
+		b.Logger().Trace("Root CA certificate pool has changed, updating the client's transport")
+		return setTLSClientConfig()
 	} else {
 		b.Logger().Trace("Root CA certificate pool is unchanged, no update required")
 	}

diff --git a/backend_test.go b/backend_test.go
@@ -25,6 +25,11 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) {
 	localCertPool := getTestCertPool(t, testLocalCACert)
 	otherCertPool := getTestCertPool(t, testOtherCACert)
 
+	systemCertPool, err := x509.SystemCertPool()
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	type testConfig struct {
 		config          *kubeConfig
 		expectTLSConfig *tls.Config
@@ -173,6 +178,23 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name:       "ca-certs-not-set",
+			httpClient: getDefaultHTTPClient(),
+			tlsConfig:  getDefaultTLSConfig(),
+			configs: []testConfig{
+				{
+					config: &kubeConfig{
+						DisableLocalCAJwt: true,
+					},
+					expectTLSConfig: &tls.Config{
+						MinVersion: minTLSVersion,
+						RootCAs:    systemCertPool,
+					},
+				},
+			},
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/common_test.go b/common_test.go
@@ -104,5 +104,19 @@ pEDcN/HcXM47TP7XgWW0rfQli3RucuqMV7LHvvpiGIWwfutrK9g7Py91W2JbQCA0
 D14XDzgsruCwlWAP1FMvLMIPhSknpIJd9Xql+0/Ae1yl9f3Uamj3mDtBKg8/U5nJ
 0wU=
 -----END CERTIFICATE-----
+`
+
+	testInvalidCACert = `
+-----BEGIN CERTIFICATE-----
+MIIC5zCCAc+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p
+a3ViZUNBMB4XDTE3MDgxMDIzMTQ1NVoXDTI3MDgwODIzMTQ1NVowFTETMBEGA1UE
+AxMKbWluaWt1YmVDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAN8d
+w2p/KXRkm+vzOO0eT1vYBWP7fKsnng9/g5nnXAJlt9NxpOSolRcyItm/04R0E1jx
+jpgsdzkybc+QU5ZiszOYN833/D5hCNVAABVivpDd2P8wVKXN46cB99e24etUVBqG
+5aR0Ku3IBsJjCN9efhF+XRCA2gy/KaXMdKJhHfdtc8hCr7G9+2wO2G58FLmIfEyH
+owviOGt0BSnCtMpsA8ZgGQyfqgSd5u466aCv6oj0MyzsMnfS38niM53Rlv4IY6ol
+taYbWXtCNndQ2S687qE0qTCxhE95Bm2Nfkqct4R1798sJz83xNv8hALvxr/vPK/J
+2XkIm3oo3YKG4n/CHXcCAwEAAaNCMEAwDgYDVR0PAQH/BAQDAgKkMB0GA1UdJQQW
+MBQGCCsGAQUFBwMCBggrBgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3
 `
 )
diff --git a/path_config.go b/path_config.go
@@ -34,10 +34,10 @@ func pathConfig(b *kubeAuthBackend) *framework.Path {
 				Type:        framework.TypeString,
 				Description: "Host must be a host string, a host:port pair, or a URL to the base of the Kubernetes API server.",
 			},
-
 			"kubernetes_ca_cert": {
-				Type:        framework.TypeString,
-				Description: "PEM encoded CA cert for use by the TLS client used to talk with the API.",
+				Type: framework.TypeString,
+				Description: `Optional PEM encoded CA cert for use by the TLS client used to talk with the API. 
+If it is not set and disable_local_ca_jwt is true, the system's trusted CA certificate pool will be used.`,
 				DisplayAttrs: &framework.DisplayAttributes{
 					Name: "Kubernetes CA Certificate",
 				},
@@ -159,16 +159,38 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ
 		return logical.ErrorResponse("no host provided"), nil
 	}
 
-	disableLocalJWT := data.Get("disable_local_ca_jwt").(bool)
+	disableLocalCAJwt := data.Get("disable_local_ca_jwt").(bool)
 	pemList := data.Get("pem_keys").([]string)
 	caCert := data.Get("kubernetes_ca_cert").(string)
 	issuer := data.Get("issuer").(string)
 	disableIssValidation := data.Get("disable_iss_validation").(bool)
 	tokenReviewer := data.Get("token_reviewer_jwt").(string)
 	useAnnotationsAsAliasMetadata := data.Get("use_annotations_as_alias_metadata").(bool)
 
-	if disableLocalJWT && caCert == "" {
-		return logical.ErrorResponse("kubernetes_ca_cert must be given when disable_local_ca_jwt is true"), nil
+	// hasCerts returns true if caCert contains at least one valid certificate. It
+	// does not check if any of the certificates from caCert are CAs, although that
+	// might be something that we want in the future.
+	hasCerts := func(certBundle string) bool {
+		var b *pem.Block
+		rest := []byte(certBundle)
+		for {
+			b, rest = pem.Decode(rest)
+			if b == nil {
+				break
+			}
+
+			if pem.EncodeToMemory(b) != nil {
+				return true
+			}
+		}
+
+		return false
+	}
+
+	if caCert != "" && !hasCerts(caCert) {
+		return logical.ErrorResponse(
+			"The provided CA PEM data contains no valid certificates",
+		), nil
 	}
 
 	config := &kubeConfig{
@@ -179,7 +201,7 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ
 		TokenReviewerJWT:              tokenReviewer,
 		Issuer:                        issuer,
 		DisableISSValidation:          disableIssValidation,
-		DisableLocalCAJwt:             disableLocalJWT,
+		DisableLocalCAJwt:             disableLocalCAJwt,
 		UseAnnotationsAsAliasMetadata: useAnnotationsAsAliasMetadata,
 	}
 

diff --git a/path_config_test.go b/path_config_test.go
@@ -174,7 +174,7 @@ func TestConfig(t *testing.T) {
 		t.Fatalf("got unexpected error: %v", resp.Error())
 	}
 
-	// test invalid cert
+	// test invalid pem_keys
 	data = map[string]interface{}{
 		"pem_keys":        "bad",
 		"kubernetes_host": "host",
@@ -195,6 +195,27 @@ func TestConfig(t *testing.T) {
 		t.Fatalf("got unexpected error: %v", resp.Error())
 	}
 
+	// test invalid kubernetes_ca_cert
+	data = map[string]interface{}{
+		"kubernetes_ca_cert": testInvalidCACert,
+		"kubernetes_host":    "host",
+	}
+
+	req = &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      configPath,
+		Storage:   storage,
+		Data:      data,
+	}
+
+	resp, err = b.HandleRequest(context.Background(), req)
+	if resp == nil || !resp.IsError() {
+		t.Fatal("expected error")
+	}
+	if resp.Error().Error() != "The provided CA PEM data contains no valid certificates" {
+		t.Fatalf("got unexpected error: %v", resp.Error())
+	}
+
 	// Test success with no certs
 	data = map[string]interface{}{
 		"kubernetes_host":    "host",
@@ -458,7 +479,7 @@ func TestConfig_LocalCaJWT(t *testing.T) {
 				DisableLocalCAJwt:    false,
 			},
 		},
-		"CA and disable local default": {
+		"disable local default, CA set": {
 			config: map[string]interface{}{
 				"kubernetes_host":      "host",
 				"kubernetes_ca_cert":   testCACert,
@@ -474,6 +495,39 @@ func TestConfig_LocalCaJWT(t *testing.T) {
 				DisableLocalCAJwt:    true,
 			},
 		},
+		"disable local default, JWT set": {
+			config: map[string]interface{}{
+				"kubernetes_host":      "host",
+				"token_reviewer_jwt":   jwtGoodDataToken,
+				"disable_local_ca_jwt": true,
+			},
+			setupInClusterFiles: true,
+			expected: &kubeConfig{
+				PublicKeys:           []crypto.PublicKey{},
+				PEMKeys:              []string{},
+				Host:                 "host",
+				CACert:               "",
+				TokenReviewerJWT:     jwtGoodDataToken,
+				DisableISSValidation: true,
+				DisableLocalCAJwt:    true,
+			},
+		},
+		"disable local default, no CA or JWT": {
+			config: map[string]interface{}{
+				"kubernetes_host":      "host",
+				"disable_local_ca_jwt": true,
+			},
+			setupInClusterFiles: true,
+			expected: &kubeConfig{
+				PublicKeys:           []crypto.PublicKey{},
+				PEMKeys:              []string{},
+				Host:                 "host",
+				CACert:               "",
+				TokenReviewerJWT:     "",
+				DisableISSValidation: true,
+				DisableLocalCAJwt:    true,
+			},
+		},
 	}
 
 	for name, tc := range testCases {