Use self-signed default router certificate #109

Miciah · 2019-01-25T06:41:08Z

Add library-go dependency

Use self-signed default router certificate

Generate a CA certificate and use it to generate default certificates for routers. Publish the CA certificate in a configmap, which can be incorporated into CA bundles in order to trust the default certificates.

This PR resolves NE-139.

manifests/00-cluster-role.yaml: Grant the operator create, get, list, watch, and delete access to configmaps.
pkg/manifests/manifests.go (RouterServiceInternal): Change serving-cert secret name so as not to conflict with the secret that the operator creates.
pkg/manifests/manifests_test.go (TestManifests): Adjust for the change to RouterServiceInternal.
pkg/operator/controller/controller.go (caCertSecretName):
(caCertConfigMapName): New constants for the name of the secret and configmap for the CA certificate.
(CACert): New type to hold a cached copy of the CA certificate and to keep track of the resource versions of the most recently successfully observed secret and configmap for the CA certificate.
(Config): Add CACert field.
(ensureRouterForIngress): Call ensureDefaultCertificateForIngress.
(ensureDefaultCertificateForIngress): New function to check that a default certificate exists for a given ClusterIngress, and generate a certificate and create a secret for it if none exists. Use ensureRouterCACertificate to get the certificate used to sign the default certificate.
(ensureRouterCACertificate): New function to get the CA certificate for signing default certificates, and generate a CA certificate and create a configmap and secret for it if none exists.
test/e2e/operator_test.go (TestRouterCACertificate): New test that gets the configmap with the CA certificate and verifies that the router's default certificate is valid using the CA certificate.

~~@ironcladlou, this is WIP for now because the implementation currently requires far too broad permissions for the operator.~~

ironcladlou · 2019-01-25T16:07:48Z

What about storing the CA cert itself in openshift-ingress-operator and reserving openshift-ingress for resources owned by a ClusterIngress? I think this would keep the dependency graph a little cleaner and also allow the CA cert to survive the destruction of the openshift-ingress namespace.

Miciah · 2019-01-25T16:34:43Z

What about storing the CA cert itself in openshift-ingress-operator and reserving openshift-ingress for resources owned by a ClusterIngress? I think this would keep the dependency graph a little cleaner and also allow the CA cert to survive the destruction of the openshift-ingress namespace.

Yeah, that seems reasonable. We still want to put the secret for the default certificate in openshift-ingress, so that doesn't resolve the RBAC issue.

Miciah · 2019-01-25T18:13:08Z

/retest

Miciah · 2019-01-25T21:04:32Z

/hold

Miciah · 2019-01-26T03:09:27Z

Latest push adds a TestRouterCACertificate end-to-end test, moves the configmap with the CA certificate to the "openshift-ingress-operator" namespace, and drops configmaps access permissions from the operator's cluster role.

Miciah · 2019-01-26T07:27:56Z

/retest

Miciah · 2019-01-26T09:05:15Z

/retest

Miciah · 2019-01-26T10:05:54Z

Latest push changes the name of the CA certificate in the router-ca configmap from tls.crt to ca-bundle.crt to match cluster-kube-apiserver-operator's expectations.

Miciah · 2019-01-26T10:24:33Z

Fix TestRouterCACertificate for the last change.

Miciah · 2019-01-26T20:12:29Z

/retest

ironcladlou · 2019-01-28T15:34:35Z

manifests/00-cluster-role.yaml

@@ -8,6 +8,7 @@ rules:
  - ""
  resources:
  - namespaces
+  - secrets


Yeah, but how can you get around that? That's the problem with this approach.

At least it's not the router that can read secrets.

True. Only way I know of to get around it is to let the CVO create the openshift-ingress namespace and a role binding in that namespace for the operator SA.

After further discussion, we decided to defer this issue for now.

knobunc · 2019-01-28T15:54:55Z

@ericavonb @enj @mrogers950 FYI & please feel free to comment

ironcladlou · 2019-01-28T16:04:03Z

LGTM but I'll hold off tagging until @Miciah has had a chance to comment on whether he's satisfied

mrogers950 · 2019-01-28T17:12:24Z

pkg/operator/controller/controller.go

+			return fmt.Errorf("failed to get CA certificate: %v", err)
+		}
+		hostnames := sets.NewString(fmt.Sprintf("*.%s", *ci.Spec.IngressDomain))
+		cert, err := ca.MakeServerCert(hostnames, 0)


Rather than a single creation of the CA and cert, you should use the library-go certificate rotators. You'll get creation and rotation on both for free.

CA rotator example:
https://github.com/openshift/cluster-kube-apiserver-operator/blob/master/pkg/operator/certrotationcontroller/certrotationcontroller.go#L150

Server cert rotator example: https://github.com/openshift/cluster-kube-apiserver-operator/blob/master/pkg/operator/certrotationcontroller/certrotationcontroller.go#L175

@mrogers950, thanks for the pointer! It looks like certrotationcontroller is designed to handle a single server certificate whereas we want to have one per router. Does library-go have something we can use to manage multiple server certifcates, or will it only work for the signing certificate and bundle? Anyway, certificate rotation is descoped for 4.0, so I'll add a TODO.

Yeah, right now there's one client+server+CA set per controller, so you would need one controller per router.

pkg/manifests/manifests.go

assets/router/deployment.yaml

pkg/manifests/manifests_test.go

Miciah · 2019-01-28T21:09:50Z

Latest push squashes the RBAC change into the main commit; removes the metrics-certs secret, volume, and environment variables; and adds a TODO to use certrotationcontroller.

Generate a CA certificate and use it to generate default certificates for routers. Publish the CA certificate in a configmap, which can be incorporated into CA bundles in order to trust the default certificates. This commit resolves NE-139. https://jira.coreos.com/browse/NE-139 * manifests/00-cluster-role.yaml: Grant the operator create, get, list, watch, and delete access to secrets. * pkg/manifests/manifests.go (RouterServiceInternal): Change serving-cert secret name so as not to conflict with the secret that the operator creates. * pkg/manifests/manifests_test.go (TestManifests): Adjust for the change to RouterServiceInternal. * pkg/operator/controller/controller.go (caCertSecretName): (caCertConfigMapName): New constants for the name of the secret and configmap for the CA certificate. (CACert): New type to hold a cached copy of the CA certificate and to keep track of the resource versions of the most recently successfully observed secret and configmap for the CA certificate. (Config): Add CACert field. (ensureRouterForIngress): Call ensureDefaultCertificateForIngress. (ensureDefaultCertificateForIngress): New function to check that a default certificate exists for a given ClusterIngress, and generate a certificate and create a secret for it if none exists. Use ensureRouterCACertificate to get the certificate used to sign the default certificate. (ensureRouterCACertificate): New function to get the CA certificate for signing default certificates, and generate a CA certificate and create a configmap and secret for it if none exists. * test/e2e/operator_test.go (TestRouterCACertificate): New test that gets the configmap with the CA certificate and verifies that the router's default certificate is valid using the CA certificate.

Miciah · 2019-01-28T22:18:39Z

Latest push changes the annotation on the internal router service so the serving-cert signer will not stomp the secret that the operator creates.

Miciah · 2019-01-29T00:19:32Z

/retest

ironcladlou · 2019-01-29T20:30:17Z

/hold cancel

ironcladlou · 2019-01-29T20:34:46Z

/lgtm

openshift-ci-robot · 2019-01-29T20:34:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah,ironcladlou]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 25, 2019

openshift-ci-robot requested review from ironcladlou and pravisankar January 25, 2019 06:41

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2019

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 25, 2019

Add library-go dependency

34d55d9

Miciah force-pushed the NE-139-use-self-signed-default-routing-certificate branch from da0cb43 to 6ef8c19 Compare January 26, 2019 03:06

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2019

Miciah force-pushed the NE-139-use-self-signed-default-routing-certificate branch from 6ef8c19 to db522b1 Compare January 26, 2019 03:27

Miciah force-pushed the NE-139-use-self-signed-default-routing-certificate branch from db522b1 to a2f816f Compare January 26, 2019 10:04

Miciah mentioned this pull request Jan 26, 2019

Trust cluster-ingress-operator's CA certificate openshift/cluster-kube-apiserver-operator#222

Closed

Miciah force-pushed the NE-139-use-self-signed-default-routing-certificate branch from a2f816f to 4fa14e3 Compare January 26, 2019 10:24

ironcladlou reviewed Jan 28, 2019

View reviewed changes

mrogers950 reviewed Jan 28, 2019

View reviewed changes

pravisankar reviewed Jan 28, 2019

View reviewed changes

pkg/manifests/manifests.go Outdated Show resolved Hide resolved

pravisankar reviewed Jan 28, 2019

View reviewed changes

assets/router/deployment.yaml Outdated Show resolved Hide resolved

pkg/manifests/manifests_test.go Outdated Show resolved Hide resolved

Miciah force-pushed the NE-139-use-self-signed-default-routing-certificate branch from 4fa14e3 to 6b9ab0a Compare January 28, 2019 21:09

Miciah changed the title ~~[WIP] Use self-signed default router certificate~~ Use self-signed default router certificate Jan 28, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2019

Miciah force-pushed the NE-139-use-self-signed-default-routing-certificate branch from 6b9ab0a to 228c9b3 Compare January 28, 2019 22:17

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2019

openshift-ci-robot assigned ironcladlou Jan 29, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2019

openshift-merge-robot merged commit 9478e28 into openshift:master Jan 29, 2019

Miciah mentioned this pull request Jan 31, 2019

Put router-ca configmap in openshift-config-managed iff needed #110

Merged

Miciah mentioned this pull request May 3, 2019

Certs for the router issued for *.router.default.svc.cluster.local openshift/origin#20716

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use self-signed default router certificate #109

Use self-signed default router certificate #109

Miciah commented Jan 25, 2019 •

edited

ironcladlou commented Jan 25, 2019

Miciah commented Jan 25, 2019

Miciah commented Jan 25, 2019

Miciah commented Jan 25, 2019

Miciah commented Jan 26, 2019 •

edited

Miciah commented Jan 26, 2019

Miciah commented Jan 26, 2019

Miciah commented Jan 26, 2019

Miciah commented Jan 26, 2019

Miciah commented Jan 26, 2019

ironcladlou Jan 28, 2019

knobunc Jan 28, 2019

ironcladlou Jan 28, 2019

Miciah Jan 28, 2019

knobunc commented Jan 28, 2019

ironcladlou commented Jan 28, 2019

mrogers950 Jan 28, 2019

Miciah Jan 28, 2019 •

edited

mrogers950 Jan 28, 2019

Miciah commented Jan 28, 2019

Miciah commented Jan 28, 2019

Miciah commented Jan 29, 2019

ironcladlou commented Jan 29, 2019

ironcladlou commented Jan 29, 2019

openshift-ci-robot commented Jan 29, 2019

Use self-signed default router certificate #109

Use self-signed default router certificate #109

Conversation

Miciah commented Jan 25, 2019 • edited

Add library-go dependency