-
Notifications
You must be signed in to change notification settings - Fork 123
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ETCD-535: Manual CA rotation should rotate all leaf certs
Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
- Loading branch information
Showing
8 changed files
with
357 additions
and
44 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package tlshelpers | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "crypto/x509" | ||
| "fmt" | ||
| "github.com/openshift/library-go/pkg/crypto" | ||
| "github.com/openshift/library-go/pkg/operator/certrotation" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "time" | ||
| ) | ||
|
|
||
| // CARotatingTargetCertCreator ensures we also rotate leaf certificates when we detect a change in signer. | ||
| // The certrotation.TargetCertCreator only assumes the bundle to change on a CA rotation, whereas we have to keep | ||
| // the bundle around for some time for a proper static pod rollout. | ||
| type CARotatingTargetCertCreator struct { | ||
| certrotation.TargetCertCreator | ||
| } | ||
|
|
||
| func (c *CARotatingTargetCertCreator) NeedNewTargetCertKeyPair( | ||
| secret *corev1.Secret, | ||
| signer *crypto.CA, | ||
| caBundleCerts []*x509.Certificate, | ||
| refresh time.Duration, | ||
| refreshOnlyWhenExpired bool) string { | ||
|
|
||
| result := c.TargetCertCreator.NeedNewTargetCertKeyPair(secret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) | ||
| if result != "" { | ||
| return result | ||
| } | ||
|
|
||
| // TODO(thomas): we measured that this parsing is not very CPU intensive compared to TLS handshakes to etcd | ||
| // we could save about 3-5% cpu usage here by caching the certs based on their hashes. | ||
| var currentCert *x509.Certificate | ||
| if crtBytes, ok := secret.Data["tls.crt"]; ok { | ||
| pemCerts, err := crypto.CertsFromPEM(crtBytes) | ||
| if err != nil { | ||
| return fmt.Sprintf("could not parse pem x509 tls.crt in secret %s: %v", secret.Name, err) | ||
| } | ||
| if len(pemCerts) > 0 { | ||
| currentCert = pemCerts[len(pemCerts)-1] | ||
| } | ||
| } | ||
|
|
||
| if currentCert == nil { | ||
| return fmt.Sprintf("missing current certificate in secret: %s", secret.Name) | ||
| } | ||
|
|
||
| // in some cases, e.g. with etcd, we need to bundle the signer CA before we can rotate a certificate | ||
| // hence we also check whether the signer itself has changed, denoted by its AKI/SKI | ||
| if len(currentCert.AuthorityKeyId) > 0 && len(signer.Config.Certs) > 0 && len(signer.Config.Certs[0].SubjectKeyId) > 0 { | ||
| if !bytes.Equal(currentCert.AuthorityKeyId, signer.Config.Certs[0].SubjectKeyId) { | ||
| return fmt.Sprintf("signer subject key for secret %s does not match cert authority key anymore", secret.Name) | ||
| } | ||
| } else { | ||
| // no AKI/SKI available to us, we have to check whether the cert was actually signed with this signer | ||
| pool := x509.NewCertPool() | ||
| for _, crt := range signer.Config.Certs { | ||
| pool.AddCert(crt) | ||
| } | ||
|
|
||
| _, err := currentCert.Verify(x509.VerifyOptions{ | ||
| Roots: pool, | ||
| KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Sprintf("cert isn't signed by most recent signer anymore: %v", err) | ||
| } | ||
| } | ||
|
|
||
| return "" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,228 @@ | ||
| package tlshelpers | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| gcrypto "crypto" | ||
| "crypto/rand" | ||
| "crypto/x509" | ||
| "crypto/x509/pkix" | ||
| "errors" | ||
| "github.com/openshift/library-go/pkg/operator/certrotation" | ||
| "github.com/stretchr/testify/require" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "math/big" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/davecgh/go-spew/spew" | ||
|
|
||
| "github.com/openshift/library-go/pkg/crypto" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| kubefake "k8s.io/client-go/kubernetes/fake" | ||
| corev1listers "k8s.io/client-go/listers/core/v1" | ||
| "k8s.io/client-go/tools/cache" | ||
| ) | ||
|
|
||
| type testEmbed struct { | ||
| result string | ||
| } | ||
|
|
||
| func (t *testEmbed) NewCertificate(_ *crypto.CA, _ time.Duration) (*crypto.TLSCertificateConfig, error) { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (t *testEmbed) SetAnnotations(_ *crypto.TLSCertificateConfig, _ map[string]string) map[string]string { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (t *testEmbed) NeedNewTargetCertKeyPair(_ *corev1.Secret, _ *crypto.CA, _ []*x509.Certificate, _ time.Duration, _ bool) string { | ||
| return t.result | ||
| } | ||
|
|
||
| func TestEmbeddedStructHasPriority(t *testing.T) { | ||
| embedded := CARotatingTargetCertCreator{&testEmbed{result: "definitive-result"}} | ||
| require.Equal(t, "definitive-result", embedded.NeedNewTargetCertKeyPair(nil, nil, nil, time.Minute, false)) | ||
| } | ||
|
|
||
| func TestSignerSignatureRotation(t *testing.T) { | ||
| newCaWithAuthority := func() (*crypto.CA, error) { | ||
| ski := make([]byte, 32) | ||
| _, err := rand.Read(ski) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return newTestCACertificateWithAuthority(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now, ski) | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| caFn func() (*crypto.CA, error) | ||
| matchingLeaf bool | ||
| expectRotation bool | ||
| }{ | ||
| { | ||
| name: "leaf matches signer, no authority set", | ||
| matchingLeaf: true, | ||
| expectRotation: false, | ||
| caFn: func() (*crypto.CA, error) { | ||
| return newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now) | ||
| }, | ||
| }, | ||
| { | ||
| name: "leaf matches signer, authority set", | ||
| matchingLeaf: true, | ||
| expectRotation: false, | ||
| caFn: newCaWithAuthority, | ||
| }, | ||
| { | ||
| name: "leaf does not match signer, authority set", | ||
| matchingLeaf: false, | ||
| expectRotation: true, | ||
| caFn: newCaWithAuthority, | ||
| }, | ||
| { | ||
| name: "leaf does not match signer, no authority set", | ||
| matchingLeaf: false, | ||
| expectRotation: true, | ||
| caFn: func() (*crypto.CA, error) { | ||
| return newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now) | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) | ||
|
|
||
| client := kubefake.NewSimpleClientset() | ||
| c := &certrotation.RotatedSelfSignedCertKeySecret{ | ||
| Namespace: "ns", | ||
| Validity: 24 * time.Hour, | ||
| Refresh: 12 * time.Hour, | ||
| Name: "target-secret", | ||
| CertCreator: &CARotatingTargetCertCreator{ | ||
| &certrotation.SignerRotation{SignerName: "lower-signer"}, | ||
| }, | ||
|
|
||
| Client: client.CoreV1(), | ||
| Lister: corev1listers.NewSecretLister(indexer), | ||
| EventRecorder: events.NewInMemoryRecorder("test"), | ||
| } | ||
|
|
||
| ca, err := test.caFn() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| secret, err := c.EnsureTargetCertKeyPair(context.TODO(), ca, ca.Config.Certs) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // need to ensure the client returns the created secret now | ||
| _ = indexer.Add(secret) | ||
|
|
||
| if !test.matchingLeaf { | ||
| ca, err = test.caFn() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| newSecret, err := c.EnsureTargetCertKeyPair(context.TODO(), ca, ca.Config.Certs) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if test.expectRotation { | ||
| if bytes.Equal(newSecret.Data["tls.crt"], secret.Data["tls.crt"]) { | ||
| t.Error("expected the certificate to rotate") | ||
| } | ||
| secretUpdated := false | ||
| for _, action := range client.Actions() { | ||
| if action.Matches("update", "secrets") { | ||
| secretUpdated = true | ||
| } | ||
| } | ||
| if !secretUpdated { | ||
| t.Errorf("expected secret to get updated, but only found actions: %s", spew.Sdump(client.Actions())) | ||
| } | ||
| } else { | ||
| if !bytes.Equal(newSecret.Data["tls.crt"], secret.Data["tls.crt"]) { | ||
| t.Error("expected the certificate to not rotate") | ||
| } | ||
|
|
||
| secretUpdated := false | ||
| for _, action := range client.Actions() { | ||
| if action.Matches("update", "secrets") { | ||
| secretUpdated = true | ||
| } | ||
| } | ||
| if secretUpdated { | ||
| t.Errorf("expected secret to not get updated, found actions: %s", spew.Sdump(client.Actions())) | ||
| } | ||
| } | ||
|
|
||
| }) | ||
| } | ||
| } | ||
|
|
||
| // NewCACertificate generates and signs new CA certificate and key. | ||
| func newTestCACertificate(subject pkix.Name, serialNumber int64, validity metav1.Duration, currentTime func() time.Time) (*crypto.CA, error) { | ||
| return newTestCACertificateWithAuthority(subject, serialNumber, validity, currentTime, nil) | ||
| } | ||
|
|
||
| func newTestCACertificateWithAuthority(subject pkix.Name, serialNumber int64, validity metav1.Duration, currentTime func() time.Time, subjectKeyId []byte) (*crypto.CA, error) { | ||
| caPublicKey, caPrivateKey, err := crypto.NewKeyPair() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| caCert := &x509.Certificate{ | ||
| Subject: subject, | ||
|
|
||
| SignatureAlgorithm: x509.SHA256WithRSA, | ||
|
|
||
| NotBefore: currentTime().Add(-1 * time.Second), | ||
| NotAfter: currentTime().Add(validity.Duration), | ||
| SerialNumber: big.NewInt(serialNumber), | ||
| AuthorityKeyId: subjectKeyId, | ||
| SubjectKeyId: subjectKeyId, | ||
|
|
||
| KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, | ||
| BasicConstraintsValid: true, | ||
| IsCA: true, | ||
| } | ||
|
|
||
| cert, err := signCertificate(caCert, caPublicKey, caCert, caPrivateKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &crypto.CA{ | ||
| Config: &crypto.TLSCertificateConfig{ | ||
| Certs: []*x509.Certificate{cert}, | ||
| Key: caPrivateKey, | ||
| }, | ||
| SerialGenerator: &crypto.RandomSerialGenerator{}, | ||
| }, nil | ||
| } | ||
|
|
||
| func signCertificate(template *x509.Certificate, requestKey gcrypto.PublicKey, issuer *x509.Certificate, issuerKey gcrypto.PrivateKey) (*x509.Certificate, error) { | ||
| derBytes, err := x509.CreateCertificate(rand.Reader, template, issuer, requestKey, issuerKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| certs, err := x509.ParseCertificates(derBytes) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(certs) != 1 { | ||
| return nil, errors.New("Expected a single certificate") | ||
| } | ||
| return certs[0], nil | ||
| } |
Oops, something went wrong.