Skip to content

fix error in process_inbound_packet #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

steffyP
Copy link
Member

@steffyP steffyP commented Mar 15, 2023

Background

Recently, we got notified about a customer that the RDS instance was not reachable, with the logs in LocalStack:

postgresql_proxy           : PostgreSQL proxy quit unexpectedly:
Traceback (most recent call last):
  File "/opt/code/localstack/.venv/lib/python3.10/site-packages/postgresql_proxy/proxy.py", line 222, in listen
    self.service_connection(key, mask)
  File "/opt/code/localstack/.venv/lib/python3.10/site-packages/postgresql_proxy/proxy.py", line 170, in service_connection
    conn.received(recv_data)
  File "/opt/code/localstack/.venv/lib/python3.10/site-packages/postgresql_proxy/connection.py", line 51, in received
    self.process_inbound_packet(header, body)
  File "/opt/code/localstack/.venv/lib/python3.10/site-packages/postgresql_proxy/connection.py", line 67, in process_inbound_packet
    self.redirect_conn.out_bytes += message
AttributeError: 'NoneType' object has no attribute 'out_bytes'

I was able to reproduce this issue locally, by running the following psql command for a RDS-instance created in LocalStack:

psql -d test -U test -p 4510 -h localhost
  • As we need a password here to connect to the instance, the command should normally include the -W option. When this option is included, the password prompt is shown instantly, and the connection succeeds
  • If the -W option is omitted, the psql still shows (a slightly different) password prompt, but before we can see some messages being send to the proxy:
    ----> Received message. Relaying. Speaker: proxy_2, message:
    b'\x00\x00\x00\n\x04\xd2\x16/\x00\x00'
    ----> Received message. Relaying. Speaker: postgresql_2, message:
    b'N'
    ----> Received message. Relaying. Speaker: proxy_2, message:
    b'\x00\x00\x00L\x00\x03\x00\x00client_encoding\x00UTF8\x00user\x00test\x00database\x00test\x00application_name\x00psql\x00\x00'
    ----> Received message. Relaying. Speaker: postgresql_2, message:
    b'R\x00\x00\x00\x08\x00\x00\x00\x03'
    ----> resetting connection proxy_2
    ----> Received message. Relaying. Speaker: postgresql_2, message:
    b'E\x00\x00\x00nSFATAL\x00VFATAL\x00C08P01\x00Mexpected password response, got message type 88\x00Fauth.c\x00L671\x00Rrecv_password_packet\x00\x00'
    ---> error, redirect_conn is not set
    
    • the last line is where the self.redirect_conn is None, and the connection to the proxy is broken with the error message from above. Somewhere in between the connection seems to be reset here.

Solution

In order to not terminate the proxy, we need a check in the process_inbound_packet that ensures the self.redirect_conn is still available.

@steffyP steffyP requested review from bentsku and whummer March 15, 2023 16:39
Copy link

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for looking into this. This seems to be a case that we overlooked, and more checks can't hurt.
Like we've seen earlier, it seems the client drops the connection to then ask for the password, and Postgres still tries to answer twice the client, first with some bytes then with some error message, but nobody is there to listen anymore.

It seems b'R\x00\x00\x00\x08\x00\x00\x00\x03' means something about authentication (https://www.postgresql.org/docs/current/protocol-message-formats.html), and that a password is required.

I suppose it goes this way:

  • if -W is included, the client knows it needs to prompt for the password first, then try to connect.
  • If it's not, it will still try to connect to Postgres first, but Postgres will respond that it needs a password first. So the client drops the connection, prompt a password, save it in memory then re-request a connection, this time including the password, and the server accepts it.

I think we were not covering the case where the server would still try to send a message after sending what could look like "a termination message" for the client.

So it's a go for me, maybe we can have @whummer opinion on this as well.

Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Great insights, kudos for sharing the detailed analysis! LGTM 👍

@steffyP steffyP merged commit 77bf840 into master Mar 16, 2023
@steffyP steffyP deleted the fix_exception_process_inbound_packet branch March 16, 2023 15:02
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