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

Introduce auth_dbname as an option #764

Merged
merged 18 commits into from
Jan 3, 2023
Merged

Introduce auth_dbname as an option #764

merged 18 commits into from
Jan 3, 2023

Conversation

goksgie
Copy link
Contributor

@goksgie goksgie commented Oct 8, 2022

This commit implements the auth_dbname option introduced by the following PR: #645

In essence, users should be able to specify an authentication database to execute auth_query against. This will help them to avoid creating the same function on every database on their server.

A user can specify auth_dbname in the connection string, or they can set it globally. In case auth_dbname is specified in the connection string (under database) it will override the global configuration.

Thank you to the original contributors (@gbartolini - Gabriele Bartolini and @leonardoce - Leonardo Cecchi) for implementing this feature.

@gbartolini
Copy link
Contributor

Hi thanks, just to mention that the original PR was co-authored by Leonardo Cecchi as well. Please make sure you mention him (more than me) in the PR. Thanks.

@goksgie
Copy link
Contributor Author

goksgie commented Oct 9, 2022

Hey @gbartolini ! Thank you for pointing that out. I have updated the description. By the way, please feel free to use this PR if needed for reviving the original PR.

src/loader.c Outdated
return true;
}

free((void *)old_auth_dbname);
Copy link
Member

Choose a reason for hiding this comment

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

nit: no need for this cast

Suggested change
free((void *)old_auth_dbname);
free(old_auth_dbname);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The explicit cast seem to be required here, as otherwise we face with following warning message during compilation:

warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers] free(old_auth_dbname);

Copy link
Member

@JelteF JelteF Dec 5, 2022

Choose a reason for hiding this comment

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

Ah, I didn't realize the const qualifier required it. Maybe add a small comment about that and/or cast to (char*).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with commit 66f0c68

src/loader.c Outdated
}

if (new_auth_dbname && !db->auth_dbname) {
log_error("auth_dbname %s could not be set for database %s", new_auth_dbname, db->name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log_error("auth_dbname %s could not be set for database %s", new_auth_dbname, db->name);
log_error("auth_dbname %s could not be set for database %s, out of memory", new_auth_dbname, db->name);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/client.c Show resolved Hide resolved
test/test.sh Outdated

curuser=`psql -X -d "dbname=authdb user=someuser password=anypasswd" -tAq -c "select current_user;"`
echo "revert test curuser=$curuser"
test "$curuser" = "$curuser" || return 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test "$curuser" = "$curuser" || return 1
test "$curuser" = "someuser" || return 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh what a miss. Updated for all comparisons for this test case.

test/test.sh Outdated
# test if auth_dbname specified in connection string takes precedence over global setting
curuser=`psql -X -d "dbname=pauthz user=someuser password=anypasswd" -tAq -c "select current_user;"`
echo "conn string test curuser=$curuser"
test "$curuser" = "$curuser" || return 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test "$curuser" = "$curuser" || return 1
test "$curuser" = "someuser" || return 1

test/test.sh Outdated
admin "set auth_dbname=''"
curuser=`psql -X -d "dbname=p4 user=someuser password=anypasswd" -tAq -c "select current_user;"`
echo "curuser=$curuser"
test "$curuser" = "$curuser" || return 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test "$curuser" = "$curuser" || return 1
test "$curuser" = "someuser" || return 1

Copy link
Member

@eulerto eulerto left a comment

Choose a reason for hiding this comment

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

This is not a complete review. I didn't check tests yet.

;; use auth_dbname to run auth_query on a specific database.
;; when auth_dbname is set in connection string, it will override
;; the global setting.
; bardb = auth_dbname=foo
Copy link
Member

Choose a reason for hiding this comment

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

We generally don't add long explanation in the configuration file. IMO you should omit the second sentence and also change the first sentence to "run auth_query on a specific database". There is also a space at the end of " bardb = ...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, applied the suggested change.

@@ -122,6 +127,9 @@ auth_file = /etc/pgbouncer/userlist.txt
;; must have 2 columns - username and password hash.
;auth_query = SELECT usename, passwd FROM pg_shadow WHERE usename=$1

;; Authentication database that can be set globally to run "auth_query".
;auth_dbname
Copy link
Member

Choose a reason for hiding this comment

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

You forgot an equal here. "auth_dbname =".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out! Added the ".

@@ -454,6 +454,7 @@ struct PgDatabase {

struct PktBuf *startup_params; /* partial StartupMessage (without user) be sent to server */
const char *dbname; /* server-side name, pointer to inside startup_msg */
const char *auth_dbname; /* server-side: when not null, auth_query will be run on this database. */
Copy link
Member

Choose a reason for hiding this comment

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

Use "if not NULL, auth_query ...". This is similar to the next lines of code. Remove "server-side", it is implicit in the auth_query concept. "if not NULL, auth_query will be run on the specified database."

* Returns:
* auth_database, based on the above preference. May return NULL when
* auth_db is disabled.
*/
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion for the initial comment is

Get authentication database for the current client. The order of preference is:
* client->db->auth_dbname: per client authentication database
* cf_auth_dbname: global authentication database
* client->db: client database

NOTE: if authentication database is found but it is disabled, the client will be disconnected.

This function is confusing. The chosen variable names are very similar (auth_db and auth_dbname). There are a few fixes/improvements: alraedy, will get disconnected -> will be disconnected, attemp, client's target database -> client target database. The error messages should not start with capitalization (check other messages). This function is very ugly. There are several returns. Prefer single sentences for error messages. There isn't a concept of hint messages so provide a clear error message to the user.

I added a comment on the other PR #645 that I'm concerned about a fallback option. IMO it should be predictable. If the authentication database could not be found, it should fail.

Copy link
Member

@JelteF JelteF Dec 5, 2022

Choose a reason for hiding this comment

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

The chosen variable names are very similar (auth_db and auth_dbname)

Any suggestions for better ones? To me they seem quite clear, one is the db object, the other is a name. They are also inline with other functions that we already have like use_client_socket, where we have db and dbname.

There are several returns.

Why is this an issue? I suggested the early-return pattern because I thought the initial version was quite hard to follow.

I added a comment on the other PR #645 that I'm concerned about a fallback option. IMO it should be predictable. If the authentication database could not be found, it should fail.

I don't have a very strong opinion either way. But failing hard in case of wrong config, indeed has my slight preference.

Copy link
Member

Choose a reason for hiding this comment

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

The early-return pattern is not an issue per se. I just said this in hopes that someone else could come up with something easier to read. We can always make hard-to-read code easier to read by adding comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To confirm, is the ask here to make auth_dbname a breaking change between versions that when not set (in connection string or globally), it will cause disconnection?

The current behavior in that case is to use dbname in the connection string.

Copy link
Member

Choose a reason for hiding this comment

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

No, (afaik) the ask is: if it is set, but the database is not defined, it should error. Right now it it falls back to use the dbname in that case.

So keep this:

	/* use the client's target database if auth_dbname is not configured. */
	if (!auth_dbname) {
		return auth_db;
	}

but remove this

	/* if auth_dbname is configured, attemp to find it.
	 * when database is found but it is disabled, it
	 * will result in client disconnection.
	 */
	auth_db = find_database(auth_dbname);
	if (!auth_db) {
		slog_info(client, "Authentication database %s could not be found. Reverting to client database.", auth_dbname);
		return client->db;
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the logic of falling back. Instead, it will disconnect the client if auth_db cannot be found.

src/janitor.c Outdated
@@ -729,6 +729,11 @@ void kill_database(PgDatabase *db)
} else {
statlist_remove(&database_list, &db->head);
}

if (db->auth_dbname) {
Copy link
Member

Choose a reason for hiding this comment

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

This new line contains spaces. Single expression doesn't require to be enclosed by brackets. Remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly added to avoid tiny mistakes that we sometimes see in production, but can remove brackets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed brackets covering one line.

src/loader.c Outdated
}

/* The cast here is required to strip const qualifier to avoid build warnings. */
free((char *)old_auth_dbname);
Copy link
Member

Choose a reason for hiding this comment

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

Remove this comment. It is not so useful.

IMO old_auth_dbname variable makes the code less readable. I prefer to explicitly free the db->auth_dbname to make it clear what you want to do.

You didn't handle the case where new_auth_dbname is NULL. The current code returns true. It is fragile to rely on checks outside the function.

Single expression doesn't require to be enclosed by brackets. Remove it. (I'm referring to the first if).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed old_auth_dbname and reiterated over the function.

src/loader.c Outdated
} else if (strcmp("client_encoding", key) == 0) {
} else if (strcmp("auth_dbname", key) == 0) {
auth_dbname = val;
}else if (strcmp("client_encoding", key) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the space between } and else if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for pointing that, reverted.

} else if ((db->auth_dbname && !auth_dbname)
|| (!db->auth_dbname && auth_dbname)
|| (auth_dbname && strcmp(auth_dbname, db->auth_dbname) != 0)) {
changed = true;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you guarantee that db->auth_dbname is not NULL in the strcmp call? I prefer safeness in case someone rearranges these expression in the future.

Copy link
Member

Choose a reason for hiding this comment

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

That's not done in for the connect_query check above either.
IMHO it is quite obvious that you shouldn't rearrange these checks.

I do think that for consistency these two checks should be done in the same way.
So: !!auth_dbname != !!db->auth_dbname. Or honestly even !auth_dbname != !db->auth_dbname (the double in the connect_query check !! seem unecessary)

Copy link
Member

Choose a reason for hiding this comment

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

That code needs to be refactored IMO. Code must be easy to read. If nobody beats me, I will change the connect_query check after the release.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the current code isn't great to read. An improvement would be to have a dedicated function for it like strings_equal(char *left, char *right) that can handle `NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the discussion and suggestions. Added strings_equal function that covers NULL case.

src/loader.c Outdated
@@ -302,6 +335,10 @@ bool parse_database(void *base, const char *name, const char *connstr)
free(db->connect_query);
db->connect_query = connect_query;

if (!set_auth_dbname(db, auth_dbname)) {
goto fail;
}
Copy link
Member

Choose a reason for hiding this comment

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

Single expression doesn't require to be enclosed by brackets. Remove it.

Copy link
Member

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

for clarity marking as "request changes" instead of "approved" because of the feedback from @eulerto

src/loader.c Outdated Show resolved Hide resolved
src/loader.c Show resolved Hide resolved
src/loader.c Outdated
@@ -127,6 +127,41 @@ static char * cstr_get_pair(char *p,
return cstr_skip_ws(p);
}

/*
* Same as stcmp, but handles NULLs. If both sides are NULL, returns "true".
Copy link
Member

Choose a reason for hiding this comment

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

small typo

Suggested change
* Same as stcmp, but handles NULLs. If both sides are NULL, returns "true".
* Same as strcmp, but handles NULLs. If both sides are NULL, returns "true".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with commit de31d44

Copy link
Member

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

LGTM

@goksgie goksgie requested a review from eulerto December 20, 2022 08:38
@JelteF
Copy link
Member

JelteF commented Jan 2, 2023

@eulerto did you want to look at this once more? Or are you okay with merging it now?

Copy link
Member

@eulerto eulerto left a comment

Choose a reason for hiding this comment

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

I have no additional comments.

goksgie and others added 10 commits January 3, 2023 17:11
This commit implements the auth_dbname option introduced by the
following PR: pgbouncer#645

In essence, users should be able to specify authentication database to
execute auth_query against. This will help them to avoid creating same
function on every database in their server.

A user can specify `auth_dbname` in the connection string, or they can
set it globally. In case `auth_dbname` is specified in connection string
(under database) it will override the global configuration.
In the previous version, we did not clear the auth_dbname, which lead to
memory leak.
When we are not able to set the auth_dbname, it is likely
due to memory issue. This commit improves the log message
we display on such error.
The previous version of the function was hard to follow
with nested branches. Instead, followed reviewer's guide
to simplify the execution flow.
According to maintainer's suggestions, this commit introduces a
new comment on why the cast is required. Additionally, it changes
the cast to (char *) to be more understandable.
Previously, the tests were not validating with which user we
were logging in as. This commit introduces those validations
to complete the tests.
Adding a new line at the end of the header file
as it seems to be the convention.
Changes the logic of reverting to client's target database to
disconnection with appropriate configuration error.

Modifies tests to reflect the change.

Re-iterates over the function description.
Based on review comments, shortening the descriptions of
auth_dbname setting.
goksgie and others added 8 commits January 3, 2023 17:11
Makes the description aligned with others.
This commit refactors the string comparisions being done in loader.
Additionally, it refactors the set_auth_dbname function.
This commit refactors the `strings_equal` function to make
it more readable and maintainable.
Removed one liner brackets on IF case to align with other parts
 of the code.
Remove superfluous whitespaces and make a few cosmetic changes.
@kotik-adjust
Copy link

Hey, wondering when you're gonna release the new version of pgbouncer with this feature. :)

@3nprob
Copy link

3nprob commented Mar 17, 2023

Cheers everyone involved for getting this merged <3

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.

6 participants