Add service UID as x509 extension to service server certs

ElasticSearch SearchGuard has a requirement that an attribute of the
certificate be used to identify intracluster traffic. That value is
currently a registeredID as a SubjectAlternativeName, which is
questionable. This PR includes the service UID as an x509 extension
(using the new OpenShift OID arc we have been allocated) in order to
enable a caller to have an unforgeable value on the certificate they can
consult to identifier other cluster members. This also adds more
traceability to the certificate for future use.

Author: smarterclayton

pkg/cmd/server/crypto/crypto.go

@@ -371,9 +371,18 @@ func (ca *CA) MakeAndWriteServerCert(certFile, keyFile string, hostnames sets.St
 	return server, nil
 }
 
-func (ca *CA) MakeServerCert(hostnames sets.String, expireDays int) (*TLSCertificateConfig, error) {
+// CertificateExtensionFunc is passed a certificate that it may extend, or return an error
+// if the extension attempt failed.
+type CertificateExtensionFunc func(*x509.Certificate) error
+
+func (ca *CA) MakeServerCert(hostnames sets.String, expireDays int, fns ...CertificateExtensionFunc) (*TLSCertificateConfig, error) {
 	serverPublicKey, serverPrivateKey, _ := NewKeyPair()
 	serverTemplate := newServerCertificateTemplate(pkix.Name{CommonName: hostnames.List()[0]}, hostnames.List(), expireDays, time.Now)
+	for _, fn := range fns {
+		if err := fn(serverTemplate); err != nil {
+			return nil, err
+		}
+	}
 	serverCrt, err := ca.signCertificate(serverTemplate, serverPublicKey)
 	if err != nil {
 		return nil, err

pkg/cmd/server/crypto/extensions/doc.go

@@ -0,0 +1,4 @@
+// Package extensions defines cryptographic extensions for OpenShift. This
+// package contains x509 extension object identifier constants and helpers
+// for generating certificates on an OpenShift cluster.
+package extensions

pkg/cmd/server/crypto/extensions/extensions.go

@@ -0,0 +1,17 @@
+package extensions
+
+import (
+	"encoding/asn1"
+)
+
+// oid is a helper function for concatenating OIDs
+func oid(o asn1.ObjectIdentifier, extra ...int) asn1.ObjectIdentifier {
+	return asn1.ObjectIdentifier(append(append([]int{}, o...), extra...))
+}
+
+var (
+	// RedHatOID is the IANA assigned ObjectIdentifier for Red Hat Inc.
+	RedHatOID = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 2312}
+	// OpenShiftOID is the Red Hat assigned OID arc for OpenShift.
+	OpenShiftOID = oid(RedHatOID, 17)
+)

pkg/cmd/server/crypto/extensions/serversigning.go

@@ -0,0 +1,42 @@
+package extensions
+
+import (
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/asn1"
+
+	kapi "k8s.io/kubernetes/pkg/api"
+
+	"github.com/openshift/origin/pkg/cmd/server/crypto"
+)
+
+var (
+	// OpenShiftServerSigningOID is the OpenShift assigned OID arc for certificates signed by the OpenShift server.
+	OpenShiftServerSigningOID = oid(OpenShiftOID, 100)
+	// OpenShiftServerSigningServiceOID describes the IANA arc for extensions to server certificates generated by the
+	// OpenShift service signing mechanism. All elements in this arc should only be used when signing server certificates
+	// for use under a service.
+	OpenShiftServerSigningServiceOID = oid(OpenShiftServerSigningOID, 2)
+	// OpenShiftServerSigningServiceUIDOID is an x509 extension that is applied to server certificates generated for services
+	// representing the UID of the service this certificate was generated for. This value is not guaranteed to match the
+	// current service UID if the certificates are in the process of being rotated out. The value MUST be an ASN.1
+	// PrintableString or UTF8String.
+	OpenShiftServerSigningServiceUIDOID = oid(OpenShiftServerSigningServiceOID, 1)
+)
+
+// ServiceServerCertificateExtension returns a CertificateExtensionFunc that will add the
+// service UID as an x509 v3 extension to the server certificate.
+func ServiceServerCertificateExtension(svc *kapi.Service) crypto.CertificateExtensionFunc {
+	return func(cert *x509.Certificate) error {
+		uid, err := asn1.Marshal(svc.UID)
+		if err != nil {
+			return err
+		}
+		cert.ExtraExtensions = append(cert.ExtraExtensions, pkix.Extension{
+			Id:       OpenShiftServerSigningServiceUIDOID,
+			Critical: false,
+			Value:    uid,
+		})
+		return nil
+	}
+}

pkg/service/controller/servingcert/secret_creating_controller.go

@@ -22,6 +22,7 @@ import (
 	"k8s.io/kubernetes/pkg/watch"
 
 	"github.com/openshift/origin/pkg/cmd/server/crypto"
+	"github.com/openshift/origin/pkg/cmd/server/crypto/extensions"
 )
 
 const (
@@ -197,7 +198,11 @@ func (sc *ServiceServingCertController) syncService(key string) error {
 	dnsName := service.Name + "." + service.Namespace + ".svc"
 	fqDNSName := dnsName + "." + sc.dnsSuffix
 	certificateLifetime := 365 * 2 // 2 years
-	servingCert, err := sc.ca.MakeServerCert(sets.NewString(dnsName, fqDNSName), certificateLifetime)
+	servingCert, err := sc.ca.MakeServerCert(
+		sets.NewString(dnsName, fqDNSName),
+		certificateLifetime,
+		extensions.ServiceServerCertificateExtension(service),
+	)
 	if err != nil {
 		return err
 	}

pkg/service/controller/servingcert/secret_creating_controller_test.go

@@ -1,6 +1,9 @@
 package servingcert
 
 import (
+	"crypto/x509"
+	"encoding/asn1"
+	"encoding/pem"
 	"fmt"
 	"io/ioutil"
 	"reflect"
@@ -16,6 +19,7 @@ import (
 	"k8s.io/kubernetes/pkg/watch"
 
 	"github.com/openshift/origin/pkg/cmd/server/admin"
+	"github.com/openshift/origin/pkg/cmd/server/crypto/extensions"
 )
 
 func controllerSetup(startingObjects []runtime.Object, stopChannel chan struct{}, t *testing.T) ( /*caName*/ string, *fake.Clientset, *watch.FakeWatcher, *ServiceServingCertController) {
@@ -51,6 +55,51 @@ func controllerSetup(startingObjects []runtime.Object, stopChannel chan struct{}
 	return caOptions.Name, kubeclient, fakeWatch, controller
 }
 
+func checkGeneratedCertificate(t *testing.T, certData []byte, service *kapi.Service) {
+	block, _ := pem.Decode(certData)
+	if block == nil {
+		t.Errorf("PEM block not found in secret")
+		return
+	}
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Errorf("expected valid certificate in first position: %v", err)
+		return
+	}
+
+	if len(cert.DNSNames) != 2 {
+		t.Errorf("unexpected DNSNames: %v", cert.DNSNames)
+	}
+	for _, s := range cert.DNSNames {
+		switch s {
+		case fmt.Sprintf("%s.%s.svc", service.Name, service.Namespace),
+			fmt.Sprintf("%s.%s.svc.cluster.local", service.Name, service.Namespace):
+		default:
+			t.Errorf("unexpected DNSNames: %v", cert.DNSNames)
+		}
+	}
+
+	found := true
+	for _, ext := range cert.Extensions {
+		if extensions.OpenShiftServerSigningServiceUIDOID.Equal(ext.Id) {
+			var value string
+			if _, err := asn1.Unmarshal(ext.Value, &value); err != nil {
+				t.Errorf("unable to parse certificate extension: %v", ext.Value)
+				continue
+			}
+			if value != string(service.UID) {
+				t.Errorf("unexpected extension value: %v", value)
+				continue
+			}
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Errorf("unable to find service UID certificate extension in cert: %#v", cert)
+	}
+}
+
 func TestBasicControllerFlow(t *testing.T) {
 	stopChannel := make(chan struct{})
 	defer close(stopChannel)
@@ -110,6 +159,8 @@ func TestBasicControllerFlow(t *testing.T) {
 				t.Errorf("expected %v, got %v", expectedSecretAnnotations, newSecret.Annotations)
 				continue
 			}
+
+			checkGeneratedCertificate(t, newSecret.Data["tls.crt"], serviceToAdd)
 			foundSecret = true
 
 		case action.Matches("update", "services"):

pkg/service/controller/servingcert/secret_updating_controller.go

@@ -19,6 +19,7 @@ import (
 	"k8s.io/kubernetes/pkg/watch"
 
 	"github.com/openshift/origin/pkg/cmd/server/crypto"
+	"github.com/openshift/origin/pkg/cmd/server/crypto/extensions"
 )
 
 // ServiceServingCertUpdateController is responsible for synchronizing Service objects stored
@@ -220,7 +221,8 @@ func (sc *ServiceServingCertUpdateController) syncSecret(key string) error {
 		return nil
 	}
 
-	if !sc.requiresRegeneration(obj.(*kapi.Secret)) {
+	regenerate, service := sc.requiresRegeneration(obj.(*kapi.Secret))
+	if !regenerate {
 		return nil
 	}
 
@@ -231,10 +233,14 @@ func (sc *ServiceServingCertUpdateController) syncSecret(key string) error {
 	}
 	secret := t.(*kapi.Secret)
 
-	dnsName := secret.Annotations[ServiceNameAnnotation] + "." + secret.Namespace + ".svc"
+	dnsName := service.Name + "." + secret.Namespace + ".svc"
 	fqDNSName := dnsName + "." + sc.dnsSuffix
 	certificateLifetime := 365 * 2 // 2 years
-	servingCert, err := sc.ca.MakeServerCert(sets.NewString(dnsName, fqDNSName), certificateLifetime)
+	servingCert, err := sc.ca.MakeServerCert(
+		sets.NewString(dnsName, fqDNSName),
+		certificateLifetime,
+		extensions.ServiceServerCertificateExtension(service),
+	)
 	if err != nil {
 		return err
 	}
@@ -248,39 +254,39 @@ func (sc *ServiceServingCertUpdateController) syncSecret(key string) error {
 	return err
 }
 
-func (sc *ServiceServingCertUpdateController) requiresRegeneration(secret *kapi.Secret) bool {
+func (sc *ServiceServingCertUpdateController) requiresRegeneration(secret *kapi.Secret) (bool, *kapi.Service) {
 	serviceName := secret.Annotations[ServiceNameAnnotation]
 	if len(serviceName) == 0 {
-		return false
+		return false, nil
 	}
 
 	serviceObj, exists, err := sc.serviceCache.GetByKey(secret.Namespace + "/" + serviceName)
 	if err != nil {
-		return false
+		return false, nil
 	}
 	if !exists {
-		return false
+		return false, nil
 	}
 
 	service := serviceObj.(*kapi.Service)
 	if secret.Annotations[ServiceUIDAnnotation] != string(service.UID) {
-		return false
+		return false, nil
 	}
 
 	// if we don't have the annotation for expiry, just go ahead and regenerate.  It's easier than writing a
 	// secondary logic flow that creates the expiry dates
 	expiryString, ok := secret.Annotations[ServingCertExpiryAnnotation]
 	if !ok {
-		return true
+		return true, service
 	}
 	expiry, err := time.Parse(time.RFC3339, expiryString)
 	if err != nil {
-		return true
+		return true, service
 	}
 
 	if time.Now().Add(sc.minTimeLeftForCert).After(expiry) {
-		return true
+		return true, service
 	}
 
-	return false
+	return false, nil
 }

pkg/service/controller/servingcert/secret_updating_controller_test.go

@@ -140,9 +140,12 @@ func TestRequiresRegenerationServiceUIDMismatch(t *testing.T) {
 			serviceCache: cache.NewStore(framework.DeletionHandlingMetaNamespaceKeyFunc),
 		}
 		tc.primeServices(c.serviceCache)
-		actual := c.requiresRegeneration(tc.secret)
+		actual, service := c.requiresRegeneration(tc.secret)
 		if tc.expected != actual {
 			t.Errorf("%s: expected %v, got %v", tc.name, tc.expected, actual)
 		}
+		if service == nil && tc.expected {
+			t.Errorf("%s: should have returned service", tc.name)
+		}
 	}
 }