Re: Massively annoying bug still not fixed in v1.20 :-( - Mailing list pgadmin-support
| From | Craig Ringer |
|---|---|
| Subject | Re: Massively annoying bug still not fixed in v1.20 :-( |
| Date | |
| Msg-id | CAMsr+YFgPA690RZhutwaPdH2PTVJCva_0V1hVUHbLBzo=P3P_w@mail.gmail.com Whole thread Raw |
| In response to | Re: Massively annoying bug still not fixed in v1.20 :-( (Magnus Hagander <magnus@hagander.net>) |
| List | pgadmin-support |
On 20 December 2014 at 00:08, Magnus Hagander <magnus@hagander.net> wrote:
On Dec 19, 2014 5:02 PM, "Craig Ringer" <craig@2ndquadrant.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/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services
pgadmin-support by date: