Logging of PAM Authentication Failure

On Wed, May 8, 2013 at 10:40 PM, Amit Langote <amitlangote09@gmail.com> wrote:

When client authentication method is set to "pam" in pg_hba.conf,
connecting using psql results in logging of authentication failure
even before a password prompt is provided, nonetheless user is
subsequently able to connect by providing a password. Following is
what is logged:

Password: LOG: pam_authenticate failed: Conversation error
FATAL: PAM authentication failed for user "amit"

To see what's going on I debugged psql and found that without a -W
option, this is bound to happen, since psql first attempts to connect
and without a password (which it doesn't know is required for the
first time), it fails and subsequently prompts for password. Correct
password then leads to successful connection.

I tried to observe the behavior with md5 method (without -W) and
observed that no authentication failure is logged, since server
probably behaves differently in response to the psql's first
connection request in that case. But, pam method leads to it being
logged.

Is this a problem?

Not really. We could potentially fix it by extending the wire
protocol to allow the server to respond to the client's startup packet
with a further challenge, and extend libpq to report that challenge
back to the user and allow sending a response. But that would break
on-the-wire compatibility, which we haven't done in a good 10 years,
and certainly wouldn't be worthwhile just for this.

We'd also need to be careful not to create information leaks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, May 8, 2013 at 10:40 PM, Amit Langote <amitlangote09@gmail.com> wrote:

I tried to observe the behavior with md5 method (without -W) and
observed that no authentication failure is logged, since server
probably behaves differently in response to the psql's first
connection request in that case. But, pam method leads to it being
logged.

auth_failed() in src/backend/libpq/auth.c intentionally logs nothing for
STATUS_EOF status (ie, client closed the connection without responding).
But it looks like the PAM code path doesn't have a way to return that
status code, even when pam_passwd_conv_proc() knows that that's what
happened, and intentionally printed no log message itself (around line
1870 in HEAD). If there's another response code we could return through
the PAM layer, this could be fixed, and I think it should be.

Is this a problem?

Not really. We could potentially fix it by extending the wire
protocol to allow the server to respond to the client's startup packet
with a further challenge, and extend libpq to report that challenge
back to the user and allow sending a response. But that would break
on-the-wire compatibility, which we haven't done in a good 10 years,
and certainly wouldn't be worthwhile just for this.

It's not the wire protocol that's the problem; it's that libpq's client
API doesn't provide a way to ask the calling application for a password
in the midst of a connection attempt.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

auth_failed() in src/backend/libpq/auth.c intentionally logs nothing for
STATUS_EOF status (ie, client closed the connection without responding).
But it looks like the PAM code path doesn't have a way to return that
status code, even when pam_passwd_conv_proc() knows that that's what
happened, and intentionally printed no log message itself (around line
1870 in HEAD). If there's another response code we could return through
the PAM layer, this could be fixed, and I think it should be.

So if I get this correctly, does this mean the only thing that needs
to be fixed is unnecessary logging or is there a problem with
authentication exchange itself in case of PAM? Also, when you say PAM
layer, is that pam_passwd_conv_proc() that needs to be able to return
an alternative status code?

--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

auth_failed() in src/backend/libpq/auth.c intentionally logs nothing for
STATUS_EOF status (ie, client closed the connection without responding).
But it looks like the PAM code path doesn't have a way to return that
status code, even when pam_passwd_conv_proc() knows that that's what
happened, and intentionally printed no log message itself (around line
1870 in HEAD). If there's another response code we could return through
the PAM layer, this could be fixed, and I think it should be.

So if I get this correctly, does this mean the only thing that needs
to be fixed is unnecessary logging or is there a problem with
authentication exchange itself in case of PAM? Also, when you say PAM
layer, is that pam_passwd_conv_proc() that needs to be able to return
an alternative status code?

Following is the point server requests psql to send password when
PAM is enabled.

backend/libpq/auth.c:1861

if (strlen(passwd) == 0)
{
/*
* Password wasn't passed to PAM the first time around -
* let's go ask the client to send a password, which we
* then stuff into PAM.
*/
sendAuthRequest(pam_port_cludge, AUTH_REQ_PASSWORD);
passwd = recv_password_packet(pam_port_cludge);
if (passwd == NULL)
{
/*
* Client didn't want to send password. We
* intentionally do not log anything about this.
*/
goto fail;

...

return PAM_CONV_ERR;

This code seems to me expecting for psql to send password without
closing current connnection.On the other hand psql does as
follows.

bin/psql/startup.c: 227

pset.db = PQconnectdbParams(keywords, values, true);
free(keywords);
free(values);

if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
password == NULL &&
pset.getPassword != TRI_NO)
{
PQfinish(pset.db);
password = simple_prompt(password_prompt, 100, false);
new_pass = true;
}

psql at once disconnects the current connection and reconnects
with this new password, so pam_conv_err is observed in server.

It seems to be a kind of protocol-mimatching. Client should'nt
disconnect for password request or server should fit to what psql
does. Is this wrong?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

This code seems to me expecting for psql to send password without
closing current connnection.On the other hand psql does as
follows.

bin/psql/startup.c: 227

pset.db = PQconnectdbParams(keywords, values, true);
free(keywords);
free(values);

if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
password == NULL &&
pset.getPassword != TRI_NO)
{
PQfinish(pset.db);
password = simple_prompt(password_prompt, 100, false);
new_pass = true;
}

psql at once disconnects the current connection and reconnects
with this new password, so pam_conv_err is observed in server.

It seems to be a kind of protocol-mimatching. Client should'nt
disconnect for password request or server should fit to what psql
does. Is this wrong?

In fact, this is the behavior with all the authentication methods that
require a password. But, it is only in the case of PAM authentication
that auth_failed() logs error when first connection attempt is made
(without password), since the STATUS_EOF is not passed to it in that
case.
If we did not drop the connection (unlike what we do now) and
re-attempted connection with the password added to conn, would the
backend's authentication state still be waiting for the password? Can
we do away without having to create a second connection?
--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

In fact, this is the behavior with all the authentication methods that
require a password. But, it is only in the case of PAM authentication
that auth_failed() logs error when first connection attempt is made
(without password), since the STATUS_EOF is not passed to it in that
case.

Well, if we are allowed to use a bit ugry way, the attached patch
seems to cope with this issue. As far as I can see there's no
problem since pg_fe_sendauth() refueses to send empty password.

Any suggestions?

If we did not drop the connection (unlike what we do now) and
re-attempted connection with the password added to conn, would the
backend's authentication state still be waiting for the password? Can
we do away without having to create a second connection?

Sorry, I've read there incorrectly. I had understood the code
after sendAuthRequest in pam_passwd_conv_proc but it is used
indeed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Well, if we are allowed to use a bit ugry way, the attached patch
seems to cope with this issue. As far as I can see there's no
problem since pg_fe_sendauth() refueses to send empty password.

Any suggestions?

That seems to do the trick. This probably solves the problem that I
originally posted.

Sorry, I've read there incorrectly. I had understood the code
after sendAuthRequest in pam_passwd_conv_proc but it is used
indeed.

Though, I am still not sure why we drop the existing connection and
start all over again but now with the newly entered password. This
kind of seems to leave the protocol state machine (as in
PQconnectPoll() ) halfway (after pg_fe_sendauth() failed) in the first
connection attempt for the auth requests requiring the password (or
others, too?). Although, sticking to this design may have to do with
the problems of doing otherwise that I am unaware of.

--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

Is it right that it is only in the case a password prompt is needed
that a new connection is created after dropping the just-failed
connection?
I created a patch which enables it to use the existing connection in
such a case (unlike what we currently do). It modifies
connectDBComplete() and PQconnectPoll() to also include states
pertaining to password being accepted from the user. That is, the
state machine in PQconnectPoll() is further extended to include a
connection state called CONNECTION_ASKING_PASSWORD which is entered
when server sends AUTH_REQ_MD5 or AUTH_REQ_PASSWORD auth requests.
These two request types require a password to be entered by the user.
There is a new PostgresPollingStatusType value called
PGRES_POLLING_WAITING_PASSWORD which is the polling status while a
password is being entered by the user.

When user enters the password the PQconnectPoll() continues forward in
CONNECTION_ASKING_PASSWORD wherein it sends the password to the server
(by calling pg_fe_sendauth() and this time with a potentially correct
password) and later goes back to CONNECTION_AWAITING_RESPONSE to read
server's response to the password just entered where it either
receives authorization OK or error response thus completing the
connection start-up process.

The backend waits for the password until authentication timeout
happens in which case the client can not send the password anymore
since the backend has exited due to authentication timeout. I wonder
if this is one of the reasons why this has not already been
implemented?

Comments?

On Tue, May 14, 2013 at 11:22 AM, Amit Langote <amitlangote09@gmail.com> wrote:

Well, if we are allowed to use a bit ugry way, the attached patch
seems to cope with this issue. As far as I can see there's no
problem since pg_fe_sendauth() refueses to send empty password.

Any suggestions?

That seems to do the trick. This probably solves the problem that I
originally posted.

Sorry, I've read there incorrectly. I had understood the code
after sendAuthRequest in pam_passwd_conv_proc but it is used
indeed.

Though, I am still not sure why we drop the existing connection and
start all over again but now with the newly entered password. This
kind of seems to leave the protocol state machine (as in
PQconnectPoll() ) halfway (after pg_fe_sendauth() failed) in the first
connection attempt for the auth requests requiring the password (or
others, too?). Although, sticking to this design may have to do with
the problems of doing otherwise that I am unaware of.

--
Amit Langote

--

Amit Langote

On Tue, May 14, 2013 at 11:20 AM, Amit Langote <amitlangote09@gmail.com> wrote:

Hello,

Is it right that it is only in the case a password prompt is needed
that a new connection is created after dropping the just-failed
connection?
I created a patch which enables it to use the existing connection in
such a case (unlike what we currently do). It modifies
connectDBComplete() and PQconnectPoll() to also include states
pertaining to password being accepted from the user. That is, the
state machine in PQconnectPoll() is further extended to include a
connection state called CONNECTION_ASKING_PASSWORD which is entered
when server sends AUTH_REQ_MD5 or AUTH_REQ_PASSWORD auth requests.
These two request types require a password to be entered by the user.
There is a new PostgresPollingStatusType value called
PGRES_POLLING_WAITING_PASSWORD which is the polling status while a
password is being entered by the user.

When user enters the password the PQconnectPoll() continues forward in
CONNECTION_ASKING_PASSWORD wherein it sends the password to the server
(by calling pg_fe_sendauth() and this time with a potentially correct
password) and later goes back to CONNECTION_AWAITING_RESPONSE to read
server's response to the password just entered where it either
receives authorization OK or error response thus completing the
connection start-up process.

The backend waits for the password until authentication timeout
happens in which case the client can not send the password anymore
since the backend has exited due to authentication timeout. I wonder
if this is one of the reasons why this has not already been
implemented?

Comments?

Please add patches here so they don't get forgotten:

https://commitfest.postgresql.org/action/commitfest_view/open

Do we really need to add *2* new libpq functions just to support this?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Please add patches here so they don't get forgotten:

https://commitfest.postgresql.org/action/commitfest_view/open

Do we really need to add *2* new libpq functions just to support this?

I will add the patches to commitfest after reviewing it a bit to see
if we can do away without having to create more new functions than
necessary and make appropriate changes.

--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Is it right that it is only in the case a password prompt is needed
that a new connection is created after dropping the just-failed
connection?

It's quite doubtful.\:-p The sequense seems fragile to say the
least. Inserting password request state into the client-side state
machine looks quite reasonable.

I created a patch which enables it to use the existing connection in
such a case (unlike what we currently do). It modifies
connectDBComplete() and PQconnectPoll() to also include states
pertaining to password being accepted from the user. That is, the
state machine in PQconnectPoll() is further extended to include a
connection state called CONNECTION_ASKING_PASSWORD which is entered
when server sends AUTH_REQ_MD5 or AUTH_REQ_PASSWORD auth requests.

Great! The new client state seems to be effective also for MD5. But
it seems to break existing libpq client doing the same authentication
sequence as current psql. Some means would be necessary to switch the
behavior when password is not previously provided but needed by the
server, or make the first half of the connection sequence to be
compatible to the current sequence - in other words - It should be
that when the user finds stauts is CONNECTION_BAD and
PQconnectionNeedsPassword() == true, the user can throw away the
connection and make new connection providing password, and also can
send password on existing connection.

the old style

| db = PQconnectXXXX();
| if (PQstatus(db) == CONNECTION_BAD && PQconnectionNeedsPassword(db))
| {
| PQfinish(db);
| value[..] = password = <some means to read password>;
| db = PQconnectXXXX();
| if (PQstatus(db) == CONNECTION_BAD)
| <error>

and the new style

| db = PQconnectXXXX();
| if (PQconnectionNeedsPassword(db))
| PQsendPassword(db, password);
| if (PQstatus(db) == CONNECTION_BAD)
| <error>

should be standing together.

Where, PQsendPassword is combined function of PQcopyPassword and
PQcontinuedbConnect. If the only porpose of these functions is sending
password then these functions are needed to be separately.

What do you think for the compatibility and simpler API.

These two request types require a password to be entered by the user.
There is a new PostgresPollingStatusType value called
PGRES_POLLING_WAITING_PASSWORD which is the polling status while a
password is being entered by the user.

When user enters the password the PQconnectPoll() continues forward in
CONNECTION_ASKING_PASSWORD wherein it sends the password to the server
(by calling pg_fe_sendauth() and this time with a potentially correct
password) and later goes back to CONNECTION_AWAITING_RESPONSE to read
server's response to the password just entered where it either
receives authorization OK or error response thus completing the
connection start-up process.

The backend waits for the password until authentication timeout
happens in which case the client can not send the password anymore
since the backend has exited due to authentication timeout. I wonder
if this is one of the reasons why this has not already been
implemented?

Comments?

Hmmm. On current implement, server is not running while the client is
reading password so the authentication timeout is provided only for
hostile clients. Conversely, the new sequence can enforce true
authentication timeout. It results in error after leaving the password
prompt for 60 seconds. I suppose that more desirable behavior in spite of
the poor message..

| Password: <waiting over 60 seconds and ENTER RETURN>
| psql: fe_sendauth: error sending password authentication

The point at issue now seems how to inform the timeout to the client
under reading password, especially prohibiting using thread nor
SIGALRM.

Providing password input function in libpq like below might make it
viable using select(2).

PQsendPassword(prompt="Passowrd: ", in_fd = stdin)

Any thoughts?

regareds,

--
Kyotaro Horiguchi

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 15, 2013 at 11:04 PM, Kyotaro HORIGUCHI
<kyota.horiguchi@gmail.com> wrote:

Is it right that it is only in the case a password prompt is needed
that a new connection is created after dropping the just-failed
connection?

It's quite doubtful.\:-p The sequense seems fragile to say the
least. Inserting password request state into the client-side state
machine looks quite reasonable.

Looking at current code (well, pseudo-code!) :

do
{
new_pass = false;
<create a new connection>
if (CONNECTION_BAD && NEEDS_PASSWORD && password == NULL && ! FORCE_NO_PASSOWRD)
{
PQfinish(<current_connection>);
password = simple_prompt() ;
new_pass = true;
}
}while(new_pass)

So, it looks like the loop will be repeated only if an authentication
method requiring the user to enter password is encountered in the
PQconnectPoll() which are AUTH_REQ_MD5 & AUTH_REQ_PASSWORD. As you can
see in the following code fragment from pg_fe_sendauth() which
apparently sets conn->password_needed:

case AUTH_REQ_MD5:
case AUTH_REQ_PASSWORD:
conn->password_needed = true;
if (conn->pgpass == NULL || conn->pgpass[0] == '\0')
{
printfPQExpBuffer(&conn->errorMessage,
PQnoPasswordSupplied);
return STATUS_ERROR;
}
if (pg_password_sendauth(conn, conn->pgpass, areq) != STATUS_OK)
{
printfPQExpBuffer(&conn->errorMessage,
"fe_sendauth: error sending password authentication\n");
return STATUS_ERROR;
}
break;

this seems to be the only code path that causes conn->password_needed
to be set to true. So, these seem to be only cases when a prompt will
be provided and new_pass would become true causing the
drop-and-reconnect by repetition of the loop. Am I missing some other
case when this might happen?

I created a patch which enables it to use the existing connection in
such a case (unlike what we currently do). It modifies
connectDBComplete() and PQconnectPoll() to also include states
pertaining to password being accepted from the user. That is, the
state machine in PQconnectPoll() is further extended to include a
connection state called CONNECTION_ASKING_PASSWORD which is entered
when server sends AUTH_REQ_MD5 or AUTH_REQ_PASSWORD auth requests.

Great! The new client state seems to be effective also for MD5. But
it seems to break existing libpq client doing the same authentication
sequence as current psql. Some means would be necessary to switch the
behavior when password is not previously provided but needed by the
server, or make the first half of the connection sequence to be
compatible to the current sequence - in other words - It should be
that when the user finds stauts is CONNECTION_BAD and
PQconnectionNeedsPassword() == true, the user can throw away the
connection and make new connection providing password, and also can
send password on existing connection.

The first half of connection sequence remains same except for one
change: in PQconnectPoll(), when in case CONNECTION_AWAITING_RESPONSE,
if server sends md5/password authentication request, it returns
PGRES_POLLING_WAITING_PASSWORD, which, back in connectDBComplete()
sets conn->password = true and conn->status =
CONNECTION_ASKING_PASSWORD. Back in main(), this causes a password
prompt and then the second half of the connection sequence. Hence
pg_fe_sendauth() is not called in this first half unless it's a
different authentication method than md5 and password.

the old style

| db = PQconnectXXXX();
| if (PQstatus(db) == CONNECTION_BAD && PQconnectionNeedsPassword(db))
| {
| PQfinish(db);
| value[..] = password = <some means to read password>;
| db = PQconnectXXXX();
| if (PQstatus(db) == CONNECTION_BAD)
| <error>

and the new style

| db = PQconnectXXXX();
| if (PQconnectionNeedsPassword(db))
| PQsendPassword(db, password);
| if (PQstatus(db) == CONNECTION_BAD)
| <error>

should be standing together.

I see this accounts for CONNECTION_BAD (if any) in the first half. But
this CONNECTION_BAD has other reasons than conn->password_needed as
far as I can imagine since conn->password_needed would only be set in
connectDBComplete() in PGRES_POLLING_WAITING_PASSWORD. So, this
CONNECTION_BAD would require some different processing. Thoughts?

Where, PQsendPassword is combined function of PQcopyPassword and
PQcontinuedbConnect. If the only porpose of these functions is sending
password then these functions are needed to be separately.

What do you think for the compatibility and simpler API.

I think one function PQsendPassword(PGconn*, char *) would be
sufficient which would contain the code of both PQcopyPassword() and
PQcontinuedbConnect(). I would complete the connection sequence by
running its second half.

The backend waits for the password until authentication timeout
happens in which case the client can not send the password anymore
since the backend has exited due to authentication timeout. I wonder
if this is one of the reasons why this has not already been
implemented?

Comments?

Hmmm. On current implement, server is not running while the client is
reading password so the authentication timeout is provided only for
hostile clients. Conversely, the new sequence can enforce true
authentication timeout. It results in error after leaving the password
prompt for 60 seconds. I suppose that more desirable behavior in spite of
the poor message..

| Password: <waiting over 60 seconds and ENTER RETURN>
| psql: fe_sendauth: error sending password authentication

The point at issue now seems how to inform the timeout to the client
under reading password, especially prohibiting using thread nor
SIGALRM.

Providing password input function in libpq like below might make it
viable using select(2).

PQsendPassword(prompt="Passowrd: ", in_fd = stdin)

Any thoughts?

So, do you here propose to change simple_prompt() that would be able
to detect an authentication timeout on server and exit with an
appropriate message? I think that should be done.

I will try to revise the patch to incorporate these considerations and
post a revised patch.

--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I created a patch which enables it to use the existing connection in
such a case (unlike what we currently do). It modifies
connectDBComplete() and PQconnectPoll() to also include states
pertaining to password being accepted from the user. That is, the
state machine in PQconnectPoll() is further extended to include a
connection state called CONNECTION_ASKING_PASSWORD which is entered
when server sends AUTH_REQ_MD5 or AUTH_REQ_PASSWORD auth requests.

Great! The new client state seems to be effective also for MD5. But
it seems to break existing libpq client doing the same authentication
sequence as current psql. Some means would be necessary to switch the
behavior when password is not previously provided but needed by the
server, or make the first half of the connection sequence to be
compatible to the current sequence - in other words - It should be
that when the user finds stauts is CONNECTION_BAD and
PQconnectionNeedsPassword() == true, the user can throw away the
connection and make new connection providing password, and also can
send password on existing connection.

The first half of connection sequence remains same except for one
change: in PQconnectPoll(), when in case CONNECTION_AWAITING_RESPONSE,
if server sends md5/password authentication request, it returns
PGRES_POLLING_WAITING_PASSWORD, which, back in connectDBComplete()
sets conn->password = true and conn->status =
CONNECTION_ASKING_PASSWORD. Back in main(), this causes a password
prompt and then the second half of the connection sequence. Hence
pg_fe_sendauth() is not called in this first half unless it's a
different authentication method than md5 and password.

One more thing that I forgot to mention is that connection sequence
would enter CONNECTION_ASKING_PASSWORD in the first half, only if
password is currently not set to a non-empty value that is (
conn->pgpass ==NULL || conn->pgpass[0] = '\0' ) is true. I was
wondering what would be the case for other applications using libpq
when they return from connectionDBComplete() with conn->status set to
CONNECTION_ASKING_PASSWORD, provided they have not set conn->pgpass to
a non-empty value.If they are currently handling this based on
CONNECTION_BAD, then this change does no good to them. In fact there
needs to be a way for them to get CONNECTION_BAD.
Since, this whole patch is about not having to drop-and-reconnect *in
case of password prompt*, how it changes libpq for other applications
also needs to be addressed here. especially for md5/password
authentication cases. Currently, any attempt to connect using empty or
NULL password results in CONNECTION_BAD for all libpq based clients.
Thoughts?

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sorry that I am writing separate emails on the same topic.
I seem to have a solution that allows us to accomplish what we are
trying to without much change to the existing libpq interface
(especially what to expect about return values and connection state
that we are in when we return from connectDBComplete() and
PQconnectPoll() ).

Following are required changes roughly:

1] in src/bin/psql/startup.c, main()

if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
password == NULL &&
pset.getPassword != TRI_NO)
{
password = simple_prompt(password_prompt, 100, false);
/* How would this detect authentication_timeoue and exit
accordingly ?*/
PQsendPassword(pset.db, password);
}

And there is no do{...}while(new_pass); unlike current code.

2] in src/interfaces/libpq/fe-connect.c, new function: void
PQsendPassword(PGconn *conn, char *password) /*suggest better name?
*/

void PQsendPassword(PGconn *conn, char *password)
{
conn->pgpass = password;
conn->status = CONNECTION_SENDING_PASSWORD; /*suggest better
name for the status? */

(void) connectDBComplete(conn);

}

3] in src/interfaces/libpq/fe-connect.c, connectDBComplete(PGconn
*conn), No change required. :-)

4] in in src/interfaces/libpq/fe-connect.c, PQconnectPoll(PGconn *conn)

a) add a new case for both switch's (one before and after keep_going: )

/* These are writing states, so we just proceed. */
case CONNECTION_STARTED:
case CONNECTION_MADE:
case CONNECTION_SENDING_PASSWORD:
break;
...
...
keep_going:
...
...
case CONNECTION_SENDING_PASSWORD:
{
/*
** Note that conn->pghost must be
non-NULL if we are going to
** avoid the Kerberos code doing a
hostname look-up.
**/
if (pg_fe_sendauth(areq, conn) != STATUS_OK)
{
conn->errorMessage.len =
strlen(conn->errorMessage.data);
goto error_return;
}
conn->errorMessage.len =
strlen(conn->errorMessage.data);

/*
** Just make sure that any data sent
by pg_fe_sendauth is
** flushed out. Although this
theoretically could block, it
** really shouldn't since we don't
send large auth responses.
**/
if (pqFlush(conn))
goto error_return;

/*
* Now go to read the server's
response to password just sent
* */
conn->status = CONNECTION_AWAITING_RESPONSE;
return PGRES_POLLING_READING;
}

5] in src/interfaces/libpq/libpq-fe.h, add a new intermediate connection state

/*
* Although it is okay to add to these lists, values which become unused
* should never be removed, nor should constants be redefined - that would
* break compatibility with existing code.
*/

typedef enum
{
CONNECTION_OK,
CONNECTION_BAD,
/* Non-blocking mode only below here */

/*
* The existence of these should never be relied upon - they should only
* be used for user feedback or similar purposes.
*/
CONNECTION_STARTED, /* Waiting for
connection to be made. */
CONNECTION_MADE, /* Connection OK;
waiting to send. */
CONNECTION_AWAITING_RESPONSE, /* Waiting for a
response from the

* postmaster. */
CONNECTION_AUTH_OK, /* Received
authentication; waiting for
*
backend startup. */
CONNECTION_SETENV, /* Negotiating environment. */
CONNECTION_SSL_STARTUP, /* Negotiating SSL. */
CONNECTION_NEEDED, /* Internal state:
connect() needed */
CONNECTION_SENDING_PASSWORD
} ConnStatusType;

As you can probably see this requires minimum libpq changes:

1] Add one more connection state: CONNECTION_SENDING_PASSWORD
2] Add one more function: PQsendPassword(PGconn*, char*)
3] Modify PQconnectPoll() to allow to handle an intermediate
CONNECTION_SENDING_PASSWORD state for the clients which use
PQsendPassword() to send a password that user entered in between a
connection sequence over an existing connection.

Comments?

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attached herewith is a patch based on description in my previous mail.
This patch would need revision since the error situation in case of
authentication timeout on the server needs to be handled; probably in
simple_prompt()?

--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 16, 2013 at 3:53 PM, Amit Langote <amitlangote09@gmail.com> wrote:

Attached herewith is a patch based on description in my previous mail.
This patch would need revision since the error situation in case of
authentication timeout on the server needs to be handled; probably in
simple_prompt()?

Forgot attaching the patch in the last mail; find it with this one.

--
Amit Langote

On 2013-05-16 17:35:10 +0900, Amit Langote wrote:

On Thu, May 16, 2013 at 3:53 PM, Amit Langote <amitlangote09@gmail.com> wrote:

Attached herewith is a patch based on description in my previous mail.
This patch would need revision since the error situation in case of
authentication timeout on the server needs to be handled; probably in
simple_prompt()?

Forgot attaching the patch in the last mail; find it with this one.

The patch seems to have windows line endings...

--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -62,7 +62,11 @@ typedef enum
* backend startup. */
CONNECTION_SETENV,			/* Negotiating environment. */
CONNECTION_SSL_STARTUP,		/* Negotiating SSL. */
-	CONNECTION_NEEDED			/* Internal state: connect() needed */
+	CONNECTION_NEEDED,			/* Internal state: connect() needed */
+	CONNECTION_SENDING_PASSWORD		/* An intermediate state to help client send a password
+						 * over an existing connection	
+						 */
+						
} ConnStatusType;

typedef enum
@@ -258,6 +262,9 @@ extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
#define PQsetdb(M_PGHOST,M_PGPORT,M_PGOPT,M_PGTTY,M_DBNAME) \
PQsetdbLogin(M_PGHOST, M_PGPORT, M_PGOPT, M_PGTTY, M_DBNAME, NULL, NULL)

+/* send a password that the server asked for halfway between a connection sequence */
+extern void PQsendPassword(PGconn *conn, char *password);
+

I unfortunately have to say I don't really see the point of this. The
cost of the additional connection attempt is rather low and we have to
deal with the superflous attempts anyway since there will be old libpqs
around for years. Why is this worth the effort?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 16, 2013 at 8:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2013-05-16 17:35:10 +0900, Amit Langote wrote:

On Thu, May 16, 2013 at 3:53 PM, Amit Langote <amitlangote09@gmail.com> wrote:

Attached herewith is a patch based on description in my previous mail.
This patch would need revision since the error situation in case of
authentication timeout on the server needs to be handled; probably in
simple_prompt()?

Forgot attaching the patch in the last mail; find it with this one.

The patch seems to have windows line endings...

My bad. I will reupload the proper patch later.

--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -62,7 +62,11 @@ typedef enum
* backend startup. */
CONNECTION_SETENV,                      /* Negotiating environment. */
CONNECTION_SSL_STARTUP,         /* Negotiating SSL. */
-     CONNECTION_NEEDED                       /* Internal state: connect() needed */
+     CONNECTION_NEEDED,                      /* Internal state: connect() needed */
+     CONNECTION_SENDING_PASSWORD             /* An intermediate state to help client send a password
+                                              * over an existing connection
+                                              */
+
} ConnStatusType;

typedef enum
@@ -258,6 +262,9 @@ extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
#define PQsetdb(M_PGHOST,M_PGPORT,M_PGOPT,M_PGTTY,M_DBNAME) \
PQsetdbLogin(M_PGHOST, M_PGPORT, M_PGOPT, M_PGTTY, M_DBNAME, NULL, NULL)

+/* send a password that the server asked for halfway between a connection sequence */
+extern void PQsendPassword(PGconn *conn, char *password);
+

I unfortunately have to say I don't really see the point of this. The
cost of the additional connection attempt is rather low and we have to
deal with the superflous attempts anyway since there will be old libpqs
around for years. Why is this worth the effort?

While full connection sequence (with proper authentication exchanges)
appears to go smoothly for other cases (authentication methods), it
doesn't quite in this case probably because accounting for such a case
was not considered to be as important. But while investigating about
the PAM issue (original subject of this thread), it turned out that
the occurrence of that minor issue was due to this behavior in libpq.
Addition of this one more state (viz. input password in between an
ongoing connect sequence) to the possible connection states helps
account for such instances where this kind of password exchange has to
happen (as in psql for md5 and password). Also, others using libpq can
either use it if they wish to or just do away without having to worry
about this state. This patch does not introduce any change as to what
connection state applications can expect to be in after they return
from connectDBComplete() or PQconnectPoll(). On the other hand, we can
now enter these functions with one more possible connection state
which PQconnectPoll() is now able to handle. As a side effect, it also
helps avoid drop-and-reconnect occurrences at times.

Albeit, it is up to application (using libpq) whether to go via this
new alternate path or stick to drop-and-reconnect, should a need to
input password in between connect sequence arise. We can consider
having such an option, probably just for the sake of completeness
(even if to account for a possibly rare method of authentication
exchange)

--
Amit Langote

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, May 16, 2013 at 8:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:

I unfortunately have to say I don't really see the point of this. The
cost of the additional connection attempt is rather low and we have to
deal with the superflous attempts anyway since there will be old libpqs
around for years. Why is this worth the effort?

While full connection sequence (with proper authentication exchanges)
appears to go smoothly for other cases (authentication methods), it
doesn't quite in this case probably because accounting for such a case
was not considered to be as important. But while investigating about
the PAM issue (original subject of this thread), it turned out that
the occurrence of that minor issue was due to this behavior in libpq.

I have to agree with Andres that it's not clear this is a reasonable
fix. To get rid of extra reconnections this way will require not merely
upgrading libpq, but upgrading every single application that uses libpq
and is capable of prompting its user for a password. The odds are
pretty good that that won't ever happen.

The real complaint here is that the server-side PAM auth code path is
losing the information that the client chose to disconnect rather than
offer a password, and thus logging a message that we could do without.
What's wrong with just fixing that?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers