Merge pull request #105 from marun/4.2-automated-rotation

Bug 1774156: 4.2 cherry-picks in support of automated service ca rotation

Author: openshift-merge-robot

bindata/v4.0.0/service-serving-cert-signer-controller/defaultconfig.yaml

@@ -3,6 +3,7 @@ kind: ServiceServingCertSignerConfig
 signer:
   certFile: /var/run/secrets/signing-key/tls.crt
   keyFile: /var/run/secrets/signing-key/tls.key
+intermediateCertFile: /var/run/secrets/signing-key/intermediate-ca.crt
 authentication:
   disabled: true
 authorization:

glide.lock

@@ -15,11 +15,11 @@ import:
 - package: k8s.io/component-base
   version: kubernetes-1.14.0
 - package: github.com/openshift/api
-  version: master
+  version: release-4.2
 - package: github.com/openshift/client-go
-  version: master
+  version: release-4.2
 - package: github.com/openshift/library-go
-  version: master
+  version: release-4.2
 - package: monis.app/go
   version: master
 

glide.yaml

@@ -6,6 +6,22 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// Constants for Service CA
+const (
+	// ForcedRotationReasonAnnotationName is the name of an annotation indicating
+	// the most recent reason that a service CA rotation was forced. The annotation
+	// will be set on the signing secret after the successful completion of a forced
+	// rotation.
+	ForcedRotationReasonAnnotationName = "service-ca.operators.openshift.io/forced-rotation-reason"
+	// BundleDataKey is the key used to identify the CA bundle in the signing
+	// secret.
+	BundleDataKey = "ca-bundle.crt"
+	// IntermediateDataKey is the key used to identify the post-rotation
+	// trust-bridging certificate in the signing secret.
+	IntermediateDataKey = "intermediate-ca.crt"
+)
+
+// Constants for CA bundle injection
 const (
 	InjectCABundleAnnotationName      = "service.beta.openshift.io/inject-cabundle"
 	AlphaInjectCABundleAnnotationName = "service.alpha.openshift.io/inject-cabundle"
@@ -23,6 +39,14 @@ func HasInjectCABundleAnnotationUpdate(old, cur v1.Object) bool {
 
 // Annotations on service
 const (
+	// TODO(marun) When adding a GA serving cert annotation, consider
+	// discontinuing the practice of including the issuing cert with the serving
+	// cert. This behavior was accidental (at some point the issuing CA was an
+	// intermediate rather than the current self-signed root) but had to be
+	// maintained to support legacy clients that ended up reusing the serving cert
+	// as a CA bundle. Clients of a GA annotation should be expected to use a
+	// proper CA bundle.
+
 	// ServingCertSecretAnnotation stores the name of the secret to generate into.
 	ServingCertSecretAnnotation      = "service.beta.openshift.io/serving-cert-secret-name"
 	AlphaServingCertSecretAnnotation = "service.alpha.openshift.io/serving-cert-secret-name"

manifests/06_config.yaml

@@ -1,6 +1,8 @@
 package controller
 
 import (
+	"bytes"
+	"crypto/x509"
 	"fmt"
 	"strconv"
 	"time"
@@ -31,9 +33,10 @@ type serviceServingCertController struct {
 	serviceLister listers.ServiceLister
 	secretLister  listers.SecretLister
 
-	ca         *crypto.CA
-	dnsSuffix  string
-	maxRetries int
+	ca                 *crypto.CA
+	intermediateCACert *x509.Certificate
+	dnsSuffix          string
+	maxRetries         int
 
 	// standard controller loop
 	// services that need to be checked
@@ -43,17 +46,18 @@ type serviceServingCertController struct {
 	syncHandler controller.SyncFunc
 }
 
-func NewServiceServingCertController(services informers.ServiceInformer, secrets informers.SecretInformer, serviceClient kcoreclient.ServicesGetter, secretClient kcoreclient.SecretsGetter, ca *crypto.CA, dnsSuffix string) controller.Runner {
+func NewServiceServingCertController(services informers.ServiceInformer, secrets informers.SecretInformer, serviceClient kcoreclient.ServicesGetter, secretClient kcoreclient.SecretsGetter, ca *crypto.CA, intermediateCACert *x509.Certificate, dnsSuffix string) controller.Runner {
 	sc := &serviceServingCertController{
 		serviceClient: serviceClient,
 		secretClient:  secretClient,
 
 		serviceLister: services.Lister(),
 		secretLister:  secrets.Lister(),
 
-		ca:         ca,
-		dnsSuffix:  dnsSuffix,
-		maxRetries: 10,
+		ca:                 ca,
+		intermediateCACert: intermediateCACert,
+		dnsSuffix:          dnsSuffix,
+		maxRetries:         10,
 	}
 
 	sc.syncHandler = sc.syncService
@@ -129,7 +133,7 @@ func (sc *serviceServingCertController) generateCert(serviceCopy *corev1.Service
 	}
 
 	secret := toBaseSecret(serviceCopy)
-	if err := toRequiredSecret(sc.dnsSuffix, sc.ca, serviceCopy, secret); err != nil {
+	if err := toRequiredSecret(sc.dnsSuffix, sc.ca, sc.intermediateCACert, serviceCopy, secret); err != nil {
 		return err
 	}
 
@@ -212,8 +216,13 @@ func (sc *serviceServingCertController) requiresCertGeneration(service *corev1.S
 	return true
 }
 
-// Returns true if the secret certificate has the same issuer common name as the current CA, false
-// if not or if there is a parsing error.
+// Returns true if the secret certificate was issued by the current CA,
+// false if not or if there was a parsing error.
+//
+// Determination of issuance will default to comparison of the certificate's
+// AuthorityKeyID and the CA's SubjectKeyId, and fall back to comparison of the
+// certificate's Issuer.CommonName and the CA's Subject.CommonName (in case the CA was
+// generated prior to the addition of key identifiers).
 func (sc *serviceServingCertController) issuedByCurrentCA(secret *corev1.Secret) bool {
 	certs, err := cert.ParseCertsPEM(secret.Data[corev1.TLSCertKey])
 	if err != nil {
@@ -227,6 +236,17 @@ func (sc *serviceServingCertController) issuedByCurrentCA(secret *corev1.Secret)
 		return false
 	}
 
+	certAuthorityKeyId := certs[0].AuthorityKeyId
+	caSubjectKeyId := sc.ca.Config.Certs[0].SubjectKeyId
+	// Use key identifier chaining if the SubjectKeyId is populated in the CA
+	// certificate. AuthorityKeyId may not be set in the serving certificate if it was
+	// generated before serving cert generation was updated to include the field.
+	if len(caSubjectKeyId) > 0 {
+		return bytes.Compare(certAuthorityKeyId, caSubjectKeyId) == 0
+	}
+
+	// Fall back to name-based chaining for a legacy service CA that was generated
+	// without SubjectKeyId or AuthorityKeyId.
 	return certs[0].Issuer.CommonName == sc.commonName()
 }
 
@@ -301,23 +321,31 @@ func toBaseSecret(service *corev1.Service) *corev1.Secret {
 	}
 }
 
-func getServingCert(dnsSuffix string, ca *crypto.CA, service *corev1.Service) (*crypto.TLSCertificateConfig, error) {
-	dnsName := service.Name + "." + service.Namespace + ".svc"
+func MakeServingCert(dnsSuffix string, ca *crypto.CA, intermediateCACert *x509.Certificate, serviceObjectMeta *metav1.ObjectMeta) (*crypto.TLSCertificateConfig, error) {
+	dnsName := serviceObjectMeta.Name + "." + serviceObjectMeta.Namespace + ".svc"
 	fqDNSName := dnsName + "." + dnsSuffix
 	certificateLifetime := 365 * 2 // 2 years
 	servingCert, err := ca.MakeServerCert(
 		sets.NewString(dnsName, fqDNSName),
 		certificateLifetime,
-		cryptoextensions.ServiceServerCertificateExtensionV1(service),
+		cryptoextensions.ServiceServerCertificateExtensionV1(serviceObjectMeta.UID),
 	)
 	if err != nil {
 		return nil, err
 	}
+
+	// Including the intermediate cert will ensure that clients with a
+	// stale ca bundle (containing the previous CA but not the current
+	// one) will be able to trust the serving cert.
+	if intermediateCACert != nil {
+		servingCert.Certs = append(servingCert.Certs, intermediateCACert)
+	}
+
 	return servingCert, nil
 }
 
-func toRequiredSecret(dnsSuffix string, ca *crypto.CA, service *corev1.Service, secretCopy *corev1.Secret) error {
-	servingCert, err := getServingCert(dnsSuffix, ca, service)
+func toRequiredSecret(dnsSuffix string, ca *crypto.CA, intermediateCACert *x509.Certificate, service *corev1.Service, secretCopy *corev1.Secret) error {
+	servingCert, err := MakeServingCert(dnsSuffix, ca, intermediateCACert, &service.ObjectMeta)
 	if err != nil {
 		return err
 	}

pkg/controller/api/api.go

@@ -17,6 +17,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
@@ -29,27 +30,6 @@ import (
 
 const signerName = "openshift-service-serving-signer"
 
-const testCert = `
------BEGIN CERTIFICATE-----
-MIIDETCCAfmgAwIBAgIUTNjtvaP8ZzRNabhgLhuqHONxuTYwDQYJKoZIhvcNAQEL
-BQAwKzEpMCcGA1UEAwwgb3BlbnNoaWZ0LXNlcnZpY2Utc2VydmluZy1zaWduZXIw
-HhcNMTkwNDE3MTkyMjEwWhcNMjAwNDE2MTkyMjEwWjAPMQ0wCwYDVQQDDAR0ZXN0
-MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA6QmfkmO9eaSqONefWAKO
-ZGwYe02tBltRCWsE96GslVq+aWVc6SSOVcghv9bL4xZQy2TQNxDRKNBDX0Fwk5TR
-Aj2aMzXuJ+HzxwyCK3o5SqwQYnOlgFuUpKShtpM4jye6hxwllFr059MvRAUZZNVX
-Fkv0Gh2CJcry/wPAuXVZV03GOixB/TeFKSEmpSSdMyhK3hFve3XkeW88rtuP9cG1
-duy3onAGZQ4V86TrwYsPJVo9t7IDS+SheIqHEhbfYouS6zBEvpeZMz+evP4q2AJs
-FXfLSQJi+HyHYdGovbBO9+ZotJ609hrkJ4/cMDJOxXeG8YBr6x9hgZtH4GO55jeS
-kQIDAQABo0kwRzALBgNVHQ8EBAMCBeAwEwYDVR0lBAwwCgYIKwYBBQUHAwEwCQYD
-VR0TBAIwADAYBgNVHREEETAPgg0qLmZvby5iYXIuY29tMA0GCSqGSIb3DQEBCwUA
-A4IBAQB9HfCAUdxoVyaA7KxU1a838sC2Z/NrbqC2+u1eR1SVilRjykD+k3v6XnM9
-ku7TYpf8YRbgRmbu864zYE1ibxMwVGqQlMR9tNm2cA6nEDke2sDqH0JbS5lZPX+a
-DA9tdnJtx+/uxsuz6I68rp5kDPiTjUTjxc9/Ob3vLiCopBikuiC0H9cPdq1lNHFJ
-k89OWEXatvLbNvVRioyptH1hf5lweVtDjytnj7gaMchhlH8qK6u4iggLxViVxheO
-fiNJr2o4fexMfez1J6u7ZuM2w50CuOHuAGVAdrVdE8LYjr0SouwzVt20Uc0swLRE
-GKM7HG83Wj2hA+DWdy9ZJAdBLISB
------END CERTIFICATE-----
-`
 const testCertUnknownIssuer = `
 -----BEGIN CERTIFICATE-----
 MIIDETCCAfmgAwIBAgIUdbBKh0jOJxli4wl34q0TYJu8+n0wDQYJKoZIhvcNAQEL
@@ -107,7 +87,7 @@ func controllerSetup(startingObjects []runtime.Object, t *testing.T) ( /*caName*
 	controller := NewServiceServingCertController(
 		informerFactory.Core().V1().Services(),
 		informerFactory.Core().V1().Secrets(),
-		kubeclient.CoreV1(), kubeclient.CoreV1(), ca, "cluster.local",
+		kubeclient.CoreV1(), kubeclient.CoreV1(), ca, nil, "cluster.local",
 	)
 
 	return signerName, kubeclient, fakeWatch, fakeSecretWatch, controller.(*serviceServingCertController), informerFactory
@@ -851,9 +831,22 @@ func TestSkipGenerationControllerFlow(t *testing.T) {
 		return err
 	}
 
+	// Generate a serving cert from the ca to ensure key identity chaining
+	newServingCert, err := controller.ca.MakeServerCert(
+		sets.NewString("foo"),
+		crypto.DefaultCertificateLifetimeInDays,
+	)
+	if err != nil {
+		t.Fatalf("failed to generate serving cert: %v", err)
+	}
+	certPEM, err := crypto.EncodeCertificates(newServingCert.Certs[0])
+	if err != nil {
+		t.Fatalf("failed to encode serving cert to PEM: %v", err)
+	}
+
 	secretToAdd := &v1.Secret{
 		Data: map[string][]byte{
-			v1.TLSCertKey: []byte(testCert),
+			v1.TLSCertKey: certPEM,
 		},
 	}
 	secretToAdd.Name = expectedSecretName