modules/tls: modularize TLS provisioning #1811

s-urbaniak · 2017-08-31T08:30:37Z

Currently everything is based off the Kube CA certificate.

This changes it by:

Modularization of TLS bootstrapping into separate modules/tls subsystems. This allows swapping in/out user-provided vs. self-signed certificates.
Configuring a dedicated Dex client CA in Tectonic console pointing to the identity grpc client CA.
Configuring a dedicated OIDC CA (pointing currently to the ingress CA) certificate for the API server.
Configuring a dedicated ingress CA certificate for the Tectonic console.

modules/tls/
├── etcd (self-signed or user-provided)
├── identity
│   └── self-signed
├── ingress
│   ├── self-signed
│   └── user-provided
└── kube
    └── self-signed

/cc @aaronlevy @sym3tri Note this PR introduces manifest changes! I distilled those in 40da8e2:

Added oidc-ca.crt in kube-apiserver-secret.yaml. This can point to the Kube CA in existing clusters.
Changed --oidc-ca-file setting of the API server which previously pointed to the Kube CA and now points to the above oidc-ca.crt
Added the BRIDGE_DEX_CLIENT_CA_FILE env variable in the console deployment which points to the grpc client CA certificate tectonic-identity-grpc-client-secret/ca-cert. This secret is already present in existing clusters.
The ca-cert in the tectonic-ca-cert-secret secret now points to a dedicated ingress CA certificate, currently effectively pointing to the Kube CA in existing clusters. I don't envision a necessary upgrade path here, but added it in this list for completeness.

Fixes INST-64

s-urbaniak · 2017-08-31T08:31:04Z

/cc @alexsomesan @squat

s-urbaniak · 2017-08-31T09:54:16Z

ok to test

s-urbaniak · 2017-08-31T11:04:38Z

ok to test

s-urbaniak · 2017-08-31T12:53:53Z

ok to test

aaronlevy · 2017-08-31T16:11:53Z

My 30 second take would be:

Added oidc-ca.crt in kube-apiserver-secret.yaml. This can point to the Kube CA in existing clusters.

This would need to have migration code written to copy the kube-ca in existing clusters to the oidc-ca.crt in the secret file.

Changed --oidc-ca-file setting of the API server which previously pointed to the Kube CA and now points to the above oidc-ca.crt

Would require that the above migration is done so that the new secret/path exists. Then if patch implementation is finished this flag could be fixed as part of normal update process. If it's not finished, need to write migration for all existing clusters to update to the new path.

Added the BRIDGE_DEX_CLIENT_CA_FILE env variable in the console deployment which points to the grpc client CA certificate tectonic-identity-grpc-client-secret/ca-cert. This secret is already present in existing clusters.

If we can get the patch implementation finished, this should be handled with the normal manifest update. If not, we need to write migration code to add this env var to all existing clusters.

The ca-cert in the tectonic-ca-cert-secret secret now points to a dedicated ingress CA certificate, currently effectively pointing to the Kube CA in existing clusters. I don't envision a necessary upgrade path here, but added it in this list for completeness.

As long as names/paths didn't change, and only the content this is probably fine.

cc @diegs @yifan-gu @derekparker

diegs · 2017-08-31T17:28:39Z

Thanks for the heads up, this is a good candidate for early testing using the merge strategy.

squat · 2017-09-01T03:17:41Z

modules/tectonic/resources/manifests/secrets/ca-cert.yaml

 type: Opaque
 data:
-  ca-cert: ${ca_cert}
+  ca-cert: ${ingress_ca_cert}


This needs to be the CA cert for the CA that signed the identity certificates, not the CA that signed the ingress certs. In its current state, tectonic-console will fail to come up because the identity TLS certs are not trusted. I don't think anyone actually requires the ingress CA cert. We can remove it from the ingress certs module API.

In our current setup yes, but not in the case of user-provided, externally signed certs. In the case of user provided certs this is the public CA that signed the ingress CSR, i.e. some public authority (I used let's encrypt for my tests).

You need to set BRIDGE_DEX_CLIENT_CA_FILE (Dex's identity GRPC client CA) in console, otherwise console would crash loop as you mentioned, see 40da8e2#diff-75010753e0caeb22b20777ee897ebebdR102. (I don't see this setting in your PR https://github.com/coreos/tectonic-installer/pull/1818/files).

Side note: I tested successfully this PR with certs signed with let's encrypt, providing them as described in https://github.com/s-urbaniak/tectonic-installer/blob/40da8e2a840df0254c15c5b90947e561812bc563/modules/tls/ingress/user-provided/README.md.

I hope that clarifies it a bit. I will draw a graph today which gives more clarity about our TLS topology. @sym3tri and me yesterday did a first pass and he documented the results in https://docs.google.com/document/d/1nRkwCCL5QkqeqUwMPo1ax2gBLzQ-x7keMI98pSxljL4/edit.

Hmm there is one detail that seems odd to me. Does console use BRIDGE_CA_FILE as the Dex CA cert if that cert is not explicitly passed? That is a funny gotcha that we should rectify.

Yes, just confirmed that dex ca defaults to the console ca, that's was the source of my confusion. I'll make sure to add both flags to my PR as well.

s-urbaniak · 2017-09-12T09:17:51Z

ok to test

s-urbaniak · 2017-09-12T10:55:14Z

ok to test

s-urbaniak · 2017-09-12T12:05:38Z

ok to test

s-urbaniak · 2017-09-12T14:08:57Z

just fyi: also tested that this change becomes idempotent in conjunction with the changes made in the kvo.

s-urbaniak · 2017-09-12T18:56:20Z

@alexsomesan PTAL, CI is flaky, but this PR used to be green (just moved one line in env vars) and tested locally.

alexsomesan

Looks great from a TF perspective.
I'll trigger the tests one more time.

Ideally we pair this up with a smoke test that procures some certs, injects them into the installer and checks from a client's perspective if all looks as expected.
But I presume that's quite a bit of work, so most likely should be a follow up.

alexsomesan · 2017-09-12T21:38:53Z

retest this please

This adds a new modules under `modules/tls/*` which is necessary for swappable TLS certificates.

Currently everything is based off the Kube CA certificate. This changes it by: 1. Configuring a dedicated Dex client CA in Tectonic console pointing to the identity grpc client CA. 2. Configuring a dedicated OIDC CA certificate for the API server. 3. Configuring a dedicated ingress CA certificate for the Tectonic console. This will allow to specify separate CA certificates.

s-urbaniak · 2017-09-13T08:23:50Z

@alexsomesan

Ideally we pair this up with a smoke test that procures some certs, injects them into the installer and checks from a client's perspective if all looks as expected.
But I presume that's quite a bit of work, so most likely should be a follow up.

Definitely 👍 we can do this though only once optional modules land, currently this is a manual process.

s-urbaniak · 2017-09-13T13:17:17Z

CI failure is a known flake in the vpc test, all other tests are green
KVO upgrade migration has been implemented (but not merged yet upstream!)

hence merging to move forward.

This reverts commit db65ea2.

We set this in both kube-apiserver-secret.go and openshift-apiserver-secret.go, and we started setting this in db65ea2 (modules/tls: modularize TLS provisioning, 2017-09-13, coreos/tectonic-installer#1811). But we have been using the kube-ca value for it since at least 4d636d3 (asset/manifests: bootstrap manifest generation, 2018-08-31, openshift#286) and I see no other OpenShift consumers [1]. [1]: https://github.com/search?q=org%3Aopenshift+oidc-ca.crt&type=Code

s-urbaniak added do-not-merge run-smoke-tests waiting-response labels Aug 31, 2017

alexsomesan requested review from aaronlevy, alekssaul, alexsomesan and coresolve August 31, 2017 10:08

s-urbaniak mentioned this pull request Aug 31, 2017

[WIP] acme: initial commit #1483

Closed

squat reviewed Sep 1, 2017

View reviewed changes

squat mentioned this pull request Sep 1, 2017

Ingress certs #1818

Closed

sym3tri added the needs-upgrade-migration label Sep 4, 2017

s-urbaniak mentioned this pull request Sep 7, 2017

Add existing certificate support. #1036

Closed

sym3tri mentioned this pull request Sep 11, 2017

Support for User provided Custom CA trust #774

Closed

s-urbaniak force-pushed the inst-64 branch from 40da8e2 to ada9510 Compare September 12, 2017 08:30

cpanato closed this Sep 12, 2017

cpanato reopened this Sep 12, 2017

s-urbaniak removed the do-not-merge label Sep 12, 2017

alexsomesan previously approved these changes Sep 12, 2017

View reviewed changes

Sergiusz Urbaniak added 2 commits September 13, 2017 07:13

modules/tls: initial commit

8ac16a9

This adds a new modules under `modules/tls/*` which is necessary for swappable TLS certificates.

s-urbaniak dismissed alexsomesan’s stale review via 04fde58 September 13, 2017 05:39

s-urbaniak force-pushed the inst-64 branch from ada9510 to 04fde58 Compare September 13, 2017 05:39

s-urbaniak removed needs-upgrade-migration waiting-response labels Sep 13, 2017

alexsomesan approved these changes Sep 13, 2017

View reviewed changes

s-urbaniak merged commit db65ea2 into coreos:master Sep 13, 2017

s-urbaniak pushed a commit that referenced this pull request Sep 13, 2017

Revert "modules/tls: modularize TLS provisioning (#1811)"

3f6f7cf

This reverts commit db65ea2.

s-urbaniak mentioned this pull request Sep 13, 2017

Revert "modules/tls: modularize TLS provisioning" #1903

Merged

s-urbaniak pushed a commit that referenced this pull request Sep 13, 2017

Revert "modules/tls: modularize TLS provisioning (#1811)" (#1903)

0e3a0d6

This reverts commit db65ea2.

s-urbaniak mentioned this pull request Sep 14, 2017

modules/tls: modularize TLS provisioning #1910

Merged

wking mentioned this pull request Oct 29, 2018

Drop oidc-ca.crt? openshift/installer#564

Closed

wking mentioned this pull request Oct 29, 2018

pkg/asset/manifests: Drop oidc-ca.crt openshift/installer#568

Merged

modules/tls: modularize TLS provisioning #1811

modules/tls: modularize TLS provisioning #1811

Uh oh!

Conversation

s-urbaniak commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-urbaniak commented Aug 31, 2017

Uh oh!

s-urbaniak commented Aug 31, 2017

Uh oh!

s-urbaniak commented Aug 31, 2017

Uh oh!

s-urbaniak commented Aug 31, 2017

Uh oh!

aaronlevy commented Aug 31, 2017

Uh oh!

diegs commented Aug 31, 2017

Uh oh!

squat Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Sep 1, 2017 • 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!

squat Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

squat Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Sep 12, 2017

Uh oh!

s-urbaniak commented Sep 12, 2017

Uh oh!

s-urbaniak commented Sep 12, 2017

Uh oh!

s-urbaniak commented Sep 12, 2017

Uh oh!

s-urbaniak commented Sep 12, 2017

Uh oh!

alexsomesan left a comment

Choose a reason for hiding this comment

Uh oh!

alexsomesan commented Sep 12, 2017

Uh oh!

s-urbaniak commented Sep 13, 2017

Uh oh!

s-urbaniak commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

s-urbaniak commented Aug 31, 2017 •

edited

Loading

s-urbaniak Sep 1, 2017 •

edited

Loading

s-urbaniak commented Sep 13, 2017 •

edited

Loading