Re: Password identifiers, protocol aging and SCRAM protocol

On Sat, Nov 5, 2016 at 12:58 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> The organization of these patches makes sense to me.
>
> On 10/20/16 1:14 AM, Michael Paquier wrote:
>> - 0001, moving all the SHA2 functions to src/common/ and introducing a
>> PG-like interface. No actual changes here.
>
> That's probably alright, although the patch contains a lot more changes
> than I would imagine for a simple file move.  I'll still have to review
> that in detail.

The main point is to know if people are happy of having an interface
of the type pg_sha256_[init|update|finish] to tackle the fact that
core code contains a set of routines that map with some of the OpenSSL
APIs...

>> - 0002, replacing PostmasterRandom by pg_strong_random(), with a fix
>> for the cancel key problem.
>> - 0003, adding for pg_strong_random() a fallback for any nix platform
>> not having /dev/random. This should be grouped with 0002, but I split
>> it for clarity.
>
> Also makes sense, but will need more detailed review.  I did not follow
> the previous PostmasterRandom issues closely.

pademelon does not have /dev/random and /dev/urandom, so the issue is
related to having a fallback method... But Heikki feels that having a
method producing potentially weak keys should not be in
pg_strong_random(). I'd suggest to control that with a ./configure
switch and call it a day. Platforms without any of the four randomness
methods pg_strong_random includes play a dangerous game but...

>> - 0004, Add encoding routines for base64 without whitespace in
>> src/common/. I improved the error handling here by making them return
>> -1 in case of error and let the caller handle the error.
>
> I don't think we want to have two different copies of base64 routines.
> Surely we can make the existing routines do what we want with a
> parameter or two about whitespace and line length.

We could. Though after hacking on that I find cleaner copying the code
from encoding.c after removing the whitespace handling, as Heikki has
suggested.

>> - 0005, Refactor decision-making of password encryption into a single routine.
>
> It makes sense to factor this out.  We probably don't need the pstrdup
> if we just keep the string as is.  (You could make an argument for it if
> the input values were const char *.)  We probably also don't need the
> pfree.  The Assert(0) can probably be done better.  We usually use
> elog() in such cases.

Hm, OK. Agreed with that.

>> - 0006, Add clause PASSWORD val USING protocol to CREATE/ALTER ROLE.
>
> "protocol" is a weird choice here.  Maybe something like "method" is
> better.  The way the USING clause is placed can be confusing.  It's not
> clear that it belongs to PASSWORD.  If someone wants to augment another
> clause in CREATE ROLE with a secondary argument, then it could get
> really confusing.  I'd suggest something to group things together, like
> PASSWORD (val USING method).  The method could be an identifier instead
> of a string.

Why not.

> Please add an example to the documentation and explain better how this
> interacts with the existing ENCRYPTED PASSWORD clause.

Sure.

>> - 0007, the SCRAM implementation.
>
> No documentation about pg_hba.conf changes, so I don't know how to use
> this. ;-)

Oops. I have focused on the code a lot during last rewrite of the
patch and forgot that. I'll think about something.

> This implements SASL and SCRAM and SHA256.  We need to be clear about
> which term we advertise to users.  An explanation in the missing
> documentation would probably be a good start.

pg_hba.conf uses "scram" as keyword, but scram refers to a family of
authentication methods. There is as well SCRAM-SHA-1, SCRAM-SHA-256
(what this patch does). Hence wouldn't it make sense to use
scram_sha256 in pg_hba.conf instead? If for example in the future
there is a SHA-512 version of SCRAM we could switch easily to that and
define scram_sha512.

There is also the channel binding to think about... So we could have a
list of keywords perhaps associated with SASL? Imagine for example:
sasl    $algo,$channel_binding
Giving potentially:
sasl    scram_sha256
sasl    scram_sha256,channel
sasl    scram_sha512
sasl    scram_sha512,channel
In the case of the patch of this thread just the first entry would
make sense, once channel binding support is added a second
keyword/option could be added. And there are of course other methods
that could replace SCRAM..

> I would also like to see a test suite that covers the authentication
> specifically.

What you have in mind is a TAP test with a couple of roles and
pg_hba.conf getting rewritten then reloaded? Adding it in
src/test/recovery/ is the first place that comes in mind but that's
not really something related to recovery... Any ideas?
-- 
Michael