Re: [PoC/RFC] Multiple passwords, interval expirations

On Fri, Jul 1, 2022 at 8:13 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> On Fri, Jul 1, 2022 at 10:51 Brindle, Joshua <joshuqbr(at)amazon(dot)com> wrote:
>> On 6/30/22 8:20 PM, Stephen Frost wrote:

>> > I've gone ahead and updated it, cleaned up a couple things, and make it
>> > so that check-world actually passes with it. Attached is an updated
>> > version and I'll add it to the July commitfest.
>>
>> Ah, thanks. Hopefully it wasn't too horrible of a rebase.
>
> Wasn’t too bad.. needs more clean-up, there was some white space issues and some simple re-base stuff, but then the support for “md5” pg_hba option was broken for users with SCRAM passwords because we weren’t checking if there was a SCRAM pw stored and upgrading to SCRAM in that case. That’s the main case that I fixed. We will need to document this though, of course. The patch I submitted should basically do:
>
> pg_hba md5 + md5-only pws -> md5 auth used
> pg_hba md5 + scram-only pws -> scram
> pg_hba md5 + md5 and scram pws -> scram
> pg_hba scram -> scram
>
> Not sure if we need to try and do something to make it possible to have pg_hba md5 + mixed pws and have md5 used but it’s tricky as we would have to know on the server side early on if that’s what we want to do. We could add an option to md5 to say “only do md5” maybe but I’m also inclined to not bother and tell people to just get moved to scram already.
>
> For my 2c, I’d also like to move to having a separate column for the PW type from the actual secret but that’s largely an independent change.

The docs say this about rolpassword in case it stores SCRAM-SHA-256
encrypted password "If the password is encrypted with SCRAM-SHA-256,
it has the format SCRAM-SHA-256$... This format is the same as that
specified by RFC-5803". So I believe our hands are tied, and we
cannot change that without breaking compliance with RFC 5803.

Please see attached v4 of the patch. The patch takes care of rebase to
the master/17-devel branch, and includes some changes, too. The
rebase/merge conflicts were quite involved, since some affected files
had been removed, or even split into multiple files over the course of
the last year; resolving merge-conflicts was more of a grunt work.

The changes since V3 are (compare [1] vs. [2], Git branches linked below):
- Remove TOAST table and corresponding index from pg_authid.
- Fix memory leak/allocation bug; replace malloc() with guc_alloc().
- Fix assumptions about passed-in double-pointers to GUC handling functions.
- Remove the new function is_role_valid() and its call sites, because
I believe it made backward-incompatible change to authentication
behavior (see more below).
- Improve error handling that was missing at a few places.
- Remove unnecessary checks, like (*var != NULL) checks when we know
all callers pass a NULL by convention.
- Replace MemSet() calls with var={0} styled initialization.
- Minor edits to docs to change them from pg_authid to pg_auth_password.

More details about why I chose to remove is_role_valid() and revert to
the old code:
is_role_valid() was a new function that pulled out a small section of
code from get_role_passwords() . I don't think moving this code block
to a new function gains us anything; in fact, it now forces us to call
the new function in two new locations, which we didn't have to do
before. It has to throw the same error messages as before, to maintain
compatibility with external tools/libraries, hence it duplicates those
messages as well, which is not ideal.

Moreover, before the patch, in case of CheckPasswordAuth(), the error
(if any) would have been thrown _after_ network communication done by
sendAuthRequest() call. But with this patch, the error is thrown
before the network interaction, hence this changes the order of
network interaction and the error message. This may have security
implications, too, but I'm unable to articulate one right now.

If we really want the role-validity check to be a function of its own,
a separate patch can address that; this patch doesn't have to make
that decision.

Open question: If a client is capable of providing just md5 passwords
handshake, and because of pg_hba.conf setting, or because the role has
at least one SCRAM password (essentially the 3rd case you mention
above: pg_hba md5 + md5 and scram pws -> scram), the server will
respond with a SASL/SCRAM authentication response, and that would
break the backwards compatibility and will deny access to the client.
Does this make it necessary to use a newer libpq/client library?

Before the patch, the rolvaliduntil was used to check and complain
that the password has expired, as the docs explicitly state that
rolvaliduntil represents "Password expiry time (only used for password
authentication); null if no expiration" . Keeping that column after
the introduction of per-password expiry times now separates the
role-validity from password validity. During an internal discussion a
curiosity arose whether we can simply remove rolvaliduntil. And I
believe the answer is yes, primarily because of how the docs describe
the column. So my proposal is to remove rolvaliduntil from pg_authid,
and on a case-by-case basis, optionally replace its uses with
max(pg_auth_password.expiration).

Comments?

Next steps:
- Break the patch into smaller patches.
- Address TODO items
- Comment each new function
- Add tests
- Add/update documentation

PS: Since this is a large patch, and because in some portions the code
has been indented by a level or two (e.g. to run a `for` loop over
existing code for single-password), I have found the following Git
command to be helpful in reviewing the changes between master and this
branch,: `git diff -b --color-words -U20 origin/master...HEAD -- `

[1]: v3 patch, applied to a contemporary commit on master branch
https://github.com/gurjeet/postgres/commits/multiple_passwords_v3

[2]: main development branch, patch rebased to current master branch,
followed by many changes
https://github.com/gurjeet/postgres/commits/multiple_passwords

Best regards,
Gurjeet
http://Gurje.et