Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2

> On 24 Nov 2020, at 11:52, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> I got to look at your suggestion, and finished with the attached which
> is pretty close my previous set, except that MSVC scripts as well as
> the header includes needed a slight refresh.

Looks good, nothing major sticks out. I'm not excited about all the
allocations needed here now, but it seems the only optipn.

> Please note that the OpenSSL docs tell that EVP_DigestInit() is
> obsolete and that applications should just use EVP_DigestInit_ex(), so
> I have kept the original:
> https://www.openssl.org/docs/man1.1.1/man3/EVP_DigestInit.html

Fair enough.

> What do you think?

A few comments in no particular order:

+ if (scram_HMAC_init(&ctx, state->ServerKey, SCRAM_KEY_LEN) < 0 ||
+ scram_HMAC_update(&ctx,
+ state->client_first_message_bare,
..snip..
+ scram_HMAC_final(ServerSignature, &ctx) < 0)
+ {
+ elog(ERROR, "could not calculate server signature");
+ }
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.

+ * 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.

+ 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?

+ /* Ditto for EVP contexts */
+ while (ResourceArrayGetAny(&(owner->evparr), &foundres))
+ {
+ if (isCommit)
+ PrintEVPLeakWarning(foundres);
+#ifdef USE_OPENSSL
+ EVP_MD_CTX_destroy((EVP_MD_CTX *) DatumGetPointer(foundres));
+#endif
+ ResourceOwnerForgetEVP(owner, foundres);
+ }
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.

cheers ./daniel