Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Provide a TLS init hook

  1. ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-10-31T15:37:04Z

    This is the first of a number of patches to enhance SSL functionality,
particularly w.r.t. passphrases.


This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module. In
order for that to be effective, the startup order is modified slightly.
There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's it
before supplying the result as the passphrase.


cheers


andrew




-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: ssl passphrase callback

    Robert Haas <robertmhaas@gmail.com> — 2019-11-01T15:01:15Z

    On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> This patch provides a hook for a function that can supply an SSL
> passphrase. The hook can be filled in by a shared preloadable module. In
> order for that to be effective, the startup order is modified slightly.
> There is a test attached that builds and uses one trivial
> implementation, which just takes a configuration setting and rot13's it
> before supplying the result as the passphrase.

It seems to me that it would be a lot better to have an example in
contrib that does something which might be of actual use to users,
such as running a shell command and reading the passphrase from
stdout.

Features that are only accessible by writing C code are, in general,
not as desirable as features which can be accessed via SQL or
configuration.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-01T17:57:29Z

    On 11/1/19 11:01 AM, Robert Haas wrote:
> On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
>> This patch provides a hook for a function that can supply an SSL
>> passphrase. The hook can be filled in by a shared preloadable module. In
>> order for that to be effective, the startup order is modified slightly.
>> There is a test attached that builds and uses one trivial
>> implementation, which just takes a configuration setting and rot13's it
>> before supplying the result as the passphrase.
> It seems to me that it would be a lot better to have an example in
> contrib that does something which might be of actual use to users,
> such as running a shell command and reading the passphrase from
> stdout.
>
> Features that are only accessible by writing C code are, in general,
> not as desirable as features which can be accessed via SQL or
> configuration.
>


Well, I tried to provide the most trivial and simple test I could come
up with. Running a shell command can already be accomplished via the
ssl_passphrase_command setting.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: ssl passphrase callback

    Thomas Munro <thomas.munro@gmail.com> — 2019-11-04T21:43:45Z

    On Sat, Nov 2, 2019 at 6:57 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> On 11/1/19 11:01 AM, Robert Haas wrote:
> > On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
> > <andrew.dunstan@2ndquadrant.com> wrote:
> >> This patch provides a hook for a function that can supply an SSL
> >> passphrase. The hook can be filled in by a shared preloadable module. In
> >> order for that to be effective, the startup order is modified slightly.
> >> There is a test attached that builds and uses one trivial
> >> implementation, which just takes a configuration setting and rot13's it
> >> before supplying the result as the passphrase.
> > It seems to me that it would be a lot better to have an example in
> > contrib that does something which might be of actual use to users,
> > such as running a shell command and reading the passphrase from
> > stdout.
> >
> > Features that are only accessible by writing C code are, in general,
> > not as desirable as features which can be accessed via SQL or
> > configuration.
>
> Well, I tried to provide the most trivial and simple test I could come
> up with. Running a shell command can already be accomplished via the
> ssl_passphrase_command setting.

It looks like the new declarations in libpq-be.h are ifdef'd out in a
non-USE_SSL build, but then we still try to build the new test module
and it fails:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071

  5. Re: ssl passphrase callback

    Bruce Momjian <bruce@momjian.us> — 2019-11-07T01:23:56Z

    On Fri, Nov  1, 2019 at 01:57:29PM -0400, Andrew Dunstan wrote:
> 
> On 11/1/19 11:01 AM, Robert Haas wrote:
> > On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
> > <andrew.dunstan@2ndquadrant.com> wrote:
> >> This patch provides a hook for a function that can supply an SSL
> >> passphrase. The hook can be filled in by a shared preloadable module. In
> >> order for that to be effective, the startup order is modified slightly.
> >> There is a test attached that builds and uses one trivial
> >> implementation, which just takes a configuration setting and rot13's it
> >> before supplying the result as the passphrase.
> > It seems to me that it would be a lot better to have an example in
> > contrib that does something which might be of actual use to users,
> > such as running a shell command and reading the passphrase from
> > stdout.
> >
> > Features that are only accessible by writing C code are, in general,
> > not as desirable as features which can be accessed via SQL or
> > configuration.
> >
> 
> 
> Well, I tried to provide the most trivial and simple test I could come
> up with. Running a shell command can already be accomplished via the
> ssl_passphrase_command setting.

What is the value of a shared library over a shell command?  We had this
discussion in relation to archive_command years ago, and decided on a
shell command as the best API.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  6. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-07T17:30:50Z

    On 11/4/19 4:43 PM, Thomas Munro wrote:
>
> It looks like the new declarations in libpq-be.h are ifdef'd out in a
> non-USE_SSL build, but then we still try to build the new test module
> and it fails:
>
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071



I think this updated patch should fix things.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: ssl passphrase callback

    Simon Riggs <simon@2ndquadrant.com> — 2019-11-08T14:12:08Z

    On Thu, 7 Nov 2019 at 10:24, Bruce Momjian <bruce@momjian.us> wrote:


> What is the value of a shared library over a shell command?  We had this
> discussion in relation to archive_command years ago, and decided on a
> shell command as the best API.
>

I don't recall such a discussion, but I can give perspective:

* shell command offered the widest and simplest API for integration, which
was the most important consideration for a backup API. That choice caused
difficulty with the need to pass information to the external command, e.g.
%f %p

* shared library is more appropriate for a security-related module, so
users can't see how it is configured, as well as being more
tightly integrated so it can be better tailored to various uses

Summary is that the choice is not random, nor mere preference

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Solutions for the Enterprise

  8. Re: ssl passphrase callback

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-09T11:52:25Z

    On Fri, Nov 08, 2019 at 11:12:08PM +0900, Simon Riggs wrote:
>On Thu, 7 Nov 2019 at 10:24, Bruce Momjian <bruce@momjian.us> wrote:
>
>
>> What is the value of a shared library over a shell command?  We had
>> this discussion in relation to archive_command years ago, and decided
>> on a shell command as the best API.
>>
>
>I don't recall such a discussion, but I can give perspective:
>
>* shell command offered the widest and simplest API for integration,
>which was the most important consideration for a backup API. That
>choice caused difficulty with the need to pass information to the
>external command, e.g.  %f %p
>

It's not clear to me why simple API for integration would be less
valuable for this feature. Also, I'm sure passing data to/from shell
command may be tricky, but presumably we have figured how to do that.

>* shared library is more appropriate for a security-related module, so
>users can't see how it is configured, as well as being more
>tightly integrated so it can be better tailored to various uses
>

I don't follow. Why would there be a significant difference between a
shell command/script and shared library in this respect? If you don't
want the users to see the config, just store it in a separate file and
it's about the same as storing it in the .so library.

Is there something that can be done with a .so library but can't be done
with a shell command (which may just call a binary, with all the config
included, making it equal to the .so solution)?


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: ssl passphrase callback

    Magnus Hagander <magnus@hagander.net> — 2019-11-10T19:01:17Z

    On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:

> On Fri, Nov  1, 2019 at 01:57:29PM -0400, Andrew Dunstan wrote:
> >
> > On 11/1/19 11:01 AM, Robert Haas wrote:
> > > On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
> > > <andrew.dunstan@2ndquadrant.com> wrote:
> > >> This patch provides a hook for a function that can supply an SSL
> > >> passphrase. The hook can be filled in by a shared preloadable module.
> In
> > >> order for that to be effective, the startup order is modified
> slightly.
> > >> There is a test attached that builds and uses one trivial
> > >> implementation, which just takes a configuration setting and rot13's
> it
> > >> before supplying the result as the passphrase.
> > > It seems to me that it would be a lot better to have an example in
> > > contrib that does something which might be of actual use to users,
> > > such as running a shell command and reading the passphrase from
> > > stdout.
> > >
> > > Features that are only accessible by writing C code are, in general,
> > > not as desirable as features which can be accessed via SQL or
> > > configuration.
> > >
> >
> >
> > Well, I tried to provide the most trivial and simple test I could come
> > up with. Running a shell command can already be accomplished via the
> > ssl_passphrase_command setting.
>
> What is the value of a shared library over a shell command?


For one, platforms where shell commands are a lot less convenient, such as
Windows.



>   We had this
> discussion in relation to archive_command years ago, and decided on a
> shell command as the best API.
>
>
I don't recall that from back then, but that was a long time ago.

But it's interesting that you mention it, given the number of people I have
been discussing that with recently, under the topic of changing it from a
shell command into a shared library API (with there being a shell command
as one possible implementation of course).

One of the main reasons there being to be easily able to transfer more
state and give results other than just an exit code, no need to deal with
parameter escaping etc. Which probably wouldn't matter as much to an SSL
passphrase command, but still.

//Magnus

  10. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-11T19:19:44Z

    On 11/7/19 12:30 PM, Andrew Dunstan wrote:
> On 11/4/19 4:43 PM, Thomas Munro wrote:
>> It looks like the new declarations in libpq-be.h are ifdef'd out in a
>> non-USE_SSL build, but then we still try to build the new test module
>> and it fails:
>>
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071
>
>
> I think this updated patch should fix things.
>
>



This time with a typo fixed to keep the cfbot happy.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: ssl passphrase callback

    Bruce Momjian <bruce@momjian.us> — 2019-11-13T02:51:33Z

    On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:
>       We had this
>     discussion in relation to archive_command years ago, and decided on a
>     shell command as the best API.
>
> I don't recall that from back then, but that was a long time ago.
> 
> But it's interesting that you mention it, given the number of people I have
> been discussing that with recently, under the topic of changing it from a shell
> command into a shared library API (with there being a shell command as one
> possible implementation of course). 
> 
> One of the main reasons there being to be easily able to transfer more state
> and give results other than just an exit code, no need to deal with parameter
> escaping etc. Which probably wouldn't matter as much to an SSL passphrase
> command, but still.

I get the callback-is-easier issue with shared objects, but are we
expecting to pass in more information here than we do for
archive_command?  I would think not.  What I am saying is that if we
don't think passing things in works, we should fix all these external
commands, or something.   I don't see why ssl_passphrase_command is
different, except that it is new.  Or is it related to _securely_
passing something?

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  12. Re: ssl passphrase callback

    Bruce Momjian <bruce@momjian.us> — 2019-11-13T13:08:23Z

    On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
> On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:
> >       We had this
> >     discussion in relation to archive_command years ago, and decided on a
> >     shell command as the best API.
> >
> > I don't recall that from back then, but that was a long time ago.
> > 
> > But it's interesting that you mention it, given the number of people I have
> > been discussing that with recently, under the topic of changing it from a shell
> > command into a shared library API (with there being a shell command as one
> > possible implementation of course). 
> > 
> > One of the main reasons there being to be easily able to transfer more state
> > and give results other than just an exit code, no need to deal with parameter
> > escaping etc. Which probably wouldn't matter as much to an SSL passphrase
> > command, but still.
> 
> I get the callback-is-easier issue with shared objects, but are we
> expecting to pass in more information here than we do for
> archive_command?  I would think not.  What I am saying is that if we
> don't think passing things in works, we should fix all these external
> commands, or something.   I don't see why ssl_passphrase_command is
> different, except that it is new.  Or is it related to _securely_
> passing something?
> 
> Also, why was this patch posted without any discussion of these issues?
> Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  13. Re: ssl passphrase callback

    Simon Riggs <simon@2ndquadrant.com> — 2019-11-13T13:20:43Z

    On Wed, 13 Nov 2019 at 13:08, Bruce Momjian <bruce@momjian.us> wrote:

> On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
> > On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> > > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:
>
> > > One of the main reasons there being to be easily able to transfer more
> state
> > > and give results other than just an exit code, no need to deal with
> parameter
> > > escaping etc. Which probably wouldn't matter as much to an SSL
> passphrase
> > > command, but still.
> >
> > I get the callback-is-easier issue with shared objects, but are we
> > expecting to pass in more information here than we do for
> > archive_command?  I would think not.  What I am saying is that if we
> > don't think passing things in works, we should fix all these external
> > commands, or something.   I don't see why ssl_passphrase_command is
> > different, except that it is new.



> Or is it related to _securely_passing something?
>

Yes


> > Also, why was this patch posted without any discussion of these issues?
> > Shouldn't we ideally discuss the API first?
>
> I wonder if every GUC that takes an OS command should allow a shared
> object to be specified --- maybe control that if the command string
> starts with a # or something.
>

Very good idea

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Solutions for the Enterprise

  14. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-13T19:48:01Z

    On 11/13/19 8:08 AM, Bruce Momjian wrote:
>
>>
>> Also, why was this patch posted without any discussion of these issues?
>> Shouldn't we ideally discuss the API first?


This is a very tiny patch. I don't think it's unusual to post such
things without prior discussion. I would agree with your point if it
were thousands of lines instead of 20 or so lines of core code.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  15. Re: ssl passphrase callback

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-13T20:23:23Z

    On Wed, Nov 13, 2019 at 01:20:43PM +0000, Simon Riggs wrote:
>On Wed, 13 Nov 2019 at 13:08, Bruce Momjian <bruce@momjian.us> wrote:
>
>> On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
>> > On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
>> > > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:
>>
>> > > One of the main reasons there being to be easily able to transfer more
>> state
>> > > and give results other than just an exit code, no need to deal with
>> parameter
>> > > escaping etc. Which probably wouldn't matter as much to an SSL
>> passphrase
>> > > command, but still.
>> >
>> > I get the callback-is-easier issue with shared objects, but are we
>> > expecting to pass in more information here than we do for
>> > archive_command?  I would think not.  What I am saying is that if we
>> > don't think passing things in works, we should fix all these external
>> > commands, or something.   I don't see why ssl_passphrase_command is
>> > different, except that it is new.
>
>
>
>> Or is it related to _securely_passing something?
>>
>
>Yes
>

I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

>
>> > Also, why was this patch posted without any discussion of these issues?
>> > Shouldn't we ideally discuss the API first?
>>
>> I wonder if every GUC that takes an OS command should allow a shared
>> object to be specified --- maybe control that if the command string
>> starts with a # or something.
>>
>
>Very good idea
>

If it's about securely passing sensitive information (i.e. passphrase)
as was suggested, then I think that only applies to fairly small number
of GUCs.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  16. Re: ssl passphrase callback

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-13T20:34:38Z

    On Wed, Nov 13, 2019 at 02:48:01PM -0500, Andrew Dunstan wrote:
>
>On 11/13/19 8:08 AM, Bruce Momjian wrote:
>>
>>>
>>> Also, why was this patch posted without any discussion of these issues?
>>> Shouldn't we ideally discuss the API first?
>
>
>This is a very tiny patch. I don't think it's unusual to post such
>things without prior discussion. I would agree with your point if it
>were thousands of lines instead of 20 or so lines of core code.
>

Not sure that's really true. I think patches should provide some basic
explanation why the feature is desirable, no matter the number of lines.

Also, there were vague references to issues with passing parameters to
archive_command. A link to details, past discussion, or brief
explanation would be nice.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  17. Re: ssl passphrase callback

    Magnus Hagander <magnus@hagander.net> — 2019-11-14T10:42:05Z

    On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

> On Wed, Nov 13, 2019 at 01:20:43PM +0000, Simon Riggs wrote:
> >On Wed, 13 Nov 2019 at 13:08, Bruce Momjian <bruce@momjian.us> wrote:
> >
> >> On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:
> >> > On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:
> >> > > On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us>
> wrote:
> >>
> >> > > One of the main reasons there being to be easily able to transfer
> more
> >> state
> >> > > and give results other than just an exit code, no need to deal with
> >> parameter
> >> > > escaping etc. Which probably wouldn't matter as much to an SSL
> >> passphrase
> >> > > command, but still.
> >> >
> >> > I get the callback-is-easier issue with shared objects, but are we
> >> > expecting to pass in more information here than we do for
> >> > archive_command?  I would think not.  What I am saying is that if we
> >> > don't think passing things in works, we should fix all these external
> >> > commands, or something.   I don't see why ssl_passphrase_command is
> >> > different, except that it is new.
> >
> >
> >
> >> Or is it related to _securely_passing something?
> >>
> >
> >Yes
> >
>
> I think it would be beneficial to explain why shared object is more
> secure than an OS command. Perhaps it's common knowledge, but it's not
> quite obvious to me.
>

Yeah, that probably wouldn't hurt. It's also securely passing from more
than one perspective -- both from the "cannot be eavesdropped" (like
putting the password on the commandline for example) and the requirement
for escaping.


>
> >
> >> > Also, why was this patch posted without any discussion of these
> issues?
> >> > Shouldn't we ideally discuss the API first?
> >>
> >> I wonder if every GUC that takes an OS command should allow a shared
> >> object to be specified --- maybe control that if the command string
> >> starts with a # or something.
> >>
> >
> >Very good idea
> >
>
> If it's about securely passing sensitive information (i.e. passphrase)
> as was suggested, then I think that only applies to fairly small number
> of GUCs.
>

There aren't exactly a large number of GUCs that take OS commands in total.
Consistency itself certainly has some value.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  18. Re: ssl passphrase callback

    Sehrope Sarkuni <sehrope@jackdb.com> — 2019-11-14T13:54:27Z

    On Wed, Nov 13, 2019 at 3:23 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I think it would be beneficial to explain why shared object is more
> secure than an OS command. Perhaps it's common knowledge, but it's not
> quite obvious to me.

External command args can be viewed by other OS users (not just the
postgres user). For non-sensitive arguments (ex: WAL filename?) that's
not an issue but if you plan on passing in something potentially
secret value from the backend to the external OS command, that value
would be exposed:

Terminal 1 (run a command as some other user):
$ sudo -u nobody sleep 5

Terminal 2 (view command args as a different non-super user):
$ ps -u nobody -o args
COMMAND
sleep 5

A shared library would not have this problem as the backend directly
executes the library in the same process.

Has the idea of using environment variables (rather than command line
args) for external commands been brought up before? I couldn't find
anything in the mailing list archives.

Environment variables have the advantage of only being readable by the
process owner and super user. They also naturally have a "name" and do
not have escaping or quoting issues.

For example, archive_command %p could be exposed as
PG_ARG_ARCHIVE_PATH and %f could be exposed as
PG_ARG_ARCHIVE_FILENAME. Then you could have a script use them via:

#!/usr/bin/env bash
set -euo pipefail
main () {
    local archive_dir="/path/to/archive_dir/"
    local archive_file="${archive_dir}${PG_ARG_ARCHIVE_FILENAME}"
    test ! -f "${archive_file}" && cp -- "${PG_ARG_ARCHIVE_PATH}"
"${archive_file}"
}
main "$@"

It's not particularly useful for that basic archive case but if
there's something like PG_ARG_SUPER_SECRET then the executed command
could receive that value without it being exposed. That'd be useful
for something like a callout to an external KMS (key management
system).

Nothing stops something like this with being used in tandem with
string substitution to create the full commands. That'd give backwards
compatibility too. The main limitation compared to a shared library is
that you'd still have to explicitly pick and name the exposed argument
environment variables (i.e. like picking the set of %? substitutions).
If there's a generic shared-library-for-external-commands approach
then there could be a built-in library that provides this type of
"expose as env vars" functionality.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

  19. Re: ssl passphrase callback

    Bruce Momjian <bruce@momjian.us> — 2019-11-14T16:07:52Z

    On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
>     I think it would be beneficial to explain why shared object is more
>     secure than an OS command. Perhaps it's common knowledge, but it's not
>     quite obvious to me.
> 
> 
> Yeah, that probably wouldn't hurt. It's also securely passing from more than
> one perspective -- both from the "cannot be eavesdropped" (like putting the
> password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined.  By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  20. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-14T16:34:24Z

    On 11/14/19 11:07 AM, Bruce Momjian wrote:
> On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
>> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
>>     I think it would be beneficial to explain why shared object is more
>>     secure than an OS command. Perhaps it's common knowledge, but it's not
>>     quite obvious to me.
>>
>>
>> Yeah, that probably wouldn't hurt. It's also securely passing from more than
>> one perspective -- both from the "cannot be eavesdropped" (like putting the
>> password on the commandline for example) and the requirement for escaping.
> I think a bigger issue is that if you want to give people the option of
> using a shell command or a shared object, and if you use two commands to
> control it, it isn't clear what happens if both are defined.  By using
> some character prefix to control if a shared object is used, you can use
> a single variable and there is no confusion over having two variables
> and their conflicting behavior.
>


I'm  not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.


cheers


andrew



-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  21. Re: ssl passphrase callback

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-14T16:52:50Z

    On Thu, Nov 14, 2019 at 11:34:24AM -0500, Andrew Dunstan wrote:
>
>On 11/14/19 11:07 AM, Bruce Momjian wrote:
>> On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
>>> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
>>>     I think it would be beneficial to explain why shared object is more
>>>     secure than an OS command. Perhaps it's common knowledge, but it's not
>>>     quite obvious to me.
>>>
>>>
>>> Yeah, that probably wouldn't hurt. It's also securely passing from more than
>>> one perspective -- both from the "cannot be eavesdropped" (like putting the
>>> password on the commandline for example) and the requirement for escaping.
>> I think a bigger issue is that if you want to give people the option of
>> using a shell command or a shared object, and if you use two commands to
>> control it, it isn't clear what happens if both are defined.  By using
>> some character prefix to control if a shared object is used, you can use
>> a single variable and there is no confusion over having two variables
>> and their conflicting behavior.
>>
>
>
>I'm  not sure how that would work in the present instance. The shared
>preloaded module installs a function and defines the params it wants. If
>we somehow unify the params with ssl_passphrase_command that could look
>icky, and the module would have to parse the settings string. That's not
>a problem for the sample module which only needs one param, but it will
>be for other more complex implementations.
>
>I'm quite open to suggestions, but I want things to be tolerably clean.
>

I agree it's better to have two separate GUCs - one for command, one for
shared object, and documented order of precedence. I suppose we may log
a warning when both are specified, or perhaps "reset" the value with
lower order of precedence.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  22. Re: ssl passphrase callback

    Bruce Momjian <bruce@momjian.us> — 2019-11-14T16:53:09Z

    On Thu, Nov 14, 2019 at 11:34:24AM -0500, Andrew Dunstan wrote:
> 
> On 11/14/19 11:07 AM, Bruce Momjian wrote:
> > On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
> >> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
> >>     I think it would be beneficial to explain why shared object is more
> >>     secure than an OS command. Perhaps it's common knowledge, but it's not
> >>     quite obvious to me.
> >>
> >>
> >> Yeah, that probably wouldn't hurt. It's also securely passing from more than
> >> one perspective -- both from the "cannot be eavesdropped" (like putting the
> >> password on the commandline for example) and the requirement for escaping.
> > I think a bigger issue is that if you want to give people the option of
> > using a shell command or a shared object, and if you use two commands to
> > control it, it isn't clear what happens if both are defined.  By using
> > some character prefix to control if a shared object is used, you can use
> > a single variable and there is no confusion over having two variables
> > and their conflicting behavior.
> >
> 
> 
> I'm  not sure how that would work in the present instance. The shared
> preloaded module installs a function and defines the params it wants. If
> we somehow unify the params with ssl_passphrase_command that could look
> icky, and the module would have to parse the settings string. That's not
> a problem for the sample module which only needs one param, but it will
> be for other more complex implementations.
> 
> I'm quite open to suggestions, but I want things to be tolerably clean.

I was assuming if the variable starts with a #, it is a shared object,
if not, it is a shell command:

	ssl_passphrase_command='#/lib/x.so'
	ssl_passphrase_command='my_command a b c'

Can you show what you are talking about?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  23. Re: ssl passphrase callback

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-11-14T17:07:58Z

    On 2019-Nov-14, Bruce Momjian wrote:

> I was assuming if the variable starts with a #, it is a shared object,
> if not, it is a shell command:
> 
> 	ssl_passphrase_command='#/lib/x.so'
> 	ssl_passphrase_command='my_command a b c'

Note that the proposed patch doesn't use a separate GUC -- it just uses
shared_preload_libraries, and then it is the library that's in charge of
setting up the function.  We probably wouldn't like to have multiple
settings that all do the same thing, such as recovery target (which
seems to be a plentiful source of confusion).

Changing the interface so that the user has to specify the function name
(not the library name) in ssl_passphrase_command closes that ambiguity
hole.

Note that if you specify only the library name, it becomes redundant
w.r.t. shared_preload_libraries; you could have more than one library
setting the function callback and it's hard to see which one wins.

I think something like this would do it:
  ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

This way, the library can still create any custom GUCs it pleases/needs,
but there's no possible confusion as to the function that's going to be
called.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  24. Re: ssl passphrase callback

    Bruce Momjian <bruce@momjian.us> — 2019-11-14T17:15:44Z

    On Thu, Nov 14, 2019 at 02:07:58PM -0300, Alvaro Herrera wrote:
> On 2019-Nov-14, Bruce Momjian wrote:
> 
> > I was assuming if the variable starts with a #, it is a shared object,
> > if not, it is a shell command:
> > 
> > 	ssl_passphrase_command='#/lib/x.so'
> > 	ssl_passphrase_command='my_command a b c'
> 
> Note that the proposed patch doesn't use a separate GUC -- it just uses
> shared_preload_libraries, and then it is the library that's in charge of
> setting up the function.  We probably wouldn't like to have multiple
> settings that all do the same thing, such as recovery target (which
> seems to be a plentiful source of confusion).
> 
> Changing the interface so that the user has to specify the function name
> (not the library name) in ssl_passphrase_command closes that ambiguity
> hole.
> 
> Note that if you specify only the library name, it becomes redundant
> w.r.t. shared_preload_libraries; you could have more than one library
> setting the function callback and it's hard to see which one wins.
> 
> I think something like this would do it:
>   ssl_passphrase_command='#superlib.so,my_rot13_passphrase'
> 
> This way, the library can still create any custom GUCs it pleases/needs,
> but there's no possible confusion as to the function that's going to be
> called.

Yeah, I was unclear how the function name would be specified.  I thought
it would just be hard-coded, but I like the above better.  I am still
unclear how parameters are passed.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  25. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-14T19:29:23Z

    On 11/14/19 12:07 PM, Alvaro Herrera wrote:
> On 2019-Nov-14, Bruce Momjian wrote:
>
>> I was assuming if the variable starts with a #, it is a shared object,
>> if not, it is a shell command:
>>
>> 	ssl_passphrase_command='#/lib/x.so'
>> 	ssl_passphrase_command='my_command a b c'
> Note that the proposed patch doesn't use a separate GUC -- it just uses
> shared_preload_libraries, and then it is the library that's in charge of
> setting up the function.  We probably wouldn't like to have multiple
> settings that all do the same thing, such as recovery target (which
> seems to be a plentiful source of confusion).
>
> Changing the interface so that the user has to specify the function name
> (not the library name) in ssl_passphrase_command closes that ambiguity
> hole.
>
> Note that if you specify only the library name, it becomes redundant
> w.r.t. shared_preload_libraries; you could have more than one library
> setting the function callback and it's hard to see which one wins.
>
> I think something like this would do it:
>   ssl_passphrase_command='#superlib.so,my_rot13_passphrase'
>
> This way, the library can still create any custom GUCs it pleases/needs,
> but there's no possible confusion as to the function that's going to be
> called.


I guess this would work. There would have to be a deal of code to load
the library and lookup the symbol. Do we really think it's worth it?
Leveraging shared_preload_libraries makes this comparatively simple.


Also, calling this 'ssl_passphrase_command' seems a little odd.


A simpler way to handle it might be simply to error out and refuse to
start if both ssl_passphrase_function is set and ssl_passphrase_command
is set.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  26. Re: ssl passphrase callback

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-11-14T20:21:51Z

    On 2019-Nov-14, Andrew Dunstan wrote:

> I guess this would work. There would have to be a deal of code to load
> the library and lookup the symbol. Do we really think it's worth it?
> Leveraging shared_preload_libraries makes this comparatively simple.

Using the generic interface has the drawback that the user can make more
mistakes.  I think that's part of Bruce's issue with it (although I may
misinterpret.)

I think if you add most of it as a new entry point in dfmgr.c (where you
can leverage internal_library_load) and returns a function pointer to
the user specified function, it's all that much additional code.

(I don't think you can use load_external_function as is, because it
assumes fmgr V1 calling convention, which I'm not sure serves your case.
But then maybe it does.  And if not, then those 10 lines should be very
similar to the code you'd need to add.)

> A simpler way to handle it might be simply to error out and refuse to
> start if both ssl_passphrase_function is set and ssl_passphrase_command
> is set.

Yeah, you can do that too I guess, but I'm not sure I see that as simpler.

> Also, calling this 'ssl_passphrase_command' seems a little odd.

We could just rename ssl_passphrase_command to something more
generic, and add the existing name to map_old_guc_names to preserve
compatibility with pg12.  Maybe the new name could be simply
ssl_passphrase or perhaps ssl_passphrase_{reader,getter,pinentry}.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  27. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-15T13:59:45Z

    On 11/14/19 3:21 PM, Alvaro Herrera wrote:
> On 2019-Nov-14, Andrew Dunstan wrote:
>
>> I guess this would work. There would have to be a deal of code to load
>> the library and lookup the symbol. Do we really think it's worth it?
>> Leveraging shared_preload_libraries makes this comparatively simple.
> Using the generic interface has the drawback that the user can make more
> mistakes.  I think that's part of Bruce's issue with it (although I may
> misinterpret.)
>
> I think if you add most of it as a new entry point in dfmgr.c (where you
> can leverage internal_library_load) and returns a function pointer to
> the user specified function, it's all that much additional code.
>
> (I don't think you can use load_external_function as is, because it
> assumes fmgr V1 calling convention, which I'm not sure serves your case.
> But then maybe it does.  And if not, then those 10 lines should be very
> similar to the code you'd need to add.)



In the absence of further comment I will try to code up something along
these lines.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  28. Re: ssl passphrase callback

    Craig Ringer <craig@2ndquadrant.com> — 2019-11-22T05:19:24Z

    On Fri, 15 Nov 2019 at 00:34, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
wrote:

>
> On 11/14/19 11:07 AM, Bruce Momjian wrote:
> > On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:
> >> On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <
> tomas.vondra@2ndquadrant.com>
> >>     I think it would be beneficial to explain why shared object is more
> >>     secure than an OS command. Perhaps it's common knowledge, but it's
> not
> >>     quite obvious to me.
> >>
> >>
> >> Yeah, that probably wouldn't hurt. It's also securely passing from more
> than
> >> one perspective -- both from the "cannot be eavesdropped" (like putting
> the
> >> password on the commandline for example) and the requirement for
> escaping.
> > I think a bigger issue is that if you want to give people the option of
> > using a shell command or a shared object, and if you use two commands to
> > control it, it isn't clear what happens if both are defined.  By using
> > some character prefix to control if a shared object is used, you can use
> > a single variable and there is no confusion over having two variables
> > and their conflicting behavior.
> >
>
>
> I'm  not sure how that would work in the present instance. The shared
> preloaded module installs a function and defines the params it wants. If
> we somehow unify the params with ssl_passphrase_command that could look
> icky, and the module would have to parse the settings string. That's not
> a problem for the sample module which only needs one param, but it will
> be for other more complex implementations.
>
> I'm quite open to suggestions, but I want things to be tolerably clean.


If someone wants a shell command wrapper, they can load a contrib that
delegates the hook to a shell command. So we can just ship a contrib, which
acts both as test coverage for the feature, and a shell-command-support
wrapper for anyone who desires that.


-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

  29. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-12-06T22:21:58Z

    On 11/15/19 8:59 AM, Andrew Dunstan wrote:
> On 11/14/19 3:21 PM, Alvaro Herrera wrote:
>> On 2019-Nov-14, Andrew Dunstan wrote:
>>
>>> I guess this would work. There would have to be a deal of code to load
>>> the library and lookup the symbol. Do we really think it's worth it?
>>> Leveraging shared_preload_libraries makes this comparatively simple.
>> Using the generic interface has the drawback that the user can make more
>> mistakes.  I think that's part of Bruce's issue with it (although I may
>> misinterpret.)
>>
>> I think if you add most of it as a new entry point in dfmgr.c (where you
>> can leverage internal_library_load) and returns a function pointer to
>> the user specified function, it's all that much additional code.
>>
>> (I don't think you can use load_external_function as is, because it
>> assumes fmgr V1 calling convention, which I'm not sure serves your case.
>> But then maybe it does.  And if not, then those 10 lines should be very
>> similar to the code you'd need to add.)


I've just been looking at that. load_external_function() doesn't
actually do anything V1-ish with the value, it just looks up the symbol
using dlsym and returns it cast to a PGFunction. Is there any reason I
can't just use that and cast it again to the callback function type?


cheers


andrew



-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  30. Re: ssl passphrase callback

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-06T23:20:54Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> I've just been looking at that. load_external_function() doesn't
> actually do anything V1-ish with the value, it just looks up the symbol
> using dlsym and returns it cast to a PGFunction. Is there any reason I
> can't just use that and cast it again to the callback function type?

TBH, I think this entire discussion has gone seriously off into the
weeds.  The original design where we just let a shared_preload_library
function get into a hook is far superior to any of the overcomplicated
kluges that are being discussed now.  Something like this, for instance:

>>> ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

makes me positively ill.  It introduces problems that we don't need,
like how to parse out the sub-parts of the string, and the
quoting/escaping issues that will come along with that; while from
the user's perspective it replaces a simple and intellectually-coherent
variable definition with an unintelligible mess.

			regards, tom lane

  31. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-12-07T00:32:32Z

    On 12/6/19 6:20 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> I've just been looking at that. load_external_function() doesn't
>> actually do anything V1-ish with the value, it just looks up the symbol
>> using dlsym and returns it cast to a PGFunction. Is there any reason I
>> can't just use that and cast it again to the callback function type?
> TBH, I think this entire discussion has gone seriously off into the
> weeds.  The original design where we just let a shared_preload_library
> function get into a hook is far superior to any of the overcomplicated
> kluges that are being discussed now.  Something like this, for instance:
>
>>>> ssl_passphrase_command='#superlib.so,my_rot13_passphrase'
> makes me positively ill.  It introduces problems that we don't need,
> like how to parse out the sub-parts of the string, and the
> quoting/escaping issues that will come along with that; while from
> the user's perspective it replaces a simple and intellectually-coherent
> variable definition with an unintelligible mess.
>
> 			



Yeah, you have a point.


Bruce was worried about what would happen if we defined both
ssl_passphrase_command and ssl_passphrase_callback. The submitted patch
let's the callback have precedence, but it might be cleaner to error out
with such a config. OTOH, that wouldn't be so nice on a reload, so it
might be better just to document the behaviour.


He was also worried that multiple shared libraries might try to provide
the hook. I think that's fairly fanciful, TBH. It comes into the
category of "Don't do that."


cheers


andrew



-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  32. Re: ssl passphrase callback

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-07T17:16:17Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> Bruce was worried about what would happen if we defined both
> ssl_passphrase_command and ssl_passphrase_callback. The submitted patch
> let's the callback have precedence, but it might be cleaner to error out
> with such a config. OTOH, that wouldn't be so nice on a reload, so it
> might be better just to document the behaviour.

I think it would be up to the extension that's using the hook to
decide what to do if ssl_passphrase_command is set.  It would not
be our choice, and it would certainly not fall to us to document it.

> He was also worried that multiple shared libraries might try to provide
> the hook. I think that's fairly fanciful, TBH. It comes into the
> category of "Don't do that."

Again, it's somebody else's problem.  We have plenty of hooks that
are of dubious use for multiple extensions, so why should this one be
held to a higher standard?

			regards, tom lane

  33. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-12-07T21:03:27Z

    On 12/7/19 12:16 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> Bruce was worried about what would happen if we defined both
>> ssl_passphrase_command and ssl_passphrase_callback. The submitted patch
>> let's the callback have precedence, but it might be cleaner to error out
>> with such a config. OTOH, that wouldn't be so nice on a reload, so it
>> might be better just to document the behaviour.
> I think it would be up to the extension that's using the hook to
> decide what to do if ssl_passphrase_command is set.  It would not
> be our choice, and it would certainly not fall to us to document it.
>
>> He was also worried that multiple shared libraries might try to provide
>> the hook. I think that's fairly fanciful, TBH. It comes into the
>> category of "Don't do that."
> Again, it's somebody else's problem.  We have plenty of hooks that
> are of dubious use for multiple extensions, so why should this one be
> held to a higher standard?
>
> 			


Well that pretty much brings us back to the patch as submitted :-)


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  34. Re: ssl passphrase callback

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-07T22:32:01Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> Well that pretty much brings us back to the patch as submitted :-)

Yeah, pretty nearly.  Taking a quick look over the v3 patch, my
only quibble is that it doesn't provide any convenient way for the
external module to make decisions about how to interact with
ssl_passphrase_command --- in particular, if it would like to allow
that to take precedence, it can't because there's no way for it to
invoke the static function ssl_external_passwd_cb.

But rather than expose that globally, maybe the theory ought to be
"set up the state as we'd normally do, then let loadable modules
choose to override it".  So I'm tempted to propose a hook function
with the signature

void openssl_tls_init_hook(SSL_CTX *context, bool isServerStart);

and invoke that somewhere in be_tls_init --- maybe fairly late,
so that it can override other settings if it wants, not only the
SSL_CTX_set_default_passwd_cb setting.

			regards, tom lane

  35. Re: ssl passphrase callback

    Craig Ringer <craig@2ndquadrant.com> — 2019-12-09T02:22:12Z

    On Sat, 7 Dec 2019 at 07:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> > I've just been looking at that. load_external_function() doesn't
> > actually do anything V1-ish with the value, it just looks up the symbol
> > using dlsym and returns it cast to a PGFunction. Is there any reason I
> > can't just use that and cast it again to the callback function type?
>
> TBH, I think this entire discussion has gone seriously off into the
> weeds.  The original design where we just let a shared_preload_library
> function get into a hook is far superior to any of the overcomplicated
> kluges that are being discussed now.  Something like this, for instance:
>
> >>> ssl_passphrase_command='#superlib.so,my_rot13_passphrase'
>
> makes me positively ill.  It introduces problems that we don't need,
> like how to parse out the sub-parts of the string, and the
> quoting/escaping issues that will come along with that; while from
> the user's perspective it replaces a simple and intellectually-coherent
> variable definition with an unintelligible mess.
>

+1000 from me on that.


-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

  36. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-01-22T07:02:01Z

    On Sun, Dec 8, 2019 at 9:02 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> > Well that pretty much brings us back to the patch as submitted :-)
>
> Yeah, pretty nearly.  Taking a quick look over the v3 patch, my
> only quibble is that it doesn't provide any convenient way for the
> external module to make decisions about how to interact with
> ssl_passphrase_command --- in particular, if it would like to allow
> that to take precedence, it can't because there's no way for it to
> invoke the static function ssl_external_passwd_cb.
>
> But rather than expose that globally, maybe the theory ought to be
> "set up the state as we'd normally do, then let loadable modules
> choose to override it".  So I'm tempted to propose a hook function
> with the signature
>
> void openssl_tls_init_hook(SSL_CTX *context, bool isServerStart);
>
> and invoke that somewhere in be_tls_init --- maybe fairly late,
> so that it can override other settings if it wants, not only the
> SSL_CTX_set_default_passwd_cb setting.
>


Not sure if the placement is what you want, but maybe something like this?

cheers

andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  37. Re: ssl passphrase callback

    Robert Haas <robertmhaas@gmail.com> — 2020-01-23T17:30:36Z

    On Thu, Nov 14, 2019 at 8:54 AM Sehrope Sarkuni <sehrope@jackdb.com> wrote:
> Has the idea of using environment variables (rather than command line
> args) for external commands been brought up before? I couldn't find
> anything in the mailing list archives.

Passing data through environment variables isn't secure. Try 'ps -E'
on MacOS, or something like 'ps axe' on Linux.

If we want to pass data securely to child processes, the way to do it
is via stdin. Data sent back and forth via file descriptors can't
easily be snooped by other users on the system.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  38. Re: ssl passphrase callback

    Thomas Munro <thomas.munro@gmail.com> — 2020-02-18T03:30:32Z

    On Wed, Jan 22, 2020 at 8:02 PM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> Not sure if the placement is what you want, but maybe something like this?

Hi Andrew, FYI this failed here:

t/001_testfunc.pl .. Bailout called. Further testing stopped: pg_ctl
start failed
FAILED--Further testing stopped: pg_ctl start failed
Makefile:23: recipe for target 'prove-check' failed

Unfortunately my robot is poorly trained and does not dump any of the
interesting logs for this case, but it looks like it's failing that
way every time.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/651756191

  39. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-02-18T20:40:20Z

    On Tue, Feb 18, 2020 at 2:01 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Wed, Jan 22, 2020 at 8:02 PM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
> > Not sure if the placement is what you want, but maybe something like this?
>
> Hi Andrew, FYI this failed here:
>
> t/001_testfunc.pl .. Bailout called. Further testing stopped: pg_ctl
> start failed
> FAILED--Further testing stopped: pg_ctl start failed
> Makefile:23: recipe for target 'prove-check' failed
>
> Unfortunately my robot is poorly trained and does not dump any of the
> interesting logs for this case, but it looks like it's failing that
> way every time.
>
> https://travis-ci.org/postgresql-cfbot/postgresql/builds/651756191


Thanks for letting me know, I will investigate.

cheers

andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  40. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-02-18T22:39:20Z

    On Wed, Feb 19, 2020 at 7:10 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
>
> On Tue, Feb 18, 2020 at 2:01 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> >
> > On Wed, Jan 22, 2020 at 8:02 PM Andrew Dunstan
> > <andrew.dunstan@2ndquadrant.com> wrote:
> > > Not sure if the placement is what you want, but maybe something like this?
> >
> > Hi Andrew, FYI this failed here:
> >
> > t/001_testfunc.pl .. Bailout called. Further testing stopped: pg_ctl
> > start failed
> > FAILED--Further testing stopped: pg_ctl start failed
> > Makefile:23: recipe for target 'prove-check' failed
> >
> > Unfortunately my robot is poorly trained and does not dump any of the
> > interesting logs for this case, but it looks like it's failing that
> > way every time.
> >
> > https://travis-ci.org/postgresql-cfbot/postgresql/builds/651756191
>
>
> Thanks for letting me know, I will investigate.
>


This should fix the issue, it happened when I switched to using a
pre-generated cert/key.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  41. Re: ssl passphrase callback

    Andreas Karlsson <andreas@proxel.se> — 2020-03-16T02:14:57Z

    On 2/18/20 11:39 PM, Andrew Dunstan wrote:
> This should fix the issue, it happened when I switched to using a
> pre-generated cert/key.

# Review

The patch still applies and passes the test suite, both with openssl 
enabled and with it disabled.

As for the feature I agree that it is nice to expose this callback to 
extension writers and I agree with the approach taken. The other 
proposals up-thread seem over engineered to me. Maybe if it was a 
general feature used in many places those proposals would be worth it, 
but I am still skeptical even then. This approach is so much simpler.

The only real risk I see is that if people install multiple libraries 
for this they will overwrite the hook for each other but we have other 
cases like that already so I think that is fine.

The patch moves secure_initialize() to after 
process_shared_preload_libraries() which in theory could break some 
extension but it does not seem like a likely thing for extensions to 
rely on. Or is it?

An idea would be to have the code in ssl_passphrase_func.c to warn if 
the ssl_passphrase_command GUC is set to make it more useful as example 
code for people implementing this hook.

# Nitpicking

The certificate expires in 2030 while all other certificates used in 
tests expires in 2046. Should we be consistent?

There is text in server.crt and server.key, while other certificates and 
keys used in the tests do not have this. Again, should we be consistent?

Empty first line in 
src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl which should 
probably just be removed or replaced with a shebang.

There is an extra space between the parentheses in the line below. Does 
that follow our code style for Perl?

+unless ( ($ENV{with_openssl} || 'no') eq 'yes')

Missing space after comma in:

+ok(-e "$ddir/postmaster.pid","postgres started");

Missing space after comma in:

+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");

Andreas

  42. RE: ssl passphrase callback

    asaba.takanori@fujitsu.com <asaba.takanori@fujitsu.com> — 2020-03-19T08:10:58Z

    Hello Andrew,

From: Andreas Karlsson <andreas@proxel.se>
> # Nitpicking
> 
> The certificate expires in 2030 while all other certificates used in
> tests expires in 2046. Should we be consistent?
> 
> There is text in server.crt and server.key, while other certificates and
> keys used in the tests do not have this. Again, should we be consistent?
> 
> Empty first line in
> src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl which should
> probably just be removed or replaced with a shebang.
> 
> There is an extra space between the parentheses in the line below. Does
> that follow our code style for Perl?
> 
> +unless ( ($ENV{with_openssl} || 'no') eq 'yes')
> 
> Missing space after comma in:
> 
> +ok(-e "$ddir/postmaster.pid","postgres started");
> 
> Missing space after comma in:
> 
> +ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
> 
> Andreas
> 

Trailing space:

220 +           X509v3 Subject Key Identifier:
222 +           X509v3 Authority Key Identifier:

Missing "d"(password?):

121 +/* init hook for SSL, the default sets the passwor callback if appropriate */

Regards,

--
Takanori Asaba

  43. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-03-21T13:15:53Z

    On 3/15/20 10:14 PM, Andreas Karlsson wrote:
> On 2/18/20 11:39 PM, Andrew Dunstan wrote:
>> This should fix the issue, it happened when I switched to using a
>> pre-generated cert/key.
>
> # Review
>
> The patch still applies and passes the test suite, both with openssl
> enabled and with it disabled.
>
> As for the feature I agree that it is nice to expose this callback to
> extension writers and I agree with the approach taken. The other
> proposals up-thread seem over engineered to me. Maybe if it was a
> general feature used in many places those proposals would be worth it,
> but I am still skeptical even then. This approach is so much simpler.
>
> The only real risk I see is that if people install multiple libraries
> for this they will overwrite the hook for each other but we have other
> cases like that already so I think that is fine.


Right, me too.


>
> The patch moves secure_initialize() to after
> process_shared_preload_libraries() which in theory could break some
> extension but it does not seem like a likely thing for extensions to
> rely on. Or is it?


I don't think so.


>
> An idea would be to have the code in ssl_passphrase_func.c to warn if
> the ssl_passphrase_command GUC is set to make it more useful as
> example code for people implementing this hook.


I'll look at that. Should be possible.


>
> # Nitpicking
>
> The certificate expires in 2030 while all other certificates used in
> tests expires in 2046. Should we be consistent?


Sure. will fix.


>
> There is text in server.crt and server.key, while other certificates
> and keys used in the tests do not have this. Again, should we be
> consistent?


Not in server.key, but I will suppress it for the crt file.



>
> Empty first line in
> src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl which
> should probably just be removed or replaced with a shebang.


OK


>
> There is an extra space between the parentheses in the line below.
> Does that follow our code style for Perl?
>
> +unless ( ($ENV{with_openssl} || 'no') eq 'yes')
>
> Missing space after comma in:
>
> +ok(-e "$ddir/postmaster.pid","postgres started");
>
> Missing space after comma in:
>
> +ok(! -e "$ddir/postmaster.pid","postgres not started with bad
> passphrase");


I'll make sure to run it through our perl indenter.


Thanks for the review.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  44. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-03-21T13:18:26Z

    On 3/19/20 4:10 AM, asaba.takanori@fujitsu.com wrote:



> Trailing space:
>
> 220 +           X509v3 Subject Key Identifier:
> 222 +           X509v3 Authority Key Identifier:


We're going to remove all the text, so this becomes moot.


>
> Missing "d"(password?):
>
> 121 +/* init hook for SSL, the default sets the passwor callback if appropriate */
>

Will fix, thanks.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  45. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-03-22T00:08:19Z

    On 3/21/20 9:18 AM, Andrew Dunstan wrote:
> On 3/19/20 4:10 AM, asaba.takanori@fujitsu.com wrote:
>
>
>
>> Trailing space:
>>
>> 220 +           X509v3 Subject Key Identifier:
>> 222 +           X509v3 Authority Key Identifier:
>
> We're going to remove all the text, so this becomes moot.
>
>
>> Missing "d"(password?):
>>
>> 121 +/* init hook for SSL, the default sets the passwor callback if appropriate */
>>
> Will fix, thanks.
>
>


Latest patch attached, I think all comments have been addressed. I
propose to push this later this coming week if there are no more comments.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  46. Re: ssl passphrase callback

    Andreas Karlsson <andreas@proxel.se> — 2020-03-23T00:58:14Z

    On 3/22/20 1:08 AM, Andrew Dunstan wrote:
> Latest patch attached, I think all comments have been addressed. I
> propose to push this later this coming week if there are no more comments.

I do not have any objections.

Andreas

  47. Re: ssl passphrase callback

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-28T17:45:00Z

    Andreas Karlsson <andreas@proxel.se> writes:
> On 3/22/20 1:08 AM, Andrew Dunstan wrote:
>> Latest patch attached, I think all comments have been addressed. I
>> propose to push this later this coming week if there are no more comments.

> I do not have any objections.

This CF entry is still open, should it not be closed as committed?

https://commitfest.postgresql.org/27/2338/

			regards, tom lane

  48. Re: ssl passphrase callback

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-03-28T19:35:07Z

    On 3/28/20 1:45 PM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> On 3/22/20 1:08 AM, Andrew Dunstan wrote:
>>> Latest patch attached, I think all comments have been addressed. I
>>> propose to push this later this coming week if there are no more comments.
>> I do not have any objections.
> This CF entry is still open, should it not be closed as committed?
>
> https://commitfest.postgresql.org/27/2338/
>
> 			


Done, thanks for the reminder.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services