Re: new libpq SSL connection option

Who anyone be opposed to "ssldir = path" as a connection option?
Currently, there is no way to change the homedir method ~/.postgresql
... or am I missing something? I am willing to supply a patch.

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

On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Who anyone be opposed to "ssldir = path" as a connection option? Currently,
> there is no way to change the homedir method ~/.postgresql ... or am I
> missing something? I am willing to supply a patch.

You mean something like the
http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com.

?

Alex Hunsaker wrote:
> On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Who anyone be opposed to "ssldir = path" as a connection option? Currently,
>> there is no way to change the homedir method ~/.postgresql ... or am I
>> missing something? I am willing to supply a patch.
>
> You mean something like the
> http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com.
>
> ?
>

yes, excately like that; apparently missed it. What is the status of
that patch? I see it was left in pending review .. is the fest is over?

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

On Fri, Dec 5, 2008 at 14:22, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Alex Hunsaker wrote:
>>
>> On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>>
>>> Who anyone be opposed to "ssldir = path" as a connection option?
>>> Currently,
>>> there is no way to change the homedir method ~/.postgresql ... or am I
>>> missing something? I am willing to supply a patch.
>>
>> You mean something like the
>>
>> http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com.
>>
>> ?
>>
>
> yes, excately like that; apparently missed it. What is the status of that
> patch? I see it was left in pending review .. is the fest is over?

I think all that is left is changing PGROOTCERT to PGSSLROOTCERT,
agreeing to IFDEF the params out or not oh
and this little bit:

> Magnus Hagander escribió:
> > On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> >> Something that's bothering me is that PGSSLKEY is inconsistent with the
> >> sslkey conninfo parameter. PGSSLKEY specifies an engine (basically a
> >> driver for specialized hardware AFAICT) from which the key is to be
> >> loaded, but sslkey is a simple filename. This means that there's no way
> >> to load a key from hardware if you want to specify it per connection.
> >> Not that I have any such hardware, but it looks bogus.

>I think the above consideration needs some discussion too. Committing
>it as-is doesn't seem OK because you can't change it later -- it's
>user-visible.

Alex Hunsaker wrote:
> On Fri, Dec 5, 2008 at 14:22, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Alex Hunsaker wrote:
>>> On Fri, Dec 5, 2008 at 13:58, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>>> Who anyone be opposed to "ssldir = path" as a connection option?
>>>> Currently,
>>>> there is no way to change the homedir method ~/.postgresql ... or am I
>>>> missing something? I am willing to supply a patch.
>>> You mean something like the
>>>
>>> http://archives.postgresql.org/message-id/34d269d40811202107q489e2be0h771762398dd9fcdb@mail.gmail.com.
>>>
>>> ?
>>>
>> yes, excately like that; apparently missed it. What is the status of that
>> patch? I see it was left in pending review .. is the fest is over?
>
> I think all that is left is changing PGROOTCERT to PGSSLROOTCERT,
> agreeing to IFDEF the params out or not oh
> and this little bit:
>
>> Magnus Hagander escribió:
>>> On Fri, Aug 1, 2008 at 13:31, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>>>> Something that's bothering me is that PGSSLKEY is inconsistent with the
>>>> sslkey conninfo parameter. PGSSLKEY specifies an engine (basically a
>>>> driver for specialized hardware AFAICT) from which the key is to be
>>>> loaded, but sslkey is a simple filename. This means that there's no way
>>>> to load a key from hardware if you want to specify it per connection.
>>>> Not that I have any such hardware, but it looks bogus.
>
>> I think the above consideration needs some discussion too. Committing
>> it as-is doesn't seem OK because you can't change it later -- it's
>> user-visible.

Here's a suggested update, which does *not* yet have documentation
updates. Changes from previous patch:

* Made all parameters available always and ignored for non-SSL connections
* Renamed PGROOTCERT to PGSSLROOTCERT
* Changes the way PGSSLKEY/sslkey is handled to this: When the string
does not contain a colon, it's treated as a filename. If it does contain
a colon (and on windows, if this colon is not in the second position
indicating a drive letter), it's treated as engine:key as before.

This should keep backwards compatibility.

I would also like to look this over completely - we only support loading
the KEY from the smartcard, but you still have to manually copy the
certificate to your machine. I don't know exactly how you're supposed to
do this in OpenSSL - some googling shows almost nobody else uses the
functions quite the way we do. So I'd like to look over if we need to do
more around this later, but this patch should make it possible to use
keys from different files without breaking backwards compatibility with
what we had before. So I'm considering that a separate step, that may
not be done in time for 8.4.

So. Comments?

//Magnus

Magnus Hagander wrote:
> * Renamed PGROOTCERT to PGSSLROOTCERT
>
> + <primary><envar>PGROOTCERT</envar></primary>

Looks like the old env name is still being used in the sgml docs.

I like the flexibility this patch offers.

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

Andrew Chernow wrote:
> Magnus Hagander wrote:
>> * Renamed PGROOTCERT to PGSSLROOTCERT
>>
>> + <primary><envar>PGROOTCERT</envar></primary>
>
> Looks like the old env name is still being used in the sgml docs.

Yes - I did say I hadn't updated the docs yet :-)

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I would also like to look this over completely - we only support loading
> the KEY from the smartcard, but you still have to manually copy the
> certificate to your machine. I don't know exactly how you're supposed to
> do this in OpenSSL - some googling shows almost nobody else uses the
> functions quite the way we do. So I'd like to look over if we need to do
> more around this later, but this patch should make it possible to use
> keys from different files without breaking backwards compatibility with
> what we had before. So I'm considering that a separate step, that may
> not be done in time for 8.4.

I'm confused here. Are you proposing user-visible changes that might
not get done in time for 8.4? I don't much like the idea that the API
is going to remain a moving target --- once 8.4 is out you will have
backwards compatibility constraints with whatever it does. It would
be better to avoid extending the feature set beyond what 8.3 can do
until you are certain it's right.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> I would also like to look this over completely - we only support loading
>> the KEY from the smartcard, but you still have to manually copy the
>> certificate to your machine. I don't know exactly how you're supposed to
>> do this in OpenSSL - some googling shows almost nobody else uses the
>> functions quite the way we do. So I'd like to look over if we need to do
>> more around this later, but this patch should make it possible to use
>> keys from different files without breaking backwards compatibility with
>> what we had before. So I'm considering that a separate step, that may
>> not be done in time for 8.4.
>
> I'm confused here. Are you proposing user-visible changes that might
> not get done in time for 8.4? I don't much like the idea that the API
> is going to remain a moving target --- once 8.4 is out you will have
> backwards compatibility constraints with whatever it does. It would
> be better to avoid extending the feature set beyond what 8.3 can do
> until you are certain it's right.

I'm not proposing anything yet - I haven't read up on it.

If it does change, though, only the engine-specific stuff would change
AFAICT. The new functionality in this patch is all around specifying
filenames, so that would not change.

And most likely it would not be a change in visible behavior if I get
the time to "fix" that - it'll either just be an under-the-hood change,
or more likely an extension to the parameters. I see no reason why it
should have any user-visible change at all on the stuff that's in this
patch.

//Magnus

Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert.
Maybe this should be an OR of the two since sslrootcert is not dependent
on homedir?

around line 970 src/interfaces/libpq/fe-secure.c

if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))

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

On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe
> this should be an OR of the two since sslrootcert is not dependent on
> homedir?
>
> around line 970 src/interfaces/libpq/fe-secure.c
>
> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))

Certainly, did we miss anywhere else?

Alex Hunsaker wrote:
> On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe
>> this should be an OR of the two since sslrootcert is not dependent on
>> homedir?
>>
>> around line 970 src/interfaces/libpq/fe-secure.c
>>
>> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))
>
> Certainly, did we miss anywhere else?

I agree it looks strange.

That said, have you actually seen a case where pqGetHomeDirectory()
fails? Or did you just notice the code?

//Magnus

Magnus Hagander wrote:
> Alex Hunsaker wrote:
>> On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>> Why does pqGetHomeDirectory have to succeed to use conn->sslrootcert. Maybe
>>> this should be an OR of the two since sslrootcert is not dependent on
>>> homedir?
>>>
>>> around line 970 src/interfaces/libpq/fe-secure.c
>>>
>>> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))
>>
>> Certainly, did we miss anywhere else?
>>

Yes, the homedir variable is used again later in the function. homedir could be
invalid since pqGetHomeDirectory might not get called. Maybe something like
below would do the trick:

/* when used, it can't be an empty string. */
*homedir = 0;

/* If either are NULL, homedir is needed */
if (!conn->sslrootcert || !conn->sslcrl)
pqGetHomeDirectory(homedir, sizeof(homedir));

/* one of them must be valid */
if (conn->sslrootcert || *homedir)

> I agree it looks strange.
>
> That said, have you actually seen a case where pqGetHomeDirectory()
> fails? Or did you just notice the code?
>

It can fail. For non-windows systems, it can fail on pqGetpwuid; which equates
to getpwuid or getpwuid_r failing. On windows, it can fail on SHGetFolderPath.
I really have no idea how likely either failure case is.

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

Andrew Chernow wrote:
> Magnus Hagander wrote:
>> Alex Hunsaker wrote:
>>> On Sat, Dec 27, 2008 at 11:50, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>>>> Why does pqGetHomeDirectory have to succeed to use
>>>> conn->sslrootcert. Maybe
>>>> this should be an OR of the two since sslrootcert is not dependent on
>>>> homedir?
>>>>
>>>> around line 970 src/interfaces/libpq/fe-secure.c
>>>>
>>>> if (conn->sslrootcert || pqGetHomeDirectory(homedir, sizeof(homedir)))
>>>
>>> Certainly, did we miss anywhere else?
>>>
>
> Yes, the homedir variable is used again later in the function. homedir
> could be invalid since pqGetHomeDirectory might not get called. Maybe
> something like below would do the trick:
>
> /* when used, it can't be an empty string. */
> *homedir = 0;
>
> /* If either are NULL, homedir is needed */
> if (!conn->sslrootcert || !conn->sslcrl)
> pqGetHomeDirectory(homedir, sizeof(homedir));
>
> /* one of them must be valid */
> if (conn->sslrootcert || *homedir)

How about this patch?

There's a lot of whitespace change due to indentation change, so I've
included a version without it for reference.

Also, it looks like we have the same problem with the private key, in
client_cert_cb(), agreed?

//Magnus

>
> Also, it looks like we have the same problem with the private key, in
> client_cert_cb(), agreed?
>
>
> //Magnus
>

Yeah, same issue in that function. I missed that. My grep'n was
obviously brain dead.

It almost feels like there should be some util functions like
get_sslrootcert(conn, path_buf, buf_size) for each of the SSL files.
Isolate the logic behind a function with a success or failure return
value. It would probably make the current code easier to read/follow.
Only downside is that pqGetHomeDirectory would be called twice in some
cases, but I really don't think that makes any noticeable performance
difference.

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

On Fri, Jan 2, 2009 at 03:13, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Andrew Chernow wrote:
>> Yes, the homedir variable is used again later in the function. homedir
>> could be invalid since pqGetHomeDirectory might not get called. Maybe
>> something like below would do the trick:

> How about this patch?

looks good to me (btw it was almost hard to believe the non-white
space one was the same patch! lol)

Alex Hunsaker wrote:
> On Fri, Jan 2, 2009 at 03:13, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Andrew Chernow wrote:
>>> Yes, the homedir variable is used again later in the function. homedir
>>> could be invalid since pqGetHomeDirectory might not get called. Maybe
>>> something like below would do the trick:
>
>> How about this patch?
>
> looks good to me (btw it was almost hard to believe the non-white
> space one was the same patch! lol)

Applied, along with a fix for the second place that had the issue.

//Magnus