server,lint: fix misuse of hash.Hash.Sum

[benesch]

The hash.Hash interface is poorly designed and easily misused. Several
locations in our codebase were assuming that the Sum function returns
the hash of its input, when in fact it returns the concatenation of its
input with the hash of the empty string. See the comments within for the
full detail.s

This commit:

1. Corrects the code in our web UI login to avoid leaking session
   secrets into the audit log. Since web UI login has only been
   released in betas, there are no backwards compatibility concerns
   here. Existing web sessions will simply be invalidated when the
   cluster is upgraded and users will need to re-login.

2. Files a TODO about correcting the code in our password authentication. It
   is not a security risk there as the SHA-256 application was only
   intended to provided a fixed-length input to bcrypt. Fixing this
   code path is more difficult as it requires a migration for existing
   clusters, since this bug has been present since pre-1.0.

3. Adds a linter to prevent future misuse.

Release note: None

/cc @couchand @vilterp

[cockroach-teamcity]

This change is 

[benesch]

Oh, forgot to mention: the first commit is just code movement.

[bdarnell]

Reviewed 4 of 5 files at r1, 5 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/testutils/lint/checker_hash.go, line 51 at r2 (raw file):

// To differentiate between correct and incorrect usages, hashChecker applies a
// simple heuristic: it flags calls to Sum where a) the parameter is non-nil and
// b) the return value is used.

Sum() works like append(). It's valid to both pass a value and use the return, but in that case the argument and the return variable will almost always be the same thing. (you could do buf := h.Sum(buf) regardless of whether buf has enough capacity to write the hash in place). I'd argue that if we're going to lint here, we should require that the return value be used (and that if a non-nil argument is given, it must be the same as the one that the result is assigned to). Or we could just disallow non-nil arguments unless and until we have a need for them.

[benesch]

Sum() works like append(). It's valid to both pass a value and use the return, but in that case the argument and the return variable will almost always be the same thing.

That's strictly speaking true, but I don't think it's common in practice. I looked at a bunch of examples from the Go standard library and very few of them use the append-style x = hash.Sum(x) pattern.

you could do buf := h.Sum(buf) regardless of whether buf has enough capacity to write the hash in place

If you're not sure whether buf has enough capacity, you're probably not too worried about allocations, and could just use the buf := h.Sum(nil) pattern.

Anyway, happy to adjust the linter to be more strict by requiring a nil argument and a used return value.

[RaduBerinde]

Anyway, happy to adjust the linter to be more strict by requiring a nil argument and a used return value.

I don't see a reasonable usecase where you would not use the return value of Sum. I think it's fine to be strict here, as long as the linter error message shows an example of how to disable the check for that line if you know what you're doing.

[RaduBerinde]

BTW golang/go#21070

[benesch]

I don't see a reasonable usecase where you would not use the return value of Sum. I think it's fine to be strict here, as long as the linter error message shows an example of how to disable the check for that line if you know what you're doing.

This is the non-return-value-using case I was thinking of:

• https://github.com/golang/go/blob/de72c43f78cf31e672cfc93474c9bbb8a1eec925/src/cmd/internal/buildid/rewrite.go#L79
• https://github.com/golang/go/blob/de72c43f78cf31e672cfc93474c9bbb8a1eec925/src/cmd/go/internal/cache/cache.go#L374-L375

Granted I got the example in the docstring a bit wrong.

[benesch]

Oh, actually, we use exactly that pattern in some Cockroach code:

cockroach/pkg/workload/kv/kv.go

Line 288 in b5078df

g.hasher.Sum(g.buf[:0])

I'd prefer to leave the linter as-is, if that's alright. The current approach flags all of our misuses of the API without flagging any of the correct usages, and we can cross the x = h.Sum(x) bridge if we ever come to it.

[benesch]

TFTRs!

bors r=bdarnell

[craig]

Build succeeded

• GitHub CI (Cockroach)