Re: Logging of PAM Authentication Failure

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