From: | Bruce Momjian <bruce(at)momjian(dot)us> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> |
Subject: | Re: Proposed patch for key managment |
Date: | 2020-12-05 03:32:45 |
Message-ID: | 20201205033245.GB8757@momjian.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Dec 5, 2020 at 11:39:18AM +0900, Michael Paquier wrote:
> On Fri, Dec 04, 2020 at 09:08:03PM -0500, Bruce Momjian wrote:
> > Here is an updated patch to handle the new hash API introduced by
> > commit 87ae9691d2.
>
> + if (!ossl_initialized)
> + {
> +#ifdef HAVE_OPENSSL_INIT_SSL
> + OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
> +#else
> + OPENSSL_config(NULL);
> + SSL_library_init();
> + SSL_load_error_strings();
> +#endif
> + ossl_initialized = true;
> This is a duplicate of what's done in be-secure-openssl.c, and it does
> not strike me as a good idea to do that potentially twice.
Yeah, I kind of wondered about that. In fact, the code from the
original patch would not compile so I got this init code from somewhere
else. I have now removed it and it works fine. :-)
> git diff --check complains.
Uh, can you be more specific? I don't see any output from that command.
> +extern bool pg_HMAC_SHA512(const uint8 *key,
> + const uint8 *in, int inlen,
> + uint8 *out);
> I think that the split done in this patch makes the HMAC handling in
> the core code messier:
> - SCRAM makes use of HMAC internally, and we should try to use the
> HMAC of OpenSSL if building with it even for SCRAM.
> - For the first reason, I think that we should also have a fallback
> implementation.
> - This API layer should not depend directly on the SHA2 used (SCRAM
> uses SHA256 with HMAC).
> FWIW, I got plans to work on that once I am done with the business
> around MD5 and OpenSSL.
Uh, I just kind of kept all that code and didn't modify it. It would be
great if you can help me improve it. I will be using the hash code for
the command-line tool that alters the passphrase, so having that in
common/ does help me.
> The refactoring done with the ciphers moved from pgcrypto to
> src/common/ should be a separate patch. In short, it would be good to
Uh, I am kind of unclear exactly what was done there since I just took
that part of the patch unchanged.
> rework this patch and split it into pieces that are independently
> useful. This would make the review much easier as well.
I can break out the -R/file descriptor passing part as a separate patch,
and have the ssl_passphrase_command use that, but that's the only part I
know can be useful on its own.
Since the patch is large, I found a way to push the branch to git and
how to make a download link that tracks whatever I push to the 'key'
branch on my github account. Here is the updated patch link:
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
From | Date | Subject | |
---|---|---|---|
Next Message | Masahiko Sawada | 2020-12-05 03:38:12 | Re: Add Information during standby recovery conflicts |
Previous Message | Masahiko Sawada | 2020-12-05 03:15:13 | Re: Proposed patch for key managment |