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

Support TLS PostgreSQL connections in notification_listener #3503

Merged
merged 5 commits into from
May 3, 2022

Conversation

mb-shorai
Copy link
Contributor

@mb-shorai mb-shorai commented Apr 27, 2022

Hi, this PR implements TLS PSQL connections support for the notification_listener, as various managed database providers allow encrypted connections only, and it is generally a bad idea to disable encryption.

Related issue: #2097

With these changes it is possible to connect to databases with both TLS support and without. Please note that certificate verification is currently disabled, because it fits my use-case, and I don't have any experience with Rust whatsoever to make it configurable to support more use-cases. So I would be grateful if someone takes it over from here.

You can test it e.g. locally when running Postgres with the following command:

version: '3'
services:
  postgres:
    image: postgres
    ports:
      - '5432:5432'
    command:
      [
        "postgres",
        "-cshared_preload_libraries=pg_stat_statements",
        "-cssl=on",
        "-cssl_cert_file=/etc/ssl/certs/ssl-cert-snakeoil.pem",
        "-cssl_key_file=/etc/ssl/private/ssl-cert-snakeoil.key"
      ]
    environment:
      POSTGRES_USER: graph-node
      POSTGRES_PASSWORD: let-me-in
      POSTGRES_DB: graph-node

And monitor Postgres connections with the following command:

SELECT datname,usename,ssl,client_addr,application_name
FROM pg_stat_ssl
JOIN pg_stat_activity
ON pg_stat_ssl.pid = pg_stat_activity.pid;

@azf20
Copy link
Contributor

azf20 commented Apr 27, 2022

Thanks @mb-shorai ! @lutter interested in your thoughts here?

Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

Nice! I just checked that our current setup still works with these changes, and it does! Could you rebase onto latest master? I'll then merge this PR.

@lutter
Copy link
Collaborator

lutter commented May 3, 2022

NM - just found the rebase button 😎

@lutter lutter merged commit c5bd0cf into graphprotocol:master May 3, 2022
@lutter
Copy link
Collaborator

lutter commented May 3, 2022

Merged it - thanks a ton for doing this!

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