Re: Massively annoying bug still not fixed in v1.20 :-(

On 20 December 2014 at 00:08, Magnus Hagander <magnus(at)hagander(dot)net> wrote:

>
> On Dec 19, 2014 5:02 PM, "Craig Ringer" <craig(at)2ndquadrant(dot)com> wrote:
> >
> > On 12/19/2014 11:57 PM, Dave Page wrote:
> > > Right - we'd have to store the entries somewhere based on the target
> > > server and the SSH config, and dynamically rebuilt the pgpass file
> > > during the connection process. That seems a) ugly and b) very fragile.
> >
> > Darn. I thought libpq had a callback for a password prompt, but it
> doesn't.
> >
> > Guess we should add that. If libpq gets an auth request from the server
> > and has no password from the connection string, it should invoke a
> > callback (if supplied) that lets the client supply a password
> dynamically.
> >
>
> We definitely should. And we should make sure we design it not to just
> support passwords but anything we might need to unlock an authentication -
> say a x509 certificate (doesn't have to be the same function, but it should
> be part of the design considerations for the feature).
>
> /Magnus
>

Hm. I'm not too sure about how to make it general and useful. X.509 certs
are a particular problem because of the IMO stupid way we handle them in
libpq. Certificate handling in libpq needs to be revamped but I think
that's really a separate issue.

The callback needs to know:

* The connection identity, i.e. the PGconn
* The auth mode the server requested

which is the signature of pg_fe_sendauth(AuthRequest areq, PGconn *conn).

It might also want the PQconninfoOption array, but it can get that
from PQconninfo(PGconn*).

The other question is how to report results and what form results must
take. Can we just pass a PQconninfoOption (or a pointer to its val member)
to be modified? That might be simplest, if it's generally useful enough.

Looking at the various auth cases:

* AUTH_REQ_MD5 and AUTH_REQ_PASSWORD both require that conn->pgpass be
non-null and non-empty. A callback to populate it on demand is absolutely
trivial. This is the most important case.

* AUTH_REQ_SSPI and AUTH_REQ_GSS have much more complex requirements.
Depending on compilation options and the value of conn->gsslib either a
Kerberos library or Windows SSPI might be used to satisfy the request.
libpq needs information like the krbsrvname for this, but I don't think
it's using anything sensitive that can't simply be put in the initial
connection string.

* AUTH_REQ_SCM_CREDS is dead and uninteresting

* X.509 client certificate authentication happens separately, during SSL
negotiation before the handshake. If only an X.509 cert is required to
authenticate the client it the server just sends AUTH_REQ_OK. So an auth
callback doesn't really fit this, it's a fairly separate thing.

In other words, the only thing that's part of the auth process its self for
which we need a callback looks to be a password.

Dave, others: Do you see any reason why we'd need to supply things like
conn->krbsrvname via a callback during connection setup, instead of just
setting them in the initial connection string?

If not, I think we should keep this simple:

typedef char (*PQpasswordCallback)(PGconn *conn)

... and require it to return a dynamically allocated NULL terminated C
string, or NULL if it doesn't wish to override the default.

It's unfortunately necessary to consider that on certain platforms (ahem
Windows ahem) the callback function might be running in a module linked to
a different C library - so it's not safe to free() memory within libpq that
was malloc()'d within the callback. We can solve that by exposing a
PQmalloc(...) and declaring that memory allocated as a callback result must
use that function. We already have PQfreemem(...) for the opposite
direction so that seems to be the way to go, calling it PQallocmem(...) .

If you feel the callback should be made more general, then an alternative
form would be:

typedef void (*PQauthCallback)(AuthRequest areq, PGconn
*conn, PQconninfoOption* opt);

i.e. "As part of an auth request from the server for auth type areq on
connection conn, please populate opt->val for the options entry opt however
you feel is appropriate. Or leave it unchanged, whatever you want. I'm
about to look at this option".

Client certificate selection should have a better mechanism, but happens at
a different phase and is much more complex, so it doesn't make any sense to
try to use the same callback. (In particular, you can't select the right
client certificate until you know what the server expects it to be signed
with, and you only know that during the SSL handshake. This callback will
have to use a bunch of OpenSSL code, and it makes no sense at all to
conflate it with a password callback.) hen it comes to SSL certs,
personally I think we should be using libnss and a proper keystore, but I'm
not dumb enough to volunteer to rewrite OpenSSL code to libnss.

Oh, damn. I just realised this won't help Dave with PgAdmin, because
blasted pg_dump and pg_restore run as external processes and we can't
supply a callback function to an external process. I suppose the only
option for them is going to be supplying a password in a conninfo string.

--
Craig Ringer http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Training & Services