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.ca...@j-davis.com