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

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

Merged
merged 6 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

If it is needed to provide own certificate, 'che-tls' secret with TLS certificate must be
sleshchenko marked this conversation as resolved.
Show resolved Hide resolved
created in the configured namespace. Otherwise, it will be automatically generated.
For OpenShift, router will use default cluster certificates.

-t, --templates=templates
Expand Down Expand Up @@ -319,8 +320,9 @@ OPTIONS
persistent volume storage class name to use to store Eclipse Che Postgres database

--self-signed-cert
Authorize usage of self signed certificates for encryption. Note that `self-signed-cert` secret with CA certificate
must be created in the configured namespace.
Authorize usage of self signed certificates for encryption.
This is the flag for Che to propagate the certificate to components, so they will trust it.
Note that `che-tls` secret with CA certificate must be created in the configured namespace.
sleshchenko marked this conversation as resolved.
Show resolved Hide resolved

--skip-version-check
Skip minimal versions check.
Expand Down
11 changes: 11 additions & 0 deletions dockerfiles/cert-manager-ca-cert-generator-job/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# CA key pair generation job container
FROM alpine

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 && \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

chmod +x kubectl && \
apk del curl

COPY entrypoint.sh /entrypoint.sh
ENTRYPOINT ["/entrypoint.sh"]
3 changes: 3 additions & 0 deletions dockerfiles/cert-manager-ca-cert-generator-job/build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/bin/sh

docker build -t quay.io/eclipse/che-cert-manager-ca-cert-generator .
36 changes: 36 additions & 0 deletions dockerfiles/cert-manager-ca-cert-generator-job/entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#!/bin/sh

CA_KEY_FILE='ca.key'
CA_CERT_FILE='ca.crt'

# Generate private key for root CA
# Options:
# -out : name of file to write generated key to
# 4096 : number of bits in the key
openssl genrsa -out $CA_KEY_FILE 4096

# Generate CA certificate and sign it with previously generated key.
# Options:
# -batch : script (non-interactive) mode
# -new : creates new sertificate request
# -x509 : produces self signed sertificate instead of certificate request
# -deys : number of days this certificate will be valid for
# -key : private key to use to sign this certificate
# -subj : subject name. Should contain at least distinguished (common) name (CN). Format: /type0=value0/type1=value1
# -addext : adds extension to certificate (inline version of -reqexts with -config)
# -outform : format of the certificate container
# -out : name of file to write generated certificate to
CA_CN='eclipse-che-local-CA'
openssl req -batch -new -x509 -days 730 -key $CA_KEY_FILE \
-subj "/CN=${CA_CN}" \
-addext keyUsage=keyCertSign,cRLSign,digitalSignature \
-outform PEM -out $CA_CERT_FILE
# Do not include CA:TRUE as it is already included into default config file
#-addext basicConstraints=critical,CA:TRUE

# Create CA root certificate secret

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
Copy link
Collaborator

@nickboldt nickboldt Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

48 changes: 23 additions & 25 deletions src/api/che.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@
* SPDX-License-Identifier: EPL-2.0
**********************************************************************/
import { CoreV1Api, KubeConfig, V1Pod, Watch } from '@kubernetes/client-node'
import axios from 'axios'
import axios, { AxiosInstance } from 'axios'
import * as cp from 'child_process'
import { cli } from 'cli-ux'
import * as commandExists from 'command-exists'
import * as fs from 'fs-extra'
import * as https from 'https'
import * as yaml from 'js-yaml'
import * as path from 'path'

Expand All @@ -27,9 +28,18 @@ export class CheHelper {
kube: KubeHelper
oc = new OpenShiftHelper()

private readonly axios: AxiosInstance

constructor(flags: any) {
this.kube = new KubeHelper(flags)
this.kc.loadFromDefault()

// Make axios ignore untrusted certificate error for self-signed certificate case.
const httpsAgent = new https.Agent({ rejectUnauthorized: false })

this.axios = axios.create({
httpsAgent
})
}

/**
Expand Down Expand Up @@ -122,26 +132,14 @@ export class CheHelper {
}

async cheNamespaceExist(namespace = '') {
const k8sApi = this.kc.makeApiClient(CoreV1Api)
try {
const res = await k8sApi.readNamespace(namespace)
if (res && res.body &&
res.body.metadata && res.body.metadata.name
&& res.body.metadata.name === namespace) {
return true
} else {
return false
}
} catch {
return false
}
return this.kube.namespaceExist(namespace)
}

async getCheServerStatus(cheURL: string, responseTimeoutMs = this.defaultCheResponseTimeoutMs): Promise<string> {
const endpoint = `${cheURL}/api/system/state`
let response = null
try {
response = await axios.get(endpoint, { timeout: responseTimeoutMs })
response = await this.axios.get(endpoint, { timeout: responseTimeoutMs })
} catch (error) {
throw this.getCheApiError(error, endpoint)
}
Expand All @@ -156,7 +154,7 @@ export class CheHelper {
const headers = accessToken ? { Authorization: `${accessToken}` } : null
let response = null
try {
response = await axios.post(endpoint, null, { headers, timeout: responseTimeoutMs })
response = await this.axios.post(endpoint, null, { headers, timeout: responseTimeoutMs })
} catch (error) {
if (error.response && error.response.status === 409) {
return
Expand All @@ -182,19 +180,19 @@ export class CheHelper {
}

async isCheServerReady(cheURL: string, responseTimeoutMs = this.defaultCheResponseTimeoutMs): Promise<boolean> {
const id = await axios.interceptors.response.use(response => response, async (error: any) => {
const id = await this.axios.interceptors.response.use(response => response, async (error: any) => {
if (error.config && error.response && (error.response.status === 404 || error.response.status === 503)) {
return axios.request(error.config)
return this.axios.request(error.config)
}
return Promise.reject(error)
})

try {
await axios.get(`${cheURL}/api/system/state`, { timeout: responseTimeoutMs })
await axios.interceptors.response.eject(id)
await this.axios.get(`${cheURL}/api/system/state`, { timeout: responseTimeoutMs })
await this.axios.interceptors.response.eject(id)
return true
} catch {
await axios.interceptors.response.eject(id)
await this.axios.interceptors.response.eject(id)
return false
}
}
Expand All @@ -219,7 +217,7 @@ export class CheHelper {
json.metadata.name = workspaceName
devfile = yaml.dump(json)
}
response = await axios.post(endpoint, devfile, { headers })
response = await this.axios.post(endpoint, devfile, { headers })
} catch (error) {
if (!devfile) { throw new Error(`E_NOT_FOUND_DEVFILE - ${devfilePath} - ${error.message}`) }
if (error.response && error.response.status === 400) {
Expand All @@ -237,7 +235,7 @@ export class CheHelper {

async parseDevfile(devfilePath = ''): Promise<string> {
if (devfilePath.startsWith('http')) {
const response = await axios.get(devfilePath)
const response = await this.axios.get(devfilePath)
return response.data
} else {
return fs.readFileSync(devfilePath, 'utf8')
Expand All @@ -259,7 +257,7 @@ export class CheHelper {

try {
let workspaceConfig = fs.readFileSync(workspaceConfigPath, 'utf8')
response = await axios.post(endpoint, workspaceConfig, { headers })
response = await this.axios.post(endpoint, workspaceConfig, { headers })
} catch (error) {
if (!workspaceConfig) { throw new Error(`E_NOT_FOUND_WORKSPACE_CONFIG_FILE - ${workspaceConfigPath} - ${error.message}`) }
if (error.response && error.response.status === 400) {
Expand All @@ -279,7 +277,7 @@ export class CheHelper {
const endpoint = `${cheURL}/api/keycloak/settings`
let response = null
try {
response = await axios.get(endpoint, { timeout: responseTimeoutMs })
response = await this.axios.get(endpoint, { timeout: responseTimeoutMs })
} catch (error) {
if (error.response && (error.response.status === 404 || error.response.status === 503)) {
return false
Expand Down
Loading