contrib/go-redis/redis.v8: add Option to Exclude Raw Command #1026

sjmtan · 2021-10-08T18:39:44Z

Intends on solving #1022.

I couldn't figure out how to get the tests run locally sadly, so 🤞🏼 that this works.

Co-authored-by: Gabriel Aszalos <[email protected]>

…extExtension

rama-kundurthi · 2021-10-13T18:48:43Z

@shannontan-bolt : Thanks for working on this. I am also in need of this change.

felixge

Thanks for this PR @shannontan-bolt and sorry for not receiving a reply earlier.

I'm on the profiling team, so please take my comments with a grain of salt, but here is what I think:

This change is absolutely needed. This is a DDoS vector and a potential leak for PII. I would suggest excludeRawCommand to be true by default.
The code LGTM (but I have not tested it beyond visual inspection).

@knusbaum PTAL as well.

felixge · 2021-11-04T19:01:48Z

contrib/go-redis/redis.v8/redis.go

@@ -102,14 +102,18 @@ func (ddh *datadogHook) BeforeProcess(ctx context.Context, cmd redis.Cmder) (con
 	raw := cmd.String()
 	length := strings.Count(raw, " ")
 	p := ddh.params
-	opts := make([]ddtrace.StartSpanOption, 0, 5+len(ddh.additionalTags)+1) // 5 options below + for additionalTags + for analyticsRate
+	var opts []ddtrace.StartSpanOption


NIT: Generally I'm okay with not pre-allocating the arrays backing a slice unless it has a significant performance impact. I'm not sure if this analysis has been done here, but assuming it was, this change here could be a small performance regression.

Have you considered just keeping this line (and the one in BeforeProcessPipeline) as-is? As far as I can tell that should work just fine.

gbbr

This LGTM. Just a few nits regarding spaces, if you could please cut down 🙏🏻

gbbr · 2021-11-05T07:05:39Z

contrib/go-redis/redis.v8/redis.go

+
+	if !p.config.excludeRawCommand {
+		opts = append(opts, tracer.Tag("redis.raw_command", raw))
+	}
+


Suggested change

if !p.config.excludeRawCommand {

opts = append(opts, tracer.Tag("redis.raw_command", raw))

}

if !p.config.excludeRawCommand {

opts = append(opts, tracer.Tag("redis.raw_command", raw))

}

gbbr · 2021-11-05T07:05:48Z

contrib/go-redis/redis.v8/redis.go

+
+	if !p.config.excludeRawCommand {
+		opts = append(opts,
+			tracer.Tag("redis.raw_command", raw),
+			tracer.Tag(ext.ResourceName, raw))
+	}
+


Suggested change

if !p.config.excludeRawCommand {

opts = append(opts,

tracer.Tag("redis.raw_command", raw),

tracer.Tag(ext.ResourceName, raw))

}

if !p.config.excludeRawCommand {

opts = append(opts,

tracer.Tag("redis.raw_command", raw),

tracer.Tag(ext.ResourceName, raw))

}

gbbr · 2021-11-05T07:56:32Z

contrib/go-redis/redis.v8/option.go

+// WithExcludeRawCommand sets the option to exclude the raw command
+// for when the command/cache might contain sensitive data


I don't understand this part. The Datadog Agent obfuscates this tag and hides all parameters. Why would the fact that it contains sensitive data be an issue?

Suggested change

// WithExcludeRawCommand sets the option to exclude the raw command

// for when the command/cache might contain sensitive data

// SkipRawCommand determines the integration to skip setting the raw command

// in the redis.raw_command tag of a span.

As for the doc, we should be a bit more specific on what it does.

gbbr · 2021-11-05T07:57:18Z

contrib/go-redis/redis.v8/option.go

+
+// WithExcludeRawCommand sets the option to exclude the raw command
+// for when the command/cache might contain sensitive data
+func WithExcludeRawCommand(exclude bool) ClientOption {


Suggested change

func WithExcludeRawCommand(exclude bool) ClientOption {

func SkipRawCommand(skip bool) ClientOption {

I think this would be better, unless it's bad that we don't have the With* prefix?

gbbr · 2021-11-05T09:19:59Z

contrib/go-redis/redis.v8/redis.go

+	if !p.config.excludeRawCommand {
+		opts = append(opts,
+			tracer.Tag("redis.raw_command", raw),
+			tracer.Tag(ext.ResourceName, raw))


Sorry for missing this. The resource name is 100% mandatory for all spans. We can not leave it out.

sjmtan · 2021-11-06T00:31:45Z

@gbbr, to be perfectly honest, it might be easier if you try to fix this. I find that my local environment regarding dependencies + tests for this repo is kinda messed up so I find it hard to test this work on it without my IDE complaining about missing dependencies after syncing the go.mod file.

That's very confusing though that the ResourceName requires the raw command. The raw command contains the entire value that we are sending to Redis, which can include sensitive values. Imagine for example that you wanted to put IP address as a rate limit key. The real issue for us was that we were trying to save an object of data in a Redis cache and that meant this large blob was putting put as a tag with tracing.

If it is required though, which is fine, we should probably use something else?

gbbr · 2021-11-08T06:25:12Z

@gbbr, to be perfectly honest, it might be easier if you try to fix this. I find that my local environment regarding dependencies + tests for this repo is kinda messed up so I find it hard to test this work on it without my IDE complaining about missing dependencies after syncing the go.mod file.

Not exactly sure I understand what there is to fix? All you need to do is to run redis:3.2 locally in order for the test to run, and forward the appropriate port. When running tests for integrations, simply check our CircleCI config for the image needed and for the port that needs to be exposed. Maybe we should put this into the docs somewhere. So, you run docker run -p 6379:6379 redis:3.2 and then INTEGRATION=1 go test -v ./contrib/go-redis/redis.v8/.... Does this not work for you?

That's very confusing though that the ResourceName requires the raw command. The raw command contains the entire value that we are sending to Redis, which can include sensitive values. Imagine for example that you wanted to put IP address as a rate limit key. [...]

Let's continue this discussion in the issue you've opened (#1022). I've already explained there that these values are obfuscated, and no sensitive values will leave the Datadog Agent.

The real issue for us was that we were trying to save an object of data in a Redis cache and that meant this large blob was putting put as a tag with tracing.

Answered in the issue.

gbbr · 2021-12-14T11:00:47Z

Superseded by #1091

Shannon Tan and others added 12 commits February 23, 2021 10:29

Implement context extension

82c7cfd

Implement context extension

bd764ae

Update ddtrace/opentracer/tracer.go

c5ea938

Co-authored-by: Gabriel Aszalos <[email protected]>

Update ddtrace/opentracer/tracer.go

0c95b43

Co-authored-by: Gabriel Aszalos <[email protected]>

Fix for comment

f624bde

Fix

047de40

Merge pull request #1 from shannontan-bolt/shannontan/openTracingCont…

4754d6b

…extExtension

Merge branch 'v1' of github.com:DataDog/dd-trace-go into v1

f18ce83

Merge branch 'v1' of github.com:DataDog/dd-trace-go into v1

cba2932

Merge remote-tracking branch 'upstream/v1' into v1

34b7164

Merge remote-tracking branch 'upstream/v1' into v1

f421ca0

Add Option to Exclude Raw Command Tag

0ab271b

felixge approved these changes Nov 4, 2021

View reviewed changes

gbbr approved these changes Nov 5, 2021

View reviewed changes

gbbr changed the title ~~Add Option to Exclude Raw Command~~ contrib/go-redis/redis.v8: add Option to Exclude Raw Command Nov 5, 2021

gbbr reviewed Nov 5, 2021

View reviewed changes

gbbr added this to the 1.35.0 milestone Nov 5, 2021

gbbr suggested changes Nov 5, 2021

View reviewed changes

gbbr mentioned this pull request Nov 8, 2021

contrib/go-redis/redis.v8: Allow Option To Remove Raw Tag #1022

Closed

gbbr closed this Dec 14, 2021

knusbaum removed this from the 1.35.0 milestone Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/go-redis/redis.v8: add Option to Exclude Raw Command #1026

contrib/go-redis/redis.v8: add Option to Exclude Raw Command #1026

sjmtan commented Oct 8, 2021

rama-kundurthi commented Oct 13, 2021

felixge left a comment •

edited

Loading

felixge Nov 4, 2021

gbbr left a comment

gbbr Nov 5, 2021

gbbr Nov 5, 2021

gbbr Nov 5, 2021

gbbr Nov 5, 2021

gbbr Nov 5, 2021

sjmtan commented Nov 6, 2021

gbbr commented Nov 8, 2021 •

edited

Loading

gbbr commented Dec 14, 2021

		// WithExcludeRawCommand sets the option to exclude the raw command
		// for when the command/cache might contain sensitive data

	func WithExcludeRawCommand(exclude bool) ClientOption {
	func SkipRawCommand(skip bool) ClientOption {

contrib/go-redis/redis.v8: add Option to Exclude Raw Command #1026

contrib/go-redis/redis.v8: add Option to Exclude Raw Command #1026

Conversation

sjmtan commented Oct 8, 2021

rama-kundurthi commented Oct 13, 2021

felixge left a comment • edited Loading

Choose a reason for hiding this comment

felixge Nov 4, 2021

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

gbbr Nov 5, 2021

Choose a reason for hiding this comment

gbbr Nov 5, 2021

Choose a reason for hiding this comment

gbbr Nov 5, 2021

Choose a reason for hiding this comment

gbbr Nov 5, 2021

Choose a reason for hiding this comment

gbbr Nov 5, 2021

Choose a reason for hiding this comment

sjmtan commented Nov 6, 2021

gbbr commented Nov 8, 2021 • edited Loading

gbbr commented Dec 14, 2021

felixge left a comment •

edited

Loading

gbbr commented Nov 8, 2021 •

edited

Loading