feat: Implement TLS by default for Minikube + Helm installer #476

mmorhun · 2020-01-30T10:52:59Z

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What does this PR do?

Implements automatic installation of Che in TLS mode for Minikube infrastructure and Helm installer.
Theoretically should also work on Kubernetes and MicroK8S

How to test

To test, build this PR with changes from eclipse-che/che#15878 into Helm templates (just change in chectl's package.json eclipse-che dependency to point to the che-15810 branch).
Then, just run chectl server:start --platform=minikube --multiuser --installer=helm
To install single-user Che, omit --multiuser flag.

How it works

At the checks stage chectl checks for che-tls secret presence. If it is not present, then the procedure of auto-generation begins. If the secret is pre-created (current scenario) then nothing is changes and everything should work as it was before.
In case of auto-generation, first, chectl installs Cert Manager (using the pre-cached yaml from official site) for managing tls secrets for Che. This involves generation of CA credentials for Cert Manager, which is done via dedicated job. When all requirements are satisfied, chectl requests from Cert Manager tls secret for Che encryption.
The only manual step which is required from end users is to add CA certificate into their browsers.

What issues does this PR fix or reference?

eclipse-che/che#15313
Should be merged together with eclipse-che/che#15878

sleshchenko

Man, you did a great job around it 👍

resources/cert-manager/che-cluster-issuer.yml

src/commands/server/start.ts

src/constants.ts

sleshchenko · 2020-01-31T07:15:33Z

README.md

      (for CodeReady Containers)", "microk8s".

  -s, --tls
      Enable TLS encryption.
-                           Note that for kubernetes 'che-tls' with TLS certificate must be created in the configured 
+                           Note that for kubernetes 'che-tls' with TLS certificate must be created in the configured


It makes sense to add a note that TLS now is enabled for k8s, minikube... platforms and there is no way to disable it.
Well, maybe it would be better to leave this option for all platforms but with default option true? I mean keeping an ability to run server:start --tls=false

My plan was to remove that option at all when TLS by default is finished. In most cases people use default Che-Theia editor which in some cases doesn't work properly without TLS (all webview are broken). And, of course, the security...
We may leave the flag, but make it default to true, I do not mind. However, in such case we should test non-TLS installations as well... I think this should be decided on wider level.

As about note in readme, ok, will write it.

maybe keep it for backward compliance (but just to add deprecation notice when using it saying that now Eclipse Che is always in secured mode)

Agree with @benoitf

Ok, will do that.

src/commands/server/start.ts

src/tasks/component-installers/cert-manager.ts

sleshchenko · 2020-01-31T08:25:03Z

rm -rf ~/.cache/chectl helped to solve my issue with failed Che Server 🎉

src/tasks/component-installers/cert-manager.ts

tolusha · 2020-02-03T06:28:35Z

@mmorhun
README.md is automatically generated from sources when you launch yarn pack

tolusha · 2020-02-03T06:50:33Z

src/tasks/component-installers/cert-manager.ts

+          } else {
+            task.title = `${task.title}...not deployed`
+
+            return new Listr([


It is not good to return tasks inside task.
It is better to skip unnecessary using enable property. And the flag can be put into ctx

@tolusha I think it is a standard way to go. We may organize the sequence of tasks in more clear way: create a separate method which returns subtasks list. And that it will be clear to read (more we'll have displaying of the tasks as subtasks or will not display them at all if they are not needed to be run).
But if you really want to have flattened list of tasks, let it be so...

mmorhun · 2020-02-03T07:48:43Z

@tolusha thank you, I didn't know that readme is autogenerated... Fix move changes to the source code.

README.md

tomgeorge · 2020-02-03T14:53:12Z

At a high-level, I think it would be good to add a flag for the cert-manager namespace, in case the user already has their own cert-manager deployed that they want to use, in a namespace other than cert-manager.

Without knowing the context that went into deciding on using cert-manager for this, pulling in another cluster-addon component seems a little heavyweight when creating a TLS secret is all we need to do.

mmorhun · 2020-02-04T07:40:08Z

@tomgeorge thanks for the idea, but having two Cert Managers installed in the cluster would be weird. I think, instead we should request the secret from already existed Cert Manager, if any.
But that is out of scope of this PR.

tolusha

Sounds good.
Several remarks which can be fixed in the next PRs.

move dockerfile to che-dockerfiles
move templates to che
create and use const for certmanager docker image

tomgeorge · 2020-02-04T14:18:23Z

@mmorhun sorry, I meant that if a cert-manager is already installed, che should use that one. But understand that it is out of scope for this PR.

README.md

sleshchenko · 2020-02-04T15:11:28Z

src/tasks/component-installers/cert-manager.ts

+
+            const CA_CERT_GENERATION_SERVICE_ACCOUNT_NAME = 'ca-cert-generator'
+            const CA_CERT_GENERATION_JOB_NAME = 'ca-cert-generation-job'
+            const CA_CERT_GENERATION_JOB_IMAGE = 'quay.io/eclipse/che-cert-manager-ca-cert-generator:latest'


Is not src/constants.ts the right place for it?

Yes, that's the plan for next PR. I want to move dockerfile into Che repository and automate the build.

Could this be based on UBI instead of Alpine to make it a bit easier to productize? That way you could potentially RUN this build on demand rather than having Yet Another Container To Build, Publish, Maintain, Keep CVE Free, and Productize.

Even if you can't build this container on demand (inside Che when needed), moving to UBI would still make downstream easier. Or else I can contribute a rhel.Dockerfile variant that we can use in CRW.

templates/cert-manager/che-cluster-issuer.yml

mmorhun · 2020-02-05T07:22:23Z

@tomgeorge sorry, I probably misunderstood you. Usually, Cert Manager is installed in cert-manager namespace. For some installation methods (for example from yaml file) it is even hardcoded.

sleshchenko

Tested and it works for me. Great job 👍

Just one willing for increasing timeout for cert-manager since in my case it started in a few seconds after chectl reached timeout

mmorhun · 2020-02-05T10:54:59Z

@tomgeorge I've fixed all your remarks. Thanks for spotting.

mmorhun · 2020-02-10T08:11:05Z

@tomgeorge could you please take a look at this PR again and add more comments or unblock the merge?

metlos · 2020-02-10T10:54:04Z

README.md

@@ -270,8 +270,9 @@ OPTIONS

  -s, --tls
      Enable TLS encryption.
-                           Note that for kubernetes 'che-tls' with TLS certificate must be created in the configured 
-      namespace.
+                           Note, that this option is turned on by default for kubernetes infrastructure.


So when this is on by default, how do you actually switch it off? Do we have something like --no-tls?

No we don't have --no-tls flag, so a user cannot switch it off on Kubernetes-like infrastructures. The same is planned for Openshift soon. Although, we are going to keep the --tls flag for backward compatibility only.
Turning it off not only dangerous from user security point of view, but also some components might not work properly without TLS (as you probably know the story about Theia webviews).

so if TLS is on by default and no longer disableable, the flag would effectively do nothing?

Will you also ensure the fields in custom resource.yaml no longer work, so that it's no longer possible to set tlsSupport: false ?

@nickboldt yes the flag will do nothing... Please see the thread #476 (comment).

As about tlsSupport flag in CRDs... you are right, we have to take a look and prevent breaking stuff with wrong value of the flag.

But I think it should be done when I'll be working on operator part.

OK as long as this TODO is captured in a GH issue for that task, then +1. Just don't want to forget this part. :D

metlos · 2020-02-10T11:23:26Z

templates/cert-manager/cert-manager.yml

@@ -0,0 +1,6409 @@
+# This file is copied from:


How are we going to support this file in CRW? Have we checked that the licenses are compatible? https://github.com/jetstack/cert-manager/blob/master/LICENSES

Also, all these definitions (apart from the CRDs) are managed through helm. Would it not be possible to use helm for installing them instead of hardcoding this humongous file into our codebase?

Have we checked that the licenses are compatible?

Cert Manager is licensed under Apache License Version 2.0. That mean we can use it.

Would it not be possible to use helm for installing them

Yes, it is possible. But we plan to use the same approach for operator. And there, usage of Helm will be weird.

nickboldt · 2020-02-11T18:53:51Z

dockerfiles/cert-manager-ca-cert-generator-job/Dockerfile

+
+RUN apk add --no-cache openssl curl && \
+    cd /usr/local/bin && \
+    curl -s -LO https://storage.googleapis.com/kubernetes-release/release/`curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt`/bin/linux/amd64/kubectl && \


seems strange to join the -L and -O flags but not the -s flag here

nickboldt

see comments/questions.

nickboldt · 2020-02-11T18:59:18Z

dockerfiles/cert-manager-ca-cert-generator-job/entrypoint.sh

+CERT_MANAGER_NAMESPACE='cert-manager'
+CERT_MANAGER_CA_SECRET_NAME='ca'
+
+kubectl create secret tls $CERT_MANAGER_CA_SECRET_NAME --key=$CA_KEY_FILE --cert=$CA_CERT_FILE --namespace $CERT_MANAGER_NAMESPACE


So.. .this container is just kubectl + a cert? Instead of new container, could we instead just add this cert into https://github.com/che-dockerfiles/che-sidecar-kubernetes-tooling and keep the proliferation of containers (and therefore things that need to be productized) to a minimum? CRW 1.2 was 8 containers. 2.0 was 20. 2.1 is up to 23 and counting.

Can we try to reuse our existing tiny containers (and their existing builds/infra/sync jobs/productization pipeline) and just add a new kb of new files into them??

@nickboldt yes, this container indeed contains kubectl and openssl only, but we cannot reuse Kubernetes tolling sidecar for the Cert Manager job. First, they have different purposes: the job completes its tasks and exists, whereas Kubernetes tooling sidecar is designed to be run as a part of a workspace. Second, they have different aims: generate certificate and serve queries to Kubernetes respectively. Third, chectl (and Che installation process) should not depend on a Che sidecar. That's logically wrong and error prone: if something is changed in Kubernetes Che plugin it could break Che installation.

sigh OK, I was hoping for reuse and less proliferation of single-use containers. Your logic is sound, but still makes me sad. :(

nickboldt

+1 as is but I'd prefer if the base image was UBI.

Also if we could generate this container from inside Che rather than building it and storing it in Quay, it could be a transient artifact instead of one we maintain and manage.

mmorhun · 2020-02-14T07:26:09Z

@rhopp FYI: we are going to merge this PR right after 7.9 release. So, if you have any infrastructure which uses Helm Che installer, from that point, it will deploy Che in TLS mode with self-signed certificate.

rhopp · 2020-02-14T07:27:15Z

👍

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun requested review from benoitf, l0rd, tomgeorge, tolusha, sleshchenko and AndrienkoAleksandr January 30, 2020 10:52

mmorhun requested review from azatsarynnyy, nickboldt, rhopp and vparfonov as code owners January 30, 2020 10:52

mmorhun self-assigned this Jan 30, 2020

mmorhun force-pushed the poc-minikube-self-signed-cert branch from 7bf3083 to 28e236c Compare January 30, 2020 14:44

mmorhun changed the title ~~Implement TLS by default for Minikube + Helm installer~~ feat: Implement TLS by default for Minikube + Helm installer Jan 30, 2020

mmorhun mentioned this pull request Jan 30, 2020

Remove selfSignedCertSecretName property eclipse-che/che#15878

Merged

sleshchenko reviewed Jan 31, 2020

View reviewed changes

src/tasks/component-installers/cert-manager.ts Outdated Show resolved Hide resolved

tolusha mentioned this pull request Feb 2, 2020

Che Deploy Sprint 179 eclipse-che/che#15839

Closed

35 tasks

tolusha reviewed Feb 3, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

AndrienkoAleksandr approved these changes Feb 3, 2020

View reviewed changes

mmorhun force-pushed the poc-minikube-self-signed-cert branch from 0e93564 to d26bd44 Compare February 4, 2020 07:19

tolusha approved these changes Feb 4, 2020

View reviewed changes

mmorhun force-pushed the poc-minikube-self-signed-cert branch from d26bd44 to 99e7e1a Compare February 4, 2020 10:22

sleshchenko reviewed Feb 4, 2020

View reviewed changes

sleshchenko approved these changes Feb 5, 2020

View reviewed changes

tolusha approved these changes Feb 10, 2020

View reviewed changes

metlos reviewed Feb 10, 2020

View reviewed changes

nickboldt reviewed Feb 11, 2020

View reviewed changes

nickboldt requested changes Feb 11, 2020

View reviewed changes

nickboldt self-requested a review February 13, 2020 15:57

nickboldt approved these changes Feb 13, 2020

View reviewed changes

tomgeorge approved these changes Feb 13, 2020

View reviewed changes

This was referenced Feb 17, 2020

Che Deploy Sprint #180 eclipse-che/che#16049

Closed

chectl fails to provision Che with tls option eclipse-che/che#15411

Closed

mmorhun mentioned this pull request Feb 17, 2020

Implement TLS by default for operator Che installer eclipse-che/che#16052

Closed

mmorhun force-pushed the poc-minikube-self-signed-cert branch from 5723032 to 2f9f01b Compare February 26, 2020 08:48

mmorhun added 5 commits February 26, 2020 11:10

Implement TLS by default for Minikube + Helm installer

a28594c

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Fixes according to review

33cc5a6

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Move resources folder into templates

3c721e6

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Fixes according to review

1587bd4

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Fixes according to review

b1b05b5

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun force-pushed the poc-minikube-self-signed-cert branch from 2f9f01b to b1b05b5 Compare February 26, 2020 09:10

Update yarn.lock

bcd927a

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun merged commit 05ce79b into master Feb 26, 2020

mmorhun deleted the poc-minikube-self-signed-cert branch February 26, 2020 09:25

mmorhun mentioned this pull request Feb 26, 2020

[POC] TLS by default eclipse-che/che#15313

Closed

l0rd mentioned this pull request Mar 4, 2020

TLS by default eclipse-che/che#14742

Closed

7 tasks

sleshchenko mentioned this pull request Mar 11, 2020

[chectl] Make it possible to install Eclipse Che with "--no-tls" option eclipse-che/che#16302

Closed

feat: Implement TLS by default for Minikube + Helm installer #476

feat: Implement TLS by default for Minikube + Helm installer #476

Conversation

mmorhun commented Jan 30, 2020 • edited Loading

What does this PR do?

How to test

How it works

What issues does this PR fix or reference?

sleshchenko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Jan 31, 2020

tolusha commented Feb 3, 2020

Choose a reason for hiding this comment

mmorhun Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

mmorhun commented Feb 3, 2020

tomgeorge commented Feb 3, 2020

mmorhun commented Feb 4, 2020

tolusha left a comment • edited Loading

Choose a reason for hiding this comment

tomgeorge commented Feb 4, 2020

Choose a reason for hiding this comment

mmorhun Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickboldt Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

mmorhun commented Feb 5, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

mmorhun commented Feb 5, 2020

mmorhun commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickboldt left a comment

Choose a reason for hiding this comment

nickboldt Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickboldt left a comment

Choose a reason for hiding this comment

mmorhun commented Feb 14, 2020

rhopp commented Feb 14, 2020

mmorhun commented Jan 30, 2020 •

edited

Loading

sleshchenko left a comment •

edited

Loading

mmorhun Jan 31, 2020 •

edited

Loading

mmorhun Feb 3, 2020 •

edited

Loading

tolusha left a comment •

edited

Loading

mmorhun Feb 5, 2020 •

edited

Loading

nickboldt Feb 13, 2020 •

edited

Loading

mmorhun Feb 12, 2020 •

edited

Loading

nickboldt Feb 11, 2020 •

edited

Loading