Inline username and password no longer works for remote write after security fix

[mattdurham]

When using http://user:password@example.com for remote_write the password is redacted when generating the client URL for the HTTP client.

The instance.go:199 Clone function takes the unsanitized input and then redacts it in github.com/prometheus/common/config/http_config.go:127, the workaround is to separate the username and password into their own fields.

[rfratto]

Ah, I think this is because we brought in https://github.com/prometheus/common/commit/b6d7542023283ed4c8034c0fb0d728a6bf5687f1 as a dependency at some point.

I think we should probably fix this. There's a few things we can do here:

1. Update our YAML encoding hook to print unredacted URLs.
2. Find a way to create deep copy of objects without doing a YAML marshal+unmarshal.
3. Find a way to avoid deep copying objects, if it's even necessary at all currently.

[mattdurham]

I am on the fence on this, having authentication be separate fields allows Grafana Agent to be more intelligent on the marshal/unmarshal path. Given the security fix has been out for a while now and there have been very limited reports on this issue may indicate inline username and password are rarely used.

[rfratto]

I do think this will "just" go away if we agree that #1140 is the direction we want to go in. #1140 isn't susceptible to this because it doesn't need to ever clone or marshal instance configs aside from the config endpoint.

[tpaschalis]

I looked through the options mentioned above. 1) This is easily achieved by something like d154b63. Since we're looking towards fixing the problem with metrics V2 soon, this may be a good short-term solution 2) Without resorting to third-party libraries, we have a couple of options here

• Use gob to encode/decode into a new struct, which effectively copies pointer slices to a new place (example here). Not sure about how performant this would be.
• Build a monstrosity to manually copy all exported fields (i.e loop through original slices and re-create pointers with values), but this would require maintenance whenever the struct changes
• Define some kind of additional tags which we would use, but Prometheus code would be oblivious to

3) I'm not yet sure if there's a way to avoid copying entirely, especially in the case of the GroupManager, but I may still need to take a closer look.

All in all, I'd personally try with updating our hook since this is a temporary fix, but I'd also like to hear your opinion on using gob as an alternative

[mattdurham]

Does solution 1 also mask it when using the /-/config endpoint? IMO if its a complicated solution tabling this seems a reasonable choice given its low usage and easy workaround.

[tpaschalis]

Yeah, updating the hook keeps the password masked when fetching through /-/config. Do you think there are any other places we should look out for that may expose it?

$ curl localhost:12345/-/config
....
    remote_write:
    - url: https://<user>:xxxxx@prometheus-prod-01-eu-west-0.grafana.net/api/prom/push
....