From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2 |
Date: | 2020-11-30 13:13:02 |
Message-ID: | X8Tv3jCwi3oaYI77@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Nov 30, 2020 at 01:43:24PM +0100, Daniel Gustafsson wrote:
> Looks good, nothing major sticks out.
Thanks for the review.
> I'm not excited about all the allocations needed here now, but it
> seems the only option.
Yeah, OpenSSL forces a bit our hand here I am afraid..
> Some of these long if-statements omit the {} around the elog, while some (like
> this one) don't. I think it makes sense from a readability POV to use the
> braces.
Agreed.
> + * pg_cryptohash_create
> + *
> + * Allocate a hash context. Returns NULL on failure.
> This comment should perhaps be amended to specify that it returns NULL on
> failure in the frontend, in the backend it won't return on error.
Okay. Let's be precise.
> + case PG_SHA224:
> + pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
> + break;
> + case PG_SHA256:
> + pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
> + break;
> Would codepaths like these become more readable if pg_cryptohash_ctx used a
> union with shaxxx_ctx's rather than a void pointer for the data part?
Hmm. I am not sure that this would help much, because we'd still need
to cast to the local structures in this case as the pg_shaXXX_ctx are
local to sha2.c, and we still need to keep some void* in
cryptohash.h.
> Since the cryptohash support is now generalized behind an abstraction layer,
> wouldn't it make sense to roll the resource ownership there as well kind of
> like how JIT is handled? It would make it easier to implement TLS backend
> support, and we won't have to inject OpenSSL headers here.
So, you are referring here about using a new API in the abstraction
layer. This makes sense. What about naming that
pg_cryptohash_context_free(void *)?
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2020-11-30 13:14:30 | Re: proposal: unescape_text function |
Previous Message | Alexander Korotkov | 2020-11-30 13:12:29 | Re: [HACKERS] [PATCH] Generic type subscripting |