From 9b7d654a08d694d20226609f7075b112fb18639b Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 9 Apr 2021 16:59:17 -0700 Subject: [PATCH] force implementors of dyanmiccertificates providers to think about notify Right now, `_, ok := provider.(Notifier); !ok` can mean one of two things: 1. The provider does not support notification because the provided content is static. 2. The implementor of the provider hasn't gotten around to implementing Notifier yet. These have very different implications. We should not force consumers of these interfaces to have to figure out the static of Notifier across sometimes numerous different implementations. Instead, we should force implementors to implement Notifier, even if it's a noop. Change-Id: Ie7a26697a9a17790bfaa58d67045663bcc71e3cb --- pkg/controlplane/instance.go | 8 +-- .../authenticatorfactory/delegating.go | 3 +- .../authenticatorfactory/requestheader.go | 15 +--- .../server/dynamiccertificates/cert_key.go | 15 ---- .../server/dynamiccertificates/client_ca.go | 12 ---- .../configmap_cafile_content.go | 1 - .../dynamic_cafile_content.go | 12 ---- .../dynamic_serving_content.go | 1 - .../dynamic_sni_content.go | 1 - .../server/dynamiccertificates/interfaces.go | 68 +++++++++++++++++++ .../server/dynamiccertificates/server_test.go | 2 + .../dynamiccertificates/static_content.go | 36 ++++++---- .../dynamiccertificates/union_content.go | 5 +- .../authentication_dynamic_request_header.go | 1 - .../apiserver/pkg/server/secure_serving.go | 16 ++--- 15 files changed, 105 insertions(+), 91 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/interfaces.go diff --git a/pkg/controlplane/instance.go b/pkg/controlplane/instance.go index 9bba2ebd51b..16d376e5bef 100644 --- a/pkg/controlplane/instance.go +++ b/pkg/controlplane/instance.go @@ -464,9 +464,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget) // prime values and start listeners if m.ClusterAuthenticationInfo.ClientCA != nil { - if notifier, ok := m.ClusterAuthenticationInfo.ClientCA.(dynamiccertificates.Notifier); ok { - notifier.AddListener(controller) - } + m.ClusterAuthenticationInfo.ClientCA.AddListener(controller) if controller, ok := m.ClusterAuthenticationInfo.ClientCA.(dynamiccertificates.ControllerRunner); ok { // runonce to be sure that we have a value. if err := controller.RunOnce(); err != nil { @@ -476,9 +474,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget) } } if m.ClusterAuthenticationInfo.RequestHeaderCA != nil { - if notifier, ok := m.ClusterAuthenticationInfo.RequestHeaderCA.(dynamiccertificates.Notifier); ok { - notifier.AddListener(controller) - } + m.ClusterAuthenticationInfo.RequestHeaderCA.AddListener(controller) if controller, ok := m.ClusterAuthenticationInfo.RequestHeaderCA.(dynamiccertificates.ControllerRunner); ok { // runonce to be sure that we have a value. if err := controller.RunOnce(); err != nil { diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go index 83697bb5470..c5ecc86d194 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go @@ -32,6 +32,7 @@ import ( "k8s.io/apiserver/pkg/authentication/request/websocket" "k8s.io/apiserver/pkg/authentication/request/x509" "k8s.io/apiserver/pkg/authentication/token/cache" + "k8s.io/apiserver/pkg/server/dynamiccertificates" webhooktoken "k8s.io/apiserver/plugin/pkg/authenticator/token/webhook" authenticationclient "k8s.io/client-go/kubernetes/typed/authentication/v1" ) @@ -55,7 +56,7 @@ type DelegatingAuthenticatorConfig struct { // CAContentProvider are the options for verifying incoming connections using mTLS and directly assigning to users. // Generally this is the CA bundle file used to authenticate client certificates // If this is nil, then mTLS will not be used. - ClientCertificateCAContentProvider CAContentProvider + ClientCertificateCAContentProvider dynamiccertificates.CAContentProvider APIAudiences authenticator.Audiences diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/requestheader.go b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/requestheader.go index b5aa1c524a7..766bde51727 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/requestheader.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/requestheader.go @@ -17,9 +17,8 @@ limitations under the License. package authenticatorfactory import ( - "crypto/x509" - "k8s.io/apiserver/pkg/authentication/request/headerrequest" + "k8s.io/apiserver/pkg/server/dynamiccertificates" ) type RequestHeaderConfig struct { @@ -32,17 +31,7 @@ type RequestHeaderConfig struct { ExtraHeaderPrefixes headerrequest.StringSliceProvider // CAContentProvider the options for verifying incoming connections using mTLS. Generally this points to CA bundle file which is used verify the identity of the front proxy. // It may produce different options at will. - CAContentProvider CAContentProvider + CAContentProvider dynamiccertificates.CAContentProvider // AllowedClientNames is a list of common names that may be presented by the authenticating front proxy. Empty means: accept any. AllowedClientNames headerrequest.StringSliceProvider } - -// CAContentProvider provides ca bundle byte content -type CAContentProvider interface { - // Name is just an identifier - Name() string - // CurrentCABundleContent provides ca bundle byte content - CurrentCABundleContent() []byte - // VerifyOptions provides VerifyOptions for authenticators - VerifyOptions() (x509.VerifyOptions, bool) -} diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/cert_key.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/cert_key.go index 114002b1a07..6a7b93ad2b4 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/cert_key.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/cert_key.go @@ -20,21 +20,6 @@ import ( "bytes" ) -// CertKeyContentProvider provides a certificate and matching private key -type CertKeyContentProvider interface { - // Name is just an identifier - Name() string - // CurrentCertKeyContent provides cert and key byte content - CurrentCertKeyContent() ([]byte, []byte) -} - -// SNICertKeyContentProvider provides a certificate and matching private key as well as optional explicit names -type SNICertKeyContentProvider interface { - CertKeyContentProvider - // SNINames provides names used for SNI. May return nil. - SNINames() []string -} - // certKeyContent holds the content for the cert and key type certKeyContent struct { cert []byte diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/client_ca.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/client_ca.go index 4348fa38784..2c950f23615 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/client_ca.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/client_ca.go @@ -18,20 +18,8 @@ package dynamiccertificates import ( "bytes" - "crypto/x509" ) -// CAContentProvider provides ca bundle byte content -type CAContentProvider interface { - // Name is just an identifier - Name() string - // CurrentCABundleContent provides ca bundle byte content. Errors can be contained to the controllers initializing - // the value. By the time you get here, you should always be returning a value that won't fail. - CurrentCABundleContent() []byte - // VerifyOptions provides VerifyOptions for authenticators - VerifyOptions() (x509.VerifyOptions, bool) -} - // dynamicCertificateContent holds the content that overrides the baseTLSConfig type dynamicCertificateContent struct { // clientCA holds the content for the clientCA bundle diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go index ec0fc5096a2..f3d6b9680ee 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go @@ -58,7 +58,6 @@ type ConfigMapCAController struct { preRunCaches []cache.InformerSynced } -var _ Notifier = &ConfigMapCAController{} var _ CAContentProvider = &ConfigMapCAController{} var _ ControllerRunner = &ConfigMapCAController{} diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go index 756289a802d..6995334b37a 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go @@ -35,18 +35,6 @@ import ( // FileRefreshDuration is exposed so that integration tests can crank up the reload speed. var FileRefreshDuration = 1 * time.Minute -// Listener is an interface to use to notify interested parties of a change. -type Listener interface { - // Enqueue should be called when an input may have changed - Enqueue() -} - -// Notifier is a way to add listeners -type Notifier interface { - // AddListener is adds a listener to be notified of potential input changes - AddListener(listener Listener) -} - // ControllerRunner is a generic interface for starting a controller type ControllerRunner interface { // RunOnce runs the sync loop a single time. This useful for synchronous priming diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go index 3b7f34738b2..9a5dc0f4b50 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go @@ -47,7 +47,6 @@ type DynamicCertKeyPairContent struct { queue workqueue.RateLimitingInterface } -var _ Notifier = &DynamicCertKeyPairContent{} var _ CertKeyContentProvider = &DynamicCertKeyPairContent{} var _ ControllerRunner = &DynamicCertKeyPairContent{} diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_sni_content.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_sni_content.go index 161fa1ca759..621fc9ff0b8 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_sni_content.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_sni_content.go @@ -22,7 +22,6 @@ type DynamicFileSNIContent struct { sniNames []string } -var _ Notifier = &DynamicFileSNIContent{} var _ SNICertKeyContentProvider = &DynamicFileSNIContent{} var _ ControllerRunner = &DynamicFileSNIContent{} diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/interfaces.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/interfaces.go new file mode 100644 index 00000000000..7811c5d8bfe --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/interfaces.go @@ -0,0 +1,68 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package dynamiccertificates + +import ( + "crypto/x509" +) + +// Listener is an interface to use to notify interested parties of a change. +type Listener interface { + // Enqueue should be called when an input may have changed + Enqueue() +} + +// Notifier is a way to add listeners +type Notifier interface { + // AddListener is adds a listener to be notified of potential input changes. + // This is a noop on static providers. + AddListener(listener Listener) +} + +// CAContentProvider provides ca bundle byte content +type CAContentProvider interface { + Notifier + + // Name is just an identifier. + Name() string + // CurrentCABundleContent provides ca bundle byte content. Errors can be + // contained to the controllers initializing the value. By the time you get + // here, you should always be returning a value that won't fail. + CurrentCABundleContent() []byte + // VerifyOptions provides VerifyOptions for authenticators. + VerifyOptions() (x509.VerifyOptions, bool) +} + +// CertKeyContentProvider provides a certificate and matching private key. +type CertKeyContentProvider interface { + Notifier + + // Name is just an identifier. + Name() string + // CurrentCertKeyContent provides cert and key byte content. + CurrentCertKeyContent() ([]byte, []byte) +} + +// SNICertKeyContentProvider provides a certificate and matching private key as +// well as optional explicit names. +type SNICertKeyContentProvider interface { + Notifier + + CertKeyContentProvider + // SNINames provides names used for SNI. May return nil. + SNINames() []string +} diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go index 4a1a9761040..777abae9cc8 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go @@ -208,6 +208,8 @@ func (c *nullCAContent) Name() string { return c.name } +func (c *nullCAContent) AddListener(Listener) {} + // CurrentCABundleContent provides ca bundle byte content func (c *nullCAContent) CurrentCABundleContent() (cabundle []byte) { return nil diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/static_content.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/static_content.go index c877dfe6c6c..1934ed1d886 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/static_content.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/static_content.go @@ -46,6 +46,8 @@ func (c *staticCAContent) Name() string { return c.name } +func (c *staticCAContent) AddListener(Listener) {} + // CurrentCABundleContent provides ca bundle byte content func (c *staticCAContent) CurrentCABundleContent() (cabundle []byte) { return c.caBundle.caBundle @@ -61,11 +63,6 @@ type staticCertKeyContent struct { key []byte } -type staticSNICertKeyContent struct { - staticCertKeyContent - sniNames []string -} - // NewStaticCertKeyContent returns a CertKeyContentProvider that always returns the same value func NewStaticCertKeyContent(name string, cert, key []byte) (CertKeyContentProvider, error) { // Ensure that the key matches the cert and both are valid @@ -81,6 +78,23 @@ func NewStaticCertKeyContent(name string, cert, key []byte) (CertKeyContentProvi }, nil } +// Name is just an identifier +func (c *staticCertKeyContent) Name() string { + return c.name +} + +func (c *staticCertKeyContent) AddListener(Listener) {} + +// CurrentCertKeyContent provides cert and key content +func (c *staticCertKeyContent) CurrentCertKeyContent() ([]byte, []byte) { + return c.cert, c.key +} + +type staticSNICertKeyContent struct { + staticCertKeyContent + sniNames []string +} + // NewStaticSNICertKeyContent returns a SNICertKeyContentProvider that always returns the same value func NewStaticSNICertKeyContent(name string, cert, key []byte, sniNames ...string) (SNICertKeyContentProvider, error) { // Ensure that the key matches the cert and both are valid @@ -99,16 +113,8 @@ func NewStaticSNICertKeyContent(name string, cert, key []byte, sniNames ...strin }, nil } -// Name is just an identifier -func (c *staticCertKeyContent) Name() string { - return c.name -} - -// CurrentCertKeyContent provides cert and key content -func (c *staticCertKeyContent) CurrentCertKeyContent() ([]byte, []byte) { - return c.cert, c.key -} - func (c *staticSNICertKeyContent) SNINames() []string { return c.sniNames } + +func (c *staticSNICertKeyContent) AddListener(Listener) {} diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/union_content.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/union_content.go index 89e19ea5a3a..e10b112bc07 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/union_content.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/union_content.go @@ -26,7 +26,6 @@ import ( type unionCAContent []CAContentProvider -var _ Notifier = &unionCAContent{} var _ CAContentProvider = &unionCAContent{} var _ ControllerRunner = &unionCAContent{} @@ -77,9 +76,7 @@ func (c unionCAContent) VerifyOptions() (x509.VerifyOptions, bool) { // AddListener adds a listener to be notified when the CA content changes. func (c unionCAContent) AddListener(listener Listener) { for _, curr := range c { - if notifier, ok := curr.(Notifier); ok { - notifier.AddListener(listener) - } + curr.AddListener(listener) } } diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/authentication_dynamic_request_header.go b/staging/src/k8s.io/apiserver/pkg/server/options/authentication_dynamic_request_header.go index 5c558b06d68..e2beb5c2382 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/options/authentication_dynamic_request_header.go +++ b/staging/src/k8s.io/apiserver/pkg/server/options/authentication_dynamic_request_header.go @@ -26,7 +26,6 @@ import ( ) var _ dynamiccertificates.ControllerRunner = &DynamicRequestHeaderController{} -var _ dynamiccertificates.Notifier = &DynamicRequestHeaderController{} var _ dynamiccertificates.CAContentProvider = &DynamicRequestHeaderController{} var _ headerrequest.RequestHeaderAuthRequestProvider = &DynamicRequestHeaderController{} diff --git a/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go b/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go index 38341eb03bd..356bd3a2fe8 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go +++ b/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go @@ -86,13 +86,14 @@ func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, erro s.SNICerts, nil, // TODO see how to plumb an event recorder down in here. For now this results in simply klog messages. ) - // register if possible - if notifier, ok := s.ClientCA.(dynamiccertificates.Notifier); ok { - notifier.AddListener(dynamicCertificateController) + + if s.ClientCA != nil { + s.ClientCA.AddListener(dynamicCertificateController) } - if notifier, ok := s.Cert.(dynamiccertificates.Notifier); ok { - notifier.AddListener(dynamicCertificateController) + if s.Cert != nil { + s.Cert.AddListener(dynamicCertificateController) } + // start controllers if possible if controller, ok := s.ClientCA.(dynamiccertificates.ControllerRunner); ok { // runonce to try to prime data. If this fails, it's ok because we fail closed. @@ -113,10 +114,7 @@ func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, erro go controller.Run(1, stopCh) } for _, sniCert := range s.SNICerts { - if notifier, ok := sniCert.(dynamiccertificates.Notifier); ok { - notifier.AddListener(dynamicCertificateController) - } - + sniCert.AddListener(dynamicCertificateController) if controller, ok := sniCert.(dynamiccertificates.ControllerRunner); ok { // runonce to try to prime data. If this fails, it's ok because we fail closed. // Files are required to be populated already, so this is for convenience.