pg_auto_failover icon indicating copy to clipboard operation
pg_auto_failover copied to clipboard

Prevent unauthorized access from localhost

Open JelteF opened this issue 5 years ago • 19 comments

Right now the only way to run pg_auto_ctl is to completely trust the node it runs on, because any connection made from "localhost" is allowed to authenticate using "trust". Even when usig --skip-pg-hba. This is because we do not change the default --auth argument when we run initdb. https://www.postgresql.org/docs/12/app-initdb.html https://github.com/citusdata/pg_auto_failover/blob/7986d6a9e07b6dc925f7bffacba52b0a1eab27a2/src/bin/pg_autoctl/pgctl.c#L755 I tried changing it to --auth=reject --auth-local=trust, but this causes connection errors on the pg_auto_ctl side.

It would be nice to use UNIX socket connections from pg_auto_ctl to postgres instead of TCP connections over localhost. This way we can make sure that only the postgres user (or whatever user used pg_auto_ctl create can connect to postgres without auth).

JelteF avatar May 19 '20 09:05 JelteF

One idea to fix this is to allow specifying --pghost=/tmp. This currently does not work for multiple reasons:

  1. --pghost is not implemented on create monitor
  2. I think we should set unix_socket_directories.
  3. BONUS: We should change the initdb command to not create the open pg_hba.conf

JelteF avatar May 19 '20 09:05 JelteF

I'm slowly ramping up on this, so please let me know if the following makes sense/correct or not:

1)

Right now the only way to run pg_auto_ctl is to completely trust the node it runs on, because any connection made from "localhost" is allowed to authenticate using "trust"

I cannot understand this very well. When I debug/read-code, what I see is both on the monitor or Postgres data nodes, pg_auto_ctl connects to local server over unix domain socket, not through localhost

See pg_setup_get_local_connection_string (): https://github.com/citusdata/pg_auto_failover/blob/7986d6a9e07b6dc925f7bffacba52b0a1eab27a2/src/bin/pg_autoctl/pgsetup.c#L715-L797

And, when I print the connection strings to the logs, I see the following, which are all unix domain sockets:

connectionString: port=5432 dbname=template1 host=/tmp
connectionString: port=5432 dbname=pg_auto_failover host=/tmp user=autoctl_node
connectionString: port=9700 dbname=postgres host=/tmp
connectionString: port=9701 dbname=postgres host=/tmp

2)

I tried changing it to --auth=reject --auth-local=trust, but this causes connection errors on the pg_auto_ctl side.

I also cannot understand the reason for adding --auth=reject. Aren't we already updating pg_hba.conf for remote accesses? Did you mean --auth-host which is to control connections through localhost?

3)

My high level question: Is this the only security problem if an unauthorized user connects to the machine via ssh which runs pg_auto_ctl?

For example, I couldn't see anything special for protecting the file with KEEPER_STATE_FILENAME name. What if someone deletes the file?

Overall, do we need anything further to protect the databases and the HA system?

My Proposal

I think we should do 3 things:

  • My understanding is that Postgres' unix_socket_permissions setting actually aims to improve this situation. We want to prevent unauthorized OS users connecting via unix domain socket. We should probably set it to 0770 (only user and group) or maybe even 0700 (only the user that initiated the database can connect). With that, we prevent unauthorized users connecting via unix domain sockets. I verified that locally.

  • We should add a rule to pg_hba.conf, to prevent unauthorized TCP connections over localhost, as we do for remote accesses.

  • We should go over the code, and make sure that all local connections are always done over unix domain socket, not through tcp localhost

onderkalaci avatar May 25 '20 07:05 onderkalaci

To answer your questions:

  1. When I run the following command:
make install -j9 && pg_autoctl create monitor --pgdata monitor --ssl-self-signed --run --auth trust -vvv

There are messages like this (with host=localhost):

Connecting to "port=5432 dbname=pg_auto_failover host=localhost user=autoctl_node"
  1. Yes, --auth-host=reject --auth-local=trust result in the same in pg_hba.conf file. I agree that's more as expected.

JelteF avatar May 25 '20 07:05 JelteF

Woops, pressed send too fast: 3. Maybe not, but it's one an operator cannot fix. File system permissions an operator can change. This is not the case when Postgres is listening with trust on localhost and we require this. Then an operator cannot tighten this security down and effectively any user has postgres user permisions. Regarding your specific point about KEEPER_STATE_FILENAME. This is stored in XDG_DATA_HOME, which by default is ~/.local/share. Assuming the operator runs pg_autoctl as the postgres user. This would be in /home/postgres/.local/share, which is probably at least not writable by anyone.

About your proposal:

  1. I agree about unix_socket_permissions, I think 0700
  2. This can be done by passing --auth-host=reject --auth-local=trust to initdb.
  3. I agree this should be the default, but possibly some people will still want to connect over the network. So, I think it's good to keep the --pghost flag (and add it to create monitor).

JelteF avatar May 25 '20 08:05 JelteF

There are messages like this (with host=localhost):

For future reference: My installation/configuration might be different, but I couldn't see localhost in the output on my Mac:

MacBook-Pro-Onders:pg_auto_failover onderkalaci$ pg_autoctl create monitor --pgdata /Users/onderkalaci/Documents/pg_auto_failover/monitor --ssl-self-signed --run --auth trust -vvv
12:27:54 TRACE pgsetup.c:1459: pgsetup_validate_ssl_settings
12:27:54 INFO  pgsetup.c:1561: Using default --ssl-mode "require"
12:27:54 INFO  pgsetup.c:1566: Using --ssl-self-signed: pg_autoctl will  create self-signed certificates, allowing for encrypted network traffic
12:27:54 WARN  pgsetup.c:1569: Self-signed certificates provide protection against eavesdropping; this setup does NOT protect against Man-In-The-Middle attacks nor Impersonation attacks.
12:27:54 WARN  pgsetup.c:1571: See https://www.postgresql.org/docs/current/libpq-ssl.html for details
12:27:54 TRACE config.c:165: SetConfigFilePath: "/Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg"
12:27:54 TRACE config.c:193: SetStateFilePath: "/Users/onderkalaci/.local/share/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.state"
12:27:54 TRACE config.c:208: SetKeeperStateFilePath: "/Users/onderkalaci/.local/share/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.init"
12:27:54 TRACE config.c:234: SetPidFilePath: "/tmp/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.pid"
12:27:54 DEBUG monitor_config.c:252: Reading configuration from /Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg
12:27:54 TRACE ini_file.c:109: pg_autoctl.role = monitor
12:27:54 TRACE ini_file.c:109: pg_autoctl.nodename = 192.168.2.6
12:27:54 TRACE ini_file.c:109: postgresql.pgdata = /Users/onderkalaci/Documents/pg_auto_failover/monitor
12:27:54 TRACE ini_file.c:109: postgresql.pg_ctl = /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_ctl
12:27:54 TRACE ini_file.c:109: postgresql.username = autoctl_node
12:27:54 TRACE ini_file.c:109: postgresql.dbname = pg_auto_failover
12:27:54 TRACE ini_file.c:109: postgresql.host = /tmp
12:27:54 TRACE ini_file.c:109: postgresql.port = 5432
12:27:54 TRACE ini_file.c:109: postgresql.listen_addresses = *
12:27:54 TRACE ini_file.c:109: postgresql.auth_method = trust
12:27:54 TRACE ini_file.c:109: ssl.sslmode = require
12:27:54 TRACE ini_file.c:109: ssl.active = 1
12:27:54 TRACE ini_file.c:109: ssl.cert_file = /Users/onderkalaci/Documents/pg_auto_failover/monitor/server.crt
12:27:54 TRACE ini_file.c:109: ssl.key_file = /Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key
12:27:54 DEBUG pgctl.c:118: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_controldata /Users/onderkalaci/Documents/pg_auto_failover/monitor
12:27:54 DEBUG pgctl.c:739: pg_controldata: fatal: could not open file "/Users/onderkalaci/Documents/pg_auto_failover/monitor/global/pg_control" for reading: No such file or directory
12:27:54 DEBUG pgctl.c:155: Failed to run "/Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_controldata" on "/Users/onderkalaci/Documents/pg_auto_failover/monitor", see above for details
12:27:54 DEBUG pgctl.c:118: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_controldata /Users/onderkalaci/Documents/pg_auto_failover/monitor
12:27:54 DEBUG pgctl.c:739: pg_controldata: fatal: could not open file "/Users/onderkalaci/Documents/pg_auto_failover/monitor/global/pg_control" for reading: No such file or directory
12:27:54 DEBUG pgctl.c:155: Failed to run "/Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_controldata" on "/Users/onderkalaci/Documents/pg_auto_failover/monitor", see above for details
12:27:54 TRACE monitor_config.c:306: monitor_config_write_file "/Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg"
12:27:54 ERROR pgsetup.c:795: connectionString: port=5432 dbname=pg_auto_failover host=/tmp user=autoctl_node
12:27:54 TRACE monitor.c:96: monitor_init: port=5432 dbname=pg_auto_failover host=/tmp user=autoctl_node
12:27:58 INFO  pgctl.c:758: Initialising a PostgreSQL cluster at "/Users/onderkalaci/Documents/pg_auto_failover/monitor"
12:27:58 DEBUG pgctl.c:759: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_ctl initdb -s -D /Users/onderkalaci/Documents/pg_auto_failover/monitor [0]
12:27:58 TRACE monitor_config.c:306: monitor_config_write_file "/Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg"
12:27:58 INFO  pgctl.c:1746:  /usr/local/opt/[email protected]/bin/openssl req -new -x509 -days 365 -nodes -text -out /Users/onderkalaci/Documents/pg_auto_failover/monitor/server.crt -keyout /Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key -subj "/CN=192.168.2.6"
12:27:58 DEBUG pgctl.c:739: Generating a RSA private key
12:27:58 DEBUG pgctl.c:739: ..................................+++++
12:27:58 DEBUG pgctl.c:739: .................+++++
12:27:58 DEBUG pgctl.c:739: writing new private key to '/Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key'
12:27:58 DEBUG pgctl.c:739: -----
12:27:58 TRACE monitor_config.c:306: monitor_config_write_file "/Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg"
12:27:58 DEBUG pgctl.c:389: Configuration file "/Users/onderkalaci/Documents/pg_auto_failover/monitor/postgresql-auto-failover.conf" doesn't exists yet, creating with content:
# Settings by pg_auto_failover
shared_preload_libraries = 'pgautofailover'
listen_addresses = '*'
port = 5432
log_destination = stderr
logging_collector = on
log_directory = log
log_min_messages = info
log_connections = off
log_disconnections = off
log_lock_waits = on
ssl = on
ssl_cert_file = '/Users/onderkalaci/Documents/pg_auto_failover/monitor/server.crt'
ssl_key_file = '/Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key'
ssl_ciphers = 'ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES256-SHA384'

12:27:58 DEBUG pgctl.c:300: Adding include 'postgresql-auto-failover.conf' to "/Users/onderkalaci/Documents/pg_auto_failover/monitor/postgresql.conf"
12:27:58 INFO  pgctl.c:847:  /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_ctl --pgdata /Users/onderkalaci/Documents/pg_auto_failover/monitor --options "-p 5432" --options "-h *" --wait start
12:27:58 DEBUG pgctl.c:118: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_controldata /Users/onderkalaci/Documents/pg_auto_failover/monitor
12:27:58 DEBUG pgsetup.c:173: Found PostgreSQL system 6830726616286946078 at "/Users/onderkalaci/Documents/pg_auto_failover/monitor", version 1201, catalog version 201909212
12:27:58 TRACE pgsetup.c:641: read_pg_pidfile: pid 86824, port 5432, host /tmp, status "ready"
12:27:58 TRACE pgsetup.c:932: pg_setup_is_ready: ready
12:27:58 ERROR pgsetup.c:795: connectionString: port=5432 dbname=postgres host=/tmp
12:27:58 DEBUG pgsql.c:151: Connecting to "port=5432 dbname=postgres host=/tmp"
12:27:58 DEBUG pgsql.c:1461: Running command on Postgres: CREATE USER "autoctl" WITH LOGIN;
12:27:58 DEBUG pgsql.c:1280: Running command on Postgres: CREATE DATABASE "pg_auto_failover" WITH OWNER "autoctl";
12:27:59 DEBUG pgsql.c:128: Disconnecting from "port=5432 dbname=postgres host=/tmp"
12:27:59 ERROR pgsetup.c:795: connectionString: port=5432 dbname=pg_auto_failover host=/tmp
12:27:59 DEBUG pgsql.c:151: Connecting to "port=5432 dbname=pg_auto_failover host=/tmp"
12:27:59 DEBUG pgsql.c:1350: Running command on Postgres: CREATE EXTENSION "pgautofailover";
12:27:59 DEBUG pghba.c:327: HBA: adding CIDR from hostname "192.168.2.6"
12:27:59 DEBUG pghba.c:328: HBA: local ip address: 192.168.2.6
12:27:59 DEBUG pghba.c:329: HBA: CIDR address to open: 192.168.2.0/24
12:27:59 INFO  pghba.c:330: Granting connection privileges on 192.168.2.0/24
12:27:59 DEBUG pgsql.c:385: SELECT current_setting($1);
12:27:59 DEBUG pgsql.c:409: 'hba_file'
12:27:59 DEBUG pghba.c:112: Ensuring the HBA file "/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_hba.conf" contains the line: hostssl "pg_auto_failover" "autoctl_node" 192.168.2.0/24 trust
12:27:59 DEBUG pghba.c:169: Wrote new /Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_hba.conf
12:27:59 DEBUG pgsql.c:385: SELECT pg_reload_conf();
12:27:59 DEBUG pgsql.c:128: Disconnecting from "port=5432 dbname=pg_auto_failover host=/tmp"
12:27:59 INFO  monitor_pg_init.c:251: Your pg_auto_failover monitor instance is now ready on port 5432.
12:27:59 INFO  cli_create_drop_node.c:766: Monitor has been succesfully initialized.
12:27:59 TRACE service.c:37: monitor_service_init
12:27:59 TRACE service.c:151: create_pidfile(86801): "/tmp/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.pid"
12:27:59 DEBUG pgctl.c:1161: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_ctl status -D /Users/onderkalaci/Documents/pg_auto_failover/monitor [0]
12:27:59 TRACE monitor_config.c:306: monitor_config_write_file "/Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg"
12:27:59 INFO  pgctl.c:1746:  /usr/local/opt/[email protected]/bin/openssl req -new -x509 -days 365 -nodes -text -out /Users/onderkalaci/Documents/pg_auto_failover/monitor/server.crt -keyout /Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key -subj "/CN=192.168.2.6"
12:27:59 DEBUG pgctl.c:739: Generating a RSA private key
12:27:59 DEBUG pgctl.c:739: ............+++++
12:27:59 DEBUG pgctl.c:739: ................+++++
12:27:59 DEBUG pgctl.c:739: writing new private key to '/Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key'
12:27:59 DEBUG pgctl.c:739: -----
12:27:59 TRACE monitor_config.c:306: monitor_config_write_file "/Users/onderkalaci/.config/pg_autoctl/Users/onderkalaci/Documents/pg_auto_failover/monitor/pg_autoctl.cfg"
12:27:59 DEBUG pgctl.c:376: Default settings file "/Users/onderkalaci/Documents/pg_auto_failover/monitor/postgresql-auto-failover.conf" exists
12:27:59 DEBUG pgctl.c:293: include 'postgresql-auto-failover.conf' found in "/Users/onderkalaci/Documents/pg_auto_failover/monitor/postgresql.conf"
12:27:59 DEBUG pgctl.c:118: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_controldata /Users/onderkalaci/Documents/pg_auto_failover/monitor
12:27:59 DEBUG pgsetup.c:173: Found PostgreSQL system 6830726616286946078 at "/Users/onderkalaci/Documents/pg_auto_failover/monitor", version 1201, catalog version 201909212
12:27:59 TRACE pgsetup.c:641: read_pg_pidfile: pid 86824, port 5432, host /tmp, status "ready"
12:27:59 TRACE pgsetup.c:932: pg_setup_is_ready: ready
12:27:59 ERROR pgsetup.c:795: connectionString: port=5432 dbname=template1 host=/tmp
12:27:59 DEBUG pgsql.c:151: Connecting to "port=5432 dbname=template1 host=/tmp"
12:27:59 DEBUG pgsql.c:385: SELECT pg_reload_conf();
12:27:59 INFO  monitor_service.c:104: pg_auto_failover monitor is ready at postgres://[email protected]:5432/pg_auto_failover?sslmode=require
12:27:59 INFO  monitor_service.c:107: Contacting the monitor to LISTEN to its events.
12:27:59 DEBUG pgsql.c:151: Connecting to "port=5432 dbname=pg_auto_failover host=/tmp user=autoctl_node"
12:27:59 DEBUG pgctl.c:1161: /Users/onderkalaci/Documents/citus_code/pgenv/pgsql/bin/pg_ctl status -D /Users/onderkalaci/Documents/pg_auto_failover/monitor [0]

onderkalaci avatar May 25 '20 10:05 onderkalaci

I figured out why I was getting localhost in there. It was because I had export PGHOST=localhost in my ~/.bashrc file. Without this pg_autoctl worked fine when doing --auth-host=reject --auth-local=trust.

So I think there's a few things we should change in that case to sum up:

  1. Set unix_socket_permissions to 0700 or 0770 by default
  2. Add --pghost flag to create monitor so you can override the PGHOST variable.
  3. Change our initdb to not set up trust on localhost by default (but still do that for UNIX sockets).
  4. BONUS: If PGHOST/--pghost does not start with a / (to indicate UNIX socket dir), then allow localhost access by default.

JelteF avatar May 25 '20 11:05 JelteF

Add --pghost flag to create monitor so you can override the PGHOST variable.

We seem to have create monitor --nodename parameter. Do you have something different in mind @JelteF?

BONUS: If PGHOST/--pghost does not start with a / (to indicate UNIX socket dir), then allow localhost access by default.

One caveat here is that on the monitor, it is not possible to set --nodename = /unix/path when --no-ssl or --ssl-self-signed is chosen. I think we have a strong dependency to have a hostname related to certificate/ssl setup.

For no-ssl case, we get the following after create monitor:

19:09:45 16348 ERROR pgctl.c:981: Failed to start PostgreSQL. pg_ctl start returned: 1
19:09:45 16348 ERROR pgctl.c:985: waiting for server to start....2020-05-25 19:09:45.807 CEST [16365] FATAL:  could not load server certificate file "/Users/onderkalaci/Documents/pg_auto_failover/monitor/server.crt": No such file or directory

For --ssl-self-signed, we get the following after create monitor:

19:13:38 16382 ERROR pgctl.c:785: Generating a RSA private key
19:13:38 16382 ERROR pgctl.c:785: ...............+++++
19:13:38 16382 ERROR pgctl.c:785: ................................................+++++
19:13:38 16382 ERROR pgctl.c:785: writing new private key to '/Users/onderkalaci/Documents/pg_auto_failover/monitor/server.key'
19:13:38 16382 ERROR pgctl.c:785: -----
19:13:38 16382 ERROR pgctl.c:785: req: No value provided for Subject Attribute CN, skipped
19:13:38 16382 ERROR pgctl.c:785: req: Hit end of string before finding the equals.
19:13:38 16382 ERROR pgctl.c:785: problems making Certificate Request
19:13:38 16382 ERROR pgctl.c:1948: openssl failed with return code: 1

BONUS: If PGHOST/--pghost does not start with a / (to indicate UNIX socket dir), then allow localhost access by default.

I think instead of this hack, we could allow pg_autoctl create to accept --auth-host and auth-local parameters and pass it to initdb. And, the defaults are --auth-local=trust and --auth-host=reject

What do you think @JelteF ?

onderkalaci avatar May 26 '20 11:05 onderkalaci

We seem to have create monitor --nodename parameter. Do you have something different in mind @JelteF?

For create postgres we have both a --nodename and a --pghost parameter. --nodename is the hostname that is connected to from other servers and --pghost is the hostname that pg_autoctl uses to connect to the local postgres. So it doesn't really make sense to have /unix/path as a --nodename, but it makes sense as --pghost. The --pghost flag is missing on create monitor.

One caveat here is that on the monitor, it is not possible to set --nodename = /unix/path when --no-ssl or --ssl-self-signed is chosen. I think we have a strong dependency to have a hostname related to certificate/ssl setup.

Yes, I noticed this too in my initial investigation. Then @DimCitus told me the fact I mentioned above about the two different flags for create postgres.

I think instead of this hack, we could allow pg_autoctl create to accept --auth-host and auth-local parameters and pass it to initdb. And, the defaults are --auth-local=trust and --auth-host=reject

I think this idea is definitely in the right direction and better than the "magic" I proposed. We should think a bit about how to make it work nicely with the flags we already have, i.e. --auth and --skip-pg-hba. I think what would be nice is:

  1. We add the --auth-host and --auth-local flags.
  2. If they are specified we use those values
  3. If --auth-local is not specified we use trust
  4. If --auth-host is not specified we use: a. If --auth is specified we use it's value. b. If --skip-pg-hba is specified we use reject.

JelteF avatar May 26 '20 12:05 JelteF

  1. We add the --auth-host and --auth-local flags.
  2. If they are specified we use those values
  3. If --auth-local is not specified we use trust
  4. If --auth-host is not specified we use: a. If --auth is specified we use it's value. b. If --skip-pg-hba is specified we use reject.

I'm not too sure about this. I think it would be better to have a strong default and have users edit the HBA file if they want to change our defaults. After all when adding application nodes editing the HBA manually is already necessary.

What about using pg_ctl initdb --auth-local trust --auth-host reject and then edit the HBA normally per the --auth and --skip-pg-hba rules? I think that covers everything that has been said here, right?

DimCitus avatar May 26 '20 13:05 DimCitus

I understand what you mean, which is why I put BONUS there originally. The main problem it causes is that when doing create monitor with PGHOST=localhost without --auth-host=trust then the create monitor command fails even when not specifying --run. To fix it you have to:

  1. modify pg_hba.conf
  2. reload postgres using pg_ctl reload
  3. run create monitor again

That seems a bit annoying.

If you are opposed to adding more flags, I think that makes sense. We can also leave those out and simply always do --auth-local trust and then specify --auth-host the same as --auth (or reject if --skip-pg-hba is specified).

That would solve most cases and I don't think you gain much security from --auth-host reject if you don't specify --skip-pg-hba too. Because then you can still connect to the monitor using it's public IP, might as well make it easy for the user to connect to localhost too.

JelteF avatar May 26 '20 13:05 JelteF

After a chat with Onder, we are coming to the following conclusions:

  • the monitor is now an embedded database, not expected to be used or even usable for anything else than pg_autoctl needs ; we don't need to obey PGHOST nor --pghost and may use our own Unix Socket directory and connect through there always,
  • the default HBA given by pg_ctl initdb contains problematic lines: with --auth-host trust we allow too many connection, with --auth-host reject only SSL connections can be made because we only even append rules to the file, and we have a reject line that is hit first.

So what I think we should do is the following:

  • ignore PGHOST and skip implementing --pghost on the monitor,
  • remove and replace the pg_hba.conf file wholesale after pg_ctl initdb to have our own lines only (local all all trust, and then the usual autoctl_node from the local network) ; on the monitor only,
  • see about changing unix_socket_directories to use either a XDG_DATA or XDG_RUNTIME relative path, as we do for our state and pid files,
  • see about changing unix_socket_permissions to 0700 maybe
  • avoid any changes in behaviour on the “keeper” side of things (pg_autoctl create postgres)

We could also review our --auth option parsing on the keeper and have either only that one or potentially both --auth-local and --auth-host and forward those to pg_ctl initdb. That should be a separate PR though.

DimCitus avatar May 26 '20 14:05 DimCitus

  • ignore PGHOST and skip implementing --pghost on the monitor,

Fine by me

  • remove and replace the pg_hba.conf file wholesale after pg_ctl initdb to have our own lines only (local all all trust, and then the usual autoctl_node from the local network) ; on the monitor only,

Again fine. One thing to keep in mind though is that that will also remove the huge comment at the start of pg_hba.conf. Which I think is quite useful. So it would be nice to do this in a way that we do not lose it. Some options I can think of:

  1. Also write it in our hardcoded pg_hba.conf (easy, but we have to update the text manually if postgres changes it)
  2. Remove only the last couple of lines from the file and then append our own local lines. (seems like this might be a bit of work)
  • see about changing unix_socket_directories to use either a XDG_DATA or XDG_RUNTIME relative path, as we do for our state and pid files,

I think it makes sense to append to unix_socket_directories, but it seems nice to also put the socket in the default directory.

  • see about changing unix_socket_permissions to 0700 maybe

I think this is more important than unix_socket_directories TBH. A trust socket should not be accessible by all system users by default.

  • avoid any changes in behaviour on the “keeper” side of things (pg_autoctl create postgres)

I think we should definitely have some sane defaults for pg_autoctl create postgres too. At a minimum we should not add host all localhost trust lines to pg_hba.conf when doing --skip-pg-hba or --auth md5. (and also add unix_socket_permissions=0700) I don't see a reason why we should not do the thing I describe above on the worker:

simply always do --auth-local trust and then specify --auth-host the same as --auth (or reject if --skip-pg-hba is specified).

JelteF avatar May 27 '20 09:05 JelteF

I think we should definitely have some sane defaults for pg_autoctl create postgres too.

The idea is follows: The monitor is an embedded database to our application, and we're responsible for everything about it. For create postgres, they are not ours, the customers own them. Thus, it is their responsibility to restrict/allow accesses. We don't want to do anything further than pg_auto_ctl needs.

onderkalaci avatar May 27 '20 10:05 onderkalaci

I agree that that's the case when the database already exists. But if we create it we should not open it up by default when pg_autoctl does not need it.

JelteF avatar May 27 '20 10:05 JelteF

agree that that's the case when the database already exists. But if we create it we should not open it up by default when pg_autoctl does not need it.

That's the default of Postgres :) As far as I can see, what we do is conveniently create the databases for the users. So, interfering with the defaults might not be a good idea.

In the future, we might consider adding --auth-local and --auth-host parameters to create postgres subcommand like here, but for now, we thought lets leave is as is.

onderkalaci avatar May 27 '20 10:05 onderkalaci

I understand what you mean, but I disagree.

The default for postgres is to warn about it's insecure default, to try to get the user to change it. We actively disable that warning at the moment: https://github.com/citusdata/pg_auto_failover/blob/b609da294b30c4f7fece31c744fa7b1cb236f03e/src/bin/pg_autoctl/pgctl.c#L811-L812

So we already change the default behaviour to a less secure thing than the postgres default. I'd rather we change it to a more secure thing.

Postgres commiters seem to want to change these defaults (unclear to me if this is merged in PG13): https://postgrespro.com/list/thread-id/2442644#[email protected]

Since we actually already require the user to specify an auth mode for remote connections, we can easily pass that on to localhost connections too. So I do not see any reason to not do the following:

simply always do --auth-local trust and then specify --auth-host the same as --auth (or reject if --skip-pg-hba is specified).

Setting unix_socket_permissions to 0700 I can see maybe being undesirable, but a simple workaround is passing -h localhost to psql.

JelteF avatar May 27 '20 10:05 JelteF

My main point is, we're already adding a lot of pg_hba.conf rules anyway also for create postgres. So making them a bit secure by default seems like a nice thing to do.

JelteF avatar May 27 '20 11:05 JelteF

In the future, we might consider adding --auth-local and --auth-host parameters to create postgres subcommand like here, but for now, we thought lets leave is as is.

If we reuse our --auth flag, I actually don't see much reason to do this. --auth-local=trust we need at the moment (so a user changing this will only break things) and --auth-host we infer from our own --auth flag. So these extra flags don't seem necessary.

JelteF avatar May 27 '20 11:05 JelteF

ignore PGHOST and skip implementing --pghost on the monitor,

I wonder if this is a good idea as we seem to have a dependency in the regression tests for PGHOST being set: https://github.com/citusdata/pg_auto_failover/pull/301#partial-pull-merging

Is my understanding accurate? If so, any suggestion/guidance on how to proceed?

onderkalaci avatar Jun 17 '20 15:06 onderkalaci