Add service UID as x509 extension to service server certs #12413

smarterclayton · 2017-01-07T04:25:01Z

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.

[test]

smarterclayton · 2017-01-07T04:26:50Z

@jcantrill the OID of the certificate extension is 1.3.6.1.4.1.2312.17.100.2.1 - the value should be an asn.1 PrintableString or UTF8String (you should check for both). Reading the value when ES starts up should be sufficient.

smarterclayton · 2017-01-07T19:01:04Z

[test]

jcantrill · 2017-01-09T13:08:20Z

@smarterclayton how are we to consume this OID? Is it your intention we simply replace the existing one with the one here and it will be added as a SAN? Will it be some other attribute of the certificate for which we will need to modify the existing code which reads it?

smarterclayton · 2017-01-09T17:48:01Z

You should use this OID to lookup the extension value that matches it, and compare the remote certificate's value with the value on your local certificate. If you want to generate your config at startup, you can use a script to extract this value from your local cert and then check the hardcoded value. I.e. !/bin/sh value=$( extract_oid_value('1.3.6....') ) echo "cert.oid=1.3.6.9.2312..." >> ./searchguard.ini echo "cert.value=$(value)" >> ./searchguard.ini

…

On Mon, Jan 9, 2017 at 8:08 AM, Jeff Cantrill ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> how are we to consume this OID? Is it your intention we simply replace the existing one with the one here and it will be added as a SAN? Will it be some other attribute of the certificate for which we will need to modify the existing code which reads it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12413 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p8sWRVW2EYuR_wczcDhHBjxZ3Upxks5rQjFFgaJpZM4LdU81> .

jcantrill · 2017-01-09T21:05:38Z

@smarterclayton Is it this simple: https://github.com/floragunncom/search-guard/pull/269/files#diff-ebefeaba39aa0298a8ea57f929b88de4R32 which depends upon https://github.com/floragunncom/search-guard-ssl/pull/46

richm · 2017-01-09T21:12:01Z

@jcantrill You'll need to ensure that localValue != null and peerValue != null before doing the array equals. That is, both certs must have the oid extension value - if either of them are null, the function should return false.

smarterclayton · 2017-01-09T22:41:59Z

What rich said, and yes, looks pretty close.

tiran · 2017-01-10T09:15:13Z

@richm and @jcantrill asked me to review your proposal. A custom X509v3 extension with a registered OID is much more sane than a RID general name in SAN. A registered ID in the SAN extension is rather uncommon and even revealed a bug in Python, https://bugs.python.org/issue27691.

Your proposal could even be in line with the CA/B forum's baseline requirements, https://cabforum.org/baseline-requirements-documents/ . Section 7.1.2.3 "Subscriber Certificate" does neither forbid nor explicitly allow custom X509v3 extensions. On the other hand section 7.1.2.4 "All Certificates" states that

All other fields and extensions MUST be set in accordance with RFC 5280. The CA SHALL NOT issue a Certificate that contains a keyUsage flag, extendedKeyUsage value, Certificate extension, or other data not specified in section 7.1.2.1, 7.1.2.2, or 7.1.2.3 unless the CA is aware of a reason for including the data in the Certificate.

If you make a CA aware of the reason for the presence of your custom X509v3 extension, then you might be able to get a publicly trusted EE cert from e.g. Lets Encrypt. I'm not entirely sure, though. In case publicly trusted EE certs are a use case for OpenShift, more investigation is required.

richm · 2017-01-10T15:04:06Z

I'm not entirely sure, though. In case publicly trusted EE certs are a use case for OpenShift

Good question. OpenShift in general, yes - I believe someone is already looking into Let's Encrypt integration. However, for this very particular and specific case - these certificates are only used for intra-cluster Elasticsearch-to-Elasticsearch communication - they are not used by external clients at all, and are not even used by intra-cluster services such as OpenShift itself, or fluentd/kibana/etc. So what exactly do we need to worry about in this case?

smarterclayton · 2017-01-10T16:01:09Z

Thanks. In this case the CA is openshift - the cert is being issued by the platform. We have CSR APIs that allows an external CA to make the decision, but someone who doesn't want this field would decide not to approve it and use an alternate cert mechanism. This is an internal CA that leverages the trust relationship between the centralized infrastructure manager and the centralized signer to provide automated and bounded PKI. For reference, this usage is inline with Puppets use of CSR for authorizing nodes and they also act as a signing authority or allow delegating. https://docs.puppet.com/puppet/latest/ssl_attributes_extensions.html

liggitt · 2017-01-12T06:25:57Z

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

+// 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)


this is showing up oddly when I dump the resulting cert:

X509v3 extensions: X509v3 Key Usage: critical Digital Signature, Key Encipherment X509v3 Extended Key Usage: TLS Web Server Authentication X509v3 Basic Constraints: critical CA:FALSE X509v3 Subject Alternative Name: DNS:foo.default.svc, DNS:foo.default.svc.cluster.local 1.3.6.1.4.1.2312.17.100.2.1: .$e8162fce-d88e-11e6-b7df-acbc32c1ca87

nm, openssl is just odd

smarterclayton · 2017-01-12T06:31:01Z

Yeah, it's encoded correctly as far as i can tell, but will dig in further to verify.

…

On Thu, Jan 12, 2017 at 1:25 AM, Jordan Liggitt ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/cmd/server/crypto/extensions/serversigning.go <#12413 (review)> : > + // 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 encoded + // UTF8 string. + 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) this is showing up oddly when I dump the resulting cert: X509v3 extensions: X509v3 Key Usage: critical Digital Signature, Key Encipherment X509v3 Extended Key Usage: TLS Web Server Authentication X509v3 Basic Constraints: critical CA:FALSE X509v3 Subject Alternative Name: DNS:foo.default.svc, DNS:foo.default.svc.cluster.local 1.3.6.1.4.1.2312.17.100.2.1: .$e8162fce-d88e-11e6-b7df-acbc32c1ca87 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12413 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pxKHQAEe9RwO0ndKUqabyPYhvNAuks5rRcd2gaJpZM4LdU81> .

liggitt · 2017-01-12T06:46:22Z

yeah, dumping the asn.1 tree, it looks right:

SEQUENCE(2 elem)
  OBJECT IDENTIFIER1.3.6.1.4.1.2312.17.100.2.1
  OCTET STRING(1 elem)
    PrintableString e8162fce-d88e-11e6-b7df-acbc32c1ca87

openssl just renders it weirdly

liggitt · 2017-01-12T06:46:50Z

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

+	// 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 encoded
+	// UTF8 string.


go outputs the string as a PrintableString (not a UTF8 string) if the characters are in a limited range

Hrm, I thought I updated this comment. Not sure where it went.

liggitt · 2017-01-12T06:51:06Z

pkg/service/controller/servingcert/secret_updating_controller.go

+		sets.NewString(dnsName, fqDNSName),
+		certificateLifetime,
+		extensions.ServiceServerCertificateExtension(&kapi.Service{
+			ObjectMeta: kapi.ObjectMeta{UID: types.UID(secret.Annotations[ServiceUIDAnnotation])},


hrmm, I really don't want to take the annotation's word for it

Aren't we taking the annotation's word for it on the secret here?

On the service name.

yeah, which doesn't seem great. I expected at least verification of the data on the service object itself

@deads2k can you say why you didn't re-look up the service and verify the both way here? Is it happening earlier?

@deads2k can you say why you didn't re-look up the service and verify the both way here? Is it happening earlier?

Previously, the name was signed for, but the UID was not signed for. The worst someone could do is make a set of non-sensical metadata that made the controller do nothing. Now that you're signing this information, you need a stronger guarantee.

To state it a different way. By signing for this information, you're asserting that the information is true, you're asserting that no one can trick the signer into signing arbitrary data, and you need to be sure that if this cert is stolen the information it asserts cannot be used by someone else without significant control of the rest of system (say owning DNS and IP routing).

Yeah, this is fixed now.

Actually, all this worked before. The regenerateNeeded was already checking uid equality against the annotation. I changed the code to return the service that was already checked, and so this protected already.

liggitt · 2017-01-12T06:52:20Z

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

+	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 encoded


what does this mean? cert rotation because a service was deleted/recreated? or rotation for some other reason?

Was trying to imply that you are not guaranteed to have it be equal immediately. And right now, the SSCC doesn't regenerate certs in all cases (deletion of the secret) so I wanted to add weasel words.

deads2k · 2017-01-12T20:18:22Z

pkg/cmd/server/crypto/crypto.go

+// if the extension attempt failed.
+type CertificateExtensionFunc func(*x509.Certificate) error
+
+func (ca *CA) MakeServerCert(hostnames sets.String, expireDays int, fns ...CertificateExtensionFunc) (*TLSCertificateConfig, error) {


"bring my by big hammer". This seems excessively broad.

This is generic code, and cert extension really has nothing to do with the common spot. Also this is the normal pattern for extension in Go now when you're doing object customization.

liggitt · 2017-01-12T21:51:59Z

pkg/service/controller/servingcert/secret_updating_controller.go

+	if err != nil || !exists {
+		return nil
+	}
+	service := serviceObj.(*kapi.Service)


check the service references this secret as its serving cert secret?

liggitt · 2017-01-12T21:52:13Z

pkg/service/controller/servingcert/secret_updating_controller.go

+	}
+	serviceObj, exists, err := sc.serviceCache.GetByKey(secret.Namespace + "/" + secret.Annotations[ServiceNameAnnotation])
+	if err != nil || !exists {
+		return nil


what happens in this case? retry later?

At the resync interval or on the next change to the secret.

Crap... I just realized all this logic was already in regenerate. Simplifying this again.

smarterclayton · 2017-01-14T18:01:36Z

Any more comments?

liggitt · 2017-01-16T16:58:01Z

pkg/service/controller/servingcert/secret_updating_controller.go

 	}

 	service := serviceObj.(*kapi.Service)
 	if secret.Annotations[ServiceUIDAnnotation] != string(service.UID) {
-		return false
+		return false, nil
 	}



double check the back-reference (that the service still has an annotation asking for a cert to be put in that secret)

liggitt · 2017-01-16T17:02:00Z

check the back-reference, then LGTM

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.

smarterclayton · 2017-01-17T00:25:56Z

Back-ref-i-checked [merge]

openshift-bot · 2017-01-17T00:29:38Z

Evaluated for origin test up to 778391d

openshift-bot · 2017-01-17T00:29:48Z

Evaluated for origin merge up to 778391d

openshift-bot · 2017-01-17T01:55:54Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12925/) (Base Commit: a3e2498)

openshift-bot · 2017-01-17T04:21:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12932/) (Base Commit: 0b38fe0) (Image: devenv-rhel7_5695)

smarterclayton requested a review from liggitt January 9, 2017 14:10

liggitt reviewed Jan 12, 2017

View reviewed changes

deads2k reviewed Jan 12, 2017

View reviewed changes

smarterclayton force-pushed the x509_uid_extension branch 2 times, most recently from 62084c9 to 4a5e6de Compare January 12, 2017 21:39

liggitt reviewed Jan 12, 2017

View reviewed changes

smarterclayton force-pushed the x509_uid_extension branch 2 times, most recently from 744e3b0 to 1c48f88 Compare January 12, 2017 22:26

liggitt reviewed Jan 16, 2017

View reviewed changes

smarterclayton force-pushed the x509_uid_extension branch from 1c48f88 to 778391d Compare January 17, 2017 00:25

openshift-bot merged commit 9b125cb into openshift:master Jan 17, 2017

mtrmac mentioned this pull request Feb 23, 2021

WIP: Generate per-pod certificates for StatefulSets openshift/service-ca-operator#144

Closed

Add service UID as x509 extension to service server certs #12413

Add service UID as x509 extension to service server certs #12413

Conversation

smarterclayton commented Jan 7, 2017

smarterclayton commented Jan 7, 2017

smarterclayton commented Jan 7, 2017 via email

jcantrill commented Jan 9, 2017

smarterclayton commented Jan 9, 2017 via email

jcantrill commented Jan 9, 2017 • edited Loading

richm commented Jan 9, 2017

smarterclayton commented Jan 9, 2017

tiran commented Jan 10, 2017

richm commented Jan 10, 2017

smarterclayton commented Jan 10, 2017 via email • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 12, 2017 via email

liggitt commented Jan 12, 2017

liggitt Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 14, 2017

Choose a reason for hiding this comment

liggitt commented Jan 16, 2017

smarterclayton commented Jan 17, 2017

openshift-bot commented Jan 17, 2017

openshift-bot commented Jan 17, 2017

openshift-bot commented Jan 17, 2017

openshift-bot commented Jan 17, 2017 • edited Loading

jcantrill commented Jan 9, 2017 •

edited

Loading

smarterclayton commented Jan 10, 2017 via email •

edited

Loading

liggitt Jan 12, 2017 •

edited

Loading

openshift-bot commented Jan 17, 2017 •

edited

Loading