On Mon, Dec 14, 2020 at 06:06:15PM -0500, Bruce Momjian wrote: > I am getting close to applying these patches, probably this week. The > patches are cumulative: > > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff > > https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff
I strongly object to a commit based on the current state of the patch. Based on my lookup of the patches you are referring to, I see a couple of things that should be splitted up and refactored properly before thinking about the core part of the patch (FWIW, I don't have much thoughts to offer about the core part because I haven't really thought about it, but it does not prevent to do a correct refactoring). Here are some notes: - The code lacks a lot of comments IMO. Why is retrieve_passphrase() doing what it does? It seems to me that pg_altercpass needs a large brushup. - There are no changes to src/tools/msvc/. Seeing the diffs in src/common/, this stuff breaks Windows builds. - The HMAC split is terrible, as mentioned upthread. I think that you would need to extract this stuff as a separate patch, and not add more mess to the existing mess (SCRAM uses its own HMAC with SHA256, which is already wrong). We can and should have a fallback implementation, because that's easy to do. And we need to have the fallback implementation depend on the contents of cryptohash.c (in a src/common/hmac.c), while the OpenSSL portion requires a hmac_openssl.c where you can choose the hash type based on pg_cryptohash_type. So ossl_HMAC_SHA512() does not do the right thing. APIs flexible enough to allow a new SSL library to plug into this stuff are required. - Not much a fan of the changes done in cryptohash.c for the resource owners as well. It also feels like this could be thought as something directly for resowner.c. - The cipher split also should be done in its own patch, and reviewed on its own. There is a mix of dependencies between non-OpenSSL and OpenSSL code paths, making the pluggin of a new SSL library harder to do. In short, this requires proper API design, which is not something we have here. There are also no changes in pgcrypto for that stuff. > I do have a few questions: That looks like a lot of things to sort out as well. > Can anyone test this on Windows, particularly -R handling? > > What testing infrastructure should this have? Using TAP tests would be adapted for those two points. > There are a few shell script I should include to show how to create > commands. Where should they be stored? /contrib module? Why are these needed. Is it a matter of documentation? > Are people okay with having the feature enabled, but invisible > since the docs and --help output are missing? When we enable > ssl_passphrase_command to prompt from the terminal, some of the > command-line options will be useful. You are suggesting to enable the feature by default, make it invisible to the users and leave it undocumented? Is there something I missing here? > Do people like the command-letter choices? > > I called the alter passphrase utility pg_altercpass. I could > have called it pg_clusterpass, but I wanted to highlight it is > only for changing the passphrase, not for creating them. I think that you should attach such patches directly to the emails sent to pgsql-hackers, if those github links disappear for some reason, it would become impossible to refer to see what has been discussed here. +/* + * We have to use postgres.h not postgres_fe.h here, because there's so much + * backend-only stuff in the kmgr include files we need. But we need a + * frontend-ish environment otherwise. Hence this ugly hack. + */ +#define FRONTEND 1 + +#include "postgres.h" I would advise to really understand why this happens and split up the backend and frontend parts cleanly. This style ought to be avoided as much as possible. @@ -95,9 +101,9 @@ pg_cryptohash_create(pg_cryptohash_type type) if (state->evpctx == NULL) { +#ifndef FRONTEND explicit_bzero(state, sizeof(pg_cryptohash_state)); explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); -#ifndef FRONTEND I think that this change is incorrect. Any clean up of memory should be done for the frontend *and* the backend. +#ifdef USE_OPENSSL + ctx = (PgCipherCtx *) palloc0(sizeof(PgCipherCtx)); + + ctx->encctx = ossl_cipher_ctx_create(cipher, key, klen, true); + ctx->decctx = ossl_cipher_ctx_create(cipher, key, klen, false); +#endif There is a cipher_openssl.c and a cipher.c that includes USE_OPENSSL. This requires a cleaner split IMO. We should avoid as much as possible OpenSSL-specific code paths in files part of src/common/ when not building with OpenSSL. So this is now a mixed bag of dependencies. -- Michael
signature.asc
Description: PGP signature