From 15d00068418105548de9dc81cabc71212aa65c1e Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Fri, 26 Jul 2024 09:15:47 -0500 Subject: [PATCH] Pull tlsconfigutil.CABundle into a separate file --- .../jwtcachefiller/jwtcachefiller.go | 5 +- .../github_upstream_watcher.go | 1 + .../controller/tlsconfigutil/ca_bundle.go | 52 ++++++++++++ .../tlsconfigutil/ca_bundle_test.go | 76 +++++++++++++++++ .../tlsconfigutil/tls_config_util.go | 42 +--------- .../tlsconfigutil/tls_config_util_test.go | 81 ++----------------- internal/upstreamldap/upstreamldap.go | 3 +- 7 files changed, 143 insertions(+), 117 deletions(-) create mode 100644 internal/controller/tlsconfigutil/ca_bundle.go create mode 100644 internal/controller/tlsconfigutil/ca_bundle_test.go diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 2db9bea1e..272541c53 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -222,7 +222,6 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co conditions := make([]*metav1.Condition, 0) caBundle, conditions, tlsBundleOk := c.validateTLSBundle(jwtAuthenticator.Spec.TLS, conditions) - caBundlePEMSHA256 := caBundle.GetCABundleHash() // Only revalidate and update the cache if the cached authenticator is different from the desired authenticator. // There is no need to repeat validations for a spec that was already successfully validated. We are making a @@ -238,7 +237,7 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co if jwtAuthenticatorFromCache != nil && reflect.DeepEqual(jwtAuthenticatorFromCache.spec, &jwtAuthenticator.Spec) && tlsBundleOk && // if there was any error while validating the CA bundle, then run remaining validations and update status - jwtAuthenticatorFromCache.caBundlePEMSHA256 == caBundlePEMSHA256 { + jwtAuthenticatorFromCache.caBundlePEMSHA256 == caBundle.GetCABundleHash() { c.log.WithValues("jwtAuthenticator", klog.KObj(jwtAuthenticator), "issuer", jwtAuthenticator.Spec.Issuer). Info("actual jwt authenticator and desired jwt authenticator are the same") // Stop, no more work to be done. This authenticator is already validated and cached. @@ -270,7 +269,7 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co client, jwtAuthenticator.Spec.DeepCopy(), // deep copy to avoid caching original object keySet, - caBundlePEMSHA256, + caBundle.GetCABundleHash(), conditions, okSoFar) errs = append(errs, err) diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index db1301376..fcd700478 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -425,6 +425,7 @@ func validateHost(gitHubAPIConfig idpv1alpha1.GitHubAPIConfig) (*metav1.Conditio }, &hostPort } +// TODO: this should take in a tlsconfigutil.CABundle func (c *gitHubWatcherController) validateGitHubConnection( hostPort *endpointaddr.HostPort, caBundlePEM []byte, diff --git a/internal/controller/tlsconfigutil/ca_bundle.go b/internal/controller/tlsconfigutil/ca_bundle.go new file mode 100644 index 000000000..c2726f3f3 --- /dev/null +++ b/internal/controller/tlsconfigutil/ca_bundle.go @@ -0,0 +1,52 @@ +package tlsconfigutil + +import ( + "crypto/sha256" + "crypto/x509" +) + +// CABundle abstracts the internal representation of CA certificate bundles. +type CABundle struct { + caBundle []byte + sha256 [32]byte + certPool *x509.CertPool +} + +func NewCABundle(caBundle []byte, certPool *x509.CertPool) *CABundle { + return &CABundle{ + caBundle: caBundle, + sha256: sha256.Sum256(caBundle), + certPool: certPool, + } +} + +// GetCABundle returns the CA certificate bundle PEM bytes. +func (c *CABundle) GetCABundle() []byte { + return c.caBundle +} + +// GetCABundlePemString returns the certificate bundle PEM formatted as a string. +func (c *CABundle) GetCABundlePemString() string { + return string(c.caBundle) +} + +// GetCertPool returns a X509 cert pool with the CA certificate bundle. +func (c *CABundle) GetCertPool() *x509.CertPool { + return c.certPool +} + +// GetCABundleHash returns a sha256 sum of the CA bundle bytes. +func (c *CABundle) GetCABundleHash() [32]byte { + return sha256.Sum256(c.caBundle) // note that this will always return the same hash for nil input +} + +// IsEqual returns whether a CABundle has the same CA certificate bundle as another. +func (c *CABundle) IsEqual(other *CABundle) bool { + if c == nil && other == nil { + return true + } + if c == nil || other == nil { + return false + } + return sha256.Sum256(c.caBundle) == sha256.Sum256(other.GetCABundle()) +} diff --git a/internal/controller/tlsconfigutil/ca_bundle_test.go b/internal/controller/tlsconfigutil/ca_bundle_test.go new file mode 100644 index 000000000..1265bbf37 --- /dev/null +++ b/internal/controller/tlsconfigutil/ca_bundle_test.go @@ -0,0 +1,76 @@ +package tlsconfigutil + +import ( + "crypto/x509" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/certauthority" +) + +func TestCABundleIsEqual(t *testing.T) { + testCA, err := certauthority.New("Test CA", 1*time.Hour) + require.NoError(t, err) + certPool := x509.NewCertPool() + require.True(t, certPool.AppendCertsFromPEM(testCA.Bundle())) + + tests := []struct { + name string + left *CABundle + right *CABundle + expected bool + }{ + { + name: "should return equal when left and right are nil", + left: nil, + right: nil, + expected: true, + }, + { + name: "should return not equal when left is nil and right is not", + left: nil, + right: &CABundle{}, + expected: false, + }, + { + name: "should return not equal when right is nil and left is not", + left: &CABundle{}, + right: nil, + expected: false, + }, + { + name: "should return equal when both left and right have same CA certificate bytes", + left: &CABundle{ + caBundle: testCA.Bundle(), + certPool: certPool, + }, + right: &CABundle{ + caBundle: testCA.Bundle(), + certPool: certPool, + }, + expected: true, + }, + { + name: "should return not equal when both left and right do not have same CA certificate bytes", + left: &CABundle{ + caBundle: testCA.Bundle(), + certPool: certPool, + }, + right: &CABundle{ + caBundle: []byte("something that is not a cert"), + certPool: nil, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + actual := tt.left.IsEqual(tt.right) + require.Equal(t, tt.expected, actual) + }) + } +} diff --git a/internal/controller/tlsconfigutil/tls_config_util.go b/internal/controller/tlsconfigutil/tls_config_util.go index a26e7738e..1a3ceeaa5 100644 --- a/internal/controller/tlsconfigutil/tls_config_util.go +++ b/internal/controller/tlsconfigutil/tls_config_util.go @@ -4,7 +4,6 @@ package tlsconfigutil import ( - "crypto/sha256" "crypto/x509" "encoding/base64" "fmt" @@ -81,43 +80,6 @@ func TLSSpecForConcierge(source *authenticationv1alpha1.TLSSpec) *TLSSpec { return dest } -// CABundle abstracts the internal representation of CA certificate bundles. -type CABundle struct { - caBundle []byte - caCertPool *x509.CertPool -} - -// GetCABundle returns the CA certificate bundle PEM bytes. -func (c *CABundle) GetCABundle() []byte { - return c.caBundle -} - -// GetCABundlePemString returns the certificate bundle PEM formatted as a string. -func (c *CABundle) GetCABundlePemString() string { - return string(c.caBundle) -} - -// GetCertPool returns a X509 cert pool with the CA certificate bundle. -func (c *CABundle) GetCertPool() *x509.CertPool { - return c.caCertPool -} - -// GetCABundleHash returns a sha256 sum of the CA bundle bytes. -func (c *CABundle) GetCABundleHash() [32]byte { - return sha256.Sum256(c.caBundle) // note that this will always return the same hash for nil input -} - -// IsEqual returns whether a CABundle has the same CA certificate bundle as another. -func (l *CABundle) IsEqual(r *CABundle) bool { - if l == nil && r == nil { - return true - } - if l == nil || r == nil { - return false - } - return sha256.Sum256(l.caBundle) == sha256.Sum256(r.GetCABundle()) -} - // ValidateTLSConfig reads ca bundle in the tlsSpec, supplied either inline using the CertificateAuthorityDate // or as a reference to a kubernetes secret or configmap using the CertificateAuthorityDataSource, and returns // - a condition of type TLSConfigurationValid based on the validity of the ca bundle, @@ -137,14 +99,14 @@ func ValidateTLSConfig( certPool, bundle, err := getCertPool(tlsSpec, conditionPrefix, namespace, secretInformer, configMapInformer) if err != nil { - return invalidTLSCondition(err.Error()), &CABundle{} + return invalidTLSCondition(err.Error()), nil } if bundle == nil { // An empty or nil CA bundle results in a valid TLS condition which indicates that no CA data was supplied. return validTLSCondition(fmt.Sprintf("%s is valid: %s", conditionPrefix, noTLSConfigurationMessage)), nil } return validTLSCondition(fmt.Sprintf("%s is valid: %s", conditionPrefix, loadedTLSConfigurationMessage)), - &CABundle{bundle, certPool} + NewCABundle(bundle, certPool) } // getCertPool reads the unified tlsSpec and returns an X509 cert pool with the CA data that is read either from diff --git a/internal/controller/tlsconfigutil/tls_config_util_test.go b/internal/controller/tlsconfigutil/tls_config_util_test.go index eb626013a..9c38d7715 100644 --- a/internal/controller/tlsconfigutil/tls_config_util_test.go +++ b/internal/controller/tlsconfigutil/tls_config_util_test.go @@ -65,8 +65,8 @@ func TestValidateTLSConfig(t *testing.T) { CertificateAuthorityData: base64EncodedBundle, }, expectedCABundle: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, + caBundle: testCA.Bundle(), + certPool: certPool, }, expectedCondition: &metav1.Condition{ Type: typeTLSConfigurationValid, @@ -139,8 +139,8 @@ func TestValidateTLSConfig(t *testing.T) { }, }, expectedCABundle: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, + caBundle: testCA.Bundle(), + certPool: certPool, }, expectedCondition: &metav1.Condition{ Type: typeTLSConfigurationValid, @@ -172,8 +172,8 @@ func TestValidateTLSConfig(t *testing.T) { }, }, expectedCABundle: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, + caBundle: testCA.Bundle(), + certPool: certPool, }, expectedCondition: &metav1.Condition{ Type: typeTLSConfigurationValid, @@ -404,8 +404,8 @@ func TestValidateTLSConfig(t *testing.T) { }, }, expectedCABundle: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, + caBundle: testCA.Bundle(), + certPool: certPool, }, expectedCondition: &metav1.Condition{ Type: typeTLSConfigurationValid, @@ -662,68 +662,3 @@ func TestTLSSpecForConcierge(t *testing.T) { }) } } - -func TestCABundleIsEqual(t *testing.T) { - testCA, err := certauthority.New("Test CA", 1*time.Hour) - require.NoError(t, err) - certPool := x509.NewCertPool() - require.True(t, certPool.AppendCertsFromPEM(testCA.Bundle())) - - tests := []struct { - name string - left *CABundle - right *CABundle - expected bool - }{ - { - name: "should return equal when left and right are nil", - left: nil, - right: nil, - expected: true, - }, - { - name: "should return not equal when left is nil and right is not", - left: nil, - right: &CABundle{}, - expected: false, - }, - { - name: "should return not equal when right is nil and left is not", - left: &CABundle{}, - right: nil, - expected: false, - }, - { - name: "should return equal when both left and right have same CA certificate bytes", - left: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, - }, - right: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, - }, - expected: true, - }, - { - name: "should return not equal when both left and right do not have same CA certificate bytes", - left: &CABundle{ - caBundle: testCA.Bundle(), - caCertPool: certPool, - }, - right: &CABundle{ - caBundle: []byte("something that is not a cert"), - caCertPool: nil, - }, - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - actual := tt.left.IsEqual(tt.right) - require.Equal(t, tt.expected, actual) - }) - } -} diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 2821a4483..8828ccd9f 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -74,7 +74,7 @@ const ( TLS = LDAPConnectionProtocol("TLS") ) -// ProviderConfig includes all of the settings for connection and searching for users and groups in +// ProviderConfig includes all the settings for connection and searching for users and groups in // the upstream LDAP IDP. It also provides methods for testing the connection and performing logins. // The nested structs are not pointer fields to enable deep copy on function params and return values. type ProviderConfig struct { @@ -92,6 +92,7 @@ type ProviderConfig struct { ConnectionProtocol LDAPConnectionProtocol // PEM-encoded CA cert bundle to trust when connecting to the LDAP server. Can be nil. + // TODO: should this be a tlsconfigutil.CABundle? CABundle []byte // BindUsername is the username to use when performing a bind with the upstream LDAP IDP.