Re: Password identifiers, protocol aging and SCRAM protocol

On Thu, Sep 29, 2016 at 12:48 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Sep 28, 2016 at 8:55 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Our b64_encode routine does use whitespace, so we can't use it as is for
>>> SCRAM. As the patch stands, we might never output anything long enough to
>>> create linefeeds, but let's be tidy. The base64 implementation is about 100
>>> lines of code, so perhaps we should just leave src/backend/utils/encode.c
>>> alone, and make a new copy of the base64 routines in src/common.
>>
>> OK, I'll refresh that tomorrow with the rest. Thanks for the commit to
>> extend password_encryption.
>
> OK, so after more chatting with Heikki, here is a list of TODO items
> and a summary of the state of things:
> - base64 encoding routines should drop whitespace (' ', \r, \t), and
> it would be better to just copy those from the backend's encode.c to
> src/common/. No need to move escape and binary things, nor touch
> backend's base64 routines.
> - No need to move sha1.c to src/common/. Better to just get sha2.c
> into src/common/ as we aim at SCRAM-SHA-256.
> - random() called in the client is no good. We need something better here.
> - The error handling needs to be reworked and should follow the
> protocol presented by RFC5802, by sending back e= messages. This needs
> a bit of work, not much I think though as the infra is in place in the
> core patch.
> - Let's discard the md5-or-scram optional thing in pg_hba.conf. This
> complicates the error handling protocol.
>
> I am marking this patch as returned with feedback for current CF and
> will post a new set soon, moving it to the next CF once I have the new
> set of patches ready for posting.

And so we are back on that, with a new set:
- 0001, introducing pg_strong_random() in src/port/ to have the
backend portion of SCRAM use it instead of random(). This patch is
from Magnus who has kindly sent is to me, so the authorship goes to
him. This patch replaces at the same time PostmasterRandom() with it,
this way once SCRAM gets integrated both the frontend and the backend
finish using the same facility. I think that's good for consistency.
Compared to the version Magnus has sent me, I have changed two things:
-- Reading from /dev/urandom and /dev/random is not influenced by
EINTR. read() handling is also made better in case of partial reads
from a given source.
-- Win32 Crypto routines use MS_DEF_PROV instead of NULL. I think
that's a better idea to not let the user the choice of the encryption
source here.
- 0002, moving all the SHA2 functions to src/common/. As mentioned
upthread, this keeps the amount of code moved to src/common/ to a
minimum. I have been careful to get the header files and copyright
mentions into a correct shape at the same time. I have moved a couple
of code blocks in a shape that make a bit more sense, not sure how you
feel about that, Heikki.
- 0003, creating a set of base64 routines without whitespace handling.
That's more or less a copy of what is in encode.c, simplified for
SCRAM. At the same time I have prefixed the routines with pg_ to make
a difference with what is in encode.c.
- 0004 does some refactoring regarding encrypted passwords in user.c
- 0005 creates a generic routine to fetch password and valid until
values for a role
- 0006 adds support for SCRAM-SHA-256. I have not yet addressed the
concerns regarding the handling of e= messages yet. I have fixed the
nonce generation with random() though.
- 0007 adds the extension for CREATE ROLE .. PASSWORD foo USING protocol
- 0008 is a basic set of regression tests to test passwords.

To be honest, I have now put some love into 0001~0004, but less in the
rest. The first refactoring patches are going to be subject to enough
comments I guess :) I'll put more love into 0005~ in the next couple
of days though while reworking the message interface.

Thanks,
--
Michael