Redact client_secret from logs

[vcheung-stripe]

cc @stripe/api-libraries @richardm-stripe
r? @brandur-stripe @remi-stripe

Redacts API keys in logs. The redaction is done on the client as opposed to the server.

Added a unit test. All tests pass.

[brandur-stripe]

Try just:

var apiKeyRegex *regexp.Regexp = regexp.MustCompile(...)

Also, mind using either Regexp or RE as a suffix? I don't think we should mix the abbreviations "regex" and "regexp" which are very similar but different.

[brandur-stripe]

Left a couple comments, but in general, not super crazy about this one! We make logging slower and there's other secrets beyond keys that could be logged too.

And on that note: are we even sure that the original ticket that this is what's needed for the fix? It looked like the field in question was client_secret, which by my read of the docs, refers to a secret from something like a payment intent, setup intent, or source. It should still not be logged, but it looks more like pi_1DijHt2eZvKYlo2C0VCV90Tf_secret_24N2Yr44VXRpt5Np2mTIWgRDX.

ptal @vcheung-stripe

[brandur-stripe]

Wonder if we should just making these .k_(live|test) for some future proofing.

[remi-stripe]

Lurking in: Historical API keys did not have sk_ prefix or live_ in them so that would not catch all of them.

I'm also curious why we're adding this. The internal ask I'm aware of is specifically about client_secret which is a property on PaymentIntent and SetupIntent, is this related or orthogonal?

[brandur-stripe]

I've tried to keep files like this organized so that you have the public-facing types/functions nearer to the top, and the internal ones below (see comments like "Public types". Can we move all of this down to a new section at the bottom (except printLog which belongs on LeveledLogger, but try to order it alphabetically so it appears below Warnf and above stderr.

[vcheung-stripe]

And on that note: are we even sure that the original ticket that this is what's needed for the fix? It looked like the field in question was client_secret, which by my read of the docs, refers to a secret from something like a payment intent, setup intent, or source. It should still not be logged, but it looks more like pi_1DijHt2eZvKYlo2C0VCV90Tf_secret_24N2Yr44VXRpt5Np2mTIWgRDX.

This is a mistake, I completely misread client_secret as an API key 🤦. Thank you for catching this.

We make logging slower and there's other secrets beyond keys that could be logged too.

This is a concern I have as well. One approach I can think of is we could redact client_secret when we receive an error response, right after we deserialize a response body. This way we don't do the redaction unless there is an error, and we do this for all errors. Or do you have a better recommendation for how to go about this?

@brandur-stripe

[brandur-stripe]

This is a concern I have as well. One approach I can think of is we could redact client_secret when we receive an error response, right after we deserialize a response body. This way we don't do the redaction unless there is an error, and we do this for all errors. Or do you have a better recommendation for how to go about this?

I'd like something like that better. Basically have a function that we only use when dumping JSON blobs out to logs which will recursively spalunk the structure and redact knowingly problematic fields in it. That way, we wouldn't have to run it on every single logging statement either.

We could also consider something much simpler like just not dumping entire JSON structs out in logs anymore. I don't think we do that for most languages, and users now have access to the raw response, so if they wanted, they could dump it out for themselves and take the various risks inherent in doing so.

[richardm-stripe]

Closing this particular PR, since it sounds like we're going to take a different approach altogether. Eh, keeping it open for continuity, but changing the implementation.

[richardm-stripe]

r? @brandur-stripe take a look at this version? This just redacts client-secret. The tricky bit is that I'm afraid of mutating the error object and not sure how/whether to make a deep copy.

I think if there are more sensitive fields it might make sense to use struct annotations and reflect. That would certainly feel cleaner than this.

I'm interested in your suggestion of dropping logging JSON structs, but I do think it is pretty nice to have? It's true the user has access to the raw response, but even so Level: LevelInfo provides an easy way to toggle the logs in a single place, and if we removed it the user might have to either add logging statements in multiple places or wrap the stripe client. And getting useful logs should be as low-friction as possible. We don't have this kind of log in other libraries but I am aware of a feature request in stripe-java to make logging more sophisticated.

[brandur-stripe]

Nice! Yeah, this works for me and shouldn't add too much overhead. Left a couple minor comments.

ptal @richardm-stripe

[brandur-stripe]

What do you think about collapsing these two print calls into just one. i.e.

fmt.Printf(output, "[%s] " + format + "\n", tag v...)

(Not sure that exact line works, but something like it should.)

I gotta say too that I'm generally a fan of extracting things into small functions, but I'm not sure that we're getting our money's worth here. The previous standard of 3 lines per error/warn/info was really not that bad, and we've made this incrementally more difficult to understand in that if l.Level >= level is harder to read than if l.Level >= LevelWarn {.

[richardm-stripe]

r? @brandur-stripe great feedback! actioned all of it.