Fix proxy hash #1157
Fix proxy hash #1157
Conversation
njvrzm
requested review from
wbrowne,
andresmgot and
oshirohugo
and removed request for
a team
| start := max(len(key)-4, 0) | ||
| return key[start:] | ||
| contents := c.config[proxy.PluginSecureSocksProxyClientKeyContents] | ||
| block, _ := pem.Decode([]byte(contents)) |
There was a problem hiding this comment.
Do we actually need to add any more logic here or can we simplify by using the client key in its entirety?
There was a problem hiding this comment.
I suppose we could! It's 120 bytes long currently, or 64 if we extract only the encoded part. Potentially longer later. Granted there shouldn't be ever be more than few hundred of them at a time tops, but it seems wasteful, and the key would be hashed internally for every map access; the longer key adds a small but nonzero latency to lookups, although that would be more than made up for by not doing the PEM decode and base64 decode.
I guess one concern is that the key is a bit more likely to leak out in an error message this way, but without the other values the key is probably not dangerous.
I dunno - it feels odd to me, but if you think it's the best approach I can change it.
There was a problem hiding this comment.
Let's go with how it is now and see how we get on 😄
Co-authored-by: Will Browne <[email protected]>
What this PR does / why we need it:
The proxy hash added to the datasource instance cache key in #1133 ends up always being
---\n, because the contents of the proxy client key are PEM-encoded. This PR fixes that by PEM-decoding the contents, which gives a key in[]bytes, then base64-encoding the final three bytes, giving a four-character hash.Since this is a bit more complex than the previous approach, I added a benchmark, which times this to about 250ns on my laptop, compared to the 1-2 microseconds for the original version discussed in #1133. Seems acceptable to me, but if need be we can do a hackier approach and just extract some characters directly from the key contents.
Which issue(s) this PR fixes:
https://github.com/grafana/app-platform-wg/issues/174, again.