Fetch channel binding digest explicitly with OpenSSL 3.0 and later
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253328psql -h localhost -U postgresBuilt from patchset v4 (message #4), August 13, 2026 at 01:23 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253328_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253328_4 && git checkout t253328_4Patchset v4 (message #4) is on t253328_4
The attached patch makes the tls-server-end-point channel binding hash
(RFC 5929) select its digest with EVP_MD_fetch() when building against OpenSSL
3.0 or newer.
be_tls_get_certificate_hash() and pgtls_get_peer_certificate_hash() compute the
certificate hash using the implicit EVP_sha256() / EVP_get_digestbynid()
digests, which do not deterministically dispatch through a loaded provider.
The patch selects the digest by name and fetches it, so the certificate hash is
computed by the active provider. The implicit path is retained for older
OpenSSL and for LibreSSL, guarded by OPENSSL_VERSION_NUMBER >= 0x30000000L.
The fetch uses the default library context and a NULL property query, so no
dependency is added and no particular provider is required.
The fetched EVP_MD is freed on every path, including the error paths: it is not
tracked by a resource owner, and the backend raises errors with elog(ERROR),
which does not return. That is the part of this patch most worth a careful
look.
I checked what the digest path resolves to at run time using
EVP_MD_get0_provider(). On OpenSSL 3.0.13, EVP_sha256() and the MD a context
ends up with after EVP_DigestInit_ex(ctx, EVP_sha256(), NULL) both report no
provider (the legacy built-in), while EVP_MD_fetch(NULL, "SHA256", NULL)
reports the default provider. The probe program that prints this is attached
to the related cryptohash thread.
Details:
* Against master, tested at 8b73ceb78f. It touches
src/backend/libpq/be-secure-openssl.c and
src/interfaces/libpq/fe-secure-openssl.c, and applies on its own; there is
no dependency on the two related patches I am posting in separate threads.
* Built and tested with OpenSSL 3.0.13 on Ubuntu 24.04 (x86-64): clean build,
src/test/regress, src/test/ssl and src/test/authentication all pass.
src/test/ssl covers tls-server-end-point channel binding in 002_scram.pl,
which is the path this patch changes on both the server and libpq side.
* No new regression tests. This changes how an existing hash is computed
without changing the result or any API, and 002_scram.pl already exercises
both sides.
* No documentation change.
* No performance impact expected. The fetch is a provider lookup done at
connection setup, not in any tight loop.
This was previously posted as a three-patch series in a single thread [1]/messages/by-id/20260805004805.1174492-1-mark@reviewcommit.com.
Reposting as separate threads with the patch attached, per review request.
Intended for the next commitfest.
[1]: /messages/by-id/20260805004805.1174492-1-mark@reviewcommit.com
--
Mark
Attachments:
v1-0003-Fetch-the-channel-binding-digest-explicitly-with-.patchtext/x-diff; charset=utf-8Download+125-35
Correction: my rationale for this patch was wrong.
Implicit digests DO reach providers. evp_md_init_internal() re-fetches an MD
with type->prov == NULL by name, so EVP_sha256() and EVP_get_digestbynid()
end up provider-backed. The probe I cited measured ctx->reqdigest, the MD
passed in, not the one used. Details and a corrected probe are on the
cryptohash thread [1]/messages/by-id/178596055358.1584287.8485463954311014881@reviewcommit.com.
So this patch is not a bypass fix. What survives: the internal fetch
hardcodes libctx=NULL and propq="", so a non-default OSSL_LIB_CTX gets no
provider control, and a registered ENGINE bypasses providers outright.
Control and clarity, not bypass.
Michael, you named channel binding as your interest, so you should have that
before spending time on it. The patch behavior is unchanged, and the question
about freeing the EVP_MD on every error path still stands.
Repost on the narrower basis, or withdraw?
[1]: /messages/by-id/178596055358.1584287.8485463954311014881@reviewcommit.com
--
Mark
On Tue, Aug 11, 2026 at 01:33:31PM -0700, Mark Atwood wrote:
Implicit digests DO reach providers. evp_md_init_internal() re-fetches an MD
with type->prov == NULL by name, so EVP_sha256() and EVP_get_digestbynid()
end up provider-backed. The probe I cited measured ctx->reqdigest, the MD
passed in, not the one used. Details and a corrected probe are on the
cryptohash thread [1].
This does not change the fact that removing the dependency to the
deprecated EVP_sha256() is still something that we need to do in the
long-term in the frontend and the backend code, because OpenSSL wants
code to switch to EVP.
Michael, you named channel binding as your interest, so you should have that
before spending time on it. The patch behavior is unchanged, and the question
about freeing the EVP_MD on every error path still stands.
Using EVP_MD_free() where adapted is an implementation artifact, there
is no big deal in them except making sure that nothing leaks, and
the channel binding paths are, contrary to the crypthash and hmac
bits, purely local, meaning that there is no need for post-cleanup
actions based on resowners and the kind. The commit message just has
to reflect what's been done, let's make it simpler without the
provider part then.
Now I think that this patch should be reworked in a style closer to
what has been done in 1f3b9bb109b8 and b91f79cd08ab:
- Keep the variable declarations at the top of each function. For
example algo_type is shared between the pre-3.0 block and the post-3.0
block. No need for two declarations.
- The #if parts work as the way as the curly brackets, let's remove
one level of indentation. Your patch makes the whole diff harder to
parse and the pre-3.0 code is still the same.
- Minimization of the diffs by planting more #if blocks. Here I am
looking at the EVP_MD_free() calls. Let's minimize the duplicated
libpq_append_conn_error() and elog(ERROR) calls in the final result.
I was wondering a bit about OBJ_nid2sn(), to retrieve the EVP_MD from
an algorithm name, and it looks like it's safe choice at the end.
Could you rework the patch among these lines, please?
--
Michael
On Wed, Aug 12, 2026 at 10:20:33AM +0900, Michael Paquier wrote:
Now I think that this patch should be reworked in a style closer to
what has been done in 1f3b9bb109b8 and b91f79cd08ab:
- Keep the variable declarations at the top of each function. For
example algo_type is shared between the pre-3.0 block and the post-3.0
block. No need for two declarations.
- The #if parts work as the way as the curly brackets, let's remove
one level of indentation. Your patch makes the whole diff harder to
parse and the pre-3.0 code is still the same.
- Minimization of the diffs by planting more #if blocks. Here I am
looking at the EVP_MD_free() calls. Let's minimize the duplicated
libpq_append_conn_error() and elog(ERROR) calls in the final result.
As this part was itching me, I have taken a shot at simplifying the
patch, and it looks much better once adapted among these lines. The
EVP_MD_free() feel slightly annoying, but they're isolated enough that
they don't matter to me. A second thing is the const marker for
EVP_MD, which avoids some casts or some unconstify(). At the end that
feels like the best thing to do.
Attached is the refined version. Comments and/or objections?
--
Michael
On Thu, Aug 13, 2026 at 10:06:19AM +0900, Michael Paquier wrote:
As this part was itching me, I have taken a shot at simplifying the
patch, and it looks much better once adapted among these lines. The
EVP_MD_free() feel slightly annoying, but they're isolated enough that
they don't matter to me. A second thing is the const marker for
EVP_MD, which avoids some casts or some unconstify(). At the end that
feels like the best thing to do.
And applied this one as 28995f051e72, which should be the last piece..
--
Michael
On Fri, Aug 14, 2026 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Aug 13, 2026 at 10:06:19AM +0900, Michael Paquier wrote:
As this part was itching me, I have taken a shot at simplifying the
patch, and it looks much better once adapted among these lines. The
EVP_MD_free() feel slightly annoying, but they're isolated enough that
they don't matter to me. A second thing is the const marker for
EVP_MD, which avoids some casts or some unconstify(). At the end that
feels like the best thing to do.And applied this one as 28995f051e72, which should be the last piece..
Thanks for working on this!
+ algo_name = OBJ_nid2sn(algo_nid);
+ if (algo_name == NULL)
+ elog(ERROR, "could not find digest for NID %s",
+ OBJ_nid2sn(algo_nid));+ algo_name = OBJ_nid2sn(algo_nid);
+ if (algo_name == NULL)
+ {
+ libpq_append_conn_error(conn, "could not find digest for NID %s",
+ OBJ_nid2sn(algo_nid));In these cases, algo_name is NULL, so OBJ_nid2sn(algo_nid) is also
NULL. That means NULL would be passed to %s in both elog() and
libpq_append_conn_error().
Isn't it better to use %d and pass algo_nid instead?
Regards,
--
Fujii Masao
On Fri, Aug 14, 2026 at 09:23:28AM +0900, Fujii Masao wrote:
In these cases, algo_name is NULL, so OBJ_nid2sn(algo_nid) is also
NULL. That means NULL would be passed to %s in both elog() and
libpq_append_conn_error().
And that's a very stupid thing to do. Not sure what I was thinking
here..
Isn't it better to use %d and pass algo_nid instead?
And that would be a better thing to do yes. It's a nice copy-pasto
from the pre-3.0 path, which is not wrong as we derive the error from
a EVP_get_digestbynid().
Switching to the NID number in the report is the right call, will fix.
--
Michael