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

Hi!

I'm interested in this feature presence in the PostgreSQL core. Will
try to provide valuable review/comments/suggestions and other help.

On Tue, 10 Oct 2023 at 16:17, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
>
> > On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> > >
> > > Next steps:
> > > - Break the patch into a series of smaller patches.
> > > - Add TAP tests (test the ability to actually login with these passwords)
> > > - Add/update documentation
> > > - Add more regression tests
>
> Please see attached the v4 of the patchset that introduces the notion
> of named passwords slots, namely 'first' and 'second' passwords, and
> allows users to address each of these passwords separately for the
> purposes of adding, dropping, or assigning expiration times.
>
> Apart from the changes described by each patch's commit title, one
> significant change since v3 is that now (included in v4-0002...patch)
> it is not allowed for a role to have a mix of a types of passwords.
> When adding a password, the patch ensures that the password being
> added uses the same hashing algorithm (md5 or scram-sha-256) as the
> existing password, if any. Having all passwords of the same type
> helps the server pick the corresponding authentication method during
> connection attempt.
>
> The v3 patch also had a few bugs that were exposed by cfbot's
> automatic run. All those bugs have now been fixed, and the latest run
> on the v4 branch [1] on my private Git repo shows a clean run [1].
>
> The list of patches, and their commit titles are as follows:
>
> > v4-0001-...patch Add new columns to pg_authid
> > v4-0002-...patch Update password verification infrastructure to handle two passwords
> > v4-0003-...patch Added SQL support for ALTER ROLE to manage two passwords
> > v4-0004-...patch Updated pg_dumpall to support exporting a role's second password
> > v4-0005-...patch Update system views pg_roles and pg_shadow
> > v4-0006-...patch Updated pg_authid catalog documentation
> > v4-0007-...patch Updated psql's describe-roles meta-command
> > v4-0008-...patch Added documentation for ALTER ROLE command
> > v4-0009-...patch Added TAP tests to prove that a role can use two passwords to login
> > v4-0010-...patch pgindent run
> > v4-0011-...patch Run pgperltidy on files changed by this patchset
>
> Running pgperltidy updated many perl files unrelated to this patch, so
> in the last patch I chose to include only the one perl file that is
> affected by this patchset.
>
> [1]: password_rollover_v4 (910f81be54)
> https://github.com/gurjeet/postgres/commits/password_rollover_v4
>
> [2]: https://cirrus-ci.com/build/4675613999497216
>
> Best regards,
> Gurjeet
> http://Gurje.et

Latest attachment does not apply to HEAD anymore. I have rebased
them. While rebasing, a couple of minor changes were done:

1) Little correction in the `plain_crypt_verify` comment. IMO this
sounds a little better and comprehensible, is it?

> - * 'shadow_pass' is the user's correct password hash, as stored in
> - * pg_authid's rolpassword or rolsecondpassword.
> + * 'shadow_pass' is one of the user's correct password hashes, as stored in
> + * pg_authid's.

2) in v4-0004:

> /* note: rolconfig is dumped later */
> - if (server_version >= 90600)
> + if (server_version >= 170000)
> printfPQExpBuffer(buf,
> "SELECT oid, rolname, rolsuper, rolinherit, "
> "rolcreaterole, rolcreatedb, "
> - "rolcanlogin, rolconnlimit, rolpassword, "
> - "rolvaliduntil, rolreplication, rolbypassrls, "
> + "rolcanlogin, rolconnlimit, "
> + "rolpassword, rolvaliduntil, "
> + "rolsecondpassword, rolsecondvaliduntil, "
> + "rolreplication, rolbypassrls, "
> + "pg_catalog.shobj_description(oid, '%s') as rolcomment, "
> + "rolname = current_user AS is_current_user "
> + "FROM %s "
> + "WHERE rolname !~ '^pg_' "
> + "ORDER BY 2", role_catalog, role_catalog);
> + else if (server_version >= 90600)
> + printfPQExpBuffer(buf,
> + "SELECT oid, rolname, rolsuper, rolinherit, "
> + "rolcreaterole, rolcreatedb, "
> + "rolcanlogin, rolconnlimit, "
> + "rolpassword, rolvaliduntil, "
> + "rolsecondpassword, rolsecodnvaliduntil, "
> + "null as rolsecondpassword, null as rolsecondvaliduntil, "
> + "rolreplication, rolbypassrls, "

Is it a bug in server_version < 17000 && server_version >= 90600 case?
we are trying to get "rolsecondpassword, rolsecodnvaliduntil, " in
this case? I have removed this, let me know if there is my
misunderstanding.
Also, if stmt changed to ` if (server_version >= 180000)` since pg17
feature freeze.

v4-0005 - v4-000 Applied cleanly, didn't touch them. But, I haven't
reviewed them yet either.

v4-0010-pgindent-run.patch - is it actually needed?

Overall comments:

1) AFAIU we are forcing all passwords to have/interact with the same
salt. We really have no choice here, because in the case of SCRAM auth
salt is used client-side to authenticate the server.I dont feel this
is the best approach to restrict auth. process this much, though I
didn't come up with strong objections to it. Anyway, what if we give
the server a hint, which password to use in the startup message (in
case users have more than one password)? This way we still can use
different salts.

2) I'm also not a big fan of max-2-password restriction. There are
objections in the thread that having multiple passwords and named
passwords leads to problems on the server-side, but I think that at
least named passwords are a useful feature for those who use external
Vault systems for password management. In this case, you can set the
name of your password to the version of Vault secret, which is a very
natural thing to do (and, thus, support it).

3) Should we unite 0001 & 0004 patches in one? Or maybe 0003 & 0004?
It is not a good idea to commit one of these patches without
another.... (Since we can reach a database state that cannot be
`pg_dump`-ed)

So, that's it.