Re: Password identifiers, protocol aging and SCRAM protocol

On Sat, Nov 5, 2016 at 9:36 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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...

Or in short that:
+extern void pg_sha256_init(pg_sha256_ctx *ctx);
+extern void pg_sha256_update(pg_sha256_ctx *ctx,
+                       const uint8 *input0, size_t len);
+extern void pg_sha256_final(pg_sha256_ctx *ctx, uint8 *dest);

>>> - 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.

I have replaced the Assert(0) with an elog(ERROR). OK for the
additional palloc and pfree calls. I just made that for consistency in
the routine for all the password types, but changed your way.

>>> - 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.

Done.

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

Done.

>>> - 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.

OK, I have added more docs regarding the use of scram in pg_hba.conf,
particularly in client-auth.sgml to describe what scram is better than
md5 in terms of protection, and also completed the data of pg_hba.conf
about the new keyword used in it.

>> 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?

OK, hearing no complaints I have done exactly that and added a test in
src/test/recovery/ with patch 0009. This place may not be the best fit
though, but it looks like an overkill to add a new module in
src/test/modules just for that and that's a pretty compact test.

On Wed, Nov 9, 2016 at 3:13 PM, Victor Wagner <vitus@wagner.pp.ru> wrote:
> On Tue, 18 Oct 2016 16:35:27 +0900
> Michael Paquier <michael.paquier@gmail.com> wrote:
>> Attached is a rebased patch set for SCRAM, with the following things:
>> - 0001, moving all the SHA2 functions to src/common/ and introducing a
>> PG-like interface. No actual changes here.
>
> It seems, that client nonce generation in this patch is not
> RFC-compliant.
>
> RFC 5802 states that SCRAM nonce should be
>
> a sequence of random printable ASCII
>       characters excluding ','
>
> while this patch uses sequence of random bytes from pg_strong_random
> function with zero byte appended.

Right, I have fixed that in 0007 with a solution less exotic than what
you suggested upthread by scanning the ASCII characters between '!'
and '~', ignoring comma if selected.
-- 
Michael