Message-ID: <3bbf5968ea705700a2605d6db74d8510bea7940b.camel@j-davis.com>
Subject: Re: Add "password_protocol" connection parameter to libpq
From: Jeff Davis <pgsql@j-davis.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
Date: Thu, 08 Aug 2019 23:16:24 -0700
In-Reply-To: <20190809030021.GG3194@paquier.xyz>
References: <227015d8417f2b4fef03f8966dbfa5cbcc4f44da.camel@j-davis.com>
	 <20190809030021.GG3194@paquier.xyz>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote:
> What about auth_protocol then?  It seems to me that it could be
> useful
> to have the restriction on AUTH_REQ_MD5 as well.

auth_protocol does sound like a good name. I'm not sure what you mean
regarding MD5 though.

> This makes it sound like there is a linear hierarchy among all those
> protocols, which is true in this case, but if the list of supported
> protocols is extended in the future it may be not.

We already have that concept to a lesser extent, with the md5
authentication method also permitting scram-sha-256.

Also note that the server chooses what kind of authentication request
to send, which imposes a hierarchy of password < md5 < sasl. Within the
sasl authentication request the server can advertise multiple supported
mechanisms, though, so there doesn't need to be a hierarchy among sasl
mechanisms.

> I think that this should have TAP tests in src/test/authentication/
> so
> as we make sure of the semantics.  For the channel-binding part, the
> logic path for the test would be src/test/ssl.

Will do.

> Another thing that was discussed on the topic would be to allow a
> list
> of authorized protocols instead.  I personally don't think that we
> need to go necessarily this way, but it could make the integration of
> things line scram-sha-256,scram-sha-256-plus easier to integrate in
> application flows.

That sounds good, but there are a lot of possibilities and I can't
quite decide which way to go.

We could expose it as an SASL option like:

   saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha-
256-plus}

But that doesn't allow for multiple acceptable mechanisms, which could
make migration a pain. We try to use a comma-list like:

   saslmode = {disable|prefer|require}
   saslmech = all | {scram-hash-256|scram-hash-256-plus}[,...]

Or we could over-engineer it to do something like:

   saslmode = {disable|prefer|require}
   saslmech = all | {scram|future_mech}[,...]
   scramhash = all | {sha-256|future_hash}[,...]
   scram_channel_binding = {disable|prefer|require}

(Aside: is the channel binding only a SCRAM concept, or also a SASL
concept?)

Also, working with libpq I found myself wondering why everything is
based on strings instead of enums or some other structure. Do you know
why it's done that way?

Regards,
	Jeff Davis