Introduce auth_ident_file setting and enhance cert and peer auth methods with user name maps by emelsimsek · Pull Request #996 · pgbouncer/pgbouncer

emelsimsek · 2023-12-18T11:17:10Z

Postgres supports user name maps[1] for cert and peer authentication methods. This change provides the same functionality with some limitations for pgbouncer. The limitations are listed in "Ident map file format" section of the docs.

https://www.postgresql.org/docs/current/auth-username-maps.html

Fixes #767
Fixes #148

JelteF · 2023-12-21T22:41:21Z

+    bouncer.write_ini(f"auth_hba_file = pgbouncer_hba.conf")
+    bouncer.write_ini(f"auth_pgident_file = pgident.conf")


These two files are not committed, which is why this test is failing.

JelteF · 2024-01-09T13:20:56Z

+			goto fail;
+
+		if (strcmp(client->login_user->name, rule->identmap->postgres_user_name)) {
+			if (strcmp(rule->identmap->postgres_user_name, "all"))


There's two ways all can be formatted in the ident file, which means two different things:

all (literal string), this is the special keyword that matches all users

"all" (quoted), this means it only matches the user called all (assuming this user exists)

Postgres tracks this difference by setting a flag in case the special keyword is used.

For HBA parsing we use the NAME_ALL flag in pgbouncer too actually.

JelteF · 2024-01-10T09:45:05Z

+		return false;
+	}
+
+	if (!expect(tp, TOK_IDENT, &map_name)) {


Right now this does not support TOK_STRING, so any quoted names will fail. Let's fix that. To not duplicate this logic for each of the columns, I think it probably makes sense to create a new function that is very similar to the already existing parse_names function but handles ident names instead of hba names (e.g. called parse_ident_names).

This one still requires TOK_IDENT for the map_name afaict.

onurctirtir · 2024-01-24T22:55:52Z

SIGSEGV happens when trying to connect to a database that's present in [databases].

The setup doesn't configure auth_ident_file but has some entries for cert auth in auth_hba_file.

#0  0x000055a024613e61 in finish_set_pool (client=client@entry=0x55a025a09a80, takeover=takeover@entry=false) at src/client.c:310
#1  0x000055a024615918 in handle_client_startup (client=client@entry=0x55a025a09a80, pkt=pkt@entry=0x7ffc669afb10) at src/client.c:928
#2  0x000055a024616549 in client_proto (sbuf=0x55a025a09c50, evtype=<optimized out>, data=0x7ffc669afb90) at src/client.c:1365
#3  0x000055a024628f68 in sbuf_call_proto (sbuf=sbuf@entry=0x55a025a09c50, event=event@entry=0) at src/sbuf.c:498
#4  0x000055a0246294fa in sbuf_process_pending (sbuf=sbuf@entry=0x55a025a09c50) at src/sbuf.c:802
#5  0x000055a024629e24 in sbuf_main_loop (skip_recv=<optimized out>, sbuf=0x55a025a09c50) at src/sbuf.c:1031
#6  sbuf_main_loop (sbuf=0x55a025a09c50, skip_recv=<optimized out>) at src/sbuf.c:966
#7  0x000055a024616230 in handle_auth_query_response (client=0x55a025a09a80, pkt=pkt@entry=0x7ffc669b05f0) at src/client.c:545
#8  0x000055a02462cf56 in handle_server_work (pkt=0x7ffc669b05f0, server=0x55a025a23a80) at src/server.c:458
#9  server_proto (sbuf=0x55a025a23c50, evtype=<optimized out>, data=<optimized out>) at src/server.c:702
#10 0x000055a024628f68 in sbuf_call_proto (sbuf=sbuf@entry=0x55a025a23c50, event=event@entry=0) at src/sbuf.c:498
#11 0x000055a0246294fa in sbuf_process_pending (sbuf=sbuf@entry=0x55a025a23c50) at src/sbuf.c:802
#12 0x000055a024629e24 in sbuf_main_loop (skip_recv=<optimized out>, sbuf=0x55a025a23c50) at src/sbuf.c:1031
#13 sbuf_main_loop (sbuf=0x55a025a23c50, skip_recv=<optimized out>) at src/sbuf.c:966
#14 0x00007ff08f61e13f in ?? () from /lib/x86_64-linux-gnu/libevent-2.1.so.7
#15 0x00007ff08f61e87f in event_base_loop () from /lib/x86_64-linux-gnu/libevent-2.1.so.7
#16 0x000055a02461ccba in main_loop_once () at src/main.c:794
#17 0x000055a02460ec45 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:1082

Assuming that ident_map is non-NULL seems problematic at src/client.c:310:

log_warning("hba_eval returned Ident map %s %s %s", rule->identmap->map_name, rule->identmap->system_user_name, rule->identmap->postgres_user_name);

JelteF · 2024-02-07T14:49:32Z

-		slog_error(client, "TLS certificate name mismatch");
+
+	if (rule && rule->identmap) {
+		if (!tls_peer_cert_contains_name(client->sbuf.tls, rule->identmap->system_user_name))


We should log a unique error here (and the two error cases below), failures in these kind of things are hard enough to debug already.

JelteF · 2024-02-07T14:55:51Z

-	if (!tls_peer_cert_contains_name(client->sbuf.tls, client->login_user->name)) {
-		slog_error(client, "TLS certificate name mismatch");
+
+	if (rule && rule->identmap) {


Let's add similar logic to use the map when available to login_as_unix_peer, because postgres allow username mapping for that one too: https://www.postgresql.org/docs/current/auth-peer.html

Afaict to support that too, we only need these if lines (and some tests). If it's much more than that, let's make it a separate PR.

I have done this change but could not add tests for it. I was able to manually test.

I tried adding a test via:

result = subprocess.run([f"sudo -u asystemuser psql -p {bouncer.port} -h {bouncer.admin_host} p0 -c "SELECT 1;""], stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
print(result.returncode, result.stdout, result.stderr)

returns error

..... Permission denied\npsql: error: connection to server on socket "/tmp/pytest-of-emel/pytest-34/test_client_hba_cert0/bouncer/.s.PGSQL.10202" failed: Permission denied\n\tIs the server running locally and accepting connections on that socket?\n'

Maybe we should make this a seperate PR with proper tests added.

I think probably the easiest way to create a test is to manually create an ident file, that contains the current system user. So something like this (untested code):

cur_user = os.current_user() with open("myident.conf", 'w') as f: f.write(f"mymap {cur_user} postgres") bouncer.write_ini(f"auth_ident_file = pgident.conf")

Maybe we should make this a seperate PR with proper tests added.

One big reason why I'd like it in too, is because it's generally much easier to do some manual testing with it. So if we cannot easily do automated tests (in the way I suggested in the previous comment) I think it's fine to merge without tests, because I'll do some manual testing with it.

Thanks @JelteF. I pushed a simple test now.

JelteF · 2024-02-07T15:00:04Z

+# =========================
+
+# MAPNAME       SYSTEM-USERNAME         PG-USERNAME
+test		pgbouncer.acme.org	someuser


I think it would be good to test a case where there are multiple mappings for the same name, then the cert should be able to authenticate as both:

Suggested change

test pgbouncer.acme.org someuser

test pgbouncer.acme.org someuser

test pgbouncer.acme.org someuser2

The current design does not support this. The first encountered map is effective. But I see that postgres supports multiple mappings with the same name:
https://www.postgresql.org/docs/current/auth-username-maps.html#:~:text=%23%20MAPNAME%20%20%20%20%20%20%20SYSTEM%2DUSERNAME,omicron%20%20%20%20%20%20%20%20%20bryanh%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20guest1

Need to make data structure changes to support this.

JelteF · 2024-02-07T15:04:35Z

+        sslrootcert=root,
+    )
+
+    bouncer.pg.sql("create user anotheruser with login;")


sidenote: We should change this for bouncer.pg.create_user once #876 is merged

JelteF · 2024-02-07T15:11:24Z

+	struct List rules;
+};
+
+struct IDENTMap {


Let's use CamelCase for the struct name. I realize took the naming scheme from HBA, but that is an acronym (and even then I don't agree with this type of casing personally).

Suggested change

struct IDENTMap {

struct IdentMap {

JelteF · 2024-02-07T15:29:04Z

+	if (!expect(tp, TOK_IDENT, &system_user_name)) {
+		goto failed;
+	}


This one should allow TOK_STRING too.

JelteF · 2024-02-07T15:29:40Z

+
+# MAPNAME       SYSTEM-USERNAME         PG-USERNAME
+test		pgbouncer.acme.org	someuser
+test2		bouncer			all


We should have some tests for quoted names too in all of the columns.

JelteF · 2024-02-07T15:30:55Z

+		return false;
+	}
+
+	if (!expect(tp, TOK_IDENT, &map_name)) {


This one still requires TOK_IDENT for the map_name afaict.

JelteF · 2024-02-20T15:55:05Z

+    )
+
+
+@pytest.mark.skipif("WINDOWS", reason="Windows does not have SIGHUP")


Suggested change

@pytest.mark.skipif("WINDOWS", reason="Windows does not have SIGHUP")

@pytest.mark.skipif("WINDOWS", reason="Windows does not have peer authentication")

JelteF · 2024-02-20T15:57:29Z

+
+    with open("ident.conf", "w") as f:
+        f.write(f"mymap {cur_user} postgres\n")
+        f.write(f"mymap {cur_user} someuser\n")


Should also have a test with the all system user. That's causing a segfault currently with this manual test:
pgbouncer.ini

[databases] postgres = host=localhost dbname=postgres port=9700 postgres1 = host=localhost dbname=postgres port=9700 postgres2 = host=localhost dbname=postgres port=9700 postgres3 = host=localhost dbname=postgres port=9700 [pgbouncer] pool_mode=transaction listen_addr=127.0.0.1 auth_type=hba auth_hba_file=hba.conf auth_ident_file=ident.conf admin_users=jelte auth_file=auth_file.conf so_reuseport=1 unix_socket_dir=/tmp

hba.conf

# TYPE DATABASE USER ADDRESS METHOD local postgres1 jelte peer map=allowmap

ident.conf

allowmap jelte all

auth_file.conf

"jelte" "password" ...

JelteF · 2024-02-20T16:59:03Z

+def test_peer_auth_ident_map(bouncer):
+    cur_user = getpass.getuser()
+
+    with open("ident.conf", "w") as f:


These files should be created in the config directory of pgbouncer. Otherwise they polute the current working directory when running the tests locally.

This adds support for user name maps via auth_ident_file. cf: pgbouncer/pgbouncer#996 Signed-off-by: Christopher Broglie <cbroglie@cloudflare.com>

JelteF reviewed Dec 18, 2023

View reviewed changes

Comment thread doc/config.md Outdated

JelteF reviewed Dec 18, 2023

View reviewed changes

Comment thread test/hba_test.c

JelteF reviewed Dec 21, 2023

View reviewed changes

JelteF reviewed Dec 22, 2023

View reviewed changes

Comment thread test/test_auth.py

JelteF reviewed Dec 27, 2023

View reviewed changes

Comment thread doc/config.md Outdated

emelsimsek changed the title ~~Introduce auth_pgident_file setting and enhance cert auth with user mapping~~ Introduce auth_ident_file setting and enhance cert auth with user mapping Dec 27, 2023

emelsimsek force-pushed the hba-pgident-map branch from b3d6308 to 1f306ce Compare January 2, 2024 08:47

JelteF reviewed Jan 2, 2024

View reviewed changes

Comment thread doc/config.md Outdated

emelsimsek force-pushed the hba-pgident-map branch from 1b4e846 to fea81da Compare January 2, 2024 13:43

JelteF reviewed Jan 9, 2024

View reviewed changes

Comment thread src/client.c Outdated

JelteF reviewed Jan 9, 2024

View reviewed changes

Comment thread src/main.c Outdated

JelteF reviewed Jan 10, 2024

View reviewed changes

Comment thread src/hba.c Outdated

JelteF reviewed Jan 10, 2024

View reviewed changes

Comment thread doc/config.md Outdated

JelteF reviewed Jan 10, 2024

View reviewed changes

Comment thread doc/config.md Outdated

JelteF reviewed Jan 10, 2024

View reviewed changes

emelsimsek force-pushed the hba-pgident-map branch from b963e06 to 5782254 Compare January 18, 2024 08:59

JelteF reviewed Jan 18, 2024

View reviewed changes

Comment thread src/hba.c Outdated

emelsimsek requested a review from JelteF January 22, 2024 05:34

emelsimsek force-pushed the hba-pgident-map branch from b3666f2 to 1dcc21d Compare January 25, 2024 08:36

JelteF reviewed Feb 7, 2024

View reviewed changes

JelteF requested changes Feb 7, 2024

View reviewed changes

emelsimsek requested a review from JelteF February 14, 2024 10:35

JelteF reviewed Feb 20, 2024

View reviewed changes

emelsimsek force-pushed the hba-pgident-map branch from 0878a53 to 9871ce6 Compare February 21, 2024 08:30

emelsimsek added 19 commits March 4, 2024 12:19

Fix parsing

f1a2e67

Doc

1d05460

Support NAME_ALL

1ac30f9

Fix bad merge

b46b42f

Move define to header

d3786f1

Add tests for cert auth with no map

aa3b0b2

Add error logs on failure

cc5a98c

Support multiple mappings with the same map name

978c8cf

Tests multiple mappings with the same map

0a2651b

Allow string in system user name

2778788

Fix memory leak

33b28d2

Format

666067b

Peer auth user map

a0fc76d

Add test for peer auth map

7e62334

Fix test

b813847

Fix test

c9943ad

Fix test

d019812

Handle all keyword in peer auth

9834906

Fix test file paths

dab510e

emelsimsek force-pushed the hba-pgident-map branch from 9871ce6 to dab510e Compare March 4, 2024 09:20

emelsimsek changed the title ~~Introduce auth_ident_file setting and enhance cert auth with user mapping~~ Introduce auth_ident_file setting and enhance cert and peer auth with user mapping Mar 4, 2024

emelsimsek changed the title ~~Introduce auth_ident_file setting and enhance cert and peer auth with user mapping~~ Introduce auth_ident_file setting and enhance cert and peer auth methods with user name maps Mar 4, 2024

Exclude MACOS from ssl tests

20f54d7

emelsimsek merged commit 5661fc2 into pgbouncer:master Mar 4, 2024

emelsimsek mentioned this pull request Mar 4, 2024

Revert "Introduce auth_ident_file setting and enhance cert and peer auth methods with user name maps" #1032

Closed

JelteF mentioned this pull request Dec 3, 2024

Add support for map= option #154

Closed

cbroglie mentioned this pull request Dec 31, 2024

[bitnami/pgbouncer] Add PGBOUNCER_AUTH_IDENT_FILE bitnami/containers#75939

Merged

		bouncer.write_ini(f"auth_hba_file = pgbouncer_hba.conf")
		bouncer.write_ini(f"auth_pgident_file = pgident.conf")

	test pgbouncer.acme.org someuser
	test pgbouncer.acme.org someuser
	test pgbouncer.acme.org someuser2

		)


		@pytest.mark.skipif("WINDOWS", reason="Windows does not have SIGHUP")

Conversation

emelsimsek commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

onurctirtir commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelteF Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emelsimsek commented Dec 18, 2023 •

edited

Loading

onurctirtir commented Jan 24, 2024 •

edited

Loading

JelteF Feb 7, 2024 •

edited

Loading

JelteF Feb 7, 2024 •

edited

Loading

JelteF Feb 14, 2024 •

edited

Loading

JelteF Feb 20, 2024 •

edited

Loading