Re: Proposal: Support custom authentication methods using hooks

On Tue, 2022-03-15 at 12:27 -0700, samay sharma wrote:
> This patch-set adds the following:
>
> * Allow multiple custom auth providers to be registered (Addressing
> feedback from Aleksander and Andrew)
> * Modify the test extension to use SCRAM to exchange secrets (Based
> on Andres's suggestion)
> * Add support for custom auth options to configure provider's
> behavior (by exposing a new hook) (Required by OAUTHBEARER)
> * Allow custom auth methods to use usermaps. (Required by
> OAUTHBEARER)

Review:

* I retract my previous comment that "...it seems to only be useful
with plaintext password authentication over an SSL connection"[0].
- it can be used with SCRAM, as you've shown, which could be useful
for storing the credentials elsewhere
- it can be used with one-time auth tokens, or short-lived auth
tokens, obtained from some other service
- it can be used with SASL to negotiate with special clients that
support some other auth protocol (this is not shown, but having custom
auth would make it interesting to experiment in this area)

* There's a typo in the top comment for the test module, where you say
that it lives in "contrib", but it actually lives in
"src/test/modules".

* Don't specify ".so" in shared_preload_libraries (in the test module).

* Needs docs.

* I'm wondering whether provider initialization/lookup might have a
performance impact. Does it make sense to initialize the
CustomAuthProvider once when parsing the hba.conf, and then cache it in
the HbaLine or somewhere?

* When specifying the provider in pg_hba.conf, I first made a mistake
and specified it as "test_auth_provider" (which is the module name)
rather than "test" (which is the provider name). Maybe the message
could be slightly reworded in this case, like "no authentication
provider registered with name %s"? As is, the emphasis seems to be on
failing to load the library, which confused me at first.

* Should be a check like "if
(!process_shared_preload_libraries_in_progress) ereport(...)" in
RegisterAuthProvider.

* Could use some fuzz testing on the hba line parsing. For instance, I
was able to specify "provider" twice. And if I specify "allow" first,
then "provider" second, it fails (this is probably fine to require
provider to be first, but needs to be documented/commented somewhere).

* If you are intending for your custom provider to be open source, it
would be helpful to show the code (list, github link, whatever), even
if rough. That would ensure that it's really solving at least one real
use case and we aren't forgetting something.

* In general I like this patch. Trying to catch up on the rest of the
discussion.

Regards,
Jeff Davis

[0]
https://www.postgresql.org/message-id/bfc55e8045453659df26cd60035bfbb4b9530052.camel@j-davis.com