Adds Operator support for TLS enabled observatorium-api #273

nmagnezi · 2020-05-14T08:51:08Z

Issue: #272

Depends on:

brancz

If I see this correctly this is still missing the part of actually wiring up the files mounted from the secret, right (presumably that's why it's WIP :) )?

components/observatorium-api-tls-secret.libsonnet

squat

Nir, the mTLS failures look like a configuration issue in this PR rather than a problem with the API code. Please take a look at the comments. Otherwise, this generally looks very good :)

environments/base/default-config.libsonnet

squat · 2020-06-21T07:39:13Z

environments/dev/main.jsonnet

+      namespace: obs.config.namespace,
+    },
+    data: {
+      'ca.pem': importstr '../../tmp/certs/up_client/ca.pem',


The reason that mTLS is failing is that you have generated two different sets of client certs each with a different CA. The client certs for the up binary were generated using this certificate, so using this CA as the client-ca means up will work. But the client certs for the healthchecks were generated using the observatorium/ca.pem cert, so when the API checks the client certificates against the up_client/ca.pem cert, the validation will fail

I have tested this as well, and Done per your request. Yet I still get theTLS handshake error

environments/dev/main.jsonnet

example/main.jsonnet

brancz · 2020-06-24T12:21:23Z

Looks like this needs a rebase and CI is failing. Once green we can have another look.

nmagnezi · 2020-06-24T13:43:24Z

Looks like this needs a rebase and CI is failing. Once green we can have another look.

@brancz rebase done. it will fail CI for the same reason I mentioned here

squat · 2020-06-24T14:00:38Z

This should not fail for the same reason since the healthchecks no longer use mTLS. e.g. these flags no longer exist: https://github.com/observatorium/deployments/pull/273/files#diff-600fb07f5f3b3600a9a1111605f807ceR48
I think we need to fix the rebase actually

squat · 2020-06-24T14:00:48Z

let me know if you need help :)

…-api

squat · 2020-07-16T07:29:32Z

operator/api/v1alpha1/observatorium_types.go

+	TLSSecret TLSSecret `json:"secret"`
+}
+
+type MTLSConfigMap struct {


We don't have any Observatorium-wide mTLS settings (anymore), so we should remove this struct

squat · 2020-07-16T07:32:25Z

operator/api/v1alpha1/observatorium_types.go

+	CertFile       string `json:"certFile"`
+	Name           string `json:"name"`
+	PrivateKeyFile string `json:"privateKeyFile"`
+	ReloadInterval string `json:"reloadInterval"`


We will need an optional field to load the CA that signed the certificate. This is optional because the healthchecker needs to know that the certificate is valid and if the CA is not in the system certs then it needs to be specified. Note, this is different from an mTLS CA used to check clients of the API; this is for clients to validate the server's cert

squat · 2020-07-16T07:34:19Z

components/up-job.libsonnet

@@ -38,6 +38,9 @@ local k = import 'ksonnet/ksonnet.beta.4/k.libsonnet';
          '--latency=10s',
          '--initial-query-delay=5s',
          '--threshold=0.90',
+          '--tls-ca-file=' + up.config.withTLS.clientCAFile,


This should really be called serverCAFile because it's the CA that signed the server's certificate that matters here, not the client's

squat · 2020-07-16T07:35:35Z

environments/dev/main.jsonnet

@@ -34,14 +34,41 @@ local dex = (import '../../components/dex.libsonnet') + {
 };

 local obs = (import '../base/observatorium.jsonnet') + {
+  tls_secret: {


Let's keep everything here camelCase for consistency

squat · 2020-07-16T07:40:36Z

environments/base/default-config.libsonnet

+  },
+
+  withMTLS: {
+    local apiWithMTLS = self,


We should remove this because the API doesn't support any process-wide mTLS

squat · 2020-07-16T07:41:18Z

components/observatorium-configure.libsonnet

+    config+:: obs.config.withTLS,
+  },
+
+  withMTLS+:: {


Same here: there is no longer any API-wide mTLS settings

squat · 2020-07-16T07:43:44Z

environments/dev/manifests/api-deployment.yaml

+        - --web.healthchecks.url=https://127.0.0.1:8080
+        - --tls.server.cert-file=/mnt/certs/server.pem
+        - --tls.server.key-file=/mnt/certs/server.key
+        - --tls.healthchecks.cert-file=/mnt/certs/client.pem


Several of these flags no longer exist.

squat · 2020-07-16T07:43:54Z

environments/dev/manifests/api-deployment.yaml

+        - --tls.server.cert-file=/mnt/certs/server.pem
+        - --tls.server.key-file=/mnt/certs/server.key
+        - --tls.healthchecks.cert-file=/mnt/certs/client.pem
+        - --tls.healthchecks.key-file=/mnt/certs/client.key


This one too

squat · 2020-07-16T07:44:02Z

environments/dev/manifests/api-deployment.yaml

+        - --tls.healthchecks.key-file=/mnt/certs/client.key
+        - --tls.healthchecks.server-ca-file=/mnt/certs/ca.pem
+        - --tls.reload-interval=1m
+        - --tls.server.client-ca-file=/mnt/clientca/ca.pem


And this one

squat · 2020-08-05T10:32:19Z

ping @nmagnezi will you be able to take a look at the issues/comments I added a few weeks ago?

squat · 2020-08-28T10:16:05Z

Makefile


 CONTROLLER_GEN ?= $(BIN_DIR)/controller-gen
 JB ?= $(BIN_DIR)/jb
+ GENERATE_TLS_CERT ?= $(BIN_DIR)/generate-tls-cert


nit: extra space here

squat · 2020-08-28T10:17:03Z

Makefile

+	mkdir -p $(CERT_DIR)
+
+# Generate TLS certificates for local development.
+generate-cert: $(GENERATE_TLS_CERT) $(CERT_DIR)


We should make the dependency on the cert dir target order-only

Suggested change

generate-cert: $(GENERATE_TLS_CERT) $(CERT_DIR)

generate-cert: $(GENERATE_TLS_CERT) | $(CERT_DIR)

squat · 2020-08-28T10:18:02Z

Makefile

+	mkdir -p $(CERT_DIR)
+
+# Generate TLS certificates for local development.
+generate-cert: $(GENERATE_TLS_CERT) $(CERT_DIR)


This should be a phony target

squat · 2020-09-07T14:13:27Z

Closed in #329

nmagnezi force-pushed the api-tls branch 4 times, most recently from 4ccd142 to be679f2 Compare May 14, 2020 16:20

brancz reviewed May 15, 2020

View reviewed changes

components/observatorium-api-tls-secret.libsonnet Outdated Show resolved Hide resolved

nmagnezi force-pushed the api-tls branch 23 times, most recently from b005ba9 to 493af8a Compare May 21, 2020 14:25

nmagnezi force-pushed the api-tls branch 2 times, most recently from 1f294e9 to 72792ef Compare May 24, 2020 20:53

squat requested changes Jun 21, 2020

View reviewed changes

nmagnezi force-pushed the api-tls branch from f41a7e4 to ff6e29b Compare June 21, 2020 08:15

nmagnezi mentioned this pull request Jun 22, 2020

make generate broken in master #308

Closed

nmagnezi force-pushed the api-tls branch from ff6e29b to 7193b56 Compare June 24, 2020 13:38

nmagnezi force-pushed the api-tls branch from 7193b56 to 01e65c9 Compare June 25, 2020 06:13

Adds Operator and manifests support for No-TLS/TLS/MTLS observatorium…

4a8b17f

…-api

nmagnezi force-pushed the api-tls branch from 01e65c9 to 4a8b17f Compare June 28, 2020 07:06

nmagnezi requested review from squat and brancz July 13, 2020 12:52

squat reviewed Jul 16, 2020

View reviewed changes

squat reviewed Aug 28, 2020

View reviewed changes

squat mentioned this pull request Aug 28, 2020

jsonnet: make TLS settings work with K8s Secret observatorium/api#87

Merged

squat closed this Sep 7, 2020

squat deleted the api-tls branch September 7, 2020 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Operator support for TLS enabled observatorium-api #273

Adds Operator support for TLS enabled observatorium-api #273

nmagnezi commented May 14, 2020 •

edited

brancz left a comment

squat left a comment

squat Jun 21, 2020

nmagnezi Jun 21, 2020

brancz commented Jun 24, 2020

nmagnezi commented Jun 24, 2020 •

edited

squat commented Jun 24, 2020

squat commented Jun 24, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat Jul 16, 2020

squat commented Aug 5, 2020

squat Aug 28, 2020

squat Aug 28, 2020

squat Aug 28, 2020

squat commented Sep 7, 2020

	generate-cert: $(GENERATE_TLS_CERT) $(CERT_DIR)
	generate-cert: $(GENERATE_TLS_CERT) \| $(CERT_DIR)

Adds Operator support for TLS enabled observatorium-api #273

Adds Operator support for TLS enabled observatorium-api #273

Conversation

nmagnezi commented May 14, 2020 • edited

brancz left a comment

Choose a reason for hiding this comment

squat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Jun 24, 2020

nmagnezi commented Jun 24, 2020 • edited

squat commented Jun 24, 2020

squat commented Jun 24, 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

Choose a reason for hiding this comment

squat commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squat commented Sep 7, 2020

nmagnezi commented May 14, 2020 •

edited

nmagnezi commented Jun 24, 2020 •

edited