Protect internal components using TLS #159

rubenvp8510 · 2023-01-15T04:48:40Z

This PR adds the capability to protect the internal components using TLS.

This approach generated the CA, CABundle, and all the certs/private keys for each components using GoLang. It first created all the secrets, and the CABundle ConfigMap.

Then it configure each component , mounting each secret as a volumes in the different components, also mounting the CA bundle.

There is a cert rotation controller that is in charge of monitoring the certificates expiration, and annotate the secrets and the Microservice if a new certificate is required due an expiration.

This approach was taken from the Loki operator, almost all the certrotation package is copied from there, this package is in charge of create the CA, the bundle and all the certificates and private keys, create the ConfigMap and secret objects with he corresponding annotations

The buildIn cert manager could be enabled/disabled in ProjectConfig using some FeatureFlags. I added some comments on how that should operate.

In this PR I didn't set the minTLS version and the allowed ciphers, I would leave this for another PR as this is large enough.

Create a shared library to cert-manager grafana/loki#8181

bundle/manifests/tempo-operator-manager-config_v1_configmap.yaml

iblancasa · 2023-01-20T16:54:37Z

controllers/tempo/certrotation_controller.go

+	case err != nil:
+		return ctrl.Result{}, err
+	default:
+		log.Info("Skipping cert rotation, all Microservices certificates still valid", "name", req.String())


How about adding until when they will be valid or something?

iblancasa · 2023-01-20T17:01:56Z

tests/e2e/reconcile/01-install.yaml

@@ -15,6 +15,8 @@ kind: Microservices
 metadata:
  name: simplest
 spec:
+  images:
+    tempoQuery: quay.io/rvargasp/tempo-query


What does this image contain?

that image contains the jaegerQuery pluign that supports TLS communication with the tempo front-end. We can remove this when the new version of the oficial image is released. grafana/tempo#1999

Could you add a comment explaining that? Even, I would create an issue in this repository to do that change in the future.

+1 we should create an issue to revert it back to the default

internal/certrotation/var.go

internal/certrotation/rotation_test.go

andreasgerstmayr

Looks great overall, I've added a couple of minor comments/suggestions.

andreasgerstmayr · 2023-01-24T13:37:35Z

apis/tempo/v1alpha1/microservices_types.go

@@ -185,7 +185,7 @@ type ObjectStorageSpec struct {
 // ObjectStorageTLSSpec is the TLS configuration for reaching the object storage endpoint.
 type ObjectStorageTLSSpec struct {
 	// CA is the name of a ConfigMap containing a CA certificate.
-	// It needs to be in the same namespace as the LokiStack custom resource.
+	// It needs to be in the same namespace as the tempo Microservice custom resource.


andreasgerstmayr · 2023-01-24T14:35:02Z

internal/certrotation/var.go

+// ComponentCertSecretNames returns a list of all tempo component certificate secret names.
+func ComponentCertSecretNames(stackName string) []string {
+	return []string{
+		fmt.Sprintf("tempo-%s-distributor-http", stackName),


What's the reason for creating a different cert for each port of each service?
I was looking at the loki operator, and it creates a new service for each port (https://github.com/grafana/loki/blob/61c02d78f2f54c00c6aa47943c4944dc7b0619a7/operator/internal/manifests/distributor.go#L153-L208), but I don't know why.

I think the idea is to mantain the HTTP and gRPC TLS configurations separate, as int could be enabled or disables by independent flags. but on the other side, you're right, may be we can use the same certs. I need to think a little bit more on this.

andreasgerstmayr · 2023-01-24T14:38:04Z

internal/manifests/config/var.go

+import "fmt"
+
+func fqdn(serviceName, namespace string) string {
+	return fmt.Sprintf("%s.%s.svc.cluster.local", serviceName, namespace)


I'm not sure, does this need to be configurable? https://kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-init/ provides a --service-dns-domain option (defaults to cluster.local).

Can we address this in a separate PR? this is large enough :D

internal/manifests/manifests.go

pavolloffay

LGTM, just some minor comments.

How is handled the use case when a secret with the cert is removed by user? The jaeger-operator is caching the certs on its filesystem and reconstracts the secret.

apis/config/v1alpha1/projectconfig_types.go

pavolloffay · 2023-01-25T16:08:53Z

apis/config/v1alpha1/projectconfig_types.go

+	// CACertRefresh defines the duration of the CA certificate validity until a rotation
+	// should happen. It can be set up to 80% of CA certificate validity or equal to the
+	// CA certificate validity. Latter should be used only for rotating only when expired.
+	CACertRefresh string `json:"caRefresh,omitempty"`


The same comment as previous one:

could a duration be used as a type?

what is the default?

also the same comment applies for other fields in this file.

internal/manifests/manifestutils/service.go

pavolloffay · 2023-01-25T16:58:03Z

tests/e2e/reconcile/01-install.yaml

@@ -15,6 +15,8 @@ kind: Microservices
 metadata:
  name: simplest
 spec:
+  images:
+    tempoQuery: quay.io/rvargasp/tempo-query


+1 we should create an issue to revert it back to the default

pavolloffay · 2023-01-25T16:58:18Z

tests/e2e/smoketest-with-jaeger/01-install.yaml

@@ -15,6 +15,8 @@ kind: Microservices
 metadata:
  name: simplest
 spec:
+  images:
+    tempoQuery: quay.io/rvargasp/tempo-query


the same here add a comment and link to an issue to revert it back

frzifus

Since its blocked for a while, can we disable this feature and merge? :b

andreasgerstmayr

I've added one comment regarding the Params struct.
Looks good overall, some merge conflicts need to be resolved before being able to merge this PR.

internal/manifests/manifests.go

frzifus

Do we need here another follow-up ticket to include the tempo gateway?

pavolloffay

overall LGTM just some minor comments.

Could you please rebase it?

pavolloffay · 2023-02-13T13:23:41Z

controllers/tempo/microservices_controller.go

@@ -118,7 +128,7 @@ func (r *MicroservicesReconciler) reconcileManifests(ctx context.Context, log lo
 		}
 	}

-	objects, err := manifests.BuildAll(manifestutils.Params{Tempo: tempo, StorageParams: *storageConfig})
+	objects, err := manifests.BuildAll(manifestutils.Params{Tempo: tempo, StorageParams: *storageConfig, Gates: r.FeatureGates})


shall we have a unit test to check if certs are created if TLS is enabled?

Yes, but due the size of this PR I would prefer to write more tests on follow up PR, if there is no objections I'll disable TLS until we have the tests and the right images in place. Also I would like to integrate tempo gateway to the TLS work.

apis/config/v1alpha1/projectconfig_types.go

…rt generation for internal use Signed-off-by: Ruben Vargas <[email protected]>

Signed-off-by: Ruben Vargas <[email protected]>

Co-authored-by: Israel Blancas <[email protected]>

Signed-off-by: Ruben Vargas <[email protected]>

frzifus · 2023-02-14T12:33:02Z

ahh the e2e test failed. i triggered a rerun. ^^

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the protect_intern_service branch 4 times, most recently from ebae7bb to f143b30 Compare January 17, 2023 06:01

rubenvp8510 changed the title ~~Protect intern service~~ Protect internal components using TLS Jan 17, 2023

rubenvp8510 force-pushed the protect_intern_service branch 2 times, most recently from 9b5dcb1 to 15ab541 Compare January 17, 2023 06:10

rubenvp8510 marked this pull request as ready for review January 17, 2023 06:13

rubenvp8510 force-pushed the protect_intern_service branch 11 times, most recently from b89e12b to ce8be3a Compare January 18, 2023 06:02

rubenvp8510 requested review from pavolloffay, frzifus and andreasgerstmayr January 19, 2023 05:50

iblancasa requested changes Jan 20, 2023

View reviewed changes

rubenvp8510 force-pushed the protect_intern_service branch from 04ab12f to 0f5bc6b Compare January 20, 2023 22:33

andreasgerstmayr reviewed Jan 24, 2023

View reviewed changes

iblancasa approved these changes Jan 24, 2023

View reviewed changes

pavolloffay reviewed Jan 25, 2023

View reviewed changes

rubenvp8510 force-pushed the protect_intern_service branch 3 times, most recently from fc04f26 to b9488db Compare February 1, 2023 06:40

frzifus reviewed Feb 10, 2023

View reviewed changes

andreasgerstmayr approved these changes Feb 10, 2023

View reviewed changes

internal/manifests/manifests.go Outdated Show resolved Hide resolved

frzifus approved these changes Feb 12, 2023

View reviewed changes

pavolloffay reviewed Feb 13, 2023

View reviewed changes

rubenvp8510 and others added 14 commits February 13, 2023 23:40

Protect internal components using TLS, this also includes a custom ce…

013b6ae

…rt generation for internal use Signed-off-by: Ruben Vargas <[email protected]>

Protect internal components using TLS, this also includes a custom ce…

9527a1b

…rt generation for internal use Signed-off-by: Ruben Vargas <[email protected]>

no tls for frontend-query if jaegerQuery is not enabled

c8fe392

Signed-off-by: Ruben Vargas <[email protected]>

watch for microservices controller owner secrets

e7e93d1

Signed-off-by: Ruben Vargas <[email protected]>

Update internal/certrotation/rotation_test.go

f120e7c

Co-authored-by: Israel Blancas <[email protected]>

Update internal/certrotation/var.go

f5c31ce

Co-authored-by: Israel Blancas <[email protected]>

fix linting issues

0486809

Signed-off-by: Ruben Vargas <[email protected]>

change config duration types to metav1.Duration

42c82d7

Signed-off-by: Ruben Vargas <[email protected]>

remove custom image

145d552

Signed-off-by: Ruben Vargas <[email protected]>

fix readiness probe when mTLS is enabled

83ecbd2

Signed-off-by: Ruben Vargas <[email protected]>

fix unit tests for config

818e6ce

Signed-off-by: Ruben Vargas <[email protected]>

add comments to some functions

43a95f2

Signed-off-by: Ruben Vargas <[email protected]>

cleanup code

60a518c

Signed-off-by: Ruben Vargas <[email protected]>

fix bad merge on main.go

fb988d9

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the protect_intern_service branch from b918858 to fb988d9 Compare February 14, 2023 05:42

rubenvp8510 added 4 commits February 13, 2023 23:50

fix bad merge

362bfe6

Signed-off-by: Ruben Vargas <[email protected]>

fix linting issues

704b31f

Signed-off-by: Ruben Vargas <[email protected]>

address minor comments

f03aae4

Signed-off-by: Ruben Vargas <[email protected]>

disable build in cert and tls for internam components

8439f0e

Signed-off-by: Ruben Vargas <[email protected]>

pavolloffay approved these changes Feb 14, 2023

View reviewed changes

frzifus approved these changes Feb 14, 2023

View reviewed changes

rubenvp8510 added 2 commits February 14, 2023 19:11

fix config for tempo-query

9e8eb87

Signed-off-by: Ruben Vargas <[email protected]>

revert partiali bumped to tempo 2.0 config

75c83b3

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the protect_intern_service branch from c24ecb1 to 75c83b3 Compare February 15, 2023 05:55

rubenvp8510 merged commit 9104274 into grafana:main Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect internal components using TLS #159

Protect internal components using TLS #159

rubenvp8510 commented Jan 15, 2023 •

edited by pavolloffay

Loading

iblancasa Jan 20, 2023

iblancasa Jan 20, 2023

rubenvp8510 Jan 20, 2023 •

edited

Loading

iblancasa Jan 23, 2023

pavolloffay Jan 25, 2023

andreasgerstmayr left a comment

andreasgerstmayr Jan 24, 2023

andreasgerstmayr Jan 24, 2023

rubenvp8510 Jan 25, 2023

andreasgerstmayr Jan 24, 2023

rubenvp8510 Feb 2, 2023

andreasgerstmayr Feb 2, 2023

pavolloffay left a comment

pavolloffay Jan 25, 2023

pavolloffay Jan 25, 2023

pavolloffay Jan 25, 2023

frzifus left a comment

andreasgerstmayr left a comment

frzifus left a comment

pavolloffay left a comment

pavolloffay Feb 13, 2023

rubenvp8510 Feb 14, 2023 •

edited

Loading

frzifus commented Feb 14, 2023

Protect internal components using TLS #159

Protect internal components using TLS #159

Conversation

rubenvp8510 commented Jan 15, 2023 • edited by pavolloffay Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasgerstmayr left a 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

pavolloffay left a 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

frzifus left a comment

Choose a reason for hiding this comment

andreasgerstmayr left a comment

Choose a reason for hiding this comment

frzifus left a comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

frzifus commented Feb 14, 2023

rubenvp8510 commented Jan 15, 2023 •

edited by pavolloffay

Loading

rubenvp8510 Jan 20, 2023 •

edited

Loading

rubenvp8510 Feb 14, 2023 •

edited

Loading