> On 13 Nov 2020, at 04:14, Michael Paquier <mich...@paquier.xyz> wrote:
> I got to think more about this stuff and attached is a new patch set > that redesigns the generic interface used for the crypto hash > functions I've spent some time on this, and the gist of the patchset is clearly something we want to do: move to the EVP API and ensure that we don't drop any contexts. IIUC, this patchset moves the allocation of the context inside the API rather than having the caller be responsible for providing the memory (and thus be able to use the stack). This in turn changes the API to returning int rather than being void. As an effect of this we get the below types of statements that aren't easy on the eyes: + /* + * Calculate ClientSignature. Note that we don't log directly a failure + * here even when processing the calculations as this could involve a mock + * authentication. + */ + if (scram_HMAC_init(&ctx, state->StoredKey, SCRAM_KEY_LEN) < 0 || + scram_HMAC_update(&ctx, + state->client_first_message_bare, + strlen(state->client_first_message_bare)) < 0 || + scram_HMAC_update(&ctx, ",", 1) < 0 || + scram_HMAC_update(&ctx, + state->server_first_message, + strlen(state->server_first_message)) < 0 || + scram_HMAC_update(&ctx, ",", 1) < 0 || + scram_HMAC_update(&ctx, + state->client_final_message_without_proof, + strlen(state->client_final_message_without_proof)) < 0 || + scram_HMAC_final(ClientSignature, &ctx) < 0) + elog(ERROR, "could not calculate client signature"); This seems like a complication which brings little benefit since the only real errorcase is OOM in creating the context? The built-in crypto support is designed to never fail, and reading the OpenSSL code the only failure cases are ENGINE initialization (which we don't do) and memory allocation. Did you consider using EVP_MD_CTX_init instead which place the memory allocation responsibility with the caller? Keeping this a void API and leaving the caller to decide on memory allocation would make the API a lot simpler IMHO. Are there other error cases and/or errorpaths you're considering that I'm missing here? If it's just OOM on allocation it seems a high price to pay. +#ifndef _PG_CRYPTOHASH_H_ +#define _PG_CRYPTOHASH_H_ This should probably be CRYPTOHASH_H to be consistent? + status = EVP_DigestInit_ex((EVP_MD_CTX *) ctx->data, + EVP_sha224(), NULL); Since we always use the default implementation and never select an ENGINE, why not use EVP_DigestInit instead? +/* Include internals of each implementation here */ +#include "sha2.c" Do we really want to implement this by including a .c file? Are there no other options you see? > I think that we could also rename the existing cryptohashes.c to > crypohashfuncs.c to be more consistent, but I have left that out for now. +1. That can be done as a separate patch submission though as it's unrelated. cheers ./daniel
