|
| 1 | +From e7c75f46002eb7678820f0901a40e323351f7881 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Luboslav Pivarc <lpivarc@redhat.com> |
| 3 | +Date: Mon, 14 Jul 2025 14:03:08 +0200 |
| 4 | +Subject: [PATCH] SetupTLSWithCertManager now enforces CN |
| 5 | + |
| 6 | +The CNs will be provided by Kubernetes. |
| 7 | +If empty, any client certificate validated |
| 8 | +by the authority is allowed. |
| 9 | + |
| 10 | +Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com> |
| 11 | +Signed-off-by: Azure Linux Security Servicing Account <azurelinux-security@microsoft.com> |
| 12 | +Upstream-reference: https://patch-diff.githubusercontent.com/raw/kubevirt/kubevirt/pull/15339.diff |
| 13 | +--- |
| 14 | + pkg/util/tls/ca-manager.go | 69 +++++++++++++++++++++++++++---- |
| 15 | + pkg/util/tls/ca-manager_test.go | 72 ++++++++++++++++++++++++++++++--- |
| 16 | + pkg/util/tls/tls.go | 30 +++++++++++++- |
| 17 | + pkg/util/tls/tls_test.go | 27 ++++++++++++- |
| 18 | + pkg/virt-api/api.go | 2 +- |
| 19 | + 5 files changed, 182 insertions(+), 18 deletions(-) |
| 20 | + |
| 21 | +diff --git a/pkg/util/tls/ca-manager.go b/pkg/util/tls/ca-manager.go |
| 22 | +index 950f17a..6e9f01a 100644 |
| 23 | +--- a/pkg/util/tls/ca-manager.go |
| 24 | ++++ b/pkg/util/tls/ca-manager.go |
| 25 | +@@ -21,6 +21,7 @@ package tls |
| 26 | + |
| 27 | + import ( |
| 28 | + "crypto/x509" |
| 29 | ++ "encoding/json" |
| 30 | + "fmt" |
| 31 | + "sync" |
| 32 | + |
| 33 | +@@ -40,6 +41,11 @@ type ClientCAManager interface { |
| 34 | + GetCurrentRaw() ([]byte, error) |
| 35 | + } |
| 36 | + |
| 37 | ++type KubernetesCAManager interface { |
| 38 | ++ ClientCAManager |
| 39 | ++ GetCNs() ([]string, error) |
| 40 | ++} |
| 41 | ++ |
| 42 | + type manager struct { |
| 43 | + store cache.Store |
| 44 | + lock *sync.Mutex |
| 45 | +@@ -52,14 +58,61 @@ type manager struct { |
| 46 | + lastRaw []byte |
| 47 | + } |
| 48 | + |
| 49 | +-func NewKubernetesClientCAManager(configMapCache cache.Store) ClientCAManager { |
| 50 | +- return &manager{ |
| 51 | +- store: configMapCache, |
| 52 | +- lock: &sync.Mutex{}, |
| 53 | +- namespace: metav1.NamespaceSystem, |
| 54 | +- name: util.ExtensionAPIServerAuthenticationConfigMap, |
| 55 | +- secretKey: util.RequestHeaderClientCAFileKey, |
| 56 | +- lastRevision: "-1", |
| 57 | ++type kubeManager struct { |
| 58 | ++ manager |
| 59 | ++ lastCNs []string |
| 60 | ++ lastCNRevision string |
| 61 | ++} |
| 62 | ++ |
| 63 | ++func (m *kubeManager) GetCNs() ([]string, error) { |
| 64 | ++ m.lock.Lock() |
| 65 | ++ defer m.lock.Unlock() |
| 66 | ++ obj, exists, err := m.store.GetByKey(m.namespace + "/" + m.name) |
| 67 | ++ |
| 68 | ++ if err != nil { |
| 69 | ++ return nil, err |
| 70 | ++ } else if !exists { |
| 71 | ++ if m.lastPool != nil { |
| 72 | ++ return m.lastCNs, nil |
| 73 | ++ } |
| 74 | ++ |
| 75 | ++ return nil, fmt.Errorf("configmap %s not found. Unable to detect request header CA", m.name) |
| 76 | ++ } |
| 77 | ++ |
| 78 | ++ configMap := obj.(*k8sv1.ConfigMap) |
| 79 | ++ |
| 80 | ++ // no change detected. |
| 81 | ++ if m.lastCNRevision == configMap.ResourceVersion { |
| 82 | ++ return m.lastCNs, nil |
| 83 | ++ } |
| 84 | ++ |
| 85 | ++ CNstring, ok := configMap.Data["requestheader-allowed-names"] |
| 86 | ++ if !ok { |
| 87 | ++ return nil, fmt.Errorf("requestheader-allowed-names not found in configmap %s/%s", m.namespace, m.name) |
| 88 | ++ } |
| 89 | ++ CNs := []string{} |
| 90 | ++ if CNstring != "" { |
| 91 | ++ err = json.Unmarshal([]byte(CNstring), &CNs) |
| 92 | ++ if err != nil { |
| 93 | ++ return CNs, err |
| 94 | ++ } |
| 95 | ++ } |
| 96 | ++ |
| 97 | ++ m.lastCNs = CNs |
| 98 | ++ m.lastCNRevision = configMap.ResourceVersion |
| 99 | ++ return CNs, nil |
| 100 | ++} |
| 101 | ++ |
| 102 | ++func NewKubernetesClientCAManager(configMapCache cache.Store) *kubeManager { |
| 103 | ++ return &kubeManager{ |
| 104 | ++ manager: manager{ |
| 105 | ++ store: configMapCache, |
| 106 | ++ lock: &sync.Mutex{}, |
| 107 | ++ namespace: metav1.NamespaceSystem, |
| 108 | ++ name: util.ExtensionAPIServerAuthenticationConfigMap, |
| 109 | ++ secretKey: util.RequestHeaderClientCAFileKey, |
| 110 | ++ lastRevision: "-1", |
| 111 | ++ }, |
| 112 | + } |
| 113 | + } |
| 114 | + |
| 115 | +diff --git a/pkg/util/tls/ca-manager_test.go b/pkg/util/tls/ca-manager_test.go |
| 116 | +index 32e2c68..a2e0e74 100644 |
| 117 | +--- a/pkg/util/tls/ca-manager_test.go |
| 118 | ++++ b/pkg/util/tls/ca-manager_test.go |
| 119 | +@@ -5,8 +5,8 @@ import ( |
| 120 | + |
| 121 | + . "github.com/onsi/ginkgo/v2" |
| 122 | + . "github.com/onsi/gomega" |
| 123 | +- v1 "k8s.io/api/core/v1" |
| 124 | +- v12 "k8s.io/apimachinery/pkg/apis/meta/v1" |
| 125 | ++ k8sv1 "k8s.io/api/core/v1" |
| 126 | ++ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
| 127 | + "k8s.io/client-go/tools/cache" |
| 128 | + |
| 129 | + "kubevirt.io/kubevirt/pkg/certificates/triple" |
| 130 | +@@ -16,17 +16,17 @@ import ( |
| 131 | + |
| 132 | + var _ = Describe("CaManager", func() { |
| 133 | + |
| 134 | +- var configMap *v1.ConfigMap |
| 135 | ++ var configMap *k8sv1.ConfigMap |
| 136 | + var manager ClientCAManager |
| 137 | + var store cache.Store |
| 138 | + |
| 139 | + BeforeEach(func() { |
| 140 | + ca, err := triple.NewCA("first", time.Hour) |
| 141 | + Expect(err).ToNot(HaveOccurred()) |
| 142 | +- configMap = &v1.ConfigMap{ |
| 143 | +- ObjectMeta: v12.ObjectMeta{ |
| 144 | ++ configMap = &k8sv1.ConfigMap{ |
| 145 | ++ ObjectMeta: metav1.ObjectMeta{ |
| 146 | + Name: util.ExtensionAPIServerAuthenticationConfigMap, |
| 147 | +- Namespace: v12.NamespaceSystem, |
| 148 | ++ Namespace: metav1.NamespaceSystem, |
| 149 | + ResourceVersion: "1", |
| 150 | + }, |
| 151 | + Data: map[string]string{ |
| 152 | +@@ -93,3 +93,63 @@ var _ = Describe("CaManager", func() { |
| 153 | + Expect(cert.Subjects()[0]).To(ContainSubstring("first")) |
| 154 | + }) |
| 155 | + }) |
| 156 | ++ |
| 157 | ++var _ = Describe("KubernetesCAManager", func() { |
| 158 | ++ |
| 159 | ++ prepareManagerComplex := func(f func(*k8sv1.ConfigMap)) KubernetesCAManager { |
| 160 | ++ ca, err := triple.NewCA("first", time.Hour) |
| 161 | ++ Expect(err).ToNot(HaveOccurred()) |
| 162 | ++ configMap := &k8sv1.ConfigMap{ |
| 163 | ++ ObjectMeta: metav1.ObjectMeta{ |
| 164 | ++ Name: util.ExtensionAPIServerAuthenticationConfigMap, |
| 165 | ++ Namespace: metav1.NamespaceSystem, |
| 166 | ++ ResourceVersion: "1", |
| 167 | ++ }, |
| 168 | ++ Data: map[string]string{ |
| 169 | ++ util.RequestHeaderClientCAFileKey: string(cert.EncodeCertPEM(ca.Cert)), |
| 170 | ++ }, |
| 171 | ++ } |
| 172 | ++ f(configMap) |
| 173 | ++ store := cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc) |
| 174 | ++ Expect(store.Add(configMap)).To(Succeed()) |
| 175 | ++ return NewKubernetesClientCAManager(store) |
| 176 | ++ } |
| 177 | ++ |
| 178 | ++ prepareManager := func(data string) KubernetesCAManager { |
| 179 | ++ return prepareManagerComplex(func(cm *k8sv1.ConfigMap) { |
| 180 | ++ cm.Data["requestheader-allowed-names"] = data |
| 181 | ++ }) |
| 182 | ++ } |
| 183 | ++ |
| 184 | ++ DescribeTable("should return zero CNs", func(data string) { |
| 185 | ++ manager := prepareManager(data) |
| 186 | ++ Expect(manager.GetCNs()).To(BeEmpty()) |
| 187 | ++ }, |
| 188 | ++ Entry("with empty", ""), |
| 189 | ++ Entry("with empty slice", "[]"), |
| 190 | ++ ) |
| 191 | ++ |
| 192 | ++ DescribeTable("should return", func(data string, expected []string) { |
| 193 | ++ manager := prepareManager(data) |
| 194 | ++ Expect(manager.GetCNs()).To(ConsistOf(expected)) |
| 195 | ++ }, |
| 196 | ++ Entry("with one element", `["one"]`, []string{"one"}), |
| 197 | ++ Entry("with two elements", `["one", "two"]`, []string{"one", "two"}), |
| 198 | ++ ) |
| 199 | ++ |
| 200 | ++ DescribeTable("should return error", func(data string) { |
| 201 | ++ manager := prepareManager(data) |
| 202 | ++ _, err := manager.GetCNs() |
| 203 | ++ Expect(err).To(HaveOccurred()) |
| 204 | ++ }, |
| 205 | ++ Entry("with malformed", `["one",]`), |
| 206 | ++ Entry("with malformed string", `[one]`), |
| 207 | ++ Entry("with no array", "one"), |
| 208 | ++ ) |
| 209 | ++ |
| 210 | ++ It(`should error when no "requestheader-allowed-names" is not specified`, func() { |
| 211 | ++ manager := prepareManagerComplex(func(cm *k8sv1.ConfigMap) {}) |
| 212 | ++ _, err := manager.GetCNs() |
| 213 | ++ Expect(err).To(MatchError(ContainSubstring("requestheader-allowed-names not found in"))) |
| 214 | ++ }) |
| 215 | ++}) |
| 216 | +diff --git a/pkg/util/tls/tls.go b/pkg/util/tls/tls.go |
| 217 | +index e9e1405..1d1b949 100644 |
| 218 | +--- a/pkg/util/tls/tls.go |
| 219 | ++++ b/pkg/util/tls/tls.go |
| 220 | +@@ -17,6 +17,7 @@ import ( |
| 221 | + ) |
| 222 | + |
| 223 | + const noSrvCertMessage = "No server certificate, server is not yet ready to receive traffic" |
| 224 | ++const serverNotReadyMsg = "Server is not yet ready to receive traffic" |
| 225 | + |
| 226 | + var ( |
| 227 | + cipherSuites = tls.CipherSuites() |
| 228 | +@@ -91,7 +92,7 @@ func SetupExportProxyTLS(certManager certificate.Manager, kubeVirtInformer cache |
| 229 | + return tlsConfig |
| 230 | + } |
| 231 | + |
| 232 | +-func SetupTLSWithCertManager(caManager ClientCAManager, certManager certificate.Manager, clientAuth tls.ClientAuthType, clusterConfig *virtconfig.ClusterConfig) *tls.Config { |
| 233 | ++func SetupTLSWithCertManager(caManager KubernetesCAManager, certManager certificate.Manager, clientAuth tls.ClientAuthType, clusterConfig *virtconfig.ClusterConfig) *tls.Config { |
| 234 | + tlsConfig := &tls.Config{ |
| 235 | + GetCertificate: func(info *tls.ClientHelloInfo) (certificate *tls.Certificate, err error) { |
| 236 | + cert := certManager.Current() |
| 237 | +@@ -122,6 +123,33 @@ func SetupTLSWithCertManager(caManager ClientCAManager, certManager certificate. |
| 238 | + Certificates: []tls.Certificate{*cert}, |
| 239 | + ClientCAs: clientCAPool, |
| 240 | + ClientAuth: clientAuth, |
| 241 | ++ VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { |
| 242 | ++ if len(verifiedChains) == 0 || len(verifiedChains[0]) == 0 { |
| 243 | ++ return nil |
| 244 | ++ } |
| 245 | ++ |
| 246 | ++ certificate, err := x509.ParseCertificate(rawCerts[0]) |
| 247 | ++ if err != nil { |
| 248 | ++ return fmt.Errorf("failed to parse peer certificate: %v", err) |
| 249 | ++ } |
| 250 | ++ |
| 251 | ++ CNs, err := caManager.GetCNs() |
| 252 | ++ if err != nil { |
| 253 | ++ log.Log.Reason(err).Error(serverNotReadyMsg) |
| 254 | ++ return fmt.Errorf(serverNotReadyMsg) |
| 255 | ++ } |
| 256 | ++ |
| 257 | ++ if len(CNs) == 0 { |
| 258 | ++ return nil |
| 259 | ++ } |
| 260 | ++ for _, CN := range CNs { |
| 261 | ++ if certificate.Subject.CommonName == CN { |
| 262 | ++ return nil |
| 263 | ++ } |
| 264 | ++ } |
| 265 | ++ |
| 266 | ++ return fmt.Errorf("Common name is invalid") |
| 267 | ++ }, |
| 268 | + } |
| 269 | + |
| 270 | + config.BuildNameToCertificate() |
| 271 | +diff --git a/pkg/util/tls/tls_test.go b/pkg/util/tls/tls_test.go |
| 272 | +index e3776ec..cd6ee90 100644 |
| 273 | +--- a/pkg/util/tls/tls_test.go |
| 274 | ++++ b/pkg/util/tls/tls_test.go |
| 275 | +@@ -31,6 +31,7 @@ import ( |
| 276 | + |
| 277 | + type mockCAManager struct { |
| 278 | + caBundle []byte |
| 279 | ++ cns []string |
| 280 | + } |
| 281 | + |
| 282 | + type mockCertManager struct { |
| 283 | +@@ -66,9 +67,13 @@ func (m *mockCAManager) GetCurrentRaw() ([]byte, error) { |
| 284 | + return nil, fmt.Errorf("not implemented") |
| 285 | + } |
| 286 | + |
| 287 | ++func (m *mockCAManager) GetCNs() ([]string, error) { |
| 288 | ++ return m.cns, nil |
| 289 | ++} |
| 290 | ++ |
| 291 | + var _ = Describe("TLS", func() { |
| 292 | + |
| 293 | +- var caManager kvtls.ClientCAManager |
| 294 | ++ var caManager kvtls.KubernetesCAManager |
| 295 | + var certmanagers map[string]certificate.Manager |
| 296 | + var clusterConfig *virtconfig.ClusterConfig |
| 297 | + var kubeVirtInformer cache.SharedIndexInformer |
| 298 | +@@ -220,7 +225,8 @@ var _ = Describe("TLS", func() { |
| 299 | + }), |
| 300 | + ) |
| 301 | + |
| 302 | +- DescribeTable("should verify self-signed client and server certificates", func(serverSecret, clientSecret string, errStr string) { |
| 303 | ++ DescribeTable("should verify self-signed client and server certificates", func(serverSecret, clientSecret, errStr string, cns []string) { |
| 304 | ++ caManager.(*mockCAManager).cns = cns |
| 305 | + serverTLSConfig := kvtls.SetupTLSWithCertManager(caManager, certmanagers[serverSecret], tls.RequireAndVerifyClientCert, clusterConfig) |
| 306 | + clientTLSConfig := kvtls.SetupTLSForVirtHandlerClients(caManager, certmanagers[clientSecret], false) |
| 307 | + srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| 308 | +@@ -248,18 +254,35 @@ var _ = Describe("TLS", func() { |
| 309 | + components.VirtHandlerServerCertSecretName, |
| 310 | + components.VirtHandlerCertSecretName, |
| 311 | + "", |
| 312 | ++ []string{"kubevirt.io:system:client:virt-handler"}, |
| 313 | ++ ), |
| 314 | ++ Entry( |
| 315 | ++ "connect with proper certificates with no CN auth", |
| 316 | ++ components.VirtHandlerServerCertSecretName, |
| 317 | ++ components.VirtHandlerCertSecretName, |
| 318 | ++ "", |
| 319 | ++ []string{}, |
| 320 | ++ ), |
| 321 | ++ Entry( |
| 322 | ++ "fail if client uses an invalid certificates (CN)", |
| 323 | ++ components.VirtHandlerServerCertSecretName, |
| 324 | ++ components.VirtHandlerCertSecretName, |
| 325 | ++ "remote error: tls: bad certificate", |
| 326 | ++ []string{"kubevirt.io:system:clientv2:virt-handler"}, |
| 327 | + ), |
| 328 | + Entry( |
| 329 | + "fail if client uses an invalid certificate", |
| 330 | + components.VirtHandlerServerCertSecretName, |
| 331 | + components.VirtHandlerServerCertSecretName, |
| 332 | + "remote error: tls: bad certificate", |
| 333 | ++ []string{"kubevirt.io:system:client:virt-handler"}, |
| 334 | + ), |
| 335 | + Entry( |
| 336 | + "fail if server uses an invalid certificate", |
| 337 | + components.VirtHandlerCertSecretName, |
| 338 | + components.VirtHandlerCertSecretName, |
| 339 | + "x509: certificate specifies an incompatible key usage", |
| 340 | ++ []string{"kubevirt.io:system:client:virt-handler"}, |
| 341 | + ), |
| 342 | + ) |
| 343 | + |
| 344 | +diff --git a/pkg/virt-api/api.go b/pkg/virt-api/api.go |
| 345 | +index 5b31c3d..8f693cb 100644 |
| 346 | +--- a/pkg/virt-api/api.go |
| 347 | ++++ b/pkg/virt-api/api.go |
| 348 | +@@ -858,7 +858,7 @@ func (app *virtAPIApp) registerMutatingWebhook(informers *webhooks.Informers) { |
| 349 | + }) |
| 350 | + } |
| 351 | + |
| 352 | +-func (app *virtAPIApp) setupTLS(k8sCAManager kvtls.ClientCAManager, kubevirtCAManager kvtls.ClientCAManager) { |
| 353 | ++func (app *virtAPIApp) setupTLS(k8sCAManager kvtls.KubernetesCAManager, kubevirtCAManager kvtls.ClientCAManager) { |
| 354 | + |
| 355 | + // A VerifyClientCertIfGiven request means we're not guaranteed |
| 356 | + // a client has been authenticated unless they provide a peer |
| 357 | +-- |
| 358 | +2.45.4 |
| 359 | + |
0 commit comments