Re: md5 issues Postgres14 on OL7

On Thu, Jan 06, 2022 at 11:40:04AM -0500, Tom Lane wrote:
> 1. It draws a cast-away-const warning. We'd have to make the result
> of pg_cryptohash_error be "const char *", which would be better
> practice anyway, but that propagates into some other APIs and I didn't
> take the trouble to chase it to the end.

Yeah. I wanted to switch all those routines to use a const here
anyway, just did not have the time to tackle that yesterday with the
rest of the issues I could think about. Looking at that today, I
don't see any problems in switching to const in all those places, so
done this way (two places in crypt.c are more picky, though, for
logdetail).

> 2. It feels a bit bogus to be fetching ERR_get_error() at this point.
> Maybe it's all right to assume that the OpenSSL error stack won't
> change state before we get to pg_cryptohash_error, but I don't like
> the idea much. I think it'd be better to capture ERR_get_error()
> sooner and store it in an additional field in pg_cryptohash_ctx.

Right, I forgot about the ERR_get_error() piece of it. Thanks! I'd
also rather have the check done just after the OpenSSL call. If hash
computations are split across multiple code paths, this could lead to
issues. We don't have any problems currently in the core code, but I
see no reason to not make that safer in the long run. And the
structure is flexible enough, so that's not an issue.

> Also, I wonder if this shouldn't be unified with the SSLerrmessage()
> support found in be-secure-openssl.c and fe-secure-openssl.c.

Guess so. HEAD could be poked at for this part. I recall looking at
that once by that did not seem worth the complications.

I have also looked at the ABI part of the patch. I cannot spot any
public trace of pg_md5_hash() and pg_md5_binary(). pgbouncer and
pgpool2 have each a copy of pg_md5_encrypt(), but they just share the
API name with PG, nothing more. So that looks reasonably safe to
change.

The last thing I have on my notes is to assign logdetail in
md5_crypt_verify() and plain_crypt_verify() to feed back a LOG entry
to the postmaster on those failures, and saw that it is safe to assign
directly the error returned by the cryptohash APIs, avoiding the
extra psprintf call that could become an issue under memory pressure.

What do you think?
--
Michael