Thread: SSL passphrase prompt external command
Here is a patch that adds a way to specify an external command for obtaining SSL passphrases. There is a new GUC setting ssl_passphrase_command. Right now, we rely on the OpenSSL built-in prompting mechanism, which doesn't work in some situations, including under systemd. This patch allows a configuration to make that work, e.g., with systemd-ask-password. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Thu, Feb 22, 2018 at 10:14 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Here is a patch that adds a way to specify an external command for > obtaining SSL passphrases. There is a new GUC setting > ssl_passphrase_command. > > Right now, we rely on the OpenSSL built-in prompting mechanism, which > doesn't work in some situations, including under systemd. This patch > allows a configuration to make that work, e.g., with systemd-ask-password. I have not reviewed the patch, but count me as an enthusiastic +1 for the concept. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Feb 23, 2018 at 08:16:12AM -0500, Robert Haas wrote: > On Thu, Feb 22, 2018 at 10:14 PM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> Here is a patch that adds a way to specify an external command for >> obtaining SSL passphrases. There is a new GUC setting >> ssl_passphrase_command. >> >> Right now, we rely on the OpenSSL built-in prompting mechanism, which >> doesn't work in some situations, including under systemd. This patch >> allows a configuration to make that work, e.g., with systemd-ask-password. > > I have not reviewed the patch, but count me as an enthusiastic +1 for > the concept. +1 as well, as someone who actually began looking at the patch :) -- Michael
Attachment
> On 23 Feb 2018, at 11:14, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > Here is a patch that adds a way to specify an external command for > obtaining SSL passphrases. There is a new GUC setting > ssl_passphrase_command. +1 on going down this route. > Right now, we rely on the OpenSSL built-in prompting mechanism, which > doesn't work in some situations, including under systemd. This patch > allows a configuration to make that work, e.g., with systemd-ask-password. + replaced by a prompt string. (Write <literal>%%</literal> for a + literal <literal>%</literal>.) Note that the prompt string will I might be thick, but I don’t see where the %% handled? Also, AFAICT a string ending with %\0 will print a literal % without requiring %% (which may be a perfectly fine case to allow, depending on how strict we want to be with the format). cheers ./daniel
On 2/26/18 01:32, Daniel Gustafsson wrote: > + replaced by a prompt string. (Write <literal>%%</literal> for a > + literal <literal>%</literal>.) Note that the prompt string will > > I might be thick, but I don’t see where the %% handled? Ah, I had broken that in my submitted patch. New patch attached. > Also, AFAICT a string > ending with %\0 will print a literal % without requiring %% (which may be a > perfectly fine case to allow, depending on how strict we want to be with the > format). That is intentional. I made the behavior match that of archive_command, restore_command, etc. It's note quite how printf works, but we might as well stick with what we already have. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
First: thanks a lot for hacking on the SSL code, I might be biased but I really
appreciate it!
The patch no longer applies due to ff18115ae9 and f96f48113f, but the conflicts
are trivial so nothing to worry about there. Documentation exist and reads
well, the added tests pass and seem quite reasonable.
A few comments:
* In src/backend/libpq/be-secure-common.c:
+int
+run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
+{
[..]
+ size_t len = 0;
[..]
+ return len;
+}
run_ssl_passphrase_command() is defined to return a signed int but returns
size_t which is unsigned. It’s obviously a remote cornercase to hit, but
wouldn’t it be better to return the same signedness?
* The documentation for the passphrase command format states:
"If the setting starts with -, then it will be ignored during a reload and
the SSL configuration will not be reloaded if a passphrase is needed."
In run_ssl_passphrase_command() the leading ‘-‘ is however just shifted away
regardless of the state of is_server_start.
+ p = ssl_passphrase_command;
+
+ if (p[0] == '-')
+ p++;
+
The OpenSSL code is instead performing this in be_tls_init() in the below hunk:
+ if (ssl_passphrase_command[0] && ssl_passphrase_command[0] != '-')
In order to make this generic, shouldn’t we in run_ssl_passphrase_command() do
something along the lines of the below to ensure we aren’t executing the
passphrase callback during reloads?
if (p[0] == '-')
{
/*
* passphrase commands with a leading '-' are only invoked on server
* start, and are ignored on reload.
*/
if (!is_server_start)
goto error;
p++;
}
Unless I’m misunderstanding how the leading dash is supposed to work.
* In src/tools/msvc/Mkvcbuild.pm
# if building without OpenSSL
if (!$solution->{options}->{openssl})
{
+ $postgres->RemoveFile('src/backend/libpq/be-secure-common.c');
$postgres->RemoveFile('src/backend/libpq/be-secure-openssl.c');
}
Is this because run_ssl_passphrase_command() would otherwise generate a warning
due to not being used? Since we will need the file for future SChannel support
(should such a patch materialize), we might as well build it if we can, or at
least add a comment about it needing to be removed from this block.
This patch is further removing our dependency on OpenSSL for SSL code, which is
a direction I support. Putting on my Secure Transport patch-author hat, I
definitely want this feature to be able to use passphrase protected Keychains.
I’m marking this as waiting on author due to my question above, with that
cleared up I am very much +1’ing this to go in.
cheers ./daniel
On 3/15/18 12:13, Daniel Gustafsson wrote:
> * In src/backend/libpq/be-secure-common.c:
>
> +int
> +run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
> +{
> [..]
> + size_t len = 0;
> [..]
> + return len;
> +}
>
> run_ssl_passphrase_command() is defined to return a signed int but returns
> size_t which is unsigned. It’s obviously a remote cornercase to hit, but
> wouldn’t it be better to return the same signedness?
The OpenSSL (an GnuTLS) callbacks return an int, so we need to convert
it to int at some point anyway. Might as well make it here.
> * The documentation for the passphrase command format states:
>
> "If the setting starts with -, then it will be ignored during a reload and
> the SSL configuration will not be reloaded if a passphrase is needed."
>
> In run_ssl_passphrase_command() the leading ‘-‘ is however just shifted away
> regardless of the state of is_server_start.
>
> + p = ssl_passphrase_command;
> +
> + if (p[0] == '-')
> + p++;
> +
>
> The OpenSSL code is instead performing this in be_tls_init() in the below hunk:
>
> + if (ssl_passphrase_command[0] && ssl_passphrase_command[0] != '-')
>
> In order to make this generic, shouldn’t we in run_ssl_passphrase_command() do
> something along the lines of the below to ensure we aren’t executing the
> passphrase callback during reloads?
>
> if (p[0] == '-')
> {
> /*
> * passphrase commands with a leading '-' are only invoked on server
> * start, and are ignored on reload.
> */
> if (!is_server_start)
> goto error;
> p++;
> }
We need the OpenSSL code to know whether the callback supports reloads,
so it can call the dummy callback if not.
Maybe this is a bit too cute. We could instead add another setting
"ssl_passphrase_command_support_reload".
> * In src/tools/msvc/Mkvcbuild.pm
>
> # if building without OpenSSL
> if (!$solution->{options}->{openssl})
> {
> + $postgres->RemoveFile('src/backend/libpq/be-secure-common.c');
> $postgres->RemoveFile('src/backend/libpq/be-secure-openssl.c');
> }
>
> Is this because run_ssl_passphrase_command() would otherwise generate a warning
> due to not being used?
I have no idea. ;-) It's the same thing I was told to do for
fe-secure-common.c a while back.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 16 Mar 2018, at 17:07, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 3/15/18 12:13, Daniel Gustafsson wrote:
>> * The documentation for the passphrase command format states:
>>
>> "If the setting starts with -, then it will be ignored during a reload and
>> the SSL configuration will not be reloaded if a passphrase is needed."
>>
>> In run_ssl_passphrase_command() the leading ‘-‘ is however just shifted away
>> regardless of the state of is_server_start.
>>
>> + p = ssl_passphrase_command;
>> +
>> + if (p[0] == '-')
>> + p++;
>> +
>>
>> The OpenSSL code is instead performing this in be_tls_init() in the below hunk:
>>
>> + if (ssl_passphrase_command[0] && ssl_passphrase_command[0] != '-')
>>
>> In order to make this generic, shouldn’t we in run_ssl_passphrase_command() do
>> something along the lines of the below to ensure we aren’t executing the
>> passphrase callback during reloads?
>>
>> if (p[0] == '-')
>> {
>> /*
>> * passphrase commands with a leading '-' are only invoked on server
>> * start, and are ignored on reload.
>> */
>> if (!is_server_start)
>> goto error;
>> p++;
>> }
>
> We need the OpenSSL code to know whether the callback supports reloads,
> so it can call the dummy callback if not.
It makes sense for the OpenSSL code to know, I’m mostly wondering why
run_ssl_passphrase_command() isn’t doing the same thing wrt the leading ‘-‘?
ISTM that it should consider is_server_start as well to match the
documentation.
> Maybe this is a bit too cute. We could instead add another setting
> "ssl_passphrase_command_support_reload”.
I think thats a good idea, this feels like an easy thing to be confused about
and get wrong (which I might have done with the above).
Either way, there is a clear path forward on this (the new setting or just
ignoring me due to being confused) so I am going to mark this ready for
committer as the remaining work is known and small.
cheers ./daniel
On 3/16/18 12:38, Daniel Gustafsson wrote: >> Maybe this is a bit too cute. We could instead add another setting >> "ssl_passphrase_command_support_reload”. > I think thats a good idea, this feels like an easy thing to be confused about > and get wrong (which I might have done with the above). > > Either way, there is a clear path forward on this (the new setting or just > ignoring me due to being confused) so I am going to mark this ready for > committer as the remaining work is known and small. Committed with that change. Thanks! -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 16, 2018 at 12:07:59PM -0400, Peter Eisentraut wrote:
> On 3/15/18 12:13, Daniel Gustafsson wrote:
>> * In src/tools/msvc/Mkvcbuild.pm
>>
>> # if building without OpenSSL
>> if (!$solution->{options}->{openssl})
>> {
>> + $postgres->RemoveFile('src/backend/libpq/be-secure-common.c');
>> $postgres->RemoveFile('src/backend/libpq/be-secure-openssl.c');
>> }
>>
>> Is this because run_ssl_passphrase_command() would otherwise generate a warning
>> due to not being used?
>
> I have no idea. ;-) It's the same thing I was told to do for
> fe-secure-common.c a while back.
I don't recall saying exactly that either :)
You need to filter out OpenSSL-only code when MSVC builds without
OpenSSL because it includes all files listed in a path by default. No
need to make compilation include files which are not necessary. This
change is correct by the way.
--
Michael