Re: PQinitSSL broken in some use casesf

On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Merlin Moncure wrote:
>> > PQinitSSL(0) was specifically designed to allow applications to set up
>> > SSL on their own. How does this not work properly?
>>
>> this has nothing to do with who initializes ssl. this is all about
>> *crypto*. remember, crypto and ssl are two separate libraries. The
>> application or library in question may not even link with ssl or use
>> ssl headers.
>>
>> The problem is PQinitSSL (re-) initializes crypto without asking if that's ok.
>
> PQinitSSL(false) initializes crypto? Please point me to exact function
> calls that are the problem? Everything is very vague.

nooo, you are not listening :-)

PQinitSSL(0) initializes libpq for ssl but leaves crypto and ssl
initialization to the app
PQinitSSL(1) initializes libpq, crypto, and ssl libraries

Now, consider an app that uses libcrypto for its own requirements *but
not libssl*. It initializes libcrypto, passing its own lock vector,
etc. It cannot however initialize ssl because it does not link with
ssl, or include ssl headers. There are no ssl functions to call, and
it wouldn't make sense to expect the app to do this even if there
were.

Now, if this app also has libpq dependency, it needs a way to tell
libpq: 'i have already initialized the crypto library, but could you
please set up libssl'. otherwise you end up re-initializing libcrypto
with different lock vector which is very bad if there are any locks
already in use, which is quite likely.

There is no way to do that with libpq....so you see that no matter how
you call PQinitSSL, the application is broken in some way. Passing 0
breaks because ssl never ends up getting set up, and passing 1 breaks
because libcrypto's locks get messed up.

The main problem is that libpq PQinitSSL makes broad (and extremely
dangerous assumption) that it is the only one interested in libcrypto
lock vector. In short, it's broken.

merlin

> On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> PQinitSSL(false) initializes crypto? Please point me to exact function
>> calls that are the problem? Everything is very vague.

File: src/interfaces/libpq/fe-secure.c
Func: init_ssl_system
Line: 823

Starting at around line 853, this function prepares a lock array for
CRYPTO_set_locking_callback. This function is not part of libssl, its
part of libcrypto. It also calls CRYPTO_set_id_callback. The rest of
that function appears to only make libssl calls.

There should be an "if (pq_initcryptolib)" around those libcrypto calls,
serving the same purpose as the pq_initssllib variable.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
>
> > On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> PQinitSSL(false) initializes crypto? Please point me to exact function
> >> calls that are the problem? Everything is very vague.
>
> File: src/interfaces/libpq/fe-secure.c
> Func: init_ssl_system
> Line: 823
>
> Starting at around line 853, this function prepares a lock array for
> CRYPTO_set_locking_callback. This function is not part of libssl, its
> part of libcrypto. It also calls CRYPTO_set_id_callback. The rest of
> that function appears to only make libssl calls.
>
> There should be an "if (pq_initcryptolib)" around those libcrypto calls,
> serving the same purpose as the pq_initssllib variable.

Why not just call PQinitSSL(true) and do everything in your
application?; from the libpq manual:

If you are using <acronym>SSL</> inside your application (in addition
to inside <application>libpq</application>), you can use
<function>PQinitSSL(int)</> to tell <application>libpq</application>
that the <acronym>SSL</> library has already been initialized by your
application.

Actually, that wording doesn't say what the parameter means so I updated
the documentation:

If you are using <acronym>SSL</> inside your application (in addition
! to inside <application>libpq</application>), you can call
! <function>PQinitSSL(int)</> with <literal>0</> to tell
! <application>libpq</application> that the <acronym>SSL</> library
! has already been initialized by your application.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Chernow wrote:
> >
> > > On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > >> PQinitSSL(false) initializes crypto? Please point me to exact function
> > >> calls that are the problem? Everything is very vague.
> >
> > File: src/interfaces/libpq/fe-secure.c
> > Func: init_ssl_system
> > Line: 823
> >
> > Starting at around line 853, this function prepares a lock array for
> > CRYPTO_set_locking_callback. This function is not part of libssl, its
> > part of libcrypto. It also calls CRYPTO_set_id_callback. The rest of
> > that function appears to only make libssl calls.
> >
> > There should be an "if (pq_initcryptolib)" around those libcrypto calls,
> > serving the same purpose as the pq_initssllib variable.
>
> Why not just call PQinitSSL(true) and do everything in your

Sorry, meant 'false'. Also, I read Merlin's comments and understand he
doesn't want his application to have to set up SSL.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Tue, Feb 10, 2009 at 11:09 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Why not just call PQinitSSL(true) and do everything in your
> application?; from the libpq manual:
>
> If you are using <acronym>SSL</> inside your application (in addition
> to inside <application>libpq</application>), you can use
> <function>PQinitSSL(int)</> to tell <application>libpq</application>
> that the <acronym>SSL</> library has already been initialized by your
> application.

I think this question has been answered about four times on this
thread already. I thought the OP's explanation was pretty clear:

http://archives.postgresql.org/pgsql-hackers/2009-01/msg02488.php

It's also re-explained here, here, and here:

http://archives.postgresql.org/message-id/b42b73150902100713mdbfd64ah706ced5170897a59@mail.gmail.com
http://archives.postgresql.org/message-id/603c8f070902100849n44034028p5423f18e6e1b9317@mail.gmail.com
http://archives.postgresql.org/message-id/b42b73150902101420m6c263f7ayafc10090af841e15@mail.gmail.com

The point is that there are three sane things you might want/need
PQinitSSL() to do, and we allow two of them. I think the request for
a behavior change is totally reasonable, but I think Andrew or Merlin
should be the ones to write the patch (and then others can review). I
am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE) for
this purpose, but haven't read the code enough to determine the
least-ugly API.

...Robert

Robert Haas wrote:
> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE)
>

The issue I see is the inability to toggle crypto initialization. I
think crypto init and ssl init are 2 different things. Thus, I proposed
the below:

http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php

Andrew Chernow
eSilo, LLC
-- every bit count

On Feb 11, 2009, at 12:12 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:

> Robert Haas wrote:
>> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE)
>
> The issue I see is the inability to toggle crypto initialization.
> I think crypto init and ssl init are 2 different things. Thus, I
> proposed the below:
>
> http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php

If that can be made transparent to existing apps, I'm all for it.

...Robert

Robert Haas wrote:
> On Feb 11, 2009, at 12:12 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>
>> Robert Haas wrote:
>>> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE)
>>
>> The issue I see is the inability to toggle crypto initialization. I
>> think crypto init and ssl init are 2 different things. Thus, I
>> proposed the below:
>>
>> http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php
>
> If that can be made transparent to existing apps, I'm all for it.
>

Defaulting the crypto initialization to TRUE should make this
transparent. The downside is that this breaks backwards compatibility,
since an unknown symbol is being referenced. Although, that only occurs
when PQinitCrypto is actually used.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Robert Haas wrote:
> On Feb 11, 2009, at 12:12 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>
>> Robert Haas wrote:
>>> I am not in love with the idea of using PQinitSSL(SOME_MAGIC_VALUE)
>>
>> The issue I see is the inability to toggle crypto initialization. I
>> think crypto init and ssl init are 2 different things. Thus, I
>> proposed the below:
>>
>> http://archives.postgresql.org/pgsql-hackers/2009-02/msg00488.php
>
> If that can be made transparent to existing apps, I'm all for it.
>
> ...Robert
>

Should I create a patch implementing the PQinitCrypto idea?

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On Fri, Feb 13, 2009 at 9:17 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Should I create a patch implementing the PQinitCrypto idea?

I think that would be helpful. Seeing the code will give everyone a
better idea of exactly what the proposed change is and whether it's
acceptable.

...Robert

Robert Haas wrote:
> On Fri, Feb 13, 2009 at 9:17 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Should I create a patch implementing the PQinitCrypto idea?
>
> I think that would be helpful. Seeing the code will give everyone a
> better idea of exactly what the proposed change is and whether it's
> acceptable.
>
> ...Robert
>
>

Patch attached.

One thing I noticed is the ssl_open_connections variable is ref counting
connections when pq_initssllib is true. But, it now only affects crypto
library init and cleanup calls. Point is, ref counting is only needed
if pq_initcryptolib is true and it should be renamed to
crypto_open_connections. I didn't do this in the patch. Its the same
old name and the counter is incremented if pq_initssllib or
pq_initcryptolib is true. Please advise.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Patch attached.
>
> One thing I noticed is the ssl_open_connections variable is ref counting
> connections when pq_initssllib is true. But, it now only affects crypto
> library init and cleanup calls. Point is, ref counting is only needed if
> pq_initcryptolib is true and it should be renamed to
> crypto_open_connections. I didn't do this in the patch. Its the same old
> name and the counter is incremented if pq_initssllib or pq_initcryptolib is
> true. Please advise.

I'll review this in more detail when I have a chance, but it certainly
won't be committable without doc changes, and it's probably best if
you write those and include them in the patch.

...Robert

Robert Haas wrote:
> On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Patch attached.
>>
>> One thing I noticed is the ssl_open_connections variable is ref counting
>> connections when pq_initssllib is true. But, it now only affects crypto
>> library init and cleanup calls. Point is, ref counting is only needed if
>> pq_initcryptolib is true and it should be renamed to
>> crypto_open_connections. I didn't do this in the patch. Its the same old
>> name and the counter is incremented if pq_initssllib or pq_initcryptolib is
>> true. Please advise.
>
> I'll review this in more detail when I have a chance, but it certainly
> won't be committable without doc changes, and it's probably best if
> you write those and include them in the patch.
>
> ...Robert
>
>

Okay. Added docs in the same place PQinitSSL is. Apparently, initssl
doesn't have formal func docs.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
> Robert Haas wrote:
>> On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>> Patch attached.
>>>
>>> One thing I noticed is the ssl_open_connections variable is ref counting
>>> connections when pq_initssllib is true. But, it now only affects crypto
>>> library init and cleanup calls. Point is, ref counting is only
>>> needed if
>>> pq_initcryptolib is true and it should be renamed to
>>> crypto_open_connections. I didn't do this in the patch. Its the
>>> same old
>>> name and the counter is incremented if pq_initssllib or
>>> pq_initcryptolib is
>>> true. Please advise.
>>
>> I'll review this in more detail when I have a chance, but it certainly
>> won't be committable without doc changes, and it's probably best if
>> you write those and include them in the patch.
>>

One problem with this patch is that a libpq app using PQinitSSL(0) is
under the assumption that this shuts off ssl init and crypto init. That
app might be doing its own crypto init which would be overwritten by
libpq because the app is unaware of PQinitCrypto (if and when it
eventually links with 8.4 libpq). This feels like a very uncommon
situation, but a possible gotcha.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
> Andrew Chernow wrote:
>> Robert Haas wrote:
>>> On Fri, Feb 13, 2009 at 12:06 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>>> Patch attached.
>>>>
>>>> One thing I noticed is the ssl_open_connections variable is ref
>>>> counting
>>>> connections when pq_initssllib is true. But, it now only affects
>>>> crypto
>>>> library init and cleanup calls. Point is, ref counting is only
>>>> needed if
>>>> pq_initcryptolib is true and it should be renamed to
>>>> crypto_open_connections. I didn't do this in the patch. Its the
>>>> same old
>>>> name and the counter is incremented if pq_initssllib or
>>>> pq_initcryptolib is
>>>> true. Please advise.
>>>
>>> I'll review this in more detail when I have a chance, but it certainly
>>> won't be committable without doc changes, and it's probably best if
>>> you write those and include them in the patch.
>>>
>
> One problem with this patch is that a libpq app using PQinitSSL(0) is
> under the assumption that this shuts off ssl init and crypto init. That
> app might be doing its own crypto init which would be overwritten by
> libpq because the app is unaware of PQinitCrypto (if and when it
> eventually links with 8.4 libpq). This feels like a very uncommon
> situation, but a possible gotcha.
>

(sorry I keep posting)

>This feels like a very uncommon situation
I take that back. Not so sure it is uncommon, any threaded libpq app
would probably get bit if they called PQinitSSL. On top of that, it
could take up to a year before complaints start rolling in, as 8.4 hits
the distros. Yuck.

I now think the the orignal suggestion of PQinitSSLExtended is better
than PQinitCrypto. With PQinitSSLExtended, PQinitSSL needs a minor
implementation adjustment but the behvior remains the same. The
extended version is probably:

/* IMHO appending "Ex" is a little nicer */
void PQinitSSLEx(int ssl_init, int crypto_init);

/* PQinitSSL wraps PQinitSSLEx */
void PQinitSSL(int do_init)
{
PQinitSSLEx(do_init, do_init);
}

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow <ac(at)esilo(dot)com> writes:
>> One problem with this patch is that a libpq app using PQinitSSL(0) is
>> under the assumption that this shuts off ssl init and crypto init. That
>> app might be doing its own crypto init which would be overwritten by
>> libpq because the app is unaware of PQinitCrypto (if and when it
>> eventually links with 8.4 libpq). This feels like a very uncommon
>> situation, but a possible gotcha.

> I take that back. Not so sure it is uncommon,

I agree, we should *not* change the existing behavior for either case.
This probably means that an independent PQinitCrypto function is the
wrong thing, even though it would've been the cleanest solution if
we were starting from scratch.

At this point I like Merlin's proposal of a third parameter value to
PQinitSSL the best.

> /* IMHO appending "Ex" is a little nicer */
> void PQinitSSLEx(int ssl_init, int crypto_init);

Ugh, ugh, ugh. We do not do "Ex" around here --- uninterpretable
abbreviations are not helpful to the reader. Call it Extended
if you must have it.

Also, this definition feels a bit wrong --- it's not possible for
all four cases to be valid, is it?

regards, tom lane

> At this point I like Merlin's proposal of a third parameter value to
> PQinitSSL the best.

I'm not opposed to it, although I don't think it is as clean as a new
function.

>
> Also, this definition feels a bit wrong --- it's not possible for
> all four cases to be valid, is it?
>

Yes it is.

PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0)
PQinitSSLExtended(1, 0); // init ssl, don't init crypto
PQinitSSLExtended(0, 1); // don't init ssl, init crypto
PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1)

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
>> At this point I like Merlin's proposal of a third parameter value to
>> PQinitSSL the best.
>
> I'm not opposed to it, although I don't think it is as clean as a new
> function.
>
>>
>> Also, this definition feels a bit wrong --- it's not possible for
>> all four cases to be valid, is it?
>>
>
> Yes it is.
>
> PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0)
> PQinitSSLExtended(1, 0); // init ssl, don't init crypto
> PQinitSSLExtended(0, 1); // don't init ssl, init crypto
> PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1)
>

Maybe the argument to PQinitSSLExtended should be a bit mask, making
this version more extendable ... PG_INITSSL, PG_INITCRYPTO?

Also, how about calling this PQinitSecure(int flags), since SSL is only
one thing it can init. This is just like merlin's suggestion but
without hacking the existing PQinitSSL.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow <ac(at)esilo(dot)com> writes:
>> Also, this definition feels a bit wrong --- it's not possible for
>> all four cases to be valid, is it?

> Yes it is.

> PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0)
> PQinitSSLExtended(1, 0); // init ssl, don't init crypto
> PQinitSSLExtended(0, 1); // don't init ssl, init crypto
> PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1)

I know what you're thinking the flags should mean, I'm saying that it's
not possible for the third case to be sane. It implies that the
application initialized ssl but not crypto, which isn't possible.

regards, tom lane

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
>>> Also, this definition feels a bit wrong --- it's not possible for
>>> all four cases to be valid, is it?
>
>> Yes it is.
>
>> PQinitSSLExtended(0, 0); // don't init anything, PQinitSSL(0)
>> PQinitSSLExtended(1, 0); // init ssl, don't init crypto
>> PQinitSSLExtended(0, 1); // don't init ssl, init crypto
>> PQinitSSLExtended(1, 1); // init both, default behavior, PQinitSSL(1)
>
> I know what you're thinking the flags should mean, I'm saying that it's
> not possible for the third case to be sane. It implies that the
> application initialized ssl but not crypto, which isn't possible.
>

Or that the application called PQinitSSLExtended(0, 1) and then
initialized SSL itself, which is sane.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow <ac(at)esilo(dot)com> writes:
> Maybe the argument to PQinitSSLExtended should be a bit mask, making
> this version more extendable ... PG_INITSSL, PG_INITCRYPTO?

+1 for thinking ahead to the next time, but is a bit mask the right thing?

regards, tom lane

On Fri, Feb 13, 2009 at 3:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
>> Maybe the argument to PQinitSSLExtended should be a bit mask, making
>> this version more extendable ... PG_INITSSL, PG_INITCRYPTO?
>
> +1 for thinking ahead to the next time, but is a bit mask the right thing?

What would you suggest? struct pointer? varargs?

If you are down with the struct pointer idea, maybe we could revive
the idea of PQinit:
http://archives.postgresql.org/pgsql-hackers/2009-01/msg01349.php

We could fix up the windows issue at the same time, and leave room for
other things. Fixing windows WSA issues or installing an event proc
hook into the library :D.

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Fri, Feb 13, 2009 at 3:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> +1 for thinking ahead to the next time, but is a bit mask the right thing?

> What would you suggest? struct pointer? varargs?

Not sure. By definition, we're trying to predict an unforeseen
requirement, and that's always going to be tough.

I'm not too thrilled about a struct pointer, because that will introduce
the problem "which version of the struct is the client passing?".
A varargs arrangement could cover almost anything, but it also seems
like overkill.

I don't actually have an idea that sounds better than a bitmask,
I just wanted to see if anyone else did.

BTW, the bitmask isn't perfect either --- doesn't it just reintroduce
the problem already complained of with your idea for PQinitSSL? That
is, how does the client know whether the function recognized all the
bits it passed?

regards, tom lane

Tom Lane wrote:
> is, how does the client know whether the function recognized all the
> bits it passed?

How about returning the bits it could set?

int mask = PG_INITSSL | PG_INITCRYPTO;
if (!(PQinitSecure(mask) & PG_INITCRYPTO))
; // no support for crypto

...OR...

consider a generic PQinit call per system/object/etc..

int PQinit(int which, void *data);

int mask = PG_SECURE_SSL | PG_SECURE_CRYPTO;
PQinit(PG_INIT_SECURE, &mask); // or PG_INIT_OPENSSL

xxx_t xxx = {0, "blah", 12};
PQinit(PG_INIT_xxx, &xxx);

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

> BTW, the bitmask isn't perfect either --- doesn't it just reintroduce
> the problem already complained of with your idea for PQinitSSL? That
> is, how does the client know whether the function recognized all the
> bits it passed?

Well, if we add the PQgetLibraryVersion() function I suggested
upthread, then it can check that first. I find it difficult to
believe that isn't a good idea independently of how we solve this
particular problem.

...Robert

On Fri, Feb 13, 2009 at 4:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> BTW, the bitmask isn't perfect either --- doesn't it just reintroduce
>> the problem already complained of with your idea for PQinitSSL? That
>> is, how does the client know whether the function recognized all the
>> bits it passed?
>
> Well, if we add the PQgetLibraryVersion() function I suggested
> upthread, then it can check that first. I find it difficult to
> believe that isn't a good idea independently of how we solve this
> particular problem.

I'd prefer PQversion() as a name. Also, it doesn't necessarily handle
the issue directly. For example, it doesn't tell you which bits are
valid...you have to guess. Also, do we really need a function for
this?

Is the generic init worth discussion or a non starter? I guess we
have a scheduling problem here...I think the ssl problem is serious
enough to warrant a fast-track into 8.4, but maybe it's 'too much' to
hash out a generic library initialization routine this late in the
cycle.

merlin

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> BTW, the bitmask isn't perfect either --- doesn't it just reintroduce
>> the problem already complained of with your idea for PQinitSSL? That
>> is, how does the client know whether the function recognized all the
>> bits it passed?

> Well, if we add the PQgetLibraryVersion() function I suggested
> upthread, then it can check that first. I find it difficult to
> believe that isn't a good idea independently of how we solve this
> particular problem.

Personally, I still subscribe to the PNG design theory that
conditionalizing code on overall version numbers isn't a very good idea.
Version information should be tied as tightly as possible to the
particular behavior for which it applies.
http://www.w3.org/TR/PNG-Rationale.html#R.Chunk-naming-conventions

It's even worse when the available version numbers aren't really
"overall", which would be the case here. If we had such a function
you can bet your bottom dollar that people would test it to distinguish
behaviors that are really server-side, and then get burnt in the field
when their app gets used with mismatched server and libpq versions.

For the problem at hand, if we think it's important for an app to be
able to tell whether libpq supports a particular initialization
behavior, then adding a new function to provide that behavior seems
like the best bet to me.

So maybe that says that designing for extensibility in the function
signature is misguided anyhow ...

regards, tom lane

> Personally, I still subscribe to the PNG design theory that
> conditionalizing code on overall version numbers isn't a very good idea.
> Version information should be tied as tightly as possible to the
> particular behavior for which it applies.
> http://www.w3.org/TR/PNG-Rationale.html#R.Chunk-naming-conventions

I don't disagree with you, but it's not always easy to separate things
that cleanly. pg_dump doesn't try to distinguish server capabilities,
just server versions (and I'm sure you'd scream bloody murder if
someone submitted a patch to refactor it... because that patch would
be hideous).

> It's even worse when the available version numbers aren't really
> "overall", which would be the case here. If we had such a function
> you can bet your bottom dollar that people would test it to distinguish
> behaviors that are really server-side, and then get burnt in the field
> when their app gets used with mismatched server and libpq versions.

I think that could be largely addressed through documentation. There
is no helpful for terminal stupidity anyway.

The reality is, right now we have a way to distinguish server
versions, but not client versions. That doesn't seem particularly
logical or consistent. Testing for client versions lets you do things
like check for breakage known to exist in versions < X, where there
may be no difference in the API, just the behavior. It also reduces
the need to make every non-backward-compatible change an API change.

Many people will only ever link against the most recent version and
will not care about backward-compatibility. It doesn't hurt to
provide a hook to those who do care.

> For the problem at hand, if we think it's important for an app to be
> able to tell whether libpq supports a particular initialization
> behavior, then adding a new function to provide that behavior seems
> like the best bet to me.
>
> So maybe that says that designing for extensibility in the function
> signature is misguided anyhow ...

It could certainly be overdone.

...Robert

So far, we have the below ideas:

1. void PQinitSSL(MAGIC_VALUES)
The idea is to use some magic values to add more options, converting the do_init
argument from a flag to an enumeration. The idea is choose magic values that
would most likely never be used by existing libpq applications. The downside to
this method is there is no way to detect if the request was honored: meaning a
new app passes a magic value to an older libpq.

2. void PQinitSecure(int init_ssl, int int_crypto)
Another proposed name was PQinitSSLExtended or PQinitOpenSSL. This leaves the
existing PQinitSSL alone. The problem here is that it can't be extended like
the current PQinitSSL, so we fixed today's problem but created it again for
future (sounds like government). That leads us to #3.

3. void PQinitSecure(int init_mask)
Works just like #2 but uses a bit mask, like PG_INITSSL or something, instead of
a fixed set of flags. It trys to make the function more extendable. Although,
we bang into the #1 problem again - new apps using new bits that older libpqs
are not aware of. There is no way to know if the request was honored. This
leads us to #4 (#5 was also a suggested fix).

4. int PQinitSecure(int init_mask)
This works just like #3 but returns the bits it understood. It allows an app to
determine if the loaded libpq supports a bit it has tried to use. I have not
heard any comments on this, I am still looking for its faults. You could claim
its akward, but I've seen much worse. An alternative is to add a second
argument, int *support_mask, instead of returning it. Or, make the single
argument an input and output value .. (int *mask).

5. ??? PQgetLibraryVersion(???)
Another proposed name was PQversion. This was suggested as a way of fixing the
faults of #1 and #3. The idea is to use the libpq version to determine if a
particular init bit is supported.

6. PQinit(int init_who, void *data)
Revived from a recent WSACleanup thread. Appears to always be the last kid
picked for the kick ball team, so I won't dive to deep. The idea is to create a
single init function for libpq, that can handle init'n any
module/feature/runtime-settings/etc.. Another name could be PQcontrol.

I think that's everything. Are there any favorites or combos? Personally, I'm
not really stuck on any of them. I'd just like to see the feature added. If I
had to choose, it would be #4. A close second would be #1 + #5 (using PQversion
as the name).

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow <ac(at)esilo(dot)com> writes:
> 4. int PQinitSecure(int init_mask)
> This works just like #3 but returns the bits it understood. It allows an app to
> determine if the loaded libpq supports a bit it has tried to use.

Maybe better, have it return a zero/nonzero error code; where one of the
possibilities for failure is "you passed a bit I didn't understand".
This would depend on whether we intend that the function would ever
actually *do* anything, as opposed to just save its argument to be
consulted later. Maybe that's not appropriate/necessary.

regards, tom lane

> Maybe better, have it return a zero/nonzero error code; where one of the
> possibilities for failure is "you passed a bit I didn't understand".

Why not just return those bit(s) instead of an arbitrary code? How about:

-1 = error (if it ever does anything that can fail)
0 = success (all bits known)
>0 = unknown bits (remaining known bits *have* been set)

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
>
> > Maybe better, have it return a zero/nonzero error code; where one of the
> > possibilities for failure is "you passed a bit I didn't understand".
>
> Why not just return those bit(s) instead of an arbitrary code? How about:
>
> -1 = error (if it ever does anything that can fail)
> 0 = success (all bits known)
> >0 = unknown bits (remaining known bits *have* been set)
>

I attached a patch that implements the above, using PQinitSecure as the
function name.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
> Andrew Chernow wrote:
>>
>> > Maybe better, have it return a zero/nonzero error code; where one
>> of the
>> > possibilities for failure is "you passed a bit I didn't understand".
>>
>> Why not just return those bit(s) instead of an arbitrary code? How
>> about:
>>
>> -1 = error (if it ever does anything that can fail)
>> 0 = success (all bits known)
>> >0 = unknown bits (remaining known bits *have* been set)
>>
>
> I attached a patch that implements the above, using PQinitSecure as the
> function name.
>
>

Fixed a bug in the patch. I forgot to make PQinitSSL update the new
pg_initcryptolib flag. My fix was to make PQinitSSL proxy to PQinitSecure.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Merlin Moncure wrote:
> On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Merlin Moncure wrote:
> >> > PQinitSSL(0) was specifically designed to allow applications to set up
> >> > SSL on their own. How does this not work properly?
> >>
> >> this has nothing to do with who initializes ssl. this is all about
> >> *crypto*. remember, crypto and ssl are two separate libraries. The
> >> application or library in question may not even link with ssl or use
> >> ssl headers.
> >>
> >> The problem is PQinitSSL (re-) initializes crypto without asking if that's ok.
> >
> > PQinitSSL(false) initializes crypto? Please point me to exact function
> > calls that are the problem? Everything is very vague.
>
> nooo, you are not listening :-)
>
> PQinitSSL(0) initializes libpq for ssl but leaves crypto and ssl
> initialization to the app
> PQinitSSL(1) initializes libpq, crypto, and ssl libraries
>
> Now, consider an app that uses libcrypto for its own requirements *but
> not libssl*. It initializes libcrypto, passing its own lock vector,
> etc. It cannot however initialize ssl because it does not link with
> ssl, or include ssl headers. There are no ssl functions to call, and
> it wouldn't make sense to expect the app to do this even if there
> were.
>
> Now, if this app also has libpq dependency, it needs a way to tell
> libpq: 'i have already initialized the crypto library, but could you
> please set up libssl'. otherwise you end up re-initializing libcrypto
> with different lock vector which is very bad if there are any locks
> already in use, which is quite likely.
>
> There is no way to do that with libpq....so you see that no matter how
> you call PQinitSSL, the application is broken in some way. Passing 0
> breaks because ssl never ends up getting set up, and passing 1 breaks
> because libcrypto's locks get messed up.
>
> The main problem is that libpq PQinitSSL makes broad (and extremely
> dangerous assumption) that it is the only one interested in libcrypto
> lock vector. In short, it's broken.

I am back to looking at this. I dropped off this discussion back in
February because I felt people didn't want to answer questions I had,
but now it seems we have to close this out somehow.

I have applied the attached patch which does several things:

o documents that libssl _and_ libcrypto initialization is
turned off by PQinitSSL(0)
o clarified cases where this behavior is important
o added comments that the CRYPTO_set_* calls reference
libcrypto, not libssl

I think we can now say that the current behavior is not a bug because it
is documented, even though the PQinitSSL() function name is inaccurate.

In fact, 8.4 is the first time we are documenting the valid parameter
value to PQinitSSL(), in 8.3 we have:

to inside <application>libpq</application>), you can use
<function>PQinitSSL(int)</> to tell <application>libpq</application>
that the <acronym>SSL</> library has already been initialized by your
application.

So we have some flexibility in defining how it behaves, and this also
illustrates how little the function is used because no one ever
complained about it.

I think there is a good argument that PQinitSSL(X) where X > 1 would
work fine for more fine-grained control. The new libpq init function
idea was interesting, but having a documented solution for
WSAStartup()/WSACleanup() usage, we now don't have another libpq init
use-case so it is hard to suggest a new libpq function.

I am figuring we have to keep the current behavior and see what happens
after 8.4; the new documentation should make the behavior clear and
perhaps trigger other users to report suggestions.

I assume this item is closed for 8.4 unless I hear otherwise.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

> I think there is a good argument that PQinitSSL(X) where X > 1 would
> work fine for more fine-grained control. The new libpq init function
> idea was interesting, but having a documented solution for
> WSAStartup()/WSACleanup() usage, we now don't have another libpq init
> use-case so it is hard to suggest a new libpq function.

If you look back through the list, the PQinit idea was offered up
several times while discussing WSA* stuff. There were few buyers. I
don't see how having or not having a documented solution for WSA* usage
would make a bit of difference.

>
> I am figuring we have to keep the current behavior and see what happens
> after 8.4; the new documentation should make the behavior clear and
> perhaps trigger other users to report suggestions.
>
>

This is not a battle I find worth fighting. But I am having trouble
staying completely quiet; I typically have this issue when I disagree :)
This patch merely documents the problem, when another fully documented
working patch "fixed" it; following the discussions on the list.

http://archives.postgresql.org//pgsql-hackers/2009-02/msg01018.php

Was this reviewed and/or rejected?

Andrew Chernow

Andrew Chernow wrote:
> Bruce Momjian wrote:
>
> > I think there is a good argument that PQinitSSL(X) where X > 1 would
> > work fine for more fine-grained control. The new libpq init function
> > idea was interesting, but having a documented solution for
> > WSAStartup()/WSACleanup() usage, we now don't have another libpq init
> > use-case so it is hard to suggest a new libpq function.
>
> If you look back through the list, the PQinit idea was offered up
> several times while discussing WSA* stuff. There were few buyers. I
> don't see how having or not having a documented solution for WSA* usage
> would make a bit of difference.

It only means we don't have _another_ use for a more general libpq init
function.

> > I am figuring we have to keep the current behavior and see what happens
> > after 8.4; the new documentation should make the behavior clear and
> > perhaps trigger other users to report suggestions.
> >
> >
>
> This is not a battle I find worth fighting. But I am having trouble
> staying completely quiet; I typically have this issue when I disagree :)
> This patch merely documents the problem, when another fully documented
> working patch "fixed" it; following the discussions on the list.
>
> http://archives.postgresql.org//pgsql-hackers/2009-02/msg01018.php
>
> Was this reviewed and/or rejected?

Comments Tom made were that there was no consensus on the proper
fix/direction, and I agree.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Fri, Mar 27, 2009 at 9:38 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I have applied the attached patch which does several things:
>
>        o  documents that libssl _and_ libcrypto initialization is
>           turned off by PQinitSSL(0)
>        o  clarified cases where this behavior is important
>        o  added comments that the CRYPTO_set_* calls reference
>           libcrypto, not libssl
>
> I think we can now say that the current behavior is not a bug because it
> is documented, even though the PQinitSSL() function name is inaccurate.

It is still a bug in the sense that it is impossible to properly
initialize crypto features in some scenarios. A doc patch (which I
argued is the best way to go for 8.4) fails to properly raise the
seriousness of the issue and also fails to suggest a workaround.

I think a proper way to document this issue would be something like this:

"
If your application initializes libcrypto, but not libssl, you must
not call PQinitSSL(1) because it will overwrite your libcrypto
initialization. In order to safely use libpq in your application, you
must include ssl headers and call the following functions:

#include <openssl/ssl.h>
#include <openssl/conf.h>

OPENSSL_config(NULL);
SSL_library_init();
SSL_load_error_strings();
PQinitSSL(0);

In order to initialize libpq properly for SSL connections.
"

> I think there is a good argument that PQinitSSL(X) where X > 1 would
> work fine for more fine-grained control.  The new libpq init function
> idea was interesting, but having a documented solution for
> WSAStartup()/WSACleanup() usage, we now don't have another libpq init
> use-case so it is hard to suggest a new libpq function.

This feature when discussed at the time was not enough _by itself_ to
support a PQinit feature (I agree with this reasoning), but surely
should be considered as valid supporting evidence that a library
initialization feature is useful. IOW, the whole of the argument is
equal to the sum of its parts. (yes, we have an agenda here: we were
not happy that our events patch could not establish behavior at
library initialization time).

merlin

On Sat, Mar 28, 2009 at 9:23 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
> It is still a bug in the sense that it is impossible to properly
> initialize crypto features in some scenarios.  A doc patch (which I

Meant to say: 'your doc patch"

merlin

Tom Lane wrote:
>
> I personally would be happy with the two-argument function solution.
>

The patch I submitted pretty much does this, except it uses a flags argument
instead of 2 fixed arguments. It can be easily changed to support the 2
argument idea:

1. Change prototype to: void PQinitSecure(int ssl, int crypto);
- previous return value can go away, new version can be void.
2. Remove PG_SECURE_SSL, PG_SECURE_CRYPTO from libpq-fe.h
3. Modify wrapping code in PQinitSSL to use new prototype.
- either pass 0,0 or 1,1, to PQinitSecure.

Outside of that, the patch goes unchanged.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Merlin Moncure wrote:
> It is still a bug in the sense that it is impossible to properly
> initialize crypto features in some scenarios. A doc patch (which I
> argued is the best way to go for 8.4) fails to properly raise the
> seriousness of the issue and also fails to suggest a workaround.
>
> I think a proper way to document this issue would be something like this:
>
> "
> If your application initializes libcrypto, but not libssl, you must
> not call PQinitSSL(1) because it will overwrite your libcrypto
> initialization. In order to safely use libpq in your application, you
> must include ssl headers and call the following functions:
>
> #include <openssl/ssl.h>
> #include <openssl/conf.h>
>
> OPENSSL_config(NULL);
> SSL_library_init();
> SSL_load_error_strings();
> PQinitSSL(0);
>
> In order to initialize libpq properly for SSL connections.
> "
>
> > I think there is a good argument that PQinitSSL(X) where X > 1 would
> > work fine for more fine-grained control. ?The new libpq init function
> > idea was interesting, but having a documented solution for
> > WSAStartup()/WSACleanup() usage, we now don't have another libpq init
> > use-case so it is hard to suggest a new libpq function.
>
> This feature when discussed at the time was not enough _by itself_ to
> support a PQinit feature (I agree with this reasoning), but surely
> should be considered as valid supporting evidence that a library
> initialization feature is useful. IOW, the whole of the argument is
> equal to the sum of its parts. (yes, we have an agenda here: we were
> not happy that our events patch could not establish behavior at
> library initialization time).

Well, we are not the "Make Merlin Happy club". ;-)

Making usable software requires adjustments on everyone's part. I don't
think we have enough demand to hardcode those details in our
documentation.

Personally, I feel adding #defines for PQinitSSL is the right approach,
and I think that gives enough checks at compile time, but it doesn't
guard against cases where the application is built against one version
of libpq but is run against an older version, which I think can happen.

My personal opinion is that adding #defines to PQinitSSL() is the right
level of fix, and if we ever implement a libpq init mechanism we can
convert PGinitSSL to a macro. I am attaching a sample patch for
feedback.

However, as I mentioned above, this is not the "Make Bruce Happy club"
either so I need support from other developers that this is the right
fix.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Well, we are not the "Make Merlin Happy club". ;-)

Merlin and Andrew were the ones complaining initially. If they feel
that a proposed patch doesn't fix the problem, then I'd say that it
isn't fixing the problem.

> My personal opinion is that adding #defines to PQinitSSL() is the right
> level of fix, and if we ever implement a libpq init mechanism we can
> convert PGinitSSL to a macro. I am attaching a sample patch for
> feedback.

This is just a rehash of one of the patches that was discussed earlier.
There wasn't consensus for it then, and there's not now.

regards, tom lane

Tom Lane wrote:
> This is just a rehash of one of the patches that was discussed earlier.
> There wasn't consensus for it then, and there's not now.
>

I am personally out of ideas. It feels like this issue has been beaten
to death. There are only a few ways to do it and I believe they have
all been put on the table. Any suggestions?

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:
> Tom Lane wrote:
> > This is just a rehash of one of the patches that was discussed earlier.
> > There wasn't consensus for it then, and there's not now.
> >
>
> I am personally out of ideas. It feels like this issue has been beaten
> to death. There are only a few ways to do it and I believe they have
> all been put on the table. Any suggestions?

Well, at least the current behavior is documented now. I personally
thought the #define was the best minimal fix because it only added a few
lines to the docs and the C code.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Sat, Mar 28, 2009 at 4:26 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Andrew Chernow wrote:
>> Tom Lane wrote:
>> > This is just a rehash of one of the patches that was discussed earlier.
>> > There wasn't consensus for it then, and there's not now.
>> >
>>
>> I am personally out of ideas.  It feels like this issue has been beaten
>> to death.  There are only a few ways to do it and I believe they have
>> all been put on the table.  Any suggestions?
>
> Well, at least the current behavior is documented now.

You mean, with your proposed documentation patch? (if you meant my
proposed advice, then ignore the following paragraph)

Not only does it not suggest the problem, it actually misinforms
you...it suggests you call PQinitSSL(0) when "your application
initializes <literal>libssl</> or <literal>libcrypto</> libraries".
The operative word being 'or'. If you follow the advice after only
having initialized the libcrypto library, you would get a failure
trying to use libpq/SSL.

merlin

On Sat, Mar 28, 2009 at 3:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> Well, we are not the "Make Merlin Happy club".  ;-)
>
> Merlin and Andrew were the ones complaining initially.  If they feel
> that a proposed patch doesn't fix the problem, then I'd say that it
> isn't fixing the problem.

+1.

>> My personal opinion is that adding #defines to PQinitSSL() is the right
>> level of fix, and if we ever implement a libpq init mechanism we can
>> convert PGinitSSL to a macro. I am attaching a sample patch for
>> feedback.
>
> This is just a rehash of one of the patches that was discussed earlier.
> There wasn't consensus for it then, and there's not now.

OK, I read this patch. Wazzamatterwithit?

AIUI, we need to define an API that allows libssl and libcrypto to be
initialized or not initialized in any combination. We can do this by
modifying the meaning of the argument to PQinitSSL(), by adding a new
API function, by creating some more general initialization function,
or by some other method. It doesn't REALLY matter. I think I'm
personally of the opinion that PQinitSSL(int) should be left alone and
we should define PQinitSSLCrypto(int, int), just to avoid the
possibility of difficult-to-debug backward-compatibility problems, but
the number of people calling PQinitSSL(2) in existing applications is
doubtless vanishingly small, because there is probably no reason to
call it with a non-constant argument, so who is going to pick 2 rather
than 1? So if someone has some sort of principled objection to adding
a new API call, then let's just modify the behavior of the existing
call instead and move on.

Is there more substance here than meets the eye?

...Robert

Robert Haas wrote:
>
> Is there more substance here than meets the eye?
>

No, you about summed it up. We need a way to init libssl and libcrypto
in any combo. Along the way, PQinit() was discussed which may have
muddied the waters.

I prefer leaving the PQinitSSL function alone, thus my patch that
implements PQinitSecure(flags).

Adding PQinitSSL(new_value) seem reasonable to me. My only complaint
has been that the API user has no way of knowing if the function
understood their request. An older libpq would treat any non-zero
argument as one, which would silently fail/mis-behave from a new apps
perspective. Not sure this can be solved.

In the end, anyway you do it will have an issue or two. I agree that it
really doesn't matter, all methods would probably do the trick.

Andrew Chernow
eSilo, LLC

Andrew Chernow wrote:
> Robert Haas wrote:
> >
> > Is there more substance here than meets the eye?
> >
>
> No, you about summed it up. We need a way to init libssl and libcrypto
> in any combo. Along the way, PQinit() was discussed which may have
> muddied the waters.
>
> I prefer leaving the PQinitSSL function alone, thus my patch that
> implements PQinitSecure(flags).
>
> Adding PQinitSSL(new_value) seem reasonable to me. My only complaint
> has been that the API user has no way of knowing if the function
> understood their request. An older libpq would treat any non-zero
> argument as one, which would silently fail/mis-behave from a new apps
> perspective. Not sure this can be solved.
>
> In the end, anyway you do it will have an issue or two. I agree that it
> really doesn't matter, all methods would probably do the trick.

I think doing PQinitSSL(new_value) is probably the least invasive change
to solve this, which is why I suggested it. It does have a compile-time
check by referencing the #define.

We never documented the valid values to PQinitSSL(), and no one ever
reported this as a bug, so the existing use of PQinitSSL() is probably
very small.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Andrew Chernow wrote:
>> Adding PQinitSSL(new_value) seem reasonable to me. My only complaint
>> has been that the API user has no way of knowing if the function
>> understood their request.

> I think doing PQinitSSL(new_value) is probably the least invasive change
> to solve this, which is why I suggested it. It does have a compile-time
> check by referencing the #define.

You're missing the point, which is that it isn't certain whether the
right thing happens at runtime. It would be very hard to debug the
failure if an app compiled against new headers was run with an old
shlib. The separate two-argument function would avoid that: the failure
would manifest as "called function doesn't exist", which would at least
make it obvious what was wrong.

I personally would be happy with the two-argument function solution.
Now, that only addresses the specific problem of libcrypto vs libssl.
IIUC, Merlin's current thought is that we should be looking for a more
general solution. But it seems a bit dangerous to try to design a
general solution when we have only one example to work from.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Andrew Chernow wrote:
> >> Adding PQinitSSL(new_value) seem reasonable to me. My only complaint
> >> has been that the API user has no way of knowing if the function
> >> understood their request.
>
> > I think doing PQinitSSL(new_value) is probably the least invasive change
> > to solve this, which is why I suggested it. It does have a compile-time
> > check by referencing the #define.
>
> You're missing the point, which is that it isn't certain whether the
> right thing happens at runtime. It would be very hard to debug the
> failure if an app compiled against new headers was run with an old
> shlib. The separate two-argument function would avoid that: the failure
> would manifest as "called function doesn't exist", which would at least
> make it obvious what was wrong.
>
> I personally would be happy with the two-argument function solution.
> Now, that only addresses the specific problem of libcrypto vs libssl.
> IIUC, Merlin's current thought is that we should be looking for a more
> general solution. But it seems a bit dangerous to try to design a
> general solution when we have only one example to work from.

I think this is where we got stuck because extending libpq with a new
function is a larger API change, and not having a clear plan of what
initialization stuff we might need in the future, it seems unwise, and
also perhaps overkill.

FYI, libcrypto is initialized only in threaded libpq builds, and
non-zero calls to PQinitSSL were always no-ops because they just enabled
the default behavior.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Sun, Mar 29, 2009 at 1:56 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I think this is where we got stuck because extending libpq with a new
> function is a larger API change, and not having a clear plan of what
> initialization stuff we might need in the future, it seems unwise, and
> also perhaps overkill.

right, maybe this is a case of 'the perfect being the enemy of the
good'...I have no qualms with fixing it now with the two argument
version. The stuff I'd like to see is a separate issue, I guess.

merlin

Tom Lane wrote:
>
> I personally would be happy with the two-argument function solution.
>

I modified my previous patch to use a two-argument function solution.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow <ac(at)esilo(dot)com> writes:
> I modified my previous patch to use a two-argument function solution.

It sounds like everyone has converged on agreeing that this way is okay
for 8.4? Object now or hold your peace ...

regards, tom lane

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
> > I modified my previous patch to use a two-argument function solution.
>
> It sounds like everyone has converged on agreeing that this way is okay
> for 8.4? Object now or hold your peace ...

What are we doing with PQinitSSL()?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Tom Lane wrote:
>>
>> I personally would be happy with the two-argument function solution.
>>
>
> I modified my previous patch to use a two-argument function solution.

This looks OK to me, except I think we should modify the documentation
to PQinitSSL() to say that it you must not use both that function and
PQinitSecure(), and explain that if you need to control initialization
of libcrypto and libssl, you should use that function instead.

...Robert

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> It sounds like everyone has converged on agreeing that this way is okay
>> for 8.4? Object now or hold your peace ...

> What are we doing with PQinitSSL()?

Nothing, except improving the documentation.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> This looks OK to me, except I think we should modify the documentation
> to PQinitSSL() to say that it you must not use both that function and
> PQinitSecure(), and explain that if you need to control initialization
> of libcrypto and libssl, you should use that function instead.

Agreed, the docs need a bit more effort. I'll have a go at that.

regards, tom lane

Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > This looks OK to me, except I think we should modify the documentation
> > to PQinitSSL() to say that it you must not use both that function and
> > PQinitSecure(), and explain that if you need to control initialization
> > of libcrypto and libssl, you should use that function instead.
>
> Agreed, the docs need a bit more effort. I'll have a go at that.

Right; I saw no change to PSinitSSL documentation in the patch, which
was my concern.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> Tom Lane wrote:
>>> It sounds like everyone has converged on agreeing that this way is okay
>>> for 8.4? Object now or hold your peace ...
>
>> What are we doing with PQinitSSL()?
>
> Nothing, except improving the documentation.
>
>

My patch didn't change the docs at all. I wasn't sure what to do there.
However, I did change the implementation but left behavior alone. It
now calls PQinitSecure(do_init, do_init);

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Tom Lane wrote:
>>>
>>> I personally would be happy with the two-argument function solution.
>>>
>>
>> I modified my previous patch to use a two-argument function solution.
>
> This looks OK to me, except I think we should modify the documentation
> to PQinitSSL() to say that it you must not use both that function and
> PQinitSecure(), and explain that if you need to control initialization
> of libcrypto and libssl, you should use that function instead.

do you think PQinitSSL should be deprecated?

merlin

Merlin Moncure wrote:
> On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> >> Tom Lane wrote:
> >>>
> >>> I personally would be happy with the two-argument function solution.
> >>>
> >>
> >> I modified my previous patch to use a two-argument function solution.
> >
> > This looks OK to me, except I think we should modify the documentation
> > to PQinitSSL() to say that it you must not use both that function and
> > PQinitSecure(), and explain that if you need to control initialization
> > of libcrypto and libssl, you should use that function instead.
>
> do you think PQinitSSL should be deprecated?

Well, I think having duplicate capability in an API is confusing, so
yea.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> This looks OK to me, except I think we should modify the documentation
>> to PQinitSSL() to say that it you must not use both that function and
>> PQinitSecure(), and explain that if you need to control initialization
>> of libcrypto and libssl, you should use that function instead.

> do you think PQinitSSL should be deprecated?

I see no reason to deprecate it per se. I was planning to just define
it as equivalent to PQinitSecure(do_init, do_init).

BTW, unless someone objects I'd like to make the name of that function
PQinitSecurity. The other looks like it's implying that it is replacing
an insecure PQinit() function ...

regards, tom lane

Merlin Moncure wrote:
> On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> >> Tom Lane wrote:
> >>>
> >>> I personally would be happy with the two-argument function solution.
> >>>
> >>
> >> I modified my previous patch to use a two-argument function solution.
> >
> > This looks OK to me, except I think we should modify the documentation
> > to PQinitSSL() to say that it you must not use both that function and
> > PQinitSecure(), and explain that if you need to control initialization
> > of libcrypto and libssl, you should use that function instead.
>
> do you think PQinitSSL should be deprecated?

FYI, libcrypto has been initializing with SSL in threaded builds since
Postgres 8.0.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Merlin Moncure wrote:
>> On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>>> Tom Lane wrote:
>>>>> I personally would be happy with the two-argument function solution.
>>>>>
>>>> I modified my previous patch to use a two-argument function solution.
>>> This looks OK to me, except I think we should modify the documentation
>>> to PQinitSSL() to say that it you must not use both that function and
>>> PQinitSecure(), and explain that if you need to control initialization
>>> of libcrypto and libssl, you should use that function instead.
>> do you think PQinitSSL should be deprecated?
>
> Well, I think having duplicate capability in an API is confusing, so
> yea.
>

Maybe document that PQinitSSL(do_init) is now the same as
PQinitSecure(do_init, do_init).

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Tom Lane wrote:
> BTW, unless someone objects I'd like to make the name of that function
> PQinitSecurity.
>
>

Agreed. Although, both PQinitSecure and PQinitSecurity are very general
names. "Security" can mean a lot of things. Maybe the name should be
more particular, like PQinitOpenSSL. I think its okay to reference the
openssl project name being how this function is designed to work with
openssl's library split.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On 30 mar 2009, at 17.24, Andrew Chernow <ac(at)esilo(dot)com> wrote:

> Tom Lane wrote:
>> BTW, unless someone objects I'd like to make the name of that
>> function
>> PQinitSecurity.
>
> Agreed. Although, both PQinitSecure and PQinitSecurity are very
> general names. "Security" can mean a lot of things. Maybe the name
> should be more particular, like PQinitOpenSSL. I think its okay to
> reference the openssl project name being how this function is
> designed to work with openssl's library split.
>
+1

It's quite likely that well want to support another ssl library in the
future. At least likely enough that any api we define should take it
into consideration.

/Magnus

On Mon, Mar 30, 2009 at 11:56 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On 30 mar 2009, at 17.24, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>
>> Tom Lane wrote:
>>>
>>> BTW, unless someone objects I'd like to make the name of that function
>>> PQinitSecurity.
>>
>> Agreed.  Although, both PQinitSecure and PQinitSecurity are very general
>> names.  "Security" can mean a lot of things.  Maybe the name should be more
>> particular, like PQinitOpenSSL.  I think its okay to reference the openssl
>> project name being how this function is designed to work with openssl's
>> library split.
>>
> +1
>
> It's quite likely that well want to support another ssl library in the
> future. At least likely enough that any api we define should take it into
> consideration.

+1.

...Robert

Magnus Hagander wrote:
> On 30 mar 2009, at 17.24, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>
> > Tom Lane wrote:
> >> BTW, unless someone objects I'd like to make the name of that
> >> function
> >> PQinitSecurity.
> >
> > Agreed. Although, both PQinitSecure and PQinitSecurity are very
> > general names. "Security" can mean a lot of things. Maybe the name
> > should be more particular, like PQinitOpenSSL. I think its okay to
> > reference the openssl project name being how this function is
> > designed to work with openssl's library split.
> >
> +1
>
> It's quite likely that well want to support another ssl library in the
> future. At least likely enough that any api we define should take it
> into consideration.

Are we sure we don't want to add a more general libpq initialization
control at this time?

PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT);

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Magnus Hagander wrote:
>> On 30 mar 2009, at 17.24, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>
>>> Tom Lane wrote:
>>>> BTW, unless someone objects I'd like to make the name of that
>>>> function
>>>> PQinitSecurity.
>>> Agreed. Although, both PQinitSecure and PQinitSecurity are very
>>> general names. "Security" can mean a lot of things. Maybe the name
>>> should be more particular, like PQinitOpenSSL. I think its okay to
>>> reference the openssl project name being how this function is
>>> designed to work with openssl's library split.
>>>
>> +1
>>
>> It's quite likely that well want to support another ssl library in the
>> future. At least likely enough that any api we define should take it
>> into consideration.
>
> Are we sure we don't want to add a more general libpq initialization
> control at this time?
>
> PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT);

Could be an option but if we go down that path, it needs to be
PG_NO_OPENSSL_SSL_INIT and PG_NO_OPENSSL_CRYPTO_INIT....

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Bruce Momjian wrote:
>> Are we sure we don't want to add a more general libpq initialization
>> control at this time?
>>
>> PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT);

> Could be an option but if we go down that path, it needs to be
> PG_NO_OPENSSL_SSL_INIT and PG_NO_OPENSSL_CRYPTO_INIT....

And we get into the whole question of error handling, which is what
shot down that proposal last time.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > Bruce Momjian wrote:
> >> Are we sure we don't want to add a more general libpq initialization
> >> control at this time?
> >>
> >> PQinitOptions(PG_NO_SSL_INIT | PG_NO_CRYPTO_INIT);
>
> > Could be an option but if we go down that path, it needs to be
> > PG_NO_OPENSSL_SSL_INIT and PG_NO_OPENSSL_CRYPTO_INIT....
>
> And we get into the whole question of error handling, which is what
> shot down that proposal last time.

Can you remind me of the details? I don't remember that issue.
Currently PQinitSSL() returns void, so I don't see an issue there.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> And we get into the whole question of error handling, which is what
>> shot down that proposal last time.

> Can you remind me of the details? I don't remember that issue.
> Currently PQinitSSL() returns void, so I don't see an issue there.

The point is exactly the same as the complaint about turning PQinitSSL's
argument into a bitmask: if you are trying to define an extensible API
then you need a way for the app to determine whether all the bits it
passed were recognizable by the library.

I think we should stick with the simple two-argument function and not
try to design a solution for unknown problems. Otherwise we are right
back at the point where the previous thread petered out for lack of
consensus.

regards, tom lane

Tom Lane wrote:
>
> I think we should stick with the simple two-argument function and not

+1000000

Generic PQinit concept was already punted several times. Using a bit
mask for initsecure or something was also tried and rejected. The well
is dry on this.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> And we get into the whole question of error handling, which is what
> >> shot down that proposal last time.
>
> > Can you remind me of the details? I don't remember that issue.
> > Currently PQinitSSL() returns void, so I don't see an issue there.
>
> The point is exactly the same as the complaint about turning PQinitSSL's
> argument into a bitmask: if you are trying to define an extensible API
> then you need a way for the app to determine whether all the bits it
> passed were recognizable by the library.

It seems having the init function return the bits it recognized would be
the logical return value.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Mar 30, 2009 at 2:31 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Tom Lane wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> > Tom Lane wrote:
>> >> And we get into the whole question of error handling, which is what
>> >> shot down that proposal last time.
>>
>> > Can you remind me of the details?  I don't remember that issue.
>> > Currently PQinitSSL() returns void, so I don't see an issue there.
>>
>> The point is exactly the same as the complaint about turning PQinitSSL's
>> argument into a bitmask: if you are trying to define an extensible API
>> then you need a way for the app to determine whether all the bits it
>> passed were recognizable by the library.
>
> It seems having the init function return the bits it recognized would be
> the logical return value.

I'm disinclined to go this route because, as Tom pointed out upthread,
there's no evidence at all that this problem is an instance of some
more general class. If it turns out that we need to initialize other
things, they're likely to be totally unrelated to SSL, and whether
they are or not, they may not be things that can be indicated using a
bitmask. For example, what happens if we someday add support for a
separate SSL library called FooSSL, and we happen to need a "char *"
to describe how it should be initialized?
Or we might need a whole "char *" or "int" argument for some purpose
unrelated to SSL. Then we'll have a PQinit() function that pretends
to be a general-purpose initialization mechanism but really isn't.

With enough wrangling, we might be able to convince ourselves that
we've designed something which is general enough to cover all
contingencies, but I'm not sure there's any good reason to put in that
much work. Adding a new API call is not really that big a deal,
especially given that we're retaining the old one for backward
compatibility. If we discover we need something else in the future,
we'll just add an API call for that, too.

...Robert

Bruce Momjian wrote:
> It seems having the init function return the bits it recognized would be
> the logical return value.
>

That's what my first PQionitSecure patch did, actually it returned the
bits it didn't understand.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Robert Haas wrote:
> Or we might need a whole "char *" or "int" argument for some purpose
> unrelated to SSL. Then we'll have a PQinit() function that pretends
> to be a general-purpose initialization mechanism but really isn't.
>

I proposed a PQinit() that included a void data pointer argument. The
idea was PQinit(which, data, data_len) would be called multiple times to
init different things.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On Mon, Mar 30, 2009 at 2:59 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Robert Haas wrote:
>>
>> Or we might need a whole "char *" or "int" argument for some purpose
>> unrelated to SSL.  Then we'll have a PQinit() function that pretends
>> to be a general-purpose initialization mechanism but really isn't.
>>
>
> I proposed a PQinit() that included a void data pointer argument.  The idea
> was PQinit(which, data, data_len) would be called multiple times to init
> different things.

I guess that'd work but it might be overkill.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I guess that'd work but it might be overkill.

The real bottom line is that *all* the proposals for generic init
functions are overkill. We have no evidence that we need one and
no certainty about what the requirements for it would be if we did.

I think we should just do PQinitOpenSSL(2 args) and be done with it.

regards, tom lane

On Mon, Mar 30, 2009 at 3:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I guess that'd work but it might be overkill.
>
> The real bottom line is that *all* the proposals for generic init
> functions are overkill.  We have no evidence that we need one and
> no certainty about what the requirements for it would be if we did.
>
> I think we should just do PQinitOpenSSL(2 args) and be done with it.

I was pushing for generic PQinit, but have come around and agree with
this point of view.

merlin

Merlin Moncure wrote:
> On Mon, Mar 30, 2009 at 3:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> I guess that'd work but it might be overkill.
> >
> > The real bottom line is that *all* the proposals for generic init
> > functions are overkill. ?We have no evidence that we need one and
> > no certainty about what the requirements for it would be if we did.
> >
> > I think we should just do PQinitOpenSSL(2 args) and be done with it.
>
> I was pushing for generic PQinit, but have come around and agree with
> this point of view.

OK, we are _go_ then.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Chernow <ac(at)esilo(dot)com> writes:
> I modified my previous patch to use a two-argument function solution.

Applied with minor changes (rename to PQinitOpenSSL per discussion,
some comment and docs improvements).

regards, tom lane

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Merlin Moncure wrote:
>>> I think we should just do PQinitOpenSSL(2 args) and be done with it.
>>
>> I was pushing for generic PQinit, but have come around and agree with
>> this point of view.

> OK, we are _go_ then.

Done. Let's hope this thread stays dead.

regards, tom lane