Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always publish router-ca configmap #329

Conversation

benjaminapetersen
Copy link

It seems likely we will always have a need for the router-ca, given several of the bugs we have been dealing with. Is there any negative in always publishing this CA?

/assign @Miciah

/cc @spadgett @deads2k
per ongoing conversations

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 20, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: benjaminapetersen
To complete the pull request process, please assign knobunc
You can assign the PR to them by writing /assign @knobunc in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@Miciah
Copy link
Contributor

Miciah commented Nov 20, 2019

It seems likely we will always have a need for the router-ca, given several of the bugs we have been dealing with. Is there any negative in always publishing this CA?

This strategy was rejected by the auth team: openshift/cluster-kube-apiserver-operator#222 (comment)

Also, I don't think it solves the problem you want to solve—if the router-ca configmap is not published now, it is because it is not used to generate the default certificate. What we want (per @deads2k) is to publish the default certificate itself, if the administrator provided one, for the default ingresscontroller (or whichever ingresscontroller has the cluster ingress domain).

@ironcladlou
Copy link
Contributor

Can we get an EP instead? This topic is too confused to reason about through disconnected conversations in the context of the ingress operator. Can we spell out exactly what is published and under what conditions, who consumes what and how?

@openshift-ci-robot
Copy link
Contributor

@benjaminapetersen: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws 31c75d3 link /test e2e-aws
ci/prow/e2e-aws-operator 31c75d3 link /test e2e-aws-operator

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@benjaminapetersen
Copy link
Author

@Miciah thanks for the details!

@ironcladlou definitely in favor of that request, I would like a doc describing what is published under what circumstances so we can make a clean decision about how to consume. Is anyone owning that?

@Miciah
Copy link
Contributor

Miciah commented Nov 20, 2019

I can put together an enhancement proposal describing router-ca next week; do we need something sooner?

@deads2k
Copy link
Contributor

deads2k commented Nov 20, 2019

Can we get an EP instead? This topic is too confused to reason about through disconnected conversations in the context of the ingress operator. Can we spell out exactly what is published and under what conditions, who consumes what and how?

authentication and console operators and logically oauth-proxies all need to be able to trust the default wildcard certificate for the router, regardless of where it comes from. This is needed for end-to-end health checks and oauth server trust during the three legged oauth dance.

I'd like to see an enhancement from the ingress team or even better a PR from the networking team. I'm glad to see active involvement from the console team trying to resolve the issue.

@sttts this has a big impact, @benjaminapetersen may need some help while I'm out on PTO.

@Miciah
Copy link
Contributor

Miciah commented Nov 20, 2019

authentication and console operators and logically oauth-proxies all need to be able to trust the default wildcard certificate for the router

The situation with oauth-proxy is even worse than that: It needs both the certificate and key because oauth-proxy terminates TLS for its passthrough route with a host under the ingress domain. For that reason, the authentication operator is now using the router-certs secret, which has ingresscontrollers' certificates and keys and which we are even more hesitant to codify.

@Miciah
Copy link
Contributor

Miciah commented Nov 21, 2019

openshift/enhancements#126 adds an enhancement to describe the current situation. It's a quick draft and does not address where we need to go, but it might useful as a starting point.

@stlaz
Copy link
Member

stlaz commented Nov 21, 2019

@Miciah I'm a bit confused by some of these comments:

This strategy was rejected by the auth team: openshift/cluster-kube-apiserver-operator#222 (comment)

As far as I can tell that comment tries to prevent you from adding your CA in the bundle for client-CAs, and that comment is right. I don't see how that relates to always publishing the router-ca.

The situation with oauth-proxy is even worse than that

I think this might be just a typo, but oauth-proxy does not require you to do that, it's the oauth-server.

if the router-ca configmap is not published now, it is because it is not used to generate the default certificate.

Why wouldn't you publish the correct CA in the router-ca configMap in that case?

@Miciah
Copy link
Contributor

Miciah commented Nov 21, 2019

As far as I can tell that comment tries to prevent you from adding your CA in the bundle for client-CAs, and that comment is right. I don't see how that relates to always publishing the router-ca.

All right, but if we published router-ca unconditionally, then anything that used library-go's resource sync controller to consume router-ca would need logic to determine when to use and when to ignore router-ca, right? The requirement is not to trust it if it is not in use by the ingress operator.

I think this might be just a typo, but oauth-proxy does not require you to do that, it's the oauth-server.

You're right, it is oauth-server that needs the certificate and key. Does oauth-proxy need the CA or certificate at all?

Why wouldn't you publish the correct CA in the router-ca configMap in that case?

Because we do not have it if a custom default certificate is used (or we may have it, but we would need to analyze the provided default certificate secret and check the proxy trusted CA and any other locations where it might have been provided).

@benjaminapetersen
Copy link
Author

 Because we do not have it if a custom default certificate is used (or we may have it, but we
 would need to analyze the provided default certificate secret and check the proxy trusted 
 CA and any other locations where it might have been provided).

It seems we do want/need one component to do the analysis & get the correct cert to the target location in openshift-config-managed where the rest of the components can scoop it up and use it, or did I misunderstand?

@ironcladlou
Copy link
Contributor

I recommend the conversation be taken to openshift/enhancements#126

@stlaz
Copy link
Member

stlaz commented Nov 22, 2019

All right, but if we published router-ca unconditionally, then anything that used library-go's resource sync controller to consume router-ca would need logic to determine when to use and when to ignore router-ca, right? The requirement is not to trust it if it is not in use by the ingress operator.

No, you would have to make sure you are publishing the correct CA cert. If it signs the server cert for routes, it is in use by the ingress-operator, isn't it?

You're right, it is oauth-server that needs the certificate and key. Does oauth-proxy need the CA or certificate at all?

oauth-proxy needs at least the router-ca, it consumes it from /var/run/secrets/kubernetes.io/serviceaccount/ca.crt, where it's put by the KCM by taking it from router-ca CM IIRC.

Because we do not have it if a custom default certificate is used (or we may have it, but we would need to analyze the provided default certificate secret

In that case, why wouldn't you grab its CA from it (or from the secret) and publish it in router-ca?

and check the proxy trusted CA and any other locations where it might have been provided).

You shouldn't need to check the proxy-trusted CA, that CM should only contain a CA to connect to a MitM proxy, not to a route.

I recommend the conversation be taken to openshift/enhancements#126

Sure, if you could only answer the questions from this comment, I'm still trying to understand the problem.

@Miciah
Copy link
Contributor

Miciah commented Nov 22, 2019

If it signs the server cert for routes, it is in use by the ingress-operator, isn't it?

Correct.

In that case, why wouldn't you grab its CA from it (or from the secret) and publish it in router-ca?

That would be an option, but the secret may not have the CA, and publishing routaer-ca if, and only if, the secret includes the CA was considered too implicit an API. It wouldn't object to revisiting the idea though.

@stlaz
Copy link
Member

stlaz commented Nov 22, 2019

Hm, that's bad, I would have kind of expected the CA to be always included. I guess it might make sense to revisit it in the enhancement, although it sounds like it might be hard to get such requirement in at this point

@Miciah
Copy link
Contributor

Miciah commented Dec 16, 2019

Superseded by #331
/close

@openshift-ci-robot
Copy link
Contributor

@Miciah: Closed this PR.

In response to this:

Superseded by #331
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@benjaminapetersen benjaminapetersen deleted the configmap/router-ca/always-publish branch December 17, 2019 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants