Skip to content
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

Disconnect Postgres clients attempting plaintext connection #10089

Closed

Conversation

sjmiller609
Copy link

@sjmiller609 sjmiller609 commented Aug 24, 2023

What does this PR do?

TCP disconnect when a Postgres client is attempting a plaintext connection, instead of just hanging the connection.

Motivation

#9929

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

@sjmiller609
Copy link
Author

sjmiller609 commented Aug 24, 2023

Having an error in tests like

fatal error: all goroutines are asleep - deadlock!

I expect that's because I'm trying to read at least 5 bytes before doing anything else, and that's invalid for short messages. The check at the top of the loop isn't doing what I thought, so maybe we have to return false, nil in the case of a short message, however then it seems that a short message that is an exact match for the initial bytes of PostgresStartTLS would have an issue? For example if there is some protocol that initially sends 4 bytes as int32(8).

@ldez
Copy link
Member

ldez commented Aug 24, 2023

can you rebase your PR onto the v3.0 branch?

@sjmiller609 sjmiller609 changed the base branch from master to v3.0 August 24, 2023 21:28
@sjmiller609
Copy link
Author

@ldez

Ok done! Thank you.

@nmengin nmengin added this to the 3.0 milestone Aug 28, 2023
@rtribotte
Copy link
Member

Hello @sjmiller609,

Thank you for your contribution!

We reviewed the changes and have some feedback on the drawbacks of it, as you already have identified.

The isPostgres function was previously returning as soon as one byte would not match one of the 8 bytes of the TLS startup message. This was done not to block protocols that could use less than 8 bytes.
With the changes, Traefik would now block on peek operation for protocols using less than 5 bytes.

While we don't have in mind any protocol for which the client initiates a first message lower than 5 bytes, we do not want to introduce a breaking behavior.

We lack a non-blocking way to check the first bytes sent by a client, and the only other alternative would be to make that behavior optional. Still, we are not comfortable with adding yet another very specific option to the Traefik static configuration.

For these reasons, we are declining this PR.

As a workaround, and as long as the entryPoint would not be used to handle non-TLS HTTP nor another non-TLS TCP service, one can attach a TCP catchAll router routing to an empty TCP server load balancer.
This way, psql connections that would not use the correct sslmode would be closed.

Example with the file provider:

tcp:
  routers:
    catchAll:
      entryPoints:
        - "[PostGRES dedicated entryPoint]"
      rule: "HostSNI(`*`)"
      service: empty
  
  services:
    empty:
      loadBalancer:
        servers: {}

@sjmiller609
Copy link
Author

It works! 🎉

apiVersion: v1
kind: ConfigMap
metadata:
  name: postgres-catch-all
  namespace: traefik
data:
  postgres-catch-all.yaml: |
    tcp:
      routers:
        catchAll:
          entryPoints:
            - "postgresql"
          rule: "HostSNI(`*`)"
          service: empty
      services:
        empty:
          loadBalancer:
            servers: {}

related helm chart values

          volumes:
          - name: 'postgres-catch-all'
            mountPath: "/config"
            type: configMap
          additionalArguments:
            - "--entryPoints.postgresql.address=:5432/tcp"
...
            - "--providers.file.filename=/config/postgres-catch-all.yaml"

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

Successfully merging this pull request may close these issues.

None yet

5 participants