Implement cluster-wide support for passcheck hook

adamguo0 commented 1 year ago

Issue #, if available: #229 and #230

Description of changes:

Uses a very similar approach to clientauth hook. Passcheck hook spins up dynamic background workers to query passcheck database, communicating via shared memory. The main difference is that background workers are registered dynamically when the password_check_hook is triggered -- they are terminated once the query is done. There is a single "slot" in the shared memory struct and only one background worker can exist at once.

There is potentially some refactoring that can be done to combine code between this and the clientauth hook. For example, passcheck.c registers its own shmem_startup_hook and shmem_request_hook. We can think about having unified shmem hooks if that's better, however I'm not sure how passcheck_ss and clientauth_ss would be scoped in that case. The BGW code could potentially be refactored, but the dynamic versus static approaches are a little different and the shared memory structs are different too. In summary there could be some refactoring but I'm not sure how much / if it's worth it. Perhaps if we re-use the BGW scheme for future features we'll have a better idea of how to refactor.

Introduces a new GUC, pgtle.passcheck_db_name. A user's existing passcheck functions will be unaffected, but functions that are not in pgtle.passcheck_db_name are ignored after upgrade. These functions can be re-created in pgtle.passcheck_db_name and registered using pgtle.register_feature to re-enable them. Since the background workers are dynamic, pgtle.passcheck_db_name is on SIGHUP context (no need to be postmaster).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

adamguo0 commented 1 year ago

Rebased against HEAD

anth0nyleung commented 1 year ago

IIUC, this commit would force passcheck to be cluster-wide. Is there a valid use case where we still want to do this per database level?

anth0nyleung commented 1 year ago

It will be beneficial if we can refactor the logic to spin up dynamic background worker and the shared memory use into something re-useable and more modular, but that is probably out of scope for this issue, maybe it's worthwile to have a different GH issue to track that (if it doesnt already exist)

adamguo0 commented 1 year ago

IIUC, this commit would force passcheck to be cluster-wide. Is there a valid use case where we still want to do this per database level?

That's a good question, there are possibly scenarios where the database admin may want passcheck logic to be different per database. We would need to add a database parameter to the passcheck hook function for that to be possible, which would break previously defined functions.

We could potentially support both use cases (query a central database and query the current database) with a GUC toggle. Maybe that would be helpful for migrating any existing users as well

adamguo0 commented 1 year ago

Some thoughts:

I think we can consider using a single GUC for pgtle.clientauth_db_name and pgtle.passcheck_db_name, something like pgtle.worker_db_name.
Is there value in letting the user specify which database role to connect as? e.g. pgtle.worker_db_role. Right now we have a disclaimer in the clientauth docs that connections are made as superuser, I'm not sure that's acceptable.

anth0nyleung commented 12 months ago

I'd suggest checking in with @jkatz for thoughts on the transitioning from per-database to cluster-wide - https://github.com/aws/pg_tle/pull/232#issuecomment-1749070697

adamguo0 commented 12 months ago

Decided to leave the current GUCs as is (clientauth and passcheck separate, no new worker role GUC)
Let users enable cluster-wide passcheck by setting pgtle.passcheck_db_name. If the value is empty string (which is the default), then use the legacy same-database logic
Revert changes to pg_tle_api.sql to make sure existing passcheck behaviour still works, and duplicate into a new test file pg_tle_api_clusterwide.sql that tests clustewide (sets pgtle.passcheck_db_name)

anth0nyleung commented 11 months ago

Should we also update the documentation to mention that the new GUCs we introduced, and how passcheck would be enabled cluster-wide?

adamguo0 commented 11 months ago

Should we also update the documentation to mention that the new GUCs we introduced, and how passcheck would be enabled cluster-wide?

Yeah definitely, I can add that into this PR or follow up with a second one

aws / pg_tle

Implement cluster-wide support for passcheck hook #232