Append router CA to cluster CA in kubeconfig #1242

sallyom · 2019-02-13T12:39:08Z

With users created with an identity-provider, OAuth flow goes through router and router-ca is not trusted. This prohibits the user from using oc login from command line, without manually appending the router ca to the cluster ca. The openshift-ingress-operator creates the router-CA and this is not available until the very end of an install.

This PR adds the router CA from configmap router-ca -n openshift-config-managed to the kubeconfig certificate-authority-data. This allows an identity-provided user to oc login from the terminal.

staebler

Can you add some context to the commit message describing the motivation behind this change?

staebler · 2019-02-13T15:19:31Z

cmd/openshift-install/create.go

+	routerSecret, err := client.CoreV1().Secrets("openshift-ingress-operator").Get("router-ca", metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil


Do we really want to silently ignore a missing router-ca secret?

I will check, but I think there's a possible expectation that it won't be created.

The secret will always be created. However, I'd suggest using the router-ca configmap in the openshift-config-managed namespace, which will be created when some clusteringress is using the operator-generated default certificate generated using that CA certificate, and will be absent otherwise.

thanks @Miciah will do!

but also, at the very least log the error. preferably don't swallow it at all.

I've added a log msg for that error

Won't we want to poll/wait until the config-map has a non-empty CA instead of giving up?

staebler · 2019-02-13T15:19:50Z

cmd/openshift-install/create.go

+		return err
+	}
+	routerCrtBytes := routerSecret.Data["tls.crt"]
+	absDir, err := filepath.Abs(directory)


Why do we need to use the absolute directory?

no we do not, fixed

staebler · 2019-02-13T15:22:26Z

cmd/openshift-install/create.go

+		return err
+	}
+	var kconfig *clientcmdv1.Config
+	err = yaml.Unmarshal(yml, &kconfig)


It would be nice to grab the kubeconfig.Admin asset from the store, which would already have the unmarshaled config. Unfortunately, (1) the asset store used has already gone out of scope, and (2) there is currently no way of fetching from the store without purging the other assets from disk.

right, I pondered that

staebler · 2019-02-13T15:23:09Z

cmd/openshift-install/create.go

+	}
+
+	clusterCABytes := kconfig.Clusters[0].Cluster.CertificateAuthorityData
+	appendedCA := string(routerCrtBytes) + string(clusterCABytes)


"Appended" is not quite the right word here since you are adding the router CA to the beginning not the end.

correct, thanks, i'll rename : )

You can use clientcmd.LoadFromFile and clientcmd.WriteToFile . Might want to use certpool helpers to concat the CAs as well

ah, awesome, thanks

staebler · 2019-02-13T15:23:40Z

cmd/openshift-install/create.go

+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return err


Wrap the error here and elsewhere in this function.

ericavonb · 2019-02-13T15:36:43Z

cmd/openshift-install/create.go

@@ -116,6 +119,10 @@ var (
 					logrus.Fatal(err)
 				}

+				if err = appendRouterCAToClusterCA(config, rootOpts.dir); err != nil {
+					logrus.Fatal(err)


Should this be fatal or only a warning? I thinking since it doesn't block accessing the cluster, but maybe it's ok to fail since this is the last step?

Should this be fatal or only a warning?

I'm fine failing fast. We can relax later if necessary, but unless this is fatal by default we don't get CI coverage (or will e2e start failing without this step?).

ericavonb · 2019-02-13T18:07:46Z

cmd/openshift-install/create.go

+	routerSecret, err := client.CoreV1().Secrets("openshift-ingress-operator").Get("router-ca", metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil


but also, at the very least log the error. preferably don't swallow it at all.

cmd/openshift-install/create.go

ericavonb · 2019-02-13T18:11:32Z

cmd/openshift-install/create.go

+		return err
+	}
+
+	clusterCABytes := kconfig.Clusters[0].Cluster.CertificateAuthorityData


check that kconfig != nil and len(kconfig.Clusters) > 0 before accessing nested fields

done, and outdated here

ericavonb · 2019-02-13T18:42:52Z

cmd/openshift-install/create.go

+	}
+
+	clusterCABytes := kconfig.Clusters[0].Cluster.CertificateAuthorityData
+	appendedCA := string(routerCrtBytes) + string(clusterCABytes)


You can use clientcmd.LoadFromFile and clientcmd.WriteToFile . Might want to use certpool helpers to concat the CAs as well

abhinavdahiya · 2019-02-13T23:39:00Z

The kubeconfig that you are adding the router ca to has admin credentials, and since you cannot distribute that kubeconfig to a user that is trying to oc login. So i'm still unclear on why this PR is ~~required~~ trying to solve?

I tried to login following the gist

$ unset KUBECONFIG
$ oc login adahiya-1-api.devcluster.openshift.com:6443 --v=6
I0213 14:57:06.751825    7586 round_trippers.go:405] HEAD https://adahiya-1-api.devcluster.openshift.com:6443/  in 273 milliseconds
The server uses a certificate signed by an unknown authority.
You can bypass the certificate check, but any data you send to the server could be intercepted by others.
Use insecure connections? (y/n): y

I0213 14:57:11.749795    7586 round_trippers.go:405] GET https://adahiya-1-api.devcluster.openshift.com:6443/.well-known/oauth-authorization-server 200 OK in 304 milliseconds
I0213 14:57:12.122486    7586 round_trippers.go:405] GET https://openshift-authentication-openshift-authentication.apps.adahiya-1.devcluster.openshift.com/oauth/authorize?client_id=openshift-challenging-client&code_challenge=pQE6QaNhx1d-qtBSA-Wer9BepDDep_Zqn8oT3KnS9mM&code_challenge_method=S256&redirect_uri=https%3A%2F%2Fopenshift-authentication-openshift-authentication.apps.adahiya-1.devcluster.openshift.com%2Foauth%2Ftoken%2Fimplicit&response_type=code 400 Bad Request in 372 milliseconds
I0213 14:57:12.123212    7586 helpers.go:201] server response object: [{
  "metadata": {},
  "status": "Failure",
  "message": "Internal error occurred: unexpected response: 400",
  "reason": "InternalError",
  "details": {
    "causes": [
      {
        "message": "unexpected response: 400"
      }
    ]
  },
  "code": 500
}]
F0213 14:57:12.123312    7586 helpers.go:119] Error from server (InternalError): Internal error occurred: unexpected response: 400

It looks like with the current setup oc login client requires you to trust 2 servers kube-apiserver and oauth server using the route.
So IMO what makes sense to me is:

either creating a bundle that has CAs for both and telling users to use that for --certificate-authority
or creating a special kubeconfig with apiserver url and CA set to the bundle..

ericavonb · 2019-02-14T01:41:57Z

The kubeconfig that you are adding the router ca to has admin credentials, and since you cannot distribute that kubeconfig to a user that is trying to oc login. So i'm still unclear on why this PR is required trying to solve?

This PR makes it so that an administrator can test their IDP configuration using the installer generated kubeconfig. For letting other users access the cluster, the admin will need to provide them with the CA bundle.

It looks like with the current setup oc login client requires you to trust 2 servers kube-apiserver and oauth server using the route.
Yes, that is correct. You need to trust the kube-apiserver for using the api and discovering the auth server, and you need to trust the auth server to securely authenticate according to the oauth spec.

* either creating a bundle that has CAs for both and telling users to use that for `--certificate-authority`
* or creating a special kubeconfig with `apiserver url` and CA set to the bundle..

The cluster data in the kubeconfig is meant to store that context. I think it's the appropriate place to put it.

sallyom · 2019-02-14T16:30:30Z

either creating a bundle that has CAs for both and telling users to use that for --certificate-authority

That would work, but would be a major shift from 3.x. People expect to be able to just
oc login -u user -p password or oc login <server> --token blah <--not sure where we stand on that atm

sallyom · 2019-02-14T21:24:49Z

/refresh

sallyom · 2019-02-14T21:25:16Z

/test e2e-aws

mrogers950 · 2019-02-14T21:58:52Z

cmd/openshift-install/create.go

+		return errors.New("kubeconfig CertificateAuthorityData not found")
+	}
+	certPool := x509.NewCertPool()
+	if !certPool.AppendCertsFromPEM(clusterCABytes) {


You're using this just for a parsing check, correct? If so, the error should say something about the CA being not in valid PEM format.

correct, thanks, I've updated the error msg

ericavonb

/lgtm

abhinavdahiya · 2019-02-15T16:12:43Z

@sallyom can you update the commit message
https://github.com/openshift/installer/blob/master/CONTRIBUTING.md#commit-message-format

Then we lgtm and merge

wking · 2019-02-15T16:24:26Z

cmd/openshift-install/create.go

+	}
+
+	routerCrtBytes := []byte(caConfigMap.Data["ca-bundle.crt"])
+	kubeconfig := filepath.Join(directory, "auth", "kubeconfig")


Ideally we'd hold this in memory, but this is fine short-term.

yes I think there's a FIXME already elsewhere in this file for that

wking · 2019-02-15T16:27:22Z

cmd/openshift-install/create.go

+		return errors.Wrap(err, "loading kubeconfig")
+	}
+
+	if kconfig == nil || len(kconfig.Clusters) == 0 {


I think we want:

if kconfig == nil || len(kconfig.Clusters) != 1 { return errors.New("invalid kubeconfig") }

which will also allow us to drop the following loop over Clusters. We will be able to drop this guard once we switch to keeping the kubeconfig in memory.

I'll open a follow-up? I could not find how to get this string:

// Clusters is a map of referencable names to cluster configs Clusters map[string]*Cluster

here

referencable names to cluster configs? What is that reference-able name? I'm happy to do some testing (and open a PR to clarify for others) to figure that out (or maybe someone knows and can tell me). But if there_were_ more than 1 cluster I'd want to add to each, so the != 0 seems ok here. If not I can also include that in the follow-up PR.

abhinavdahiya · 2019-02-15T17:24:54Z

/approve

openshift-ci-robot · 2019-02-15T17:25:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ericavonb, sallyom

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~~ [abhinavdahiya]

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

wking · 2019-02-19T07:17:19Z

Hmm. Does this mean "we never rotate the router CA"? Because if we did, it would break oc login with this kubeconfig, right?

sallyom · 2019-02-22T19:30:53Z

@wking yes, correct, a change in router-CA will break oc login unless kubeconfig is modified. Atm, cluster-ingress-operator does not rotate certificates.

enj · 2019-02-23T18:23:26Z

Hmm. Does this mean "we never rotate the router CA"? Because if we did, it would break oc login with this kubeconfig, right?

Yes, and this is expected. This change is only expected to handle the OOTB experience. oc honors your system built in roots, so it work if the router CA is replaced as long as it is a globally valid cert or the admin has arranged for the cert to be trusted within the corporate environment (the latter is what I expect to be the common case).

This was added in 4033577 (Append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242), where the motivation was [1]: With users created with an identity-provider, OAuth flow goes through router and router-ca is not trusted. This prohibits the user from using oc login from command line, without manually appending the router ca to the cluster ca. The openshift-ingress-operator creates the router-CA and this is not available until the very end of an install. This PR adds the router CA from configmap router-ca -n openshift-config-managed to the kubeconfig certificate-authority-data. This allows an identity-provided user to oc login from the terminal. But the admin kubeconfig is already authenticated, so folks shouldn't be using 'oc login' with that kubeconfig. For example, see dee6929 (Modify kubeadmin usage message, admins should not use kubeadmin via CLI, 2019-04-01, openshift#1513). That leaves us with no use case for this modification, and making the finish code watch-only sets us up to rename away from the user-provided-infrastructure subcommand. [1]: openshift#1242 (comment)

The console may become optional [1], so teach the installer to handle its absence gracefully. We've waited on the console since way back in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806). Back then, install-complete timing was much less organized, and since e17ba3c (cmd: wait for the cluster to be initialized, 2019-01-25, openshift#1132) we've blocked on ClusterVersion going Available=True. So the current dependency chain is: 1. Console route admission blocks console operator from going Available=True in its ClusterOperator. 2. Console ClusterOperator blocks cluster-version operator from going Available=True in ClusterVersion. 3. ClusterVersion blocks installer's waitForInitializedCluster. So we no longer need to wait for the route to show up, and can fail fast if we get a clear IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup. We don't want to drop the console check entirely, because when it is found, we want: * To continue to log that access pathway on install-complete. * To continue to append the router CA to the kubeconfig. That latter point has been done since 4033577 (append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242). The motivation in that commit message is not explicit, but the idea is to support folks who naively run 'oc login' with the kubeadmin kubeconfig [2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default. [1]: openshift/enhancements#922 [2]: openshift#1541 (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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2019

openshift-ci-robot requested review from abhinavdahiya and hardys February 13, 2019 12:39

staebler reviewed Feb 13, 2019

View reviewed changes

sallyom changed the title ~~WIP: append router CA to cluster CA in kubeconfig~~ Append router CA to cluster CA in kubeconfig Feb 13, 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 Feb 13, 2019

stlaz mentioned this pull request Feb 13, 2019

Delete operator 1 openshift/cluster-authentication-operator#59

Merged

ericavonb reviewed Feb 13, 2019

View reviewed changes

sallyom force-pushed the append-router-ca-to-kc branch 2 times, most recently from 83ed4f6 to 2624e1b Compare February 13, 2019 22:44

sallyom force-pushed the append-router-ca-to-kc branch from 2624e1b to 40311e5 Compare February 13, 2019 23:58

sallyom force-pushed the append-router-ca-to-kc branch 2 times, most recently from 354a8b6 to fcef841 Compare February 14, 2019 02:01

sallyom force-pushed the append-router-ca-to-kc branch from fcef841 to 0afd02b Compare February 14, 2019 18:29

mrogers950 reviewed Feb 14, 2019

View reviewed changes

sallyom force-pushed the append-router-ca-to-kc branch 2 times, most recently from 29cc938 to 5d0e43b Compare February 15, 2019 15:15

append router CA to cluster CA in kubeconfig

4033577

sallyom force-pushed the append-router-ca-to-kc branch from 5d0e43b to 4033577 Compare February 15, 2019 15:22

ericavonb approved these changes Feb 15, 2019

View reviewed changes

openshift-ci-robot assigned ericavonb Feb 15, 2019

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

stlaz mentioned this pull request Feb 15, 2019

operator: change source of apiURL to infrastructure.config.openshift.io openshift/cluster-authentication-operator#72

Merged

wking reviewed Feb 15, 2019

View reviewed changes

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

openshift-merge-robot merged commit 35c81e1 into openshift:master Feb 15, 2019

wking mentioned this pull request Apr 5, 2019

cmd/openshift-install/create: Drop addRouterCAToClusterCA #1541

Closed

wking mentioned this pull request Oct 27, 2021

cmd/openshift-install/create: One shot console access #5336

Merged

Append router CA to cluster CA in kubeconfig #1242

Append router CA to cluster CA in kubeconfig #1242

Uh oh!

Conversation

sallyom commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler left a comment

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!

Choose a reason for hiding this comment

Uh oh!

sallyom Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

abhinavdahiya commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericavonb commented Feb 14, 2019 • edited by abhinavdahiya Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sallyom commented Feb 14, 2019

Uh oh!

sallyom commented Feb 14, 2019

Uh oh!

sallyom commented Feb 14, 2019

Uh oh!

mrogers950 Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sallyom Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericavonb left a comment

sallyom commented Feb 13, 2019 •

edited

Loading

sallyom Feb 13, 2019 •

edited

Loading

abhinavdahiya commented Feb 13, 2019 •

edited

Loading

ericavonb commented Feb 14, 2019 •

edited by abhinavdahiya

Loading

mrogers950 Feb 14, 2019 •

edited

Loading

sallyom Feb 14, 2019 •

edited

Loading

sallyom Feb 15, 2019 •

edited

Loading