Add default-ingress-cert ConfigMap enhancement #126

Miciah · 2019-11-21T00:13:40Z

No description provided.

benjaminapetersen · 2019-11-21T18:54:50Z

enhancements/network/router-ca-configmap.md

+  has a custom default certificate?
+- If so, should the operator publish the default certificate itself to
+  `router-ca`?
+- If the operator publishes custom default certificates to `router-ca`,


This bullet point seems to summarize the need for a single well-known reference point for components to collect the relevant cert(s) that allow them to communicate with other components on the cluster.

I'm very interested in how these questions are answered. If for some reason a bundle of all relevant certs should not be published, I'd definitely like to see that the alternative is clearly documented.

benjaminapetersen · 2019-11-21T18:58:32Z

enhancements/network/router-ca-configmap.md

+
+## Summary
+
+The ingress operator publishes a `router-ca` configmap in the


This is what it does now, correct? Not a summary of what will be added?

Correct. This describes the current behavior.

benjaminapetersen · 2019-11-21T19:00:49Z

enhancements/network/router-ca-configmap.md

+
+### Goal
+
+Publish the ingress operator's signing certificate, if used, in a well-known


Also, if custom default certs are used, publish those?

benjaminapetersen · 2019-11-21T19:05:15Z

enhancements/network/router-ca-configmap.md

+
+Publishing the ingress operator's signing certificate does not help
+if the administrator configures a custom default certificate.
+


Though we do need a path forward for this case as well.

ironcladlou · 2019-11-21T19:17:14Z

See enhancements/x509-egress-trust (#115)

benjaminapetersen · 2019-11-21T19:59:45Z

Interesting. Would be curious to know why we would want different approaches. Namely, why config.openshift.io/inject-proxy-cabundle: "true" is used for the proxy CA, but not something like config.openshift.io/inject-default-cabundle: "true" would not be used to get the default CA. I'm not opposed to the well-known router-ca in openshift-config-managed, just contemplating the divergence of patterns.

In practice, we have to add a ResourceSyncController() to sync the router-ca to a configmap within our namespace, and then use the configmap.

Could the approaches be unified?

benjaminapetersen · 2019-11-21T20:14:27Z

Also, is there ever a case where a custom CA is used without proxy?

Miciah · 2019-11-22T03:49:58Z

Interesting. Would be curious to know why we would want different approaches. Namely, why config.openshift.io/inject-proxy-cabundle: "true" is used for the proxy CA, but not something like config.openshift.io/inject-default-cabundle: "true" would not be used to get the default CA. I'm not opposed to the well-known router-ca in openshift-config-managed, just contemplating the divergence of patterns.

Good idea. The router-ca configmap was ad hoc; the proxy API was designed later, with more deliberation. It makes sense to learn from the design process of the latter and re-use the pattern.

Miciah · 2019-11-22T03:51:15Z

Latest push adds the suggestion to inject the ingress CA into specifically labeled configmaps à la proxy config.

benjaminapetersen · 2019-11-22T16:21:55Z

That sounds great, like the consistency, like dropping the ResourceSyncer(). Just need to make sure we know what to do if it is conditionally published (I assume the annotation would be ignored, the CM would not be populated... need to know what to fallback on).

benjaminapetersen · 2019-11-22T16:30:17Z

enhancements/network/router-ca-configmap.md

+
+### Inject the Ingress CA into ConfigMaps with a Well Known Label
+
+Instead of or in addition to publishing the `router-ca` ConfigMap, the ingress


This sounds good to me. Since it is inject-ingress-cabundl instead of inject-default-cabundle, will we be able to assume that we can simply use it? As a consumer, we don't have to care if it is default or custom?

benjaminapetersen · 2019-11-27T15:35:45Z

I've commented mostly on UX/correct usage path, lets definitely ensure security issues are documented here as well.

benjaminapetersen · 2019-11-27T15:39:14Z

Going to cross reference this openshift/cluster-ingress-operator#331

danehans · 2019-11-27T19:54:00Z

enhancements/network/router-ca-configmap.md

+
+Instead of or in addition to publishing the `router-ca` ConfigMap, the ingress
+operator could inject the ingress CA into all ConfigMaps with a specific label,
+for example `config.openshift.io/inject-ingress-cabundle: "true"`.  This approach


Instead of having the ingress operator inject the ingress ca cert, why not have the existing trust bundle publishing controller handle this? The controller can be expanded to watch for the router-ca configmap, add the cert to the cluster-wide trusted ca bundle configmap and have the combined bundle injected into components. Most of this workflow is already implemented and this approach would allow a single operator to manage cluster-wide trust bundle publishing and injection. All components that rely on routes to verify access, should already be setup for the trust bundle injection so no additional changes should be required.

I like the idea of re-using an existing controller. However, we would still need a distinct annotation because components that need to trust the proxy CA may not need to trust router-ca, and we must maintain separation of chains of trust.

ack, I get your point and agree. I did a review of the the ingress operator cert code and I see the router-ca configmap is only created when it's the self generated cert (i.e. no ingress controllers define defaultCertificate). Why does the ingress operator publish the self generated cert and not user-defined certs?

The operator publishes the CA that it uses to sign operator-generated default certificates. If a custom default certificate is provided, we have the certificate but not the signing CA for that certificate (unless the administrator happens to include the CA in the default certificate secret, which we cannot assume to be the case). What @deads2k has proposed is to publish the custom default certificate itself as if it were the CA, which is technically wrong but will work for Go-based clients.

danehans · 2019-11-27T20:16:59Z

How does this enhancement relate to https://github.com/openshift/enhancements/pull/115/files? It may be a good idea to reference this enhancement and how the two relate.

Miciah · 2019-12-09T18:00:13Z

How does this enhancement relate to https://github.com/openshift/enhancements/pull/115/files? It may be a good idea to reference this enhancement and how the two relate.

I think the enhancements are not directly related. #126 concerns documenting and modifying how the ingress operator communicates with other components about certificates that the administrator can already configure through an established API whereas #115 concerns a new API for administrators to configure certificates and explicitly does not concern how components communicates with each other:

This enhancement does not address components with existing X.509 trust-distribution mechanisms.

Miciah · 2019-12-09T18:03:41Z

openshift/cluster-ingress-operator#331 to add default-ingress-cert has merged, and I have opened openshift/cluster-ingress-operator#336 to backport openshift/cluster-ingress-operator#331 to 4.3.z, so we are well underway in executing this enhancement.

ironcladlou · 2019-12-18T22:37:53Z

/lgtm

openshift-ci-robot · 2019-12-18T22:38:08Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [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 the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 21, 2019

openshift-ci-robot requested review from dgoodwin and soltysh November 21, 2019 00:14

Miciah mentioned this pull request Nov 21, 2019

Always publish router-ca configmap openshift/cluster-ingress-operator#329

Closed

benjaminapetersen reviewed Nov 21, 2019

View reviewed changes

Miciah force-pushed the router-ca-configmap branch from ae26a08 to 81be130 Compare November 22, 2019 03:46

benjaminapetersen reviewed Nov 22, 2019

View reviewed changes

benjaminapetersen mentioned this pull request Nov 26, 2019

bug 1772759: fix console operator panic when RouterCAValidationDegraded openshift/console-operator#355

Merged

danehans reviewed Nov 27, 2019

View reviewed changes

Miciah force-pushed the router-ca-configmap branch from 81be130 to 78d64f8 Compare December 3, 2019 00:54

Miciah mentioned this pull request Dec 3, 2019

Bug 1788712: publish a router-ca that can be used to verify routes in golang clients openshift/cluster-ingress-operator#331

Merged

Add default-ingress-cert ConfigMap enhancement

0e0d6ad

Miciah force-pushed the router-ca-configmap branch from 78d64f8 to 0e0d6ad Compare December 3, 2019 15:31

Miciah changed the title ~~Add router-ca configmap enhancement~~ Add default-ingress-cert ConfigMap enhancement Dec 9, 2019

jhadvig mentioned this pull request Dec 10, 2019

Sync default-ingress-cert to the console namespace openshift/console-operator#361

Merged

openshift-ci-robot assigned ironcladlou Dec 18, 2019

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

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

openshift-merge-robot merged commit 5fea2e7 into openshift:master Dec 18, 2019


		## Summary

		The ingress operator publishes a `router-ca` configmap in the


		### Goal

		Publish the ingress operator's signing certificate, if used, in a well-known


		Publishing the ingress operator's signing certificate does not help
		if the administrator configures a custom default certificate.


		### Inject the Ingress CA into ConfigMaps with a Well Known Label

		Instead of or in addition to publishing the `router-ca` ConfigMap, the ingress

Add default-ingress-cert ConfigMap enhancement #126

Add default-ingress-cert ConfigMap enhancement #126

Uh oh!

Conversation

Miciah commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminapetersen commented Nov 21, 2019

Uh oh!

benjaminapetersen commented Nov 21, 2019

Uh oh!

Miciah commented Nov 22, 2019

Uh oh!

Miciah commented Nov 22, 2019

Uh oh!

benjaminapetersen commented Nov 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Nov 27, 2019

Uh oh!

benjaminapetersen commented Nov 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danehans commented Nov 27, 2019

Uh oh!

Miciah commented Dec 9, 2019

Uh oh!

Miciah commented Dec 9, 2019

Uh oh!

ironcladlou commented Dec 18, 2019

Uh oh!

openshift-ci-robot commented Dec 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ironcladlou commented Nov 21, 2019 •

edited

Loading