Skip to content

Check the message after sending a PasswordMessage#5006

Merged
roji merged 2 commits intonpgsql:mainfrom
yoshihikoueno:feature/check_authentication_ok
Mar 21, 2023
Merged

Check the message after sending a PasswordMessage#5006
roji merged 2 commits intonpgsql:mainfrom
yoshihikoueno:feature/check_authentication_ok

Conversation

@yoshihikoueno
Copy link
Copy Markdown
Contributor

@yoshihikoueno yoshihikoueno commented Mar 21, 2023

This PR fixes an issue that Npgsql fails to connect to a PostgreSQL cluster via PgPool-II as both the client and the server fall into waiting status.

Issue description

Npgsql currently assumes the message sent from the server after sending PasswordMessage to be AuthenticationOk.
However, this assumption doesn't always stand as the server may send some other messages other than AuthenticationOk such as another authentication request.

That occurs when Npgsql is trying to connect to a PgPool-II server, where the following procedure is required as PgPool-II wants to have the password in both Cleartext and MD5 formats:

  1. [Client -> Server] StartupMessage
  2. [Server -> Client] AuthenticationCleartextPassword
  3. [Client -> Server] PasswordMessage
  4. [Server -> Client] AuthenticationMD5Password
  5. [Client -> Server] PasswordMessage
  6. [Server -> Client] AuthenticationOk
  7. [Server -> Client] ReadyForQuery

With the current implementation of Npgsql, the AuthenticationMD5Password sent by PgPool-II is erroneously considered to be AuthenticationOk by Npgsql. At this point, Npgsql waits for ReadyForQuery and PgPool-II waits for PasswordMessage, which causes a timeout error.

Solution description

Clients such as psql and pgAdmin can connect to PgPool-II just fine with the procedure above, so making Npgsql check the message sent from the server after PasswordMessage seems to be a solution.

Thank you for checking out this PR. Please let me know if there's anything that is missing or needs improvements with this PR.

…thenticationOk, otherwise follow the authentication request sent by the server
Copy link
Copy Markdown
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am not necessarily against merging this, but can you provide a bit more context on what this means, and why it authentication would need to take place twice, once in cleartext and once in MD5?

Also, note that this is explicitly against the PostgreSQL protocol docs, which state the folliowing:

For all authentication methods except GSSAPI, SSPI and SASL, there is at most one request and one response.

So PgPool-II seems to be violating the PostgreSQL protocol on this point.

@yoshihikoueno
Copy link
Copy Markdown
Contributor Author

yoshihikoueno commented Mar 21, 2023

@roji
Thank you for taking the time to have a look at this.

This PR makes it possible for Npgsql to react to multiple authentication requests just the same as psql and pgAdmin.

As far as I understand, PgPool-II requires this two-step authentication because PgPool-II, which doesn't necessarily have information of the user that a client is trying to connect as, needs to authenticate PgPool-II itself to the backend PostgreSQL servers and authenticate the client on behalf of the actual PostgreSQL servers behind the PgPool-II.
With the first authentication request (cleartext), PgPool-II gets the user-password mapping, which will be used to set up connections to the possibly multiple PostgreSQL backends. Then PgPool-II needs to authenticate a client connecting to the PgPool-II, thus requesting another authentication.
This whole process seems unintuitive but considering that PgPool-II has the connection pooling feature, it makes sense that it needs to do so.

Also, note that this is explicitly against the PostgreSQL protocol docs, which state the following:

For all authentication methods except GSSAPI, SSPI and SASL, there is at most one request and one response.

So PgPool-II seems to be violating the PostgreSQL protocol on this point.

I'm not sure about the interpretation of the cited text but the implementation of the official PostgreSQL client, libpq, does allow multiple authentication requests to occur. (ref: After sending authentication information to the server, it checks if the returned message is AuthenticationOk [L3621]. If it is not, it goes back to the beginning of the loop [L3635], which makes it possible to react to the second authentication request.)
So this PR will align the behavior of Npgsql with the behavior of libpq (psql, pgAdmin, and etc).

@roji
Copy link
Copy Markdown
Member

roji commented Mar 21, 2023

Thanks for the additional information @yoshihikoueno. I think it's fine for us to do this.

@roji roji enabled auto-merge (squash) March 21, 2023 11:08
@roji roji merged commit fbc7538 into npgsql:main Mar 21, 2023
@vonzshik vonzshik added this to the 8.0.0 milestone Mar 21, 2023
@vonzshik vonzshik added the bug label Mar 21, 2023
roji pushed a commit that referenced this pull request Mar 21, 2023
@roji
Copy link
Copy Markdown
Member

roji commented Mar 21, 2023

Backported to 7.0.4 via ca43399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants