NETOBSERV-1346: Added checks to prevent dereferencing Provided cert n…

…il pointer (#444) * Added checks to prevent dereferencing Provided cert nil pointer * Added error when tls set to provided but provided section is nil
netobserv · Oct 9, 2023 · 51025b4 · 51025b4
1 parent d8655d0
commit 51025b4
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 32 deletions.
diff --git a/controllers/flowlogspipeline/flp_common_objects.go b/controllers/flowlogspipeline/flp_common_objects.go
@@ -71,15 +71,18 @@ type builder struct {
 	volumes  volumes.Builder
 }
 
-func newBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec, ck ConfKind) builder {
+func newBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec, ck ConfKind) (builder, error) {
 	version := helper.ExtractVersion(info.Image)
 	name := name(ck)
-	var promTLS flowslatest.CertificateReference
+	var promTLS *flowslatest.CertificateReference
 	switch desired.Processor.Metrics.Server.TLS.Type {
 	case flowslatest.ServerTLSProvided:
-		promTLS = *desired.Processor.Metrics.Server.TLS.Provided
+		promTLS = desired.Processor.Metrics.Server.TLS.Provided
+		if promTLS == nil {
+			return builder{}, fmt.Errorf("processor tls configuration set to provided but none is provided")
+		}
 	case flowslatest.ServerTLSAuto:
-		promTLS = flowslatest.CertificateReference{
+		promTLS = &flowslatest.CertificateReference{
 			Type:     "secret",
 			Name:     promServiceName(ck),
 			CertFile: "tls.crt",
@@ -97,8 +100,8 @@ func newBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSp
 		},
 		desired:  desired,
 		confKind: ck,
-		promTLS:  &promTLS,
-	}
+		promTLS:  promTLS,
+	}, nil
 }
 
 func name(ck ConfKind) string                 { return constants.FLPName + FlpConfSuffix[ck] }
@@ -658,9 +661,11 @@ func (b *builder) configMap(stages []config.Stage, parameters []config.StagePara
 	}
 	if b.desired.Processor.Metrics.Server.TLS.Type != flowslatest.ServerTLSDisabled {
 		cert, key := b.volumes.AddCertificate(b.promTLS, "prom-certs")
-		metricsSettings.TLS = &api.PromTLSConf{
-			CertPath: cert,
-			KeyPath:  key,
+		if cert != "" && key != "" {
+			metricsSettings.TLS = &api.PromTLSConf{
+				CertPath: cert,
+				KeyPath:  key,
+			}
 		}
 	}
 	config := map[string]interface{}{
@@ -811,7 +816,9 @@ func (b *builder) serviceMonitor() *monitoringv1.ServiceMonitor {
 				InsecureSkipVerify: b.desired.Processor.Metrics.Server.TLS.InsecureSkipVerify,
 			},
 		}
-		if !b.desired.Processor.Metrics.Server.TLS.InsecureSkipVerify && b.desired.Processor.Metrics.Server.TLS.ProvidedCaFile.File != "" {
+		if !b.desired.Processor.Metrics.Server.TLS.InsecureSkipVerify &&
+			b.desired.Processor.Metrics.Server.TLS.ProvidedCaFile != nil &&
+			b.desired.Processor.Metrics.Server.TLS.ProvidedCaFile.File != "" {
 			flpServiceMonitorObject.Spec.Endpoints[0].TLSConfig.SafeTLSConfig.CA = helper.GetSecretOrConfigMap(b.desired.Processor.Metrics.Server.TLS.ProvidedCaFile)
 		}
 	}

diff --git a/controllers/flowlogspipeline/flp_ingest_objects.go b/controllers/flowlogspipeline/flp_ingest_objects.go
@@ -17,11 +17,11 @@ type ingestBuilder struct {
 	generic builder
 }
 
-func newIngestBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec) ingestBuilder {
-	gen := newBuilder(info, desired, ConfKafkaIngester)
+func newIngestBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec) (ingestBuilder, error) {
+	gen, err := newBuilder(info, desired, ConfKafkaIngester)
 	return ingestBuilder{
 		generic: gen,
-	}
+	}, err
 }
 
 func (b *ingestBuilder) daemonSet(annotations map[string]string) *appsv1.DaemonSet {

diff --git a/controllers/flowlogspipeline/flp_ingest_reconciler.go b/controllers/flowlogspipeline/flp_ingest_reconciler.go
@@ -84,7 +84,10 @@ func (r *flpIngesterReconciler) reconcile(ctx context.Context, desired *flowslat
 		return nil
 	}
 
-	builder := newIngestBuilder(r.Instance, &desired.Spec)
+	builder, err := newIngestBuilder(r.Instance, &desired.Spec)
+	if err != nil {
+		return err
+	}
 	newCM, configDigest, err := builder.configMap()
 	if err != nil {
 		return err

diff --git a/controllers/flowlogspipeline/flp_monolith_objects.go b/controllers/flowlogspipeline/flp_monolith_objects.go
@@ -17,11 +17,11 @@ type monolithBuilder struct {
 	generic builder
 }
 
-func newMonolithBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec) monolithBuilder {
-	gen := newBuilder(info, desired, ConfMonolith)
+func newMonolithBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec) (monolithBuilder, error) {
+	gen, err := newBuilder(info, desired, ConfMonolith)
 	return monolithBuilder{
 		generic: gen,
-	}
+	}, err
 }
 
 func (b *monolithBuilder) daemonSet(annotations map[string]string) *appsv1.DaemonSet {

diff --git a/controllers/flowlogspipeline/flp_monolith_reconciler.go b/controllers/flowlogspipeline/flp_monolith_reconciler.go
@@ -87,7 +87,10 @@ func (r *flpMonolithReconciler) reconcile(ctx context.Context, desired *flowslat
 		return nil
 	}
 
-	builder := newMonolithBuilder(r.Instance, &desired.Spec)
+	builder, err := newMonolithBuilder(r.Instance, &desired.Spec)
+	if err != nil {
+		return err
+	}
 	newCM, configDigest, err := builder.configMap()
 	if err != nil {
 		return err

diff --git a/controllers/flowlogspipeline/flp_test.go b/controllers/flowlogspipeline/flp_test.go
@@ -157,12 +157,14 @@ func getAutoScalerSpecs() (ascv2.HorizontalPodAutoscaler, flowslatest.FlowCollec
 
 func monoBuilder(ns string, cfg *flowslatest.FlowCollectorSpec) monolithBuilder {
 	info := reconcilers.Common{Namespace: ns}
-	return newMonolithBuilder(info.NewInstance(image), cfg)
+	b, _ := newMonolithBuilder(info.NewInstance(image), cfg)
+	return b
 }
 
 func transfBuilder(ns string, cfg *flowslatest.FlowCollectorSpec) transfoBuilder {
 	info := reconcilers.Common{Namespace: ns}
-	return newTransfoBuilder(info.NewInstance(image), cfg)
+	b, _ := newTransfoBuilder(info.NewInstance(image), cfg)
+	return b
 }
 
 func annotate(digest string) map[string]string {
@@ -536,15 +538,15 @@ func TestServiceMonitorChanged(t *testing.T) {
 
 	// Check labels change
 	info := reconcilers.Common{Namespace: "namespace2"}
-	b = newMonolithBuilder(info.NewInstance(image2), &cfg)
+	b, _ = newMonolithBuilder(info.NewInstance(image2), &cfg)
 	third := b.generic.serviceMonitor()
 
 	report = helper.NewChangeReport("")
 	assert.True(helper.ServiceMonitorChanged(second, third, &report))
 	assert.Contains(report.String(), "ServiceMonitor labels changed")
 
 	// Check scheme changed
-	b = newMonolithBuilder(info.NewInstance(image2), &cfg)
+	b, _ = newMonolithBuilder(info.NewInstance(image2), &cfg)
 	fourth := b.generic.serviceMonitor()
 	fourth.Spec.Endpoints[0].Scheme = "https"
 
@@ -589,7 +591,7 @@ func TestPrometheusRuleChanged(t *testing.T) {
 
 	// Check labels change
 	info := reconcilers.Common{Namespace: "namespace2"}
-	b = newMonolithBuilder(info.NewInstance(image2), &cfg)
+	b, _ = newMonolithBuilder(info.NewInstance(image2), &cfg)
 	third := b.generic.prometheusRule()
 
 	report = helper.NewChangeReport("")
@@ -682,9 +684,9 @@ func TestLabels(t *testing.T) {
 
 	cfg := getConfig()
 	info := reconcilers.Common{Namespace: "ns"}
-	builder := newMonolithBuilder(info.NewInstance(image), &cfg)
-	tBuilder := newTransfoBuilder(info.NewInstance(image), &cfg)
-	iBuilder := newIngestBuilder(info.NewInstance(image), &cfg)
+	builder, _ := newMonolithBuilder(info.NewInstance(image), &cfg)
+	tBuilder, _ := newTransfoBuilder(info.NewInstance(image), &cfg)
+	iBuilder, _ := newIngestBuilder(info.NewInstance(image), &cfg)
 
 	// Deployment
 	depl := tBuilder.deployment(annotate("digest"))
@@ -763,7 +765,7 @@ func TestPipelineConfig(t *testing.T) {
 	// Kafka Ingester
 	cfg.DeploymentModel = flowslatest.DeploymentModelKafka
 	info := reconcilers.Common{Namespace: ns}
-	bi := newIngestBuilder(info.NewInstance(image), &cfg)
+	bi, _ := newIngestBuilder(info.NewInstance(image), &cfg)
 	stages, parameters, err = bi.buildPipelineConfig()
 	assert.NoError(err)
 	assert.True(validatePipelineConfig(stages, parameters))

diff --git a/controllers/flowlogspipeline/flp_transfo_objects.go b/controllers/flowlogspipeline/flp_transfo_objects.go
@@ -18,11 +18,11 @@ type transfoBuilder struct {
 	generic builder
 }
 
-func newTransfoBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec) transfoBuilder {
-	gen := newBuilder(info, desired, ConfKafkaTransformer)
+func newTransfoBuilder(info *reconcilers.Instance, desired *flowslatest.FlowCollectorSpec) (transfoBuilder, error) {
+	gen, err := newBuilder(info, desired, ConfKafkaTransformer)
 	return transfoBuilder{
 		generic: gen,
-	}
+	}, err
 }
 
 func (b *transfoBuilder) deployment(annotations map[string]string) *appsv1.Deployment {

diff --git a/controllers/flowlogspipeline/flp_transfo_reconciler.go b/controllers/flowlogspipeline/flp_transfo_reconciler.go
@@ -88,7 +88,10 @@ func (r *flpTransformerReconciler) reconcile(ctx context.Context, desired *flows
 		return nil
 	}
 
-	builder := newTransfoBuilder(r.Instance, &desired.Spec)
+	builder, err := newTransfoBuilder(r.Instance, &desired.Spec)
+	if err != nil {
+		return err
+	}
 	newCM, configDigest, err := builder.configMap()
 	if err != nil {
 		return err

diff --git a/pkg/volumes/builder.go b/pkg/volumes/builder.go
@@ -37,7 +37,7 @@ func (b *Builder) AddCACertificate(config *flowslatest.ClientTLS, namePrefix str
 }
 
 func (b *Builder) AddCertificate(ref *flowslatest.CertificateReference, volumeName string) (certPath, keyPath string) {
-	if ref.Name != "" {
+	if ref != nil && ref.Name != "" {
 		certPath = fmt.Sprintf("/var/%s/%s", volumeName, ref.CertFile)
 		keyPath = fmt.Sprintf("/var/%s/%s", volumeName, ref.CertKey)
 		vol, vm := buildVolumeAndMount(ref.Type, ref.Name, volumeName)