PQinitSSL broken in some use cases

Andrew Chernow wrote:

I am using a library that links with and initializes libcrypto (ie.
CRYPTO_set_locking_callback) but not SSL. This causes problems even
when using PQinitSSL(FALSE) because things like SSL_library_init(); are
not called (unless I manually call them, copy and paste code from
fe-secure.c which may change). If libpq does init ssl, it overwrites
(and breaks) the other library's crypto.

Shouldn't crypto and ssl init be treated as two different things? If
not, how does one determine a version portable way of initializing SSL
in a manner required by libpq? Lots of apps using encryption but don't
necessarily use ssl, so they need to know how to init ssl for libpq.

I didn't realize they were could be initialized separately, so we really
don't have an answer for you. This is the first time I have heard of
this requirement.

--
Bruce Momjian <bruce@momjian.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:

I am using a library that links with and initializes libcrypto (ie.
CRYPTO_set_locking_callback) but not SSL. This causes problems even
when using PQinitSSL(FALSE) because things like SSL_library_init(); are
not called (unless I manually call them, copy and paste code from
fe-secure.c which may change). If libpq does init ssl, it overwrites
(and breaks) the other library's crypto.

Shouldn't crypto and ssl init be treated as two different things? If
not, how does one determine a version portable way of initializing SSL
in a manner required by libpq? Lots of apps using encryption but don't
necessarily use ssl, so they need to know how to init ssl for libpq.

I didn't realize they were could be initialized separately, so we really
don't have an answer for you. This is the first time I have heard of
this requirement.

Just bringing it to everyones attention. I have no idea how common this
use case is or if it deserves a patch. From your comments, it sounds
uncommon.

How we came across this:
We have an internal library that links with libcrypto.so but not
libssl.so. The library uses digests and ciphers from libcrypto. It
initializes libcrypto for thread safety and seeds the PRNG. So, one of
our applications is linking with both libpq and this library; which
caused the conflict.

How we worked around it:
We solved it by copying the SSL init sequence from fe-secure.c. Doesn't
seem like something that would change very often. So we
init_our_library(), PQinitSSL(0) and then do a few lines of SSL init stuff.

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

Andrew Chernow wrote:

Bruce Momjian wrote:

Andrew Chernow wrote:

I am using a library that links with and initializes libcrypto (ie.
CRYPTO_set_locking_callback) but not SSL. This causes problems even
when using PQinitSSL(FALSE) because things like SSL_library_init();
are not called (unless I manually call them, copy and paste code from
fe-secure.c which may change). If libpq does init ssl, it overwrites
(and breaks) the other library's crypto.

Shouldn't crypto and ssl init be treated as two different things? If
not, how does one determine a version portable way of initializing
SSL in a manner required by libpq? Lots of apps using encryption but
don't necessarily use ssl, so they need to know how to init ssl for
libpq.

I didn't realize they were could be initialized separately, so we really
don't have an answer for you. This is the first time I have heard of
this requirement.

Just bringing it to everyones attention. I have no idea how common this
use case is or if it deserves a patch. From your comments, it sounds
uncommon.

How we came across this:
We have an internal library that links with libcrypto.so but not
libssl.so. The library uses digests and ciphers from libcrypto. It
initializes libcrypto for thread safety and seeds the PRNG. So, one of
our applications is linking with both libpq and this library; which
caused the conflict.

How we worked around it:
We solved it by copying the SSL init sequence from fe-secure.c. Doesn't
seem like something that would change very often. So we
init_our_library(), PQinitSSL(0) and then do a few lines of SSL init stuff.

Seems unusual, but certainly not "nearly impossible". But we're back to
the discussions around the WSA code - our API provides no really good
place to do this, so perhaps we should just clearly document how it's
done and how to work around it?

//Magnus

On Tue, Feb 10, 2009 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

How we worked around it:
We solved it by copying the SSL init sequence from fe-secure.c. Doesn't
seem like something that would change very often. So we
init_our_library(), PQinitSSL(0) and then do a few lines of SSL init stuff.

Seems unusual, but certainly not "nearly impossible". But we're back to
the discussions around the WSA code - our API provides no really good
place to do this, so perhaps we should just clearly document how it's
done and how to work around it?

I'm not so sure that's appropriate in this case. I think the existing
libpq behavior is simply wrong...crypto and ssl are two separate
libraries and PQinitSSL does not expose the necessary detail. This is
going to break apps in isolated but spectacular fashion when they link
to both pq and crypto for different reasons.

maybe invent a special value to PQinitSSL for ssl only init?

merlin

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

How we worked around it:
We solved it by copying the SSL init sequence from fe-secure.c. Doesn't
seem like something that would change very often. So we
init_our_library(), PQinitSSL(0) and then do a few lines of SSL init stuff.

Seems unusual, but certainly not "nearly impossible". But we're back to
the discussions around the WSA code - our API provides no really good
place to do this, so perhaps we should just clearly document how it's
done and how to work around it?

I'm not so sure that's appropriate in this case. I think the existing
libpq behavior is simply wrong...crypto and ssl are two separate
libraries and PQinitSSL does not expose the necessary detail. This is
going to break apps in isolated but spectacular fashion when they link
to both pq and crypto for different reasons.

They could, but nobody has reported it yet, so it's not a common scenario.

maybe invent a special value to PQinitSSL for ssl only init?

We could do that, I guess. However, if an application passes this in to
an old version of libpq, there is no way to know that it didn't know
about it.

//Magnus

maybe invent a special value to PQinitSSL for ssl only init?

We could do that, I guess. However, if an application passes this in to
an old version of libpq, there is no way to know that it didn't know
about it.

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...

It would help if there were a PQgetLibraryVersion() function.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

We could do that, I guess. However, if an application passes this in to
an old version of libpq, there is no way to know that it didn't know
about it.

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...

It would help if there were a PQgetLibraryVersion() function.

Help how? There is nothing an app can do to work around the problem
AFAICS. Or if there were, we should just document it and not change
the code --- the use case for this is evidently too narrow to justify
complicating libpq's API even more.

regards, tom lane

Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

We could do that, I guess. However, if an application passes this in to
an old version of libpq, there is no way to know that it didn't know
about it.

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...

It would help if there were a PQgetLibraryVersion() function.

Help how? There is nothing an app can do to work around the problem
AFAICS. Or if there were, we should just document it and not change
the code --- the use case for this is evidently too narrow to justify
complicating libpq's API even more.

Sure, there is a way to work around it in this case. By manually
initializing ssl+crypto even though you only need crypto, and then tell
libpq you have taken care of the initialization.

//Magnus

On Tue, Feb 10, 2009 at 10:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

We could do that, I guess. However, if an application passes this in to
an old version of libpq, there is no way to know that it didn't know
about it.

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...

It would help if there were a PQgetLibraryVersion() function.

Help how? There is nothing an app can do to work around the problem
AFAICS. Or if there were, we should just document it and not change
the code --- the use case for this is evidently too narrow to justify
complicating libpq's API even more.

It would let you assert that you were running against a version of
libpq that has the functionality that you are attempting to use, thus
eliminating the risk of silent failure.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...
It would help if there were a PQgetLibraryVersion() function.

Help how? There is nothing an app can do to work around the problem
AFAICS. Or if there were, we should just document it and not change
the code --- the use case for this is evidently too narrow to justify
complicating libpq's API even more.

It would let you assert that you were running against a version of
libpq that has the functionality that you are attempting to use, thus
eliminating the risk of silent failure.

If that's all you want, then PQinitSSLExtended is a perfectly good
answer. Your app will fail to link if you try to use a library
version that hasn't got it.

I think documenting the workaround is a sufficient answer though.

regards, tom lane

Magnus Hagander wrote:

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 9:32 AM, Magnus Hagander <magnus@hagander.net> wrote:

How we worked around it:
We solved it by copying the SSL init sequence from fe-secure.c. Doesn't
seem like something that would change very often. So we
init_our_library(), PQinitSSL(0) and then do a few lines of SSL init stuff.

Seems unusual, but certainly not "nearly impossible". But we're back to
the discussions around the WSA code - our API provides no really good
place to do this, so perhaps we should just clearly document how it's
done and how to work around it?

I'm not so sure that's appropriate in this case. I think the existing
libpq behavior is simply wrong...crypto and ssl are two separate
libraries and PQinitSSL does not expose the necessary detail. This is
going to break apps in isolated but spectacular fashion when they link
to both pq and crypto for different reasons.

They could, but nobody has reported it yet, so it's not a common scenario.

Agreed.

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

--
Bruce Momjian <bruce@momjian.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:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...
It would help if there were a PQgetLibraryVersion() function.

Help how? There is nothing an app can do to work around the problem
AFAICS. Or if there were, we should just document it and not change
the code --- the use case for this is evidently too narrow to justify
complicating libpq's API even more.

It would let you assert that you were running against a version of
libpq that has the functionality that you are attempting to use, thus
eliminating the risk of silent failure.

If that's all you want, then PQinitSSLExtended is a perfectly good
answer. Your app will fail to link if you try to use a library
version that hasn't got it.

I agree. I was thinking that there might not be enough interest in
this feature to add an API call just to support it, but I thought
PQgetVersion() might be a more general solution.

I think documenting the workaround is a sufficient answer though.

I don't have a strong opinion on that one way or the other, but Andrew
seemed to be concerned that he was cut-and-pasting a fair amount of
code.

...Robert

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

That initializes both libcrypto and libssl. The problem arises when
libcrypto has been initialized but libssl has not.

...Robert

Tom Lane wrote:

If that's all you want, then PQinitSSLExtended is a perfectly good
answer.

How about PQinitCrypto(bool do_init), which would default to TRUE if
never called. PQinitSSL already handles the SSL part, just need control
over initializing crypto.

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

Andrew Chernow wrote:

Tom Lane wrote:

If that's all you want, then PQinitSSLExtended is a perfectly good
answer.

How about PQinitCrypto(bool do_init), which would default to TRUE if
never called. PQinitSSL already handles the SSL part, just need control
over initializing crypto.

Folks, we need a lot more demand before we add functions to libpq.

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

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

Robert Haas wrote:

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

That initializes both libcrypto and libssl. The problem arises when
libcrypto has been initialized but libssl has not.

So initialize ssl in your application? What is the problem?

--
Bruce Momjian <bruce@momjian.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:52 AM, Bruce Momjian <bruce@momjian.us> wrote:

Robert Haas wrote:

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

That initializes both libcrypto and libssl. The problem arises when
libcrypto has been initialized but libssl has not.

So initialize ssl in your application? What is the problem?

then libpq doesn't work.

merlin

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 11:52 AM, Bruce Momjian <bruce@momjian.us> wrote:

Robert Haas wrote:

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

That initializes both libcrypto and libssl. The problem arises when
libcrypto has been initialized but libssl has not.

So initialize ssl in your application? What is the problem?

then libpq doesn't work.

Why?

--
Bruce Momjian <bruce@momjian.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:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Well, you could create PQinitSSLExtended, but, as you say, the use
case is pretty narrow...
It would help if there were a PQgetLibraryVersion() function.

Help how? There is nothing an app can do to work around the problem
AFAICS. Or if there were, we should just document it and not change
the code --- the use case for this is evidently too narrow to justify
complicating libpq's API even more.

It would let you assert that you were running against a version of
libpq that has the functionality that you are attempting to use, thus
eliminating the risk of silent failure.

If that's all you want, then PQinitSSLExtended is a perfectly good
answer. Your app will fail to link if you try to use a library
version that hasn't got it.

I think documenting the workaround is a sufficient answer though.

I don't think you can get way with that this time. wsa cleanup was a
mainly harmless side effect. This is a nasty 'maybe it works, maybe
it doesn't' virtually impossible to debug problem. We caught it on a
particular platform (windows, iirc) when deep in our code a mutex call
deadlocked when it shouldn't have, after weeks of working ok.
debugging nightmare.

PQinitSSL is *broken*. It's always been broken. Since it already
takes a parameter, I say add a special switch...the backwards
compatibility danger doesn't seem too bad.

merlin

On Tue, Feb 10, 2009 at 11:54 AM, Bruce Momjian <bruce@momjian.us> wrote:

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 11:52 AM, Bruce Momjian <bruce@momjian.us> wrote:

Robert Haas wrote:

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

That initializes both libcrypto and libssl. The problem arises when
libcrypto has been initialized but libssl has not.

So initialize ssl in your application? What is the problem?

then libpq doesn't work.

PQinitSSL is required if you want to make any ssl calls (it does some
libpq setup beyond the ssl library initialization).

merlin

On Tue, Feb 10, 2009 at 4:57 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

PQinitSSL is *broken*. It's always been broken. Since it already
takes a parameter, I say add a special switch...the backwards
compatibility danger doesn't seem too bad.

Add a switch to what? I get very nervous for our Windows users when
people start talking about changing the libpq API (for those that
don't know, Windows doesn't have DLL versioning like Unix - so any
non-backwards compatible API change really needs a corresponding
filename change to avoid pain and suffering).

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Bruce Momjian wrote:

Andrew Chernow wrote:

Tom Lane wrote:

If that's all you want, then PQinitSSLExtended is a perfectly good
answer.

How about PQinitCrypto(bool do_init), which would default to TRUE if
never called. PQinitSSL already handles the SSL part, just need control
over initializing crypto.

Folks, we need a lot more demand before we add functions to libpq.

Honestly, I'm not suggesting that a function is added. If others decide
to do that, I think the function's purpose should be to init crypto. We
don't need another way to init ssl.

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

On Tue, Feb 10, 2009 at 12:03 PM, Dave Page <dpage@pgadmin.org> wrote:

On Tue, Feb 10, 2009 at 4:57 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

PQinitSSL is *broken*. It's always been broken. Since it already
takes a parameter, I say add a special switch...the backwards
compatibility danger doesn't seem too bad.

Add a switch to what? I get very nervous for our Windows users when
people start talking about changing the libpq API (for those that
don't know, Windows doesn't have DLL versioning like Unix - so any
non-backwards compatible API change really needs a corresponding
filename change to avoid pain and suffering).

PQinitSSL(SSL_ONLY) or something, where the constant is carefully
chosen to not be accidentally passed in by older libpq users.

merlin

On Tue, Feb 10, 2009 at 12:03 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

On Tue, Feb 10, 2009 at 11:54 AM, Bruce Momjian <bruce@momjian.us> wrote:

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 11:52 AM, Bruce Momjian <bruce@momjian.us> wrote:

Robert Haas wrote:

Would someone remind me why turning off ssl initialization in libpq does
not work for this case?

That initializes both libcrypto and libssl. The problem arises when
libcrypto has been initialized but libssl has not.

So initialize ssl in your application? What is the problem?

then libpq doesn't work.

PQinitSSL is required if you want to make any ssl calls (it does some
libpq setup beyond the ssl library initialization).

that was worded badly. Rather, PQinitSSL is required if you need to
use ssl features withing libpq.

merlin

On Tue, Feb 10, 2009 at 5:05 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

PQinitSSL(SSL_ONLY) or something, where the constant is carefully
chosen to not be accidentally passed in by older libpq users.

Ahh, OK. That would be painless.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 12:03 PM, Dave Page <dpage@pgadmin.org> wrote:

On Tue, Feb 10, 2009 at 4:57 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

PQinitSSL is *broken*. It's always been broken. Since it already
takes a parameter, I say add a special switch...the backwards
compatibility danger doesn't seem too bad.

Add a switch to what? I get very nervous for our Windows users when
people start talking about changing the libpq API (for those that
don't know, Windows doesn't have DLL versioning like Unix - so any
non-backwards compatible API change really needs a corresponding
filename change to avoid pain and suffering).

PQinitSSL(SSL_ONLY) or something, where the constant is carefully
chosen to not be accidentally passed in by older libpq users.

So how are you planinng to deal with it when your application passes
that to a version of libpq that doesn't support it?

//Magnus

On Tue, Feb 10, 2009 at 1:02 PM, Magnus Hagander <magnus@hagander.net> wrote:

Merlin Moncure wrote:

On Tue, Feb 10, 2009 at 12:03 PM, Dave Page <dpage@pgadmin.org> wrote:

On Tue, Feb 10, 2009 at 4:57 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

PQinitSSL is *broken*. It's always been broken. Since it already
takes a parameter, I say add a special switch...the backwards
compatibility danger doesn't seem too bad.

Add a switch to what? I get very nervous for our Windows users when
people start talking about changing the libpq API (for those that
don't know, Windows doesn't have DLL versioning like Unix - so any
non-backwards compatible API change really needs a corresponding
filename change to avoid pain and suffering).

PQinitSSL(SSL_ONLY) or something, where the constant is carefully
chosen to not be accidentally passed in by older libpq users.

So how are you planinng to deal with it when your application passes
that to a version of libpq that doesn't support it?

well, either nothing, which is no worse off than we are now, or
backpatch the fix. probably nothing :-)

merlin

On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@esilo.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@sss.pgh.pa.us> wrote:

Andrew Chernow <ac@esilo.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@gmail.com> writes:

On Fri, Feb 13, 2009 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.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@gmail.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@gmail.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@esilo.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@momjian.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@momjian.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@momjian.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@momjian.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@gmail.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@sss.pgh.pa.us> wrote:

Bruce Momjian <bruce@momjian.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.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@esilo.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@esilo.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@momjian.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@esilo.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@momjian.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@gmail.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@gmail.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@momjian.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@momjian.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@gmail.com> wrote:

On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.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@gmail.com> wrote:

On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Merlin Moncure <mmoncure@gmail.com> writes:

On Mon, Mar 30, 2009 at 10:22 AM, Robert Haas <robertmhaas@gmail.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@gmail.com> wrote:

On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.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@momjian.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@gmail.com> wrote:

On Mon, Mar 30, 2009 at 9:36 AM, Andrew Chernow <ac@esilo.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@esilo.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@hagander.net> wrote:

On 30 mar 2009, at 17.24, Andrew Chernow <ac@esilo.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@esilo.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@momjian.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@esilo.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@hagander.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@hagander.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.us> wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.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@esilo.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@gmail.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@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.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@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Andrew Chernow <ac@esilo.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@momjian.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