From 3c98c18dd2b6cc558ffb7bbab5719b2579f11028 Mon Sep 17 00:00:00 2001 From: Adrien Boulay Date: Wed, 31 Jul 2024 11:08:33 +0200 Subject: [PATCH 1/3] feat(security): implement tls block for grafana external --- api/v1beta1/grafana_types.go | 11 +++ api/v1beta1/zz_generated.deepcopy.go | 25 +++++++ .../grafana.integreatly.org_grafanas.yaml | 22 ++++++ controllers/client/grafana_client.go | 66 +++++++++++++--- controllers/client/http_client.go | 38 ++++++++++ controllers/client/round_tripper.go | 11 +-- controllers/fetchers/grafana_com_fetcher.go | 4 +- controllers/fetchers/url_fetcher.go | 4 +- controllers/grafana_controller.go | 11 ++- .../grafana.integreatly.org_grafanas.yaml | 22 ++++++ deploy/kustomize/base/crds.yaml | 22 ++++++ docs/docs/api.md | 75 +++++++++++++++++++ 12 files changed, 286 insertions(+), 25 deletions(-) create mode 100644 controllers/client/http_client.go diff --git a/api/v1beta1/grafana_types.go b/api/v1beta1/grafana_types.go index 430192f2a..529c8f351 100644 --- a/api/v1beta1/grafana_types.go +++ b/api/v1beta1/grafana_types.go @@ -89,6 +89,17 @@ type External struct { AdminUser *v1.SecretKeySelector `json:"adminUser,omitempty"` // AdminPassword key to talk to the external grafana instance. AdminPassword *v1.SecretKeySelector `json:"adminPassword,omitempty"` + // TLS Configuration used to talk with the external grafana instance. + TLS *ExternalTLSConfig `json:"tls,omitempty"` +} + +type ExternalTLSConfig struct { + // Disable the CA check of the server + // +optional + InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty"` + // Use a secret as a reference to give TLS Certificate information + // +optional + CertSecretRef *v1.SecretReference `json:"certSecretRef,omitempty"` } type JsonnetConfig struct { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index e7648f6c0..df6db0651 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -371,6 +371,11 @@ func (in *External) DeepCopyInto(out *External) { *out = new(v1.SecretKeySelector) (*in).DeepCopyInto(*out) } + if in.TLS != nil { + in, out := &in.TLS, &out.TLS + *out = new(ExternalTLSConfig) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new External. @@ -383,6 +388,26 @@ func (in *External) DeepCopy() *External { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ExternalTLSConfig) DeepCopyInto(out *ExternalTLSConfig) { + *out = *in + if in.CertSecretRef != nil { + in, out := &in.CertSecretRef, &out.CertSecretRef + *out = new(v1.SecretReference) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ExternalTLSConfig. +func (in *ExternalTLSConfig) DeepCopy() *ExternalTLSConfig { + if in == nil { + return nil + } + out := new(ExternalTLSConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Grafana) DeepCopyInto(out *Grafana) { *out = *in diff --git a/config/crd/bases/grafana.integreatly.org_grafanas.yaml b/config/crd/bases/grafana.integreatly.org_grafanas.yaml index 77839faf5..83195f509 100644 --- a/config/crd/bases/grafana.integreatly.org_grafanas.yaml +++ b/config/crd/bases/grafana.integreatly.org_grafanas.yaml @@ -8255,6 +8255,28 @@ spec: - key type: object x-kubernetes-map-type: atomic + tls: + description: TLS Configuration used to talk with the external + grafana instance. + properties: + certSecretRef: + description: Use a secret as a reference to give TLS Certificate + information + properties: + name: + description: name is unique within a namespace to reference + a secret resource. + type: string + namespace: + description: namespace defines the space within which + the secret name must be unique. + type: string + type: object + x-kubernetes-map-type: atomic + insecureSkipVerify: + description: Disable the CA check of the server + type: boolean + type: object url: description: URL of the external grafana instance you want to manage. diff --git a/controllers/client/grafana_client.go b/controllers/client/grafana_client.go index 44464305a..de9ec5870 100644 --- a/controllers/client/grafana_client.go +++ b/controllers/client/grafana_client.go @@ -2,6 +2,8 @@ package client import ( "context" + "crypto/tls" + "crypto/x509" "fmt" "net/http" "net/url" @@ -124,21 +126,55 @@ func getAdminCredentials(ctx context.Context, c client.Client, grafana *v1beta1. return credentials, nil } -func NewHTTPClient(grafana *v1beta1.Grafana) *http.Client { - var timeout time.Duration - if grafana.Spec.Client != nil && grafana.Spec.Client.TimeoutSeconds != nil { - timeout = time.Duration(*grafana.Spec.Client.TimeoutSeconds) - if timeout < 0 { - timeout = 0 +// build the tls.Config object based on the content of the Grafana CR object +func buildTLSConfiguration(ctx context.Context, c client.Client, grafana *v1beta1.Grafana) (*tls.Config, error) { + if grafana.IsInternal() || grafana.Spec.External.TLS == nil { + return nil, nil + } + tlsConfigBlock := grafana.Spec.External.TLS + + if tlsConfigBlock.InsecureSkipVerify { + return BuildInsecureTLSConfiguration(), nil + } + + tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12} + + secret := &v1.Secret{} + selector := client.ObjectKey{ + Name: tlsConfigBlock.CertSecretRef.Name, + Namespace: grafana.Namespace, + } + err := c.Get(ctx, selector, secret) + if err != nil { + return nil, err + } + + if secret.Data == nil { + return nil, fmt.Errorf("empty credential secret: %v/%v", grafana.Namespace, tlsConfigBlock.CertSecretRef.Name) + } + + crt, crtPresent := secret.Data["tls.crt"] + key, keyPresent := secret.Data["tls.key"] + + if (crtPresent && !keyPresent) || (keyPresent && !crtPresent) { + return nil, fmt.Errorf("invalid secret %v/%v. tls.crt and tls.key needs to be present together when one of them is declared", tlsConfigBlock.CertSecretRef.Namespace, tlsConfigBlock.CertSecretRef.Name) + } else if crtPresent && keyPresent { + loadedCrt, err := tls.X509KeyPair(crt, key) + if err != nil { + return nil, fmt.Errorf("certificate from secret %v/%v cannot be parsed : %w", grafana.Namespace, tlsConfigBlock.CertSecretRef.Name, err) } - } else { - timeout = 10 + tlsConfig.Certificates = []tls.Certificate{loadedCrt} } - return &http.Client{ - Transport: NewInstrumentedRoundTripper(grafana.Name, metrics.GrafanaApiRequests, grafana.IsExternal()), - Timeout: time.Second * timeout, + if ca, ok := secret.Data["ca.crt"]; ok { + caCertPool := x509.NewCertPool() + if !caCertPool.AppendCertsFromPEM(ca) { + return nil, fmt.Errorf("failed to add ca.crt from the secret %s/%s", tlsConfigBlock.CertSecretRef.Namespace, tlsConfigBlock.CertSecretRef.Name) + } + tlsConfig.RootCAs = caCertPool } + + return tlsConfig, nil } func InjectAuthHeaders(ctx context.Context, c client.Client, grafana *v1beta1.Grafana, req *http.Request) error { @@ -170,12 +206,17 @@ func NewGeneratedGrafanaClient(ctx context.Context, c client.Client, grafana *v1 return nil, err } + tlsConfig, err := buildTLSConfiguration(ctx, c, grafana) + if err != nil { + return nil, err + } + gURL, err := url.Parse(grafana.Status.AdminUrl) if err != nil { return nil, fmt.Errorf("parsing url for client: %w", err) } - transport := NewInstrumentedRoundTripper(grafana.Name, metrics.GrafanaApiRequests, grafana.IsExternal()) + transport := NewInstrumentedRoundTripper(grafana.Name, metrics.GrafanaApiRequests, grafana.IsExternal(), tlsConfig) client := &http.Client{ Transport: transport, @@ -191,6 +232,7 @@ func NewGeneratedGrafanaClient(ctx context.Context, c client.Client, grafana *v1 // NumRetries contains the optional number of attempted retries NumRetries: 0, Client: client, + TLSConfig: tlsConfig, } if credentials.username != "" { cfg.BasicAuth = url.UserPassword(credentials.username, credentials.password) diff --git a/controllers/client/http_client.go b/controllers/client/http_client.go new file mode 100644 index 000000000..7bcc864c8 --- /dev/null +++ b/controllers/client/http_client.go @@ -0,0 +1,38 @@ +package client + +import ( + "context" + "crypto/tls" + "net/http" + "time" + + "github.com/grafana/grafana-operator/v5/api/v1beta1" + "github.com/grafana/grafana-operator/v5/controllers/metrics" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func NewHTTPClient(ctx context.Context, c client.Client, grafana *v1beta1.Grafana) (*http.Client, error) { + var timeout time.Duration + if grafana.Spec.Client != nil && grafana.Spec.Client.TimeoutSeconds != nil { + timeout = time.Duration(*grafana.Spec.Client.TimeoutSeconds) + if timeout < 0 { + timeout = 0 + } + } else { + timeout = 10 + } + + tlsConfig, err := buildTLSConfiguration(ctx, c, grafana) + if err != nil { + return nil, err + } + + return &http.Client{ + Transport: NewInstrumentedRoundTripper(grafana.Name, metrics.GrafanaApiRequests, grafana.IsExternal(), tlsConfig), + Timeout: time.Second * timeout, + }, nil +} + +func BuildInsecureTLSConfiguration() *tls.Config { + return &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} // #nosec G402 - Linter disabled because InsecureSkipVerify is the wanted behavior for this usecase +} diff --git a/controllers/client/round_tripper.go b/controllers/client/round_tripper.go index 4fda0b61b..1fd36d83e 100644 --- a/controllers/client/round_tripper.go +++ b/controllers/client/round_tripper.go @@ -14,17 +14,14 @@ type instrumentedRoundTripper struct { metric *prometheus.CounterVec } -func NewInstrumentedRoundTripper(relatedResource string, metric *prometheus.CounterVec, useProxy bool) http.RoundTripper { +func NewInstrumentedRoundTripper(relatedResource string, metric *prometheus.CounterVec, useProxy bool, tlsConfig *tls.Config) http.RoundTripper { transport := http.DefaultTransport.(*http.Transport).Clone() transport.DisableKeepAlives = true transport.MaxIdleConnsPerHost = -1 - if transport.TLSClientConfig != nil { - transport.TLSClientConfig.InsecureSkipVerify = true //nolint - } else { - transport.TLSClientConfig = &tls.Config{ - InsecureSkipVerify: true, //nolint - } + + if tlsConfig != nil { + transport.TLSClientConfig = tlsConfig } if !useProxy { diff --git a/controllers/fetchers/grafana_com_fetcher.go b/controllers/fetchers/grafana_com_fetcher.go index 806a76674..ad3544124 100644 --- a/controllers/fetchers/grafana_com_fetcher.go +++ b/controllers/fetchers/grafana_com_fetcher.go @@ -42,7 +42,9 @@ func getLatestGrafanaComRevision(dashboard *v1beta1.GrafanaDashboard) (int, erro return -1, err } - client := client2.NewInstrumentedRoundTripper(fmt.Sprintf("%v/%v", dashboard.Namespace, dashboard.Name), metrics.GrafanaComApiRevisionRequests, true) + // insecure to true because we don't know if the target URL is recognized by the default certificate + tlsConfig := client2.BuildInsecureTLSConfiguration() + client := client2.NewInstrumentedRoundTripper(fmt.Sprintf("%v/%v", dashboard.Namespace, dashboard.Name), metrics.GrafanaComApiRevisionRequests, true, tlsConfig) response, err := client.RoundTrip(request) if err != nil { return -1, err diff --git a/controllers/fetchers/url_fetcher.go b/controllers/fetchers/url_fetcher.go index 28ea42393..4bdf405a7 100644 --- a/controllers/fetchers/url_fetcher.go +++ b/controllers/fetchers/url_fetcher.go @@ -8,6 +8,7 @@ import ( "time" "github.com/grafana/grafana-operator/v5/api/v1beta1" + "github.com/grafana/grafana-operator/v5/controllers/client" client2 "github.com/grafana/grafana-operator/v5/controllers/client" "github.com/grafana/grafana-operator/v5/controllers/metrics" @@ -30,7 +31,8 @@ func FetchDashboardFromUrl(dashboard *v1beta1.GrafanaDashboard) ([]byte, error) return nil, err } - client := client2.NewInstrumentedRoundTripper(fmt.Sprintf("%v/%v", dashboard.Namespace, dashboard.Name), metrics.DashboardUrlRequests, true) + tlsConfig := client.BuildInsecureTLSConfiguration() + client := client2.NewInstrumentedRoundTripper(fmt.Sprintf("%v/%v", dashboard.Namespace, dashboard.Name), metrics.DashboardUrlRequests, true, tlsConfig) response, err := client.RoundTrip(request) if err != nil { return nil, err diff --git a/controllers/grafana_controller.go b/controllers/grafana_controller.go index 05c2c2330..e54c6b360 100644 --- a/controllers/grafana_controller.go +++ b/controllers/grafana_controller.go @@ -91,7 +91,7 @@ func (r *GrafanaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct nextStatus.Stage = grafanav1beta1.OperatorStageComplete nextStatus.StageStatus = grafanav1beta1.OperatorStageResultSuccess nextStatus.AdminUrl = grafana.Spec.External.URL - v, err := r.getVersion(grafana) + v, err := r.getVersion(ctx, grafana) if err != nil { controllerLog.Error(err, "failed to get version from external instance") } @@ -137,7 +137,7 @@ func (r *GrafanaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } if finished { - v, err := r.getVersion(grafana) + v, err := r.getVersion(ctx, grafana) if err != nil { controllerLog.Error(err, "failed to get version from instance") } @@ -148,8 +148,11 @@ func (r *GrafanaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return r.updateStatus(grafana, nextStatus) } -func (r *GrafanaReconciler) getVersion(cr *grafanav1beta1.Grafana) (string, error) { - cl := client2.NewHTTPClient(cr) +func (r *GrafanaReconciler) getVersion(ctx context.Context, cr *grafanav1beta1.Grafana) (string, error) { + cl, err := client2.NewHTTPClient(ctx, r.Client, cr) + if err != nil { + return "", fmt.Errorf("setup of the http client: %w", err) + } instanceUrl := cr.Status.AdminUrl if instanceUrl == "" && cr.Spec.External != nil { instanceUrl = cr.Spec.External.URL diff --git a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml index 77839faf5..83195f509 100644 --- a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml +++ b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml @@ -8255,6 +8255,28 @@ spec: - key type: object x-kubernetes-map-type: atomic + tls: + description: TLS Configuration used to talk with the external + grafana instance. + properties: + certSecretRef: + description: Use a secret as a reference to give TLS Certificate + information + properties: + name: + description: name is unique within a namespace to reference + a secret resource. + type: string + namespace: + description: namespace defines the space within which + the secret name must be unique. + type: string + type: object + x-kubernetes-map-type: atomic + insecureSkipVerify: + description: Disable the CA check of the server + type: boolean + type: object url: description: URL of the external grafana instance you want to manage. diff --git a/deploy/kustomize/base/crds.yaml b/deploy/kustomize/base/crds.yaml index 84a6f9adc..9413615ee 100644 --- a/deploy/kustomize/base/crds.yaml +++ b/deploy/kustomize/base/crds.yaml @@ -9959,6 +9959,28 @@ spec: - key type: object x-kubernetes-map-type: atomic + tls: + description: TLS Configuration used to talk with the external + grafana instance. + properties: + certSecretRef: + description: Use a secret as a reference to give TLS Certificate + information + properties: + name: + description: name is unique within a namespace to reference + a secret resource. + type: string + namespace: + description: namespace defines the space within which + the secret name must be unique. + type: string + type: object + x-kubernetes-map-type: atomic + insecureSkipVerify: + description: Disable the CA check of the server + type: boolean + type: object url: description: URL of the external grafana instance you want to manage. diff --git a/docs/docs/api.md b/docs/docs/api.md index da10823be..3722ebe87 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -19191,6 +19191,13 @@ External enables you to configure external grafana instances that is not managed The API key to talk to the external grafana instance, you need to define ether apiKey or adminUser/adminPassword.
false + + tls + object + + TLS Configuration used to talk with the external grafana instance.
+ + false @@ -19342,6 +19349,74 @@ TODO: Drop `kubebuilder:default` when controller-gen doesn't need it https://git +### Grafana.spec.external.tls +[↩ Parent](#grafanaspecexternal) + + + +TLS Configuration used to talk with the external grafana instance. + + + + + + + + + + + + + + + + + + + + + +
NameTypeDescriptionRequired
certSecretRefobject + Use a secret as a reference to give TLS Certificate information
+ 		false
insecureSkipVerifyboolean + Disable the CA check of the server
+ 		false
+ + +### Grafana.spec.external.tls.certSecretRef +[↩ Parent](#grafanaspecexternaltls) + + + +Use a secret as a reference to give TLS Certificate information + + + + + + + + + + + + + + + + + + + + + +
NameTypeDescriptionRequired
namestring + name is unique within a namespace to reference a secret resource.
+ 		false
namespacestring + namespace defines the space within which the secret name must be unique.
+ 		false
+ + ### Grafana.spec.ingress [↩ Parent](#grafanaspec) From c0596b3b81e089543203fdac5feec45e3c297be3 Mon Sep 17 00:00:00 2001 From: Adrien Boulay Date: Wed, 31 Jul 2024 14:18:22 +0200 Subject: [PATCH 2/3] feat: enforce rule into the .spec.external.tls block to avoid insecureSkipVerify and certSecretRef be set at the same time --- api/v1beta1/grafana_types.go | 3 +++ config/crd/bases/grafana.integreatly.org_grafanas.yaml | 5 +++++ .../crds/grafana.integreatly.org_grafanas.yaml | 5 +++++ deploy/kustomize/base/crds.yaml | 5 +++++ docs/docs/api.md | 2 ++ 5 files changed, 20 insertions(+) diff --git a/api/v1beta1/grafana_types.go b/api/v1beta1/grafana_types.go index 529c8f351..51841b407 100644 --- a/api/v1beta1/grafana_types.go +++ b/api/v1beta1/grafana_types.go @@ -90,9 +90,12 @@ type External struct { // AdminPassword key to talk to the external grafana instance. AdminPassword *v1.SecretKeySelector `json:"adminPassword,omitempty"` // TLS Configuration used to talk with the external grafana instance. + // +optional TLS *ExternalTLSConfig `json:"tls,omitempty"` } +// TLS Configuration to an external Grafana endpoint +// +kubebuilder:validation:XValidation:rule="(has(self.insecureSkipVerify) && !(has(self.certSecretRef))) || (has(self.certSecretRef) && !(has(self.insecureSkipVerify)))", message="insecureSkipVerify and certSecretRef cannot be set at the same time" type ExternalTLSConfig struct { // Disable the CA check of the server // +optional diff --git a/config/crd/bases/grafana.integreatly.org_grafanas.yaml b/config/crd/bases/grafana.integreatly.org_grafanas.yaml index 83195f509..7b481c425 100644 --- a/config/crd/bases/grafana.integreatly.org_grafanas.yaml +++ b/config/crd/bases/grafana.integreatly.org_grafanas.yaml @@ -8277,6 +8277,11 @@ spec: description: Disable the CA check of the server type: boolean type: object + x-kubernetes-validations: + - message: insecureSkipVerify and certSecretRef cannot be set + at the same time + rule: (has(self.insecureSkipVerify) && !(has(self.certSecretRef))) + || (has(self.certSecretRef) && !(has(self.insecureSkipVerify))) url: description: URL of the external grafana instance you want to manage. diff --git a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml index 83195f509..7b481c425 100644 --- a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml +++ b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml @@ -8277,6 +8277,11 @@ spec: description: Disable the CA check of the server type: boolean type: object + x-kubernetes-validations: + - message: insecureSkipVerify and certSecretRef cannot be set + at the same time + rule: (has(self.insecureSkipVerify) && !(has(self.certSecretRef))) + || (has(self.certSecretRef) && !(has(self.insecureSkipVerify))) url: description: URL of the external grafana instance you want to manage. diff --git a/deploy/kustomize/base/crds.yaml b/deploy/kustomize/base/crds.yaml index 9413615ee..88e56efa5 100644 --- a/deploy/kustomize/base/crds.yaml +++ b/deploy/kustomize/base/crds.yaml @@ -9981,6 +9981,11 @@ spec: description: Disable the CA check of the server type: boolean type: object + x-kubernetes-validations: + - message: insecureSkipVerify and certSecretRef cannot be set + at the same time + rule: (has(self.insecureSkipVerify) && !(has(self.certSecretRef))) + || (has(self.certSecretRef) && !(has(self.insecureSkipVerify))) url: description: URL of the external grafana instance you want to manage. diff --git a/docs/docs/api.md b/docs/docs/api.md index 3722ebe87..54847f08f 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -19196,6 +19196,8 @@ External enables you to configure external grafana instances that is not managed object TLS Configuration used to talk with the external grafana instance.
+
+ Validations:
  • (has(self.insecureSkipVerify) && !(has(self.certSecretRef))) || (has(self.certSecretRef) && !(has(self.insecureSkipVerify))): insecureSkipVerify and certSecretRef cannot be set at the same time
    • false From 27b217941bfe6133d659838b75dc583d71c3d9d8 Mon Sep 17 00:00:00 2001 From: Adrien Boulay Date: Mon, 19 Aug 2024 17:58:25 +0200 Subject: [PATCH 3/3] refactor: extract tls config from urlfetchers to ensure the same config is use for all the workflow --- controllers/client/grafana_client.go | 53 ---------------- controllers/client/http_client.go | 5 -- controllers/client/tls.go | 68 +++++++++++++++++++++ controllers/dashboard_controller.go | 2 +- controllers/fetchers/grafana_com_fetcher.go | 11 ++-- controllers/fetchers/url_fetcher.go | 5 +- controllers/fetchers/url_fetcher_test.go | 2 +- 7 files changed, 78 insertions(+), 68 deletions(-) create mode 100644 controllers/client/tls.go diff --git a/controllers/client/grafana_client.go b/controllers/client/grafana_client.go index de9ec5870..7113b3b1d 100644 --- a/controllers/client/grafana_client.go +++ b/controllers/client/grafana_client.go @@ -2,8 +2,6 @@ package client import ( "context" - "crypto/tls" - "crypto/x509" "fmt" "net/http" "net/url" @@ -126,57 +124,6 @@ func getAdminCredentials(ctx context.Context, c client.Client, grafana *v1beta1. return credentials, nil } -// build the tls.Config object based on the content of the Grafana CR object -func buildTLSConfiguration(ctx context.Context, c client.Client, grafana *v1beta1.Grafana) (*tls.Config, error) { - if grafana.IsInternal() || grafana.Spec.External.TLS == nil { - return nil, nil - } - tlsConfigBlock := grafana.Spec.External.TLS - - if tlsConfigBlock.InsecureSkipVerify { - return BuildInsecureTLSConfiguration(), nil - } - - tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12} - - secret := &v1.Secret{} - selector := client.ObjectKey{ - Name: tlsConfigBlock.CertSecretRef.Name, - Namespace: grafana.Namespace, - } - err := c.Get(ctx, selector, secret) - if err != nil { - return nil, err - } - - if secret.Data == nil { - return nil, fmt.Errorf("empty credential secret: %v/%v", grafana.Namespace, tlsConfigBlock.CertSecretRef.Name) - } - - crt, crtPresent := secret.Data["tls.crt"] - key, keyPresent := secret.Data["tls.key"] - - if (crtPresent && !keyPresent) || (keyPresent && !crtPresent) { - return nil, fmt.Errorf("invalid secret %v/%v. tls.crt and tls.key needs to be present together when one of them is declared", tlsConfigBlock.CertSecretRef.Namespace, tlsConfigBlock.CertSecretRef.Name) - } else if crtPresent && keyPresent { - loadedCrt, err := tls.X509KeyPair(crt, key) - if err != nil { - return nil, fmt.Errorf("certificate from secret %v/%v cannot be parsed : %w", grafana.Namespace, tlsConfigBlock.CertSecretRef.Name, err) - } - tlsConfig.Certificates = []tls.Certificate{loadedCrt} - } - - if ca, ok := secret.Data["ca.crt"]; ok { - caCertPool := x509.NewCertPool() - if !caCertPool.AppendCertsFromPEM(ca) { - return nil, fmt.Errorf("failed to add ca.crt from the secret %s/%s", tlsConfigBlock.CertSecretRef.Namespace, tlsConfigBlock.CertSecretRef.Name) - } - tlsConfig.RootCAs = caCertPool - } - - return tlsConfig, nil -} - func InjectAuthHeaders(ctx context.Context, c client.Client, grafana *v1beta1.Grafana, req *http.Request) error { creds, err := getAdminCredentials(ctx, c, grafana) if err != nil { diff --git a/controllers/client/http_client.go b/controllers/client/http_client.go index 7bcc864c8..eb3c11712 100644 --- a/controllers/client/http_client.go +++ b/controllers/client/http_client.go @@ -2,7 +2,6 @@ package client import ( "context" - "crypto/tls" "net/http" "time" @@ -32,7 +31,3 @@ func NewHTTPClient(ctx context.Context, c client.Client, grafana *v1beta1.Grafan Timeout: time.Second * timeout, }, nil } - -func BuildInsecureTLSConfiguration() *tls.Config { - return &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} // #nosec G402 - Linter disabled because InsecureSkipVerify is the wanted behavior for this usecase -} diff --git a/controllers/client/tls.go b/controllers/client/tls.go new file mode 100644 index 000000000..15f882cdc --- /dev/null +++ b/controllers/client/tls.go @@ -0,0 +1,68 @@ +package client + +import ( + "context" + "crypto/tls" + "crypto/x509" + "fmt" + + "github.com/grafana/grafana-operator/v5/api/v1beta1" + v1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + DefaultTLSConfiguration = &tls.Config{MinVersion: tls.VersionTLS12} + InsecureTLSConfiguration = &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} // #nosec G402 - Linter disabled because InsecureSkipVerify is the wanted behavior for this variable +) + +// build the tls.Config object based on the content of the Grafana CR object +func buildTLSConfiguration(ctx context.Context, c client.Client, grafana *v1beta1.Grafana) (*tls.Config, error) { + if grafana.IsInternal() || grafana.Spec.External.TLS == nil { + return nil, nil + } + tlsConfigBlock := grafana.Spec.External.TLS + + if tlsConfigBlock.InsecureSkipVerify { + return InsecureTLSConfiguration, nil + } + + tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12} + + secret := &v1.Secret{} + selector := client.ObjectKey{ + Name: tlsConfigBlock.CertSecretRef.Name, + Namespace: grafana.Namespace, + } + err := c.Get(ctx, selector, secret) + if err != nil { + return nil, err + } + + if secret.Data == nil { + return nil, fmt.Errorf("empty credential secret: %v/%v", grafana.Namespace, tlsConfigBlock.CertSecretRef.Name) + } + + crt, crtPresent := secret.Data["tls.crt"] + key, keyPresent := secret.Data["tls.key"] + + if (crtPresent && !keyPresent) || (keyPresent && !crtPresent) { + return nil, fmt.Errorf("invalid secret %v/%v. tls.crt and tls.key needs to be present together when one of them is declared", tlsConfigBlock.CertSecretRef.Namespace, tlsConfigBlock.CertSecretRef.Name) + } else if crtPresent && keyPresent { + loadedCrt, err := tls.X509KeyPair(crt, key) + if err != nil { + return nil, fmt.Errorf("certificate from secret %v/%v cannot be parsed : %w", grafana.Namespace, tlsConfigBlock.CertSecretRef.Name, err) + } + tlsConfig.Certificates = []tls.Certificate{loadedCrt} + } + + if ca, ok := secret.Data["ca.crt"]; ok { + caCertPool := x509.NewCertPool() + if !caCertPool.AppendCertsFromPEM(ca) { + return nil, fmt.Errorf("failed to add ca.crt from the secret %s/%s", tlsConfigBlock.CertSecretRef.Namespace, tlsConfigBlock.CertSecretRef.Name) + } + tlsConfig.RootCAs = caCertPool + } + + return tlsConfig, nil +} diff --git a/controllers/dashboard_controller.go b/controllers/dashboard_controller.go index fa2599d8d..c126a3c53 100644 --- a/controllers/dashboard_controller.go +++ b/controllers/dashboard_controller.go @@ -481,7 +481,7 @@ func (r *GrafanaDashboardReconciler) fetchDashboardJson(ctx context.Context, das case v1beta1.DashboardSourceTypeGzipJson: return v1beta1.Gunzip([]byte(dashboard.Spec.GzipJson)) case v1beta1.DashboardSourceTypeUrl: - return fetchers.FetchDashboardFromUrl(dashboard) + return fetchers.FetchDashboardFromUrl(dashboard, client2.InsecureTLSConfiguration) case v1beta1.DashboardSourceTypeJsonnet: envs, err := r.getDashboardEnvs(ctx, dashboard) if err != nil { diff --git a/controllers/fetchers/grafana_com_fetcher.go b/controllers/fetchers/grafana_com_fetcher.go index ad3544124..2c330439a 100644 --- a/controllers/fetchers/grafana_com_fetcher.go +++ b/controllers/fetchers/grafana_com_fetcher.go @@ -1,6 +1,7 @@ package fetchers import ( + "crypto/tls" "encoding/json" "fmt" "net/http" @@ -20,8 +21,10 @@ func FetchDashboardFromGrafanaCom(dashboard *v1beta1.GrafanaDashboard) ([]byte, source := dashboard.Spec.GrafanaCom + tlsConfig := client2.DefaultTLSConfiguration + if source.Revision == nil { - rev, err := getLatestGrafanaComRevision(dashboard) + rev, err := getLatestGrafanaComRevision(dashboard, tlsConfig) if err != nil { return nil, fmt.Errorf("failed to get latest revision for dashboard id %d: %w", source.Id, err) } @@ -30,10 +33,10 @@ func FetchDashboardFromGrafanaCom(dashboard *v1beta1.GrafanaDashboard) ([]byte, dashboard.Spec.Url = fmt.Sprintf("%s/%d/revisions/%d/download", grafanaComDashboardApiUrlRoot, source.Id, *source.Revision) - return FetchDashboardFromUrl(dashboard) + return FetchDashboardFromUrl(dashboard, tlsConfig) } -func getLatestGrafanaComRevision(dashboard *v1beta1.GrafanaDashboard) (int, error) { +func getLatestGrafanaComRevision(dashboard *v1beta1.GrafanaDashboard, tlsConfig *tls.Config) (int, error) { source := dashboard.Spec.GrafanaCom url := fmt.Sprintf("%s/%d/revisions", grafanaComDashboardApiUrlRoot, source.Id) @@ -42,8 +45,6 @@ func getLatestGrafanaComRevision(dashboard *v1beta1.GrafanaDashboard) (int, erro return -1, err } - // insecure to true because we don't know if the target URL is recognized by the default certificate - tlsConfig := client2.BuildInsecureTLSConfiguration() client := client2.NewInstrumentedRoundTripper(fmt.Sprintf("%v/%v", dashboard.Namespace, dashboard.Name), metrics.GrafanaComApiRevisionRequests, true, tlsConfig) response, err := client.RoundTrip(request) if err != nil { diff --git a/controllers/fetchers/url_fetcher.go b/controllers/fetchers/url_fetcher.go index 4bdf405a7..5196d3428 100644 --- a/controllers/fetchers/url_fetcher.go +++ b/controllers/fetchers/url_fetcher.go @@ -1,6 +1,7 @@ package fetchers import ( + "crypto/tls" "fmt" "io" "net/http" @@ -8,14 +9,13 @@ import ( "time" "github.com/grafana/grafana-operator/v5/api/v1beta1" - "github.com/grafana/grafana-operator/v5/controllers/client" client2 "github.com/grafana/grafana-operator/v5/controllers/client" "github.com/grafana/grafana-operator/v5/controllers/metrics" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func FetchDashboardFromUrl(dashboard *v1beta1.GrafanaDashboard) ([]byte, error) { +func FetchDashboardFromUrl(dashboard *v1beta1.GrafanaDashboard, tlsConfig *tls.Config) ([]byte, error) { url, err := url.Parse(dashboard.Spec.Url) if err != nil { return nil, err @@ -31,7 +31,6 @@ func FetchDashboardFromUrl(dashboard *v1beta1.GrafanaDashboard) ([]byte, error) return nil, err } - tlsConfig := client.BuildInsecureTLSConfiguration() client := client2.NewInstrumentedRoundTripper(fmt.Sprintf("%v/%v", dashboard.Namespace, dashboard.Name), metrics.DashboardUrlRequests, true, tlsConfig) response, err := client.RoundTrip(request) if err != nil { diff --git a/controllers/fetchers/url_fetcher_test.go b/controllers/fetchers/url_fetcher_test.go index 7ad9b381d..9cbf5a514 100644 --- a/controllers/fetchers/url_fetcher_test.go +++ b/controllers/fetchers/url_fetcher_test.go @@ -28,7 +28,7 @@ func TestFetchDashboardFromUrl(t *testing.T) { Status: v1beta1.GrafanaDashboardStatus{}, } - fetchedDashboard, err := FetchDashboardFromUrl(dashboard) + fetchedDashboard, err := FetchDashboardFromUrl(dashboard, nil) assert.Nil(t, err) assert.Equal(t, dashboardJSON, fetchedDashboard, "Fetched dashboard doesn't match the original")