Thread: ssl passphrase callback
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
Attachment
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
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
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
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 +
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
Attachment
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
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
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
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
Attachment
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 +
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 +
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
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
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
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
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.
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/
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 +
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
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
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 +
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
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 +
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
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
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
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.
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
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
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
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
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
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
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.
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
Attachment
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
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
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
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
Attachment
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
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
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
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
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
Attachment
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
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
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