configmap: option to not trust incoming tracing spans (#7045)

* validate the sender of tracing spans

* add location-specific setting

docs/user-guide/nginx-configuration/annotations.md

@@ -112,6 +112,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
 |[nginx.ingress.kubernetes.io/connection-proxy-header](#connection-proxy-header)|string|
 |[nginx.ingress.kubernetes.io/enable-access-log](#enable-access-log)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/enable-opentracing](#enable-opentracing)|"true" or "false"|
+|[nginx.ingress.kubernetes.io/opentracing-trust-incoming-span](#opentracing-trust-incoming-span)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/enable-influxdb](#influxdb)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/influxdb-measurement](#influxdb)|string|
 |[nginx.ingress.kubernetes.io/influxdb-port](#influxdb)|string|
@@ -768,6 +769,15 @@ to enable it or disable it for a specific ingress (e.g. to turn off tracing of e
 nginx.ingress.kubernetes.io/enable-opentracing: "true"
 ```
 
+### Opentracing Trust Incoming Span
+
+The option to trust incoming trace spans can be enabled or disabled globally through the ConfigMap but this will
+sometimes need to be overriden to enable it or disable it for a specific ingress (e.g. only enable on a private endpoint)
+
+```yaml
+nginx.ingress.kubernetes.io/opentracing-trust-incoming-span: "true"
+```
+
 ### X-Forwarded-Prefix Header
 To add the non-standard `X-Forwarded-Prefix` header to the upstream request with a string value, the following annotation can be used:
 

docs/user-guide/third-party-addons/opentracing.md

@@ -46,6 +46,9 @@ opentracing-operation-name
 # specifies specifies the name to use for the location span
 opentracing-location-operation-name
 
+# sets whether or not to trust incoming tracing spans
+opentracing-trust-incoming-span
+
 # specifies the port to use when uploading traces, Default: 9411
 zipkin-collector-port
 
@@ -114,6 +117,15 @@ datadog-sample-rate
 
 All these options (including host) allow environment variables, such as `$HOSTNAME` or `$HOST_IP`. In the case of Jaeger, if you have a Jaeger agent running on each machine in your cluster, you can use something like `$HOST_IP` (which can be 'mounted' with the `status.hostIP` fieldpath, as described [here](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/#capabilities-of-the-downward-api)) to make sure traces will be sent to the local agent.
 
+
+Note that you can also set whether to trust incoming spans (global default is true) per-location using annotations like the following:
+```
+kind: Ingress
+metadata:
+  annotations:
+    nginx.ingress.kubernetes.io/opentracing-trust-incoming-span: "true"
+```
+
 ## Examples
 
 The following examples show how to deploy and test different distributed tracing systems. These example can be performed using Minikube.

internal/ingress/annotations/opentracing/main.go

@@ -29,8 +29,10 @@ type opentracing struct {
 
 // Config contains the configuration to be used in the Ingress
 type Config struct {
-	Enabled bool `json:"enabled"`
-	Set     bool `json:"set"`
+	Enabled      bool `json:"enabled"`
+	Set          bool `json:"set"`
+	TrustEnabled bool `json:"trust-enabled"`
+	TrustSet     bool `json:"trust-set"`
 }
 
 // Equal tests for equality between two Config types
@@ -43,6 +45,14 @@ func (bd1 *Config) Equal(bd2 *Config) bool {
 		return false
 	}
 
+	if bd1.TrustSet != bd2.TrustSet {
+		return false
+	}
+
+	if bd1.TrustEnabled != bd2.TrustEnabled {
+		return false
+	}
+
 	return true
 }
 
@@ -54,8 +64,13 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 func (s opentracing) Parse(ing *networking.Ingress) (interface{}, error) {
 	enabled, err := parser.GetBoolAnnotation("enable-opentracing", ing)
 	if err != nil {
-		return &Config{Set: false, Enabled: false}, nil
+		return &Config{}, nil
 	}
 
-	return &Config{Set: true, Enabled: enabled}, nil
+	trustSpan, err := parser.GetBoolAnnotation("opentracing-trust-incoming-span", ing)
+	if err != nil {
+		return &Config{Set: true, Enabled: enabled}, nil
+	}
+
+	return &Config{Set: true, Enabled: enabled, TrustSet: true, TrustEnabled: trustSpan}, nil
 }

internal/ingress/annotations/opentracing/main_test.go

@@ -106,6 +106,29 @@ func TestIngressAnnotationOpentracingSetFalse(t *testing.T) {
 	}
 }
 
+func TestIngressAnnotationOpentracingTrustSetTrue(t *testing.T) {
+	ing := buildIngress()
+
+	data := map[string]string{}
+	data[parser.GetAnnotationWithPrefix("enable-opentracing")] = "true"
+	data[parser.GetAnnotationWithPrefix("opentracing-trust-incoming-span")] = "true"
+	ing.SetAnnotations(data)
+
+	val, _ := NewParser(&resolver.Mock{}).Parse(ing)
+	openTracing, ok := val.(*Config)
+	if !ok {
+		t.Errorf("expected a Config type")
+	}
+
+	if !openTracing.Enabled {
+		t.Errorf("expected annotation value to be true, got false")
+	}
+
+	if !openTracing.TrustEnabled {
+		t.Errorf("expected annotation value to be true, got false")
+	}
+}
+
 func TestIngressAnnotationOpentracingUnset(t *testing.T) {
 	ing := buildIngress()
 

internal/ingress/controller/config/config.go

@@ -537,6 +537,11 @@ type Configuration struct {
 	// OpentracingOperationName specifies a custom name for the location span
 	OpentracingLocationOperationName string `json:"opentracing-location-operation-name"`
 
+	// OpentracingTrustIncomingSpan sets whether or not to trust incoming trace spans
+	// If false, incoming span headers will be rejected
+	// Default: true
+	OpentracingTrustIncomingSpan bool `json:"opentracing-trust-incoming-span"`
+
 	// ZipkinCollectorHost specifies the host to use when uploading traces
 	ZipkinCollectorHost string `json:"zipkin-collector-host"`
 
@@ -874,6 +879,7 @@ func NewDefault() Configuration {
 		LimitConnZoneVariable:                  defaultLimitConnZoneVariable,
 		BindAddressIpv4:                        defBindAddress,
 		BindAddressIpv6:                        defBindAddress,
+		OpentracingTrustIncomingSpan:           true,
 		ZipkinCollectorPort:                    9411,
 		ZipkinServiceName:                      "nginx",
 		ZipkinSampleRate:                       1.0,

internal/ingress/controller/template/template.go

@@ -1440,7 +1440,7 @@ func httpsListener(addresses []string, co string, tc config.TemplateConfig) []st
 	return out
 }
 
-func buildOpentracingForLocation(isOTEnabled bool, location *ingress.Location) string {
+func buildOpentracingForLocation(isOTEnabled bool, isOTTrustSet bool, location *ingress.Location) string {
 	isOTEnabledInLoc := location.Opentracing.Enabled
 	isOTSetInLoc := location.Opentracing.Set
 
@@ -1448,25 +1448,21 @@ func buildOpentracingForLocation(isOTEnabled bool, location *ingress.Location) s
 		if isOTSetInLoc && !isOTEnabledInLoc {
 			return "opentracing off;"
 		}
-
-		opc := opentracingPropagateContext(location)
-		if opc != "" {
-			opc = fmt.Sprintf("opentracing on;\n%v", opc)
-		}
-
-		return opc
+	} else if !isOTSetInLoc || !isOTEnabledInLoc {
+		return ""
 	}
 
-	if isOTSetInLoc && isOTEnabledInLoc {
-		opc := opentracingPropagateContext(location)
-		if opc != "" {
-			opc = fmt.Sprintf("opentracing on;\n%v", opc)
-		}
-
-		return opc
+	opc := opentracingPropagateContext(location)
+	if opc != "" {
+		opc = fmt.Sprintf("opentracing on;\n%v", opc)
 	}
 
-	return ""
+	if (!isOTTrustSet && !location.Opentracing.TrustSet) ||
+		(location.Opentracing.TrustSet && !location.Opentracing.TrustEnabled) {
+		opc = opc + "\nopentracing_trust_incoming_span off;"
+	}
+
+	return opc
 }
 
 // shouldLoadOpentracingModule determines whether or not the Opentracing module needs to be loaded.

internal/ingress/controller/template/template_test.go

@@ -1637,32 +1637,44 @@ func TestShouldLoadModSecurityModule(t *testing.T) {
 
 func TestOpentracingForLocation(t *testing.T) {
 	trueVal := true
+	falseVal := false
 
 	loadOT := `opentracing on;
 opentracing_propagate_context;`
+	loadOTUntrustedSpan := `opentracing on;
+opentracing_propagate_context;
+opentracing_trust_incoming_span off;`
 	testCases := []struct {
-		description string
-		globalOT    bool
-		isSetInLoc  bool
-		isOTInLoc   *bool
-		expected    string
+		description     string
+		globalOT        bool
+		isSetInLoc      bool
+		isOTInLoc       *bool
+		globalTrust     bool
+		isTrustSetInLoc bool
+		isTrustInLoc    *bool
+		expected        string
 	}{
-		{"globally enabled, without annotation", true, false, nil, loadOT},
-		{"globally enabled and enabled in location", true, true, &trueVal, loadOT},
-		{"globally disabled and not enabled in location", false, false, nil, ""},
-		{"globally disabled but enabled in location", false, true, &trueVal, loadOT},
-		{"globally disabled, enabled in location but false", false, true, &trueVal, loadOT},
+		{"globally enabled, without annotation", true, false, nil, true, false, nil, loadOT},
+		{"globally enabled and enabled in location", true, true, &trueVal, true, false, nil, loadOT},
+		{"globally disabled and not enabled in location", false, false, nil, true, false, nil, ""},
+		{"globally disabled but enabled in location", false, true, &trueVal, true, false, nil, loadOT},
+		{"globally trusted, not trusted in location", true, false, nil, true, true, &falseVal, loadOTUntrustedSpan},
+		{"not globally trusted, trust set in location", true, false, nil, false, true, &trueVal, loadOT},
+		{"not globally trusted, trust not set in location", true, false, nil, false, false, nil, loadOTUntrustedSpan},
 	}
 
 	for _, testCase := range testCases {
 		il := &ingress.Location{
-			Opentracing: opentracing.Config{Set: testCase.isSetInLoc},
+			Opentracing: opentracing.Config{Set: testCase.isSetInLoc, TrustSet: testCase.isTrustSetInLoc},
 		}
 		if il.Opentracing.Set {
 			il.Opentracing.Enabled = *testCase.isOTInLoc
 		}
+		if il.Opentracing.TrustSet {
+			il.Opentracing.TrustEnabled = *testCase.isTrustInLoc
+		}
 
-		actual := buildOpentracingForLocation(testCase.globalOT, il)
+		actual := buildOpentracingForLocation(testCase.globalOT, testCase.globalTrust, il)
 
 		if testCase.expected != actual {
 			t.Errorf("%v: expected '%v' but returned '%v'", testCase.description, testCase.expected, actual)

rootfs/etc/nginx/template/nginx.tmpl

@@ -1096,7 +1096,7 @@ stream {
             set $location_path  {{ $ing.Path | escapeLiteralDollar | quote }};
             set $global_rate_limit_exceeding n;
 
-            {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $location }}
+            {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $all.Cfg.OpentracingTrustIncomingSpan $location }}
 
             {{ if $location.Mirror.Source }}
             mirror {{ $location.Mirror.Source }};

test/e2e/settings/opentracing.go

@@ -29,7 +29,8 @@ import (
 )
 
 const (
-	enableOpentracing = "enable-opentracing"
+	enableOpentracing            = "enable-opentracing"
+	opentracingTrustIncomingSpan = "opentracing-trust-incoming-span"
 
 	zipkinCollectorHost = "zipkin-collector-host"
 
@@ -81,6 +82,21 @@ var _ = framework.IngressNginxDescribe("Configure OpenTracing", func() {
 			})
 	})
 
+	ginkgo.It("should include opentracing_trust_incoming_span off directive when disabled", func() {
+		config := map[string]string{}
+		config[enableOpentracing] = "true"
+		config[opentracingTrustIncomingSpan] = "false"
+		config[zipkinCollectorHost] = "127.0.0.1"
+		f.SetNginxConfigMapData(config)
+
+		f.EnsureIngress(framework.NewSingleIngress(enableOpentracing, "/", enableOpentracing, f.Namespace, "http-svc", 80, nil))
+
+		f.WaitForNginxConfiguration(
+			func(cfg string) bool {
+				return strings.Contains(cfg, "opentracing_trust_incoming_span off")
+			})
+	})
+
 	ginkgo.It("should not exists opentracing_operation_name directive when is empty", func() {
 		config := map[string]string{}
 		config[enableOpentracing] = "true"