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

This is not intended for PG15.

Attached are a proof of concept patchset to implement multiple valid
passwords, which have independent expirations, set by a GUC or SQL
using an interval.

This allows the superuser to set a password validity period of e.g.,
60 days, and for users to create new passwords before the old ones
expire, and use both until the old one expires. This will aid in
password rollovers for apps and other systems that need to connect
with password authentication.

The first patch simply moves password to a new catalog, no functional changes.
The second patch allows multiple passwords to be used simultaneously.
The third adds per-password expiration, SQL grammar, and the GUC.

Some future work intended to build on this includes:
- disallowing password reuse
- transitioning between password mechanisms

Example output (note the NOTICES can go away, but are helpful for
demo/testing purposes):

postgres=# alter system set password_valid_duration = '1 day';
NOTICE:  Setting password duration to "1 day"
ALTER SYSTEM
postgres=# select pg_reload_conf();
 pg_reload_conf
----------------
 t
(1 row)

postgres=# create user joshua password 'a' expires in '5 minutes';
NOTICE:  Setting password duration to "1 day"
NOTICE:  Password will expire at: "2022-03-02 14:52:31.217193" (from SQL)
CREATE ROLE

---

$ psql -h 127.0.0.1 -U joshua postgres
Password for user joshua:
psql (12.7, server 15devel)
WARNING: psql major version 12, server major version 15.
         Some psql features might not work.
Type "help" for help.

postgres=> alter role joshua passname 'newone' password 'asdf' expires
in '1 year';
ERROR:  must be superuser to override password_validity_duration GUC
postgres=> alter role joshua passname 'newone' password 'asdf';
NOTICE:  Password will expire at: "2022-03-03 14:47:53.728159" (from GUC)
ALTER ROLE
postgres=>

--

postgres=# select * from pg_auth_password ;
 roleid |  name   |
           password
                    |          expiration

--------+---------+-------------------------------------------------------------------------------------------------------------------
--------------------+-------------------------------
     10 | __def__ |
SCRAM-SHA-256$4096:yGiHIYPwc2az7xj/7TIyTA==$OQL/AEcEY1yOCNbrZEj4zDvNnOLpIqltOW1uQvosLvc=:9VRRppuIkSrwhiBN5ePy8wB1y
zDa/2uX0WUx6gXi93E= |
  16384 | __def__ |
SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$1Ivp4d+vAWxowpuGEn05KR9lxyGOms3yy85k3D7XpBg=:k8xUjU6xrJG17PMGa/Zya6pAE
/M7pEDaoIFmWvNIEUg= | 2022-03-02 06:52:31.217193-08
  16384 | newone  |
SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$WK3+41CCGDognSnZrtpHhv00z9LuVUjHR1hWq8T1+iE=:w2C5GuhgiEB7wXqPxYfxBKB+e
hm4h6Oeif1uzpPIFVk= | 2022-03-03 06:47:53.728159-08
(3 rows)

On Wed, Mar 2, 2022 at 9:58 AM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
>
> This is not intended for PG15.
>
> Attached are a proof of concept patchset to implement multiple valid
> passwords, which have independent expirations, set by a GUC or SQL
> using an interval.
>

<snip>

> postgres=# select * from pg_auth_password ;
>  roleid |  name   |
>            password
>                     |          expiration
>
--------+---------+-------------------------------------------------------------------------------------------------------------------
> --------------------+-------------------------------
>      10 | __def__ |
> SCRAM-SHA-256$4096:yGiHIYPwc2az7xj/7TIyTA==$OQL/AEcEY1yOCNbrZEj4zDvNnOLpIqltOW1uQvosLvc=:9VRRppuIkSrwhiBN5ePy8wB1y
> zDa/2uX0WUx6gXi93E= |
>   16384 | __def__ |
> SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$1Ivp4d+vAWxowpuGEn05KR9lxyGOms3yy85k3D7XpBg=:k8xUjU6xrJG17PMGa/Zya6pAE
> /M7pEDaoIFmWvNIEUg= | 2022-03-02 06:52:31.217193-08
>   16384 | newone  |
> SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$WK3+41CCGDognSnZrtpHhv00z9LuVUjHR1hWq8T1+iE=:w2C5GuhgiEB7wXqPxYfxBKB+e
> hm4h6Oeif1uzpPIFVk= | 2022-03-03 06:47:53.728159-08
> (3 rows)

There's obviously a salt problem here that I'll need to fix that
apparently snuck in at the last rebase, but this brings up one aspect
of the patchset I didn't mention in the original email:

For the SCRAM protocol to work as is with existing clients the salt
for each password must be the same. Right now ALTER USER will find and
reuse the salt, but a user passing in a pre-computed SCRAM secret
currently has no way to get the salt.

for \password (we'll need a new one that takes a password name) I was
thinking libpq could hold onto the salt that was used to log in, but
for outside computation we'll need some way for the client to request
it.

None of that is done yet.

On Wed, Mar 2, 2022 at 10:35 AM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
>
> On Wed, Mar 2, 2022 at 9:58 AM Joshua Brindle
> <joshua.brindle@crunchydata.com> wrote:
> >
> > This is not intended for PG15.
> >
> > Attached are a proof of concept patchset to implement multiple valid
> > passwords, which have independent expirations, set by a GUC or SQL
> > using an interval.
> >
>
> <snip>
>
> > postgres=# select * from pg_auth_password ;
> >  roleid |  name   |
> >            password
> >                     |          expiration
> >
--------+---------+-------------------------------------------------------------------------------------------------------------------
> > --------------------+-------------------------------
> >      10 | __def__ |
> > SCRAM-SHA-256$4096:yGiHIYPwc2az7xj/7TIyTA==$OQL/AEcEY1yOCNbrZEj4zDvNnOLpIqltOW1uQvosLvc=:9VRRppuIkSrwhiBN5ePy8wB1y
> > zDa/2uX0WUx6gXi93E= |
> >   16384 | __def__ |
> > SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$1Ivp4d+vAWxowpuGEn05KR9lxyGOms3yy85k3D7XpBg=:k8xUjU6xrJG17PMGa/Zya6pAE
> > /M7pEDaoIFmWvNIEUg= | 2022-03-02 06:52:31.217193-08
> >   16384 | newone  |
> > SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$WK3+41CCGDognSnZrtpHhv00z9LuVUjHR1hWq8T1+iE=:w2C5GuhgiEB7wXqPxYfxBKB+e
> > hm4h6Oeif1uzpPIFVk= | 2022-03-03 06:47:53.728159-08
> > (3 rows)
>
> There's obviously a salt problem here that I'll need to fix that
> apparently snuck in at the last rebase, but this brings up one aspect
> of the patchset I didn't mention in the original email:
>

Attached are fixed patches rebased against the lastest master.


> For the SCRAM protocol to work as is with existing clients the salt
> for each password must be the same. Right now ALTER USER will find and
> reuse the salt, but a user passing in a pre-computed SCRAM secret
> currently has no way to get the salt.
>
> for \password (we'll need a new one that takes a password name) I was
> thinking libpq could hold onto the salt that was used to log in, but
> for outside computation we'll need some way for the client to request
> it.
>
> None of that is done yet.

> > On Wed, Mar 2, 2022 at 9:58 AM Joshua Brindle
> > <joshua.brindle@crunchydata.com> wrote:
> > >
> > > This is not intended for PG15.
> > >
> > > Attached are a proof of concept patchset to implement multiple valid
> > > passwords, which have independent expirations, set by a GUC or SQL
> > > using an interval.
> > >
> >
> > <snip>
> >
> > > postgres=# select * from pg_auth_password ;
> > >  roleid |  name   |
> > >            password
> > >                     |          expiration
> > >
--------+---------+-------------------------------------------------------------------------------------------------------------------
> > > --------------------+-------------------------------
> > >      10 | __def__ |
> > >
SCRAM-SHA-256$4096:yGiHIYPwc2az7xj/7TIyTA==$OQL/AEcEY1yOCNbrZEj4zDvNnOLpIqltOW1uQvosLvc=:9VRRppuIkSrwhiBN5ePy8wB1y
> > > zDa/2uX0WUx6gXi93E= |
> > >   16384 | __def__ |
> > >
SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$1Ivp4d+vAWxowpuGEn05KR9lxyGOms3yy85k3D7XpBg=:k8xUjU6xrJG17PMGa/Zya6pAE
> > > /M7pEDaoIFmWvNIEUg= | 2022-03-02 06:52:31.217193-08
> > >   16384 | newone  |
> > >
SCRAM-SHA-256$4096:AAAAAAAAAAAAAAAAAAAAAA==$WK3+41CCGDognSnZrtpHhv00z9LuVUjHR1hWq8T1+iE=:w2C5GuhgiEB7wXqPxYfxBKB+e
> > > hm4h6Oeif1uzpPIFVk= | 2022-03-03 06:47:53.728159-08
> > > (3 rows)
> >
> > There's obviously a salt problem here that I'll need to fix that
> > apparently snuck in at the last rebase, but this brings up one aspect
> > of the patchset I didn't mention in the original email:
> >
>
> Attached are fixed patches rebased against the lastest master.
>
>
> > For the SCRAM protocol to work as is with existing clients the salt
> > for each password must be the same. Right now ALTER USER will find and
> > reuse the salt, but a user passing in a pre-computed SCRAM secret
> > currently has no way to get the salt.
> >
> > for \password (we'll need a new one that takes a password name) I was
> > thinking libpq could hold onto the salt that was used to log in, but
> > for outside computation we'll need some way for the client to request
> > it.
> >
> > None of that is done yet.

Now that the commitfest is over these are rebased on master.

It's unclear if I will be able to continue working on this featureset,
this email address will be inactive after today.

On 4/8/22 10:04, Joshua Brindle wrote:
> It's unclear if I will be able to continue working on this featureset,
> this email address will be inactive after today.

I'm assuming the answer to this was "no". Is there any interest out
there to pick this up for the July CF?

--Jacob

I am planning on picking it up next week; right now picking up steam,
and reviewing a different, smaller patch.

At his behest, I had a conversation with Joshua (OP), and have his
support to pick up and continue working on this patch. I have a some
ideas of my own, on what this patch should do, but since I haven't
fully reviewed the (bulky) patch, I'll reserve my proposals until I
wrap my head around it.

Please expect some activity on this patch towards the end of next week.

BCC: Joshua's new work email.

Best regards,
Gurjeet
http://Gurje.et

On Wed, Jun 29, 2022 at 2:27 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> On Wed, Jun 29, 2022 at 17:22 Jacob Champion <jchampion@timescale.com> wrote:
>>
>> On 4/8/22 10:04, Joshua Brindle wrote:
>> > It's unclear if I will be able to continue working on this featureset,
>> > this email address will be inactive after today.
>>
>> I'm assuming the answer to this was "no". Is there any interest out
>> there to pick this up for the July CF?
>
>
> Short answer to that is yes, I’m interested in continuing this (though certainly would welcome it if there are others
whoare also interested, and may be able to bring someone else to help work on it too but that might be more August /
Septembertime frame). 
>
> Thanks,
>
> Stephen

Greetings,

* Gurjeet Singh (gurjeet@singh.im) wrote:
> I am planning on picking it up next week; right now picking up steam,
> and reviewing a different, smaller patch.

Great!  Glad that others are interested in this.

> At his behest, I had a conversation with Joshua (OP), and have his
> support to pick up and continue working on this patch. I have a some
> ideas of my own, on what this patch should do, but since I haven't
> fully reviewed the (bulky) patch, I'll reserve my proposals until I
> wrap my head around it.

I'd be curious as to your thought as to what the patch should be doing.
Joshua and I had discussed it at some length as he was working on it.

> Please expect some activity on this patch towards the end of next week.

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.

Thanks!

Stephen

On 6/30/22 8:20 PM, Stephen Frost wrote:
> Greetings,
>
> * Gurjeet Singh (gurjeet@singh.im) wrote:
>> I am planning on picking it up next week; right now picking up steam,
>> and reviewing a different, smaller patch.
> Great!  Glad that others are interested in this.
>
>> At his behest, I had a conversation with Joshua (OP), and have his
>> support to pick up and continue working on this patch. I have a some
>> ideas of my own, on what this patch should do, but since I haven't
>> fully reviewed the (bulky) patch, I'll reserve my proposals until I
>> wrap my head around it.
> I'd be curious as to your thought as to what the patch should be doing.
> Joshua and I had discussed it at some length as he was working on it.


Adding myself to the CC list here /waves


I gave Gurjeet a bit of a brain dump on what I had planned (and what 
we'd talked about), though he's free to take it in a different direction 
if he wants.


>> Please expect some activity on this patch towards the end of next week.
> 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.


> Thanks!
>
> Stephen

On Fri, Jul 1, 2022 at 8:13 AM Stephen Frost <sfrost@snowman.net> wrote:
> On Fri, Jul 1, 2022 at 10:51 Brindle, Joshua <joshuqbr@amazon.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
supportfor “md5” pg_hba option was broken for users with SCRAM passwords because we weren’t checking if there was a
SCRAMpw 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’stricky 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
tomd5 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
largelyan 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

On Mon, 2023-09-25 at 00:31 -0700, Gurjeet Singh wrote:

> Please see attached v4 of the patch. The patch takes care of rebase
> to
> the master/17-devel branch, and includes some changes, too.

FWIW I got some failures applying. I didn't investigate much, and
instead I looked at your git branch (7a35619e).

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

You mean before v3 or before v4? Is this currently a problem in v4?

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

Perhaps you can try the MD5 passwords first, and only if they fail,
move on to try scram passwords?

> Comments?

IIUC, for the case of multiple scram passwords, we use the salt to
select the right scram password, and then proceed from there?

I'm not very excited about the idea of naming passwords, or having
passwords with default names. I can't think of anything better right
now, so it might be OK.

> - Add tests
> - Add/update documentation

These are needed to provide better review.


Regards,
    Jeff Davis

I had an idea to simplify this feature/patch and after some validation
in internal discussions, I am posting the new approach here. I'd
appreciate any feedback and comments.

To begin with, the feature we are chasing is to make it convenient for
the users to rollover their passwords. Currently there is no easy way
to rollover passwords without increasing the risk of an application
outage. After a password change, the users/admins have to rush to
change the password in all locations where it is stored. There is a
window of time where if the application password is not changed to the
new one, and the application tries to connect/reconnect for any
reason, the application will fail authentication and lead to an outage.

I personally haven't seen any attempts by any
application/driver/framework to solve this problem in the wild, so
following is just me theorizing how one may solve this problem on the
application side; there may be other ways in which others may solve
this problem. The application may be written in such a way that upon
password authentication failure, it tries again with a second
password. The application's configuration file (or environment
variables) may allow specifying 2 passwords at the same time, and the
application will keep trying these 2 passwords alternatively until it
succeeds or the user restarts it with a new configuration. With such a
logic in place in their application, the users may first change the
configuration of all the instances of the application to hold the new
password along with the old/current working password, and only then
change the password in the database. This way, in the event of an
application instance start/restart either the old password will
succeed, or the new password will.

There may be other ways to solve this problem, but I can't imagine any
of those ways to be convenient and straightforward. At least not as
convenient as it can be if the database itself allowed for storing
both the passwords, and honored both passwords at the same time, while
allowing to associate a separate validity period with each of the
passwords.

The patches posted in this thread so far attempt to add the ability to
allow the user to have an arbitrary number of passwords. I believe
that allowing arbitrary number of passwords is not only unnecessary,
but the need to name passwords, the need to store them in a shared
catalog, etc. may actually create problems in the field. The
users/admins will have to choose names for passwords, which they
didn't have to previously. The need to name them may also lead to
users storing password-hints in the password names (e.g. 'mom''s
birthday', 'ex''s phone number', 'third password'), rendering the
passwords weak.

Moreover, allowing an arbitrarily many number of passwords will
require us to provide additional infrastructure to solve problems like
observability (which passwords are currently in use, and which ones
have been effectively forgotten by applications), or create a nuisance
for admins that can create more problems than it solves.

So I propose that the feature should allow no more than 2 passwords
for a role, each with their own validity periods. This eliminates the
need to name passwords, because at any given time there are no more
than 2 passwords; current one, and old one. This also eliminates the
need for a shared catalog to hold passwords, because with the limit of
2 imposed, we can store the old password and its validity period in
additional columns in the pg_authid table.

The patches so far also add a notion of max validity period of
passwords, which only a superuser can override. I believe this is a
useful feature, but that feature can be dealt with separately,
independent of password rollover feature. So in the newer patches I
will not include the relevant GUC and code.

With the above being said, following is the user interface I can think
of that can allow for various operations that users may need to
perform to rollover their passwords. The 'ADD PASSWORD' and 'ALL
PASSWORD' are additions to the grammar. rololdpassword and
rololdvaliduntil will be new columns in pg_authid that will hold the
old password and its valid-until value.

In essence, we create a stack that can hold 2 passwords. Pushing an
element when it's full will make it forget the bottom element. Popping
the stack makes it forget the top element, and the only remaining
element, if any, becomes the top.

-- Create a user, as usual
CREATE ROLE u1 PASSWORD 'p1' VALID UNTIL '2020/01/01';

-- Add another password that the user can use for authentication. This moves
-- the 'p1' password hash and its valid-until value to rololdpassword and
-- rololdvaliduntil, respectively.
ALTER ROLE u1 ADD PASSWORD 'p2' VALID UNTIL '2021/01/01';

-- Change the password 'p2's (current password's) validity
ALTER ROLE u1 VALID UNTIL '2022/01/01';
-- Note that currently I don't have a proposal for how to change the old
-- password's validity period, without deleting the latest/main password. See
-- PASSWORD NULL command below on how to delete the current password. It's very
-- likely that in a password rollover use case it's unnecessary, even
-- undesirable, to change the old password's validity period.

-- If, for some reason, the user wants to get rid of the latest password added.
-- Remove 'p2' (current password). The old password (p1), will be restored to
-- rolpassword, along with its valid-until value.
ALTER ROLE u1 PASSWORD NULL;
-- This may come as a surprise to some users, because currently they expect the
-- user to completely lose the ability to use passwords for login after this
-- command. To get the old behavior, the user must now use the ALL PASSWORD
-- NULL incantation; see below.
-- Issuing this command one more time will remove even the restored password,
-- hence leaving the user with no passwords.

-- Change the validity of the restored/current password (p1)
ALTER ROLE u1 VALID UNTIL '2022/01/01';

-- Add a new password (p3) without affecting old password's (p1) validity
ALTER ROLE u1 ADD PASSWORD 'p3' VALID UNTIL '2023/01/01';

-- Add a new password 'p4'. This will move 'p3' to rololdpassword, and hence
-- 'p1' will be forgotten completely.
-- After this command, user can use passwords 'p3' (old) and 'p4' (current) to
-- login.
ALTER ROLE u1 ADD PASSWORD 'p4' VALID UNTIL '2024/01/01';

-- Replace 'p4' (current) password with 'p5'. Note that this command is _not_
-- using the ADD keyword, hence 'p4' is _not_ moved to rololdpassword column.
-- After this command, user can use passwords 'p3' (old) and 'p5'
-- (current) to login.
ALTER ROLE u1 PASSWORD 'p5' VALID UNTIL '2025/01/01';

-- Using the old password to login will produce a warning, hopefully nudging
-- the user to start using the new password.
export PGPASSWORD=p3
psql -U u1
...
WARNING: Used old password to login

export PGPASSWORD=p5
psql -U u1
...
=> (no warning)

-- Remove all passwords from the role. Even old password, if any, is removed.
ALTER ROLE u1 ALL PASSWORD NULL;

In normal use, the users can simply keep ADDing new passwords, and the
database will promptly remember only one old password, and keep
forgetting any passwords older than that. But on the off chance
that someone needs to forget the latest password they added, and
restore the old password to be the "current" password, they can use
the PASSWORD NULL incantation. Note that this will result in
rol*old*password being set to NULL, because our password memory stack
cannot hold more than 2 elements.

Since this feature is targeted towards password rollovers, it's a legitimate
question to ask if we should enforce that the new password being added has a
valid-until greater than the valid-until of the existing/old password. I don't
think we should enforce this, at least not in this patch, because the
user/admin may have a use case where they want a short-lived new password that
they intend/want to change very soon; I'm thinking of cases where passwords are
being rolled over while they are also moving from older clients/libraries that
don't yet support scram-sha-256; keep using md5 and add passwords to honor
password rollover policy, but then as soon as all clients have been updated and
have the ability to use scram-sha-256, rollover the password again to utilize
the better mechanism.

I realize that allowing for a maximum of 2 passwords goes against the
zero-one-infinity rule [1], but I think in the case of password
rollovers it's perfectly acceptable to limit the number of active
passwords to just 2. If there are use cases, either related to password
rollovers, or in its vicinity, that can be better addressed by
allowing an arbitrarily many passwords, I would love to learn about
them and change this design to accommodate for those use cases, or
perhaps revert to pursuing the multiple-passwords feature.

[1]: https://en.wikipedia.org/wiki/Zero_one_infinity_rule

Best regards,
Gurjeet
http://Gurje.et

On Wed, Oct 04, 2023 at 10:41:15PM -0700, Gurjeet Singh wrote:
> The patches posted in this thread so far attempt to add the ability to
> allow the user to have an arbitrary number of passwords. I believe
> that allowing arbitrary number of passwords is not only unnecessary,
> but the need to name passwords, the need to store them in a shared
> catalog, etc. may actually create problems in the field. The
> users/admins will have to choose names for passwords, which they
> didn't have to previously. The need to name them may also lead to
> users storing password-hints in the password names (e.g. 'mom''s
> birthday', 'ex''s phone number', 'third password'), rendering the
> passwords weak.
> 
> Moreover, allowing an arbitrarily many number of passwords will
> require us to provide additional infrastructure to solve problems like
> observability (which passwords are currently in use, and which ones
> have been effectively forgotten by applications), or create a nuisance
> for admins that can create more problems than it solves.

IMHO neither of these problems seems insurmountable.  Besides advising
against using hints as names, we could also automatically generate safe
names, or even disallow user-provided names entirely.  And adding
observability for passwords seems worthwhile anyway.

> So I propose that the feature should allow no more than 2 passwords
> for a role, each with their own validity periods. This eliminates the
> need to name passwords, because at any given time there are no more
> than 2 passwords; current one, and old one. This also eliminates the
> need for a shared catalog to hold passwords, because with the limit of
> 2 imposed, we can store the old password and its validity period in
> additional columns in the pg_authid table.

Another approach could be to allow an abritrary number of passwords but to
also allow administrators to limit how many passwords can be associated to
each role.  That way, we needn't restrict this feature to 2 passwords for
everyone.  Perhaps 2 should be the default, but in any case, IMO we
shouldn't design to only support 2.

> In essence, we create a stack that can hold 2 passwords. Pushing an
> element when it's full will make it forget the bottom element. Popping
> the stack makes it forget the top element, and the only remaining
> element, if any, becomes the top.

I think this would be mighty confusing to users since it's not clear that
adding a password will potentially invalidate a current password (which
might be actively in use), but only if there are already 2 in the stack.  I
worry that such a desіgn might be too closely tailored to the
implementation details.  If we proceed with this design, perhaps we should
consider ERROR-ing if a user tries to add a third password.

> -- If, for some reason, the user wants to get rid of the latest password added.
> -- Remove 'p2' (current password). The old password (p1), will be restored to
> -- rolpassword, along with its valid-until value.
> ALTER ROLE u1 PASSWORD NULL;
> -- This may come as a surprise to some users, because currently they expect the
> -- user to completely lose the ability to use passwords for login after this
> -- command. To get the old behavior, the user must now use the ALL PASSWORD
> -- NULL incantation; see below.
> -- Issuing this command one more time will remove even the restored password,
> -- hence leaving the user with no passwords.

Is it possible to remove the oldest password added without removing the
latest password added?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, 2023-10-05 at 14:04 -0500, Nathan Bossart wrote:
> IMHO neither of these problems seems insurmountable.  Besides
> advising
> against using hints as names, we could also automatically generate
> safe
> names, or even disallow user-provided names entirely.  

I'd like to see what this looks like as a user-interface. Using a name
seems weird because of the reasons Gurjeet mentioned.

Using a number seems weird to me because either:

 (a) if the number is always increasing you'd have to look to find the
number of the new slot to add and the old slot to remove; or
 (b) if switched between two numbers (say 0 and 1), it would be error
prone because you'd have to remember which is the old one that can be
safely replaced

Maybe a password is best described by its validity period rather than a
name? But what about passwords that don't expire?

> And adding
> observability for passwords seems worthwhile anyway.

That might be useful just to know whether a user's password is even
being used -- in case the admin makes a mistake and some other auth
method is being used. Also it would help to know when a password can
safely be removed.
>

>   That way, we needn't restrict this feature to 2 passwords for
> everyone.  Perhaps 2 should be the default, but in any case, IMO we
> shouldn't design to only support 2.

Are there use cases for lots of passwords, or is it just a matter of
not introducing an artificial limitation?

Would it ever make sense to have a role that has two permanent
passwords, or would that be an abuse of this feature? Any use cases I
can think of would be better solved with multiple user that are part of
the same group.

>
> I think this would be mighty confusing to users since it's not clear
> that
> adding a password will potentially invalidate a current password
> (which
> might be actively in use), but only if there are already 2 in the
> stack.  I
> worry that such a desіgn might be too closely tailored to the
> implementation details.  If we proceed with this design, perhaps we
> should
> consider ERROR-ing if a user tries to add a third password.

I agree that the proposed language is confusing, especially because ADD
causes a password to be added and another one to be removed. But
perhaps there are some non-confusing ways to expose a similar idea.

The thing I like about Gurjeet's proposal is that it's well-targeted at
a specific use case rather than trying to be too general. That makes it
a little easier to avoid certain problems like having a process that
adds passwords and never removes the old ones (leading to weird
problems like 47000 passwords for one user).

But it also feels strange to be limited to two -- perhaps the password
rotation schedule or policy just doesn't work with a limit of two, or
perhaps that introduces new kinds of mistakes.

Another idea: what if we introduce the notion of deprecating a
password? To remove a password, it would have to be deprecated first,
and maybe that would cause a LOG or WARNING message to be emitted when
used, or show up differently in some system view. And perhaps you could
have at most one non-deprecated password. That would give a workflow
something like (I'm not proposing these exact keywords):

  CREATE USER foo PASSWORD 'secret1';
  ALTER USER foo DEPRECATE PASSWORD; -- 'secret1' still works
  ALTER USER foo PASSWORD 'secret2'; -- 'secret1' or 'secret2' works
  ... fix some applications
  SET log_deprecated_password_use = WARNING;

  ...
  WARNING: deprecated password used for user 'foo'
  ... fix some applications you forgot about
  ... warnings quiet down
  ALTER USER foo DROP DEPRECATED PASSWORD; -- only 'secret2' works

If the user wants to un-deprecate a password (before they drop it, of
course), they can do something like:

  ALTER USER foo PASSWORD NULL; -- 'secret2' removed
  ALTER USER foo UNDEPRECATE PASSWORD; -- 'secret1' restored

if we allow multiple deprecated passwords, we'd still have to come up
with some way to address them (names, numbers, validity period,
something). But by isolating the problem to deprecated passwords only,
it feels like the system is still being restored to a clean state with
at most one single current password. The awkwardness is contained to
old passwords which will hopefully go away soon anyway and not
represent permanent clutter.

Regards,
    Jeff Davis

On Thu, Oct 5, 2023 at 12:04 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Wed, Oct 04, 2023 at 10:41:15PM -0700, Gurjeet Singh wrote:
> > The patches posted in this thread so far attempt to add the ability to
> > allow the user to have an arbitrary number of passwords. I believe
> > that allowing arbitrary number of passwords is not only unnecessary,
> > but the need to name passwords, the need to store them in a shared
> > catalog, etc. may actually create problems in the field. The
> > users/admins will have to choose names for passwords, which they
> > didn't have to previously. The need to name them may also lead to
> > users storing password-hints in the password names (e.g. 'mom''s
> > birthday', 'ex''s phone number', 'third password'), rendering the
> > passwords weak.
> >
> > Moreover, allowing an arbitrarily many number of passwords will
> > require us to provide additional infrastructure to solve problems like
> > observability (which passwords are currently in use, and which ones
> > have been effectively forgotten by applications), or create a nuisance
> > for admins that can create more problems than it solves.
>
> IMHO neither of these problems seems insurmountable.

Agreed.

> Besides advising
> against using hints as names, we could also automatically generate safe
> names, or even disallow user-provided names entirely.

Somehow naming passwords doesn't feel palatable to me.

> And adding
> observability for passwords seems worthwhile anyway.

Agreed.

> > So I propose that the feature should allow no more than 2 passwords
> > for a role, each with their own validity periods. This eliminates the
> > need to name passwords, because at any given time there are no more
> > than 2 passwords; current one, and old one. This also eliminates the
> > need for a shared catalog to hold passwords, because with the limit of
> > 2 imposed, we can store the old password and its validity period in
> > additional columns in the pg_authid table.
>
> Another approach could be to allow an abritrary number of passwords but to
> also allow administrators to limit how many passwords can be associated to
> each role.  That way, we needn't restrict this feature to 2 passwords for
> everyone.  Perhaps 2 should be the default, but in any case, IMO we
> shouldn't design to only support 2.

I don't see a real use case to support more than 2 passwords. Allowing
an arbitrary number of passwords might look good and clean from
aesthetics and documentation perspective (no artificially enforced
limits, as in the zero-one-infinity rule), but in absence of real use
cases for that many passwords, I'm afraid we might end up with a
feature that creates more and worse problems for the users than it
solves.

> > In essence, we create a stack that can hold 2 passwords. Pushing an
> > element when it's full will make it forget the bottom element. Popping
> > the stack makes it forget the top element, and the only remaining
> > element, if any, becomes the top.
>
> I think this would be mighty confusing to users since it's not clear that
> adding a password will potentially invalidate a current password (which
> might be actively in use), but only if there are already 2 in the stack.

Fair point. We can aid the user by emitting a NOTICE (or a WARNING)
message whenever an old password is removed from the system because of
addition of a new password.

> I
> worry that such a desіgn might be too closely tailored to the
> implementation details.  If we proceed with this design, perhaps we should
> consider ERROR-ing if a user tries to add a third password.

I did not have a stack in mind when developing the use case and the
grammar, so implementation details did not drive this design. This new
design was more of a response to the manageability nightmare that I
could see the old approach may lead to. When writing the email I
thought mentioning the stack analogy may make it easier to develop a
mental model. I certainly won't suggest using it in the docs for
explanation :-)

> > -- If, for some reason, the user wants to get rid of the latest password added.
> > -- Remove 'p2' (current password). The old password (p1), will be restored to
> > -- rolpassword, along with its valid-until value.
> > ALTER ROLE u1 PASSWORD NULL;
> > -- This may come as a surprise to some users, because currently they expect the
> > -- user to completely lose the ability to use passwords for login after this
> > -- command. To get the old behavior, the user must now use the ALL PASSWORD
> > -- NULL incantation; see below.
> > -- Issuing this command one more time will remove even the restored password,
> > -- hence leaving the user with no passwords.
>
> Is it possible to remove the oldest password added without removing the
> latest password added?

In the patch I have so far, ALTER ROLE u1 ADD PASSWORD '' (empty
string) will drop the old password (what you asked for), and move the
current password to rololdpassword (which is not exactly what you
asked for :-). Hence the oldest password will be forgotten, and the
current password will continue to work;

Perhaps an explicit syntax like ALTER ROLE u1 DROP OLD PASSWORD can be
used for this.

Best regards,
Gurjeet
http://Gurje.et

On Thu, Oct 5, 2023 at 1:09 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> >
> > I think this would be mighty confusing to users since it's not clear
> > that
> > adding a password will potentially invalidate a current password
> > (which
> > might be actively in use), but only if there are already 2 in the
> > stack.  I
> > worry that such a desіgn might be too closely tailored to the
> > implementation details.  If we proceed with this design, perhaps we
> > should
> > consider ERROR-ing if a user tries to add a third password.
>
> I agree that the proposed language is confusing, especially because ADD
> causes a password to be added and another one to be removed. But
> perhaps there are some non-confusing ways to expose a similar idea.

How about a language like the following (I haven't tried if this will
work in the grammar we have):

CREATE ROLE u1 PASSWORD 'p1';

ALTER ROLE u1ADD NEW PASSWORD 'p2';

ALTER ROLE u1 ADD NEW PASSOWRD 'p3';
ERROR: Cannot have more than 2 passwords at the same time.

ALTER ROLE u1 DROP OLD PASSWORD;

ALTER ROLE u1 ADD NEW PASSOWRD 'p3';
-- succeeds; forgets password 'p1'; p2 and p3 can be used to login

ALTER ROLE u1 DROP NEW PASSWORD;
-- forgets password 'p3'. Only 'p2' can be used to login

ALTER ROLE u1 ADD NEW PASSOWRD 'p4';
-- succeeds; 'p2' and 'p4' can be used to login

-- Set the valid-until of 'new' (p4) password
ALTER ROLE u1 VALID UNTIL '2024/01/01';

-- If we need the ability to change valid-until of both old and new,
we may allow something like the following.
ALTER ROLE u1 [_NEW_ | OLD] VALID UNTIL '2024/01/01';

This way there's a notion of a 'new' and 'old' passwords. User cannot
add a third password without explicitly dropping one of existing
passwords (either old or new). At any time the user can choose to drop
the old or the new password. Adding a new password will mark the
current password as 'old'; if there's only old password (because 'new'
was dropped) the 'old' password will remain intact and the new one
will be placed in 'current'/new spot.

So in normal course of operation, even for automated jobs, the
expected flow to roll over the passwords would be:

ALTER USER u1 DROP OLD PASSWORD;
-- success if there is an old password; otherwise NOTICE: no old password
ALTER USER u1 ADD NEW PASSWORD 'new-secret';

> The thing I like about Gurjeet's proposal is that it's well-targeted at
> a specific use case rather than trying to be too general. That makes it
> a little easier to avoid certain problems like having a process that
> adds passwords and never removes the old ones (leading to weird
> problems like 47000 passwords for one user).
>
> But it also feels strange to be limited to two -- perhaps the password
> rotation schedule or policy just doesn't work with a limit of two, or
> perhaps that introduces new kinds of mistakes.
>
> Another idea: what if we introduce the notion of deprecating a
> password?

I'll have to think more about it, but perhaps my above proposal
addresses the use case you describe.

> if we allow multiple deprecated passwords, we'd still have to come up
> with some way to address them (names, numbers, validity period,
> something). But by isolating the problem to deprecated passwords only,
> it feels like the system is still being restored to a clean state with
> at most one single current password. The awkwardness is contained to
> old passwords which will hopefully go away soon anyway and not
> represent permanent clutter.

+1

Best regards,
Gurjeet
http://Gurje.et

On Thu, Oct 05, 2023 at 01:09:36PM -0700, Jeff Davis wrote:
> On Thu, 2023-10-05 at 14:04 -0500, Nathan Bossart wrote:
>> That way, we needn't restrict this feature to 2 passwords for
>> everyone.  Perhaps 2 should be the default, but in any case, IMO we
>> shouldn't design to only support 2.
> 
> Are there use cases for lots of passwords, or is it just a matter of
> not introducing an artificial limitation?

I guess it's more of the latter.  Perhaps one potential use case would be
short-lived credentials that are created on demand.  Such a password might
only be valid for something like 15 minutes, and many users might have the
ability to request a password for the database role.  I don't know whether
there is a ton of demand for such a use case, and it might already be
solvable by just creating separate roles.  In any case, if there's general
agreement that we only want to target the rotation use case, that's fine by
me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, 2023-10-06 at 14:26 -0500, Nathan Bossart wrote:
> I guess it's more of the latter.  Perhaps one potential use case
> would be
> short-lived credentials that are created on demand.  Such a password
> might
> only be valid for something like 15 minutes, and many users might
> have the
> ability to request a password for the database role.  I don't know
> whether
> there is a ton of demand for such a use case, and it might already be
> solvable by just creating separate roles.  In any case, if there's
> general
> agreement that we only want to target the rotation use case, that's
> fine by
> me.

The basic problem, as I see it, is: how do we keep users from
accidentally dropping the wrong password? Generated unique names or
numbers don't solve that problem. Auto-incrementing or a created-at
timestamp solves it in the sense that you can at least look at a system
view and see if there's a newer one, but it's a little awkward. A
validity period is a good fit if all passwords have a validity period
and we don't change it, but gets awkward otherwise.

I'm also worried about two kinds of clutter:

* old passwords not being garbage-collected
* the identifier of the current password always changing (perhaps fine
if it'a a "created at" ID?)

Regards,
    Jeff Davis

On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote:

> This way there's a notion of a 'new' and 'old' passwords.

IIUC, you are proposing that there are exactly two slots, NEW and OLD.
When adding a password, OLD must be unset and it moves NEW to OLD, and
adds the new password in NEW. DROP only works on OLD. Is that right?

It's close to the idea of deprecation, except that adding a new
password implicitly deprecates the existing one. I'm not sure about
that -- it could be confusing.

We could also try using a verb like "expire" that could be coupled with
a date, and that way all old passwords would always have some validity
period. That might make it a bit easier to manage if we do need more
than two passwords.

Regards,
    Jeff Davis

On Fri, Oct  6, 2023 at 01:20:03PM -0700, Jeff Davis wrote:
> The basic problem, as I see it, is: how do we keep users from
> accidentally dropping the wrong password? Generated unique names or

I thought we could auto-remove old password if the valid-until date is
in the past.  You would need a separate ALTER command to sets its date
in the past without that.  Also, defining a new password could require
setting the expiration date of the old password to make future additions
easier.

For pg_authid, I was thinking of columns:

    ADD    rolpassword_old
    ADD    rolvaliduntil_old
    EXISTS    rolpassword
    EXISTS    rolvaliduntil

I did blog about the password rotation problem and suggested
certificates:

    https://momjian.us/main/blogs/pgblog/2020.html#July_17_2020

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Fri, Oct 6, 2023 at 1:46 PM Bruce Momjian <bruce@momjian.us> wrote:
>
> On Fri, Oct  6, 2023 at 01:20:03PM -0700, Jeff Davis wrote:
> > The basic problem, as I see it, is: how do we keep users from
> > accidentally dropping the wrong password? Generated unique names or
>
> I thought we could auto-remove old password if the valid-until date is
> in the past.

Autoremoving expired passwords will surprise users, and not in a good
way. Making a password, even an expired one, disappear from the system
will lead to astonishment. Among uses of an expired password are cases
of it acting like a tombstone, and the case where the user may want to
extend the validity of a password, instead of having to create a new
one and change application configuration(s) to specify the new
password.

Best regards,
Gurjeet
http://Gurje.et

On Sun, Oct  8, 2023 at 10:24:42AM -0700, Gurjeet Singh wrote:
> On Fri, Oct 6, 2023 at 1:46 PM Bruce Momjian <bruce@momjian.us> wrote:
> >
> > On Fri, Oct  6, 2023 at 01:20:03PM -0700, Jeff Davis wrote:
> > > The basic problem, as I see it, is: how do we keep users from
> > > accidentally dropping the wrong password? Generated unique names or
> >
> > I thought we could auto-remove old password if the valid-until date is
> > in the past.
> 
> Autoremoving expired passwords will surprise users, and not in a good
> way. Making a password, even an expired one, disappear from the system
> will lead to astonishment. Among uses of an expired password are cases
> of it acting like a tombstone, and the case where the user may want to
> extend the validity of a password, instead of having to create a new
> one and change application configuration(s) to specify the new
> password.

I was speaking of autoremoving in cases where we are creating a new one,
and taking the previous new one and making it the old one, if that was
not clear.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Sun, Oct 8, 2023 at 10:29 AM Bruce Momjian <bruce@momjian.us> wrote:
>
> I was speaking of autoremoving in cases where we are creating a new one,
> and taking the previous new one and making it the old one, if that was
> not clear.

Yes, I think I understood it differently. I understood it to mean that
this behaviour would apply to all passwords, those created by existing
commands, as well as to those created by new commands for rollover use
case. Whereas you meant this autoremove behaviour to apply only to
those passwords created by/for rollover related commands. I hope I've
understood your proposal correctly this time around :-)

I believe the passwords created by rollover feature should
behave by the same rules as the rules for passwords created by
existing CREATE/ALTER ROLE commands. If we implement the behaviour to
delete expired passwords, then I believe that behaviour should apply
to all passwords, irrespective of which command/feature was used to
create a password.


Best regards,
Gurjeet
http://Gurje.et

On Sun, Oct  8, 2023 at 10:50:15AM -0700, Gurjeet Singh wrote:
> On Sun, Oct 8, 2023 at 10:29 AM Bruce Momjian <bruce@momjian.us> wrote:
> >
> > I was speaking of autoremoving in cases where we are creating a new one,
> > and taking the previous new one and making it the old one, if that was
> > not clear.
> 
> Yes, I think I understood it differently. I understood it to mean that
> this behaviour would apply to all passwords, those created by existing
> commands, as well as to those created by new commands for rollover use
> case. Whereas you meant this autoremove behaviour to apply only to
> those passwords created by/for rollover related commands. I hope I've
> understood your proposal correctly this time around :-)

Yes, it is only during the addition of a new password when the previous
new password becomes the new old password.  The previous old password
would need to have an rolvaliduntil in the past.

> I believe the passwords created by rollover feature should
> behave by the same rules as the rules for passwords created by
> existing CREATE/ALTER ROLE commands. If we implement the behaviour to
> delete expired passwords, then I believe that behaviour should apply
> to all passwords, irrespective of which command/feature was used to
> create a password.

This would only apply when we are moving the previous new password to
old and the old one is removed.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote:
>
> > This way there's a notion of a 'new' and 'old' passwords.
>
> IIUC, you are proposing that there are exactly two slots, NEW and OLD.
> When adding a password, OLD must be unset and it moves NEW to OLD, and
> adds the new password in NEW. DROP only works on OLD. Is that right?

Yes, that's what I was proposing. But thinking a bit more about it,
the _implicit_ shuffling of labels 'new' and 'old' doesn't feel right
to me. The password that used to be referred to as 'new' now
automatically gets labeled 'old'.

> It's close to the idea of deprecation, except that adding a new
> password implicitly deprecates the existing one. I'm not sure about
> that -- it could be confusing.

+1

> We could also try using a verb like "expire" that could be coupled with
> a date, and that way all old passwords would always have some validity
> period.

Forcing the users to pick an expiry date for a password they intend to
rollover, when an expiry date did not exist before for that password,
feels like adding more burden to their password rollover decision
making. The dates and rules of password rollover may be a part of a
system external to their database, (wiki, docs, calendar, etc.) which
now they will be forced to translate into a timestamp to specify in
the rollover commands.

I believe we should fix the _names_ of the slots the 2 passwords are
stored in, and provide commands that manipulate those slots by
respective names; the commands should not implicitly move the
passwords between the slots. Additionally, we may provide functions
that provide observability info for these slots. I propose the slot
names FIRST and SECOND (I picked these because these keywords/tokens
already exist in the grammar, but not yet sure if the grammar rules
would allow their use; feel free to propose better names). FIRST
refers to the the existing slot, namely rolpassword. SECOND refers to
the new slot we'd add, that is, a pgauthid column named
rolsecondpassword. The existing commands, or when neither FIRST nor
SECOND are specified, the commands apply to the existing slot, a.k.a.
FIRST.

The user interface might look like the following:

-- Create a user, as usual
CREATE ROLE u1 PASSWORD 'p1' VALID UNTIL '2020/01/01';
-- This automatically occupies the 'first' slot

-- Add another password that the user can use for authentication.
ALTER ROLE u1 ADD SECOND PASSWORD 'p2' VALID UNTIL '2021/01/01';

-- Change both the passwords' validity independently; this solves the
-- problem with the previous '2-element stack' approach, where we
-- could not address the password at the bottom of the stack.
ALTER ROLE u1 SECOND PASSWORD VALID UNTIL '2022/01/01';

ALTER ROLE u1 [ [ FIRST ] PASSWORD ] VALID UNTIL '2022/01/01';

-- If, for some reason, the user wants to get rid of the latest password added.
ALTER ROLE u1 DROP SECOND PASSWORD;

-- Add a new password (p3) in 'second' slot
ALTER ROLE u1 ADD SECOND PASSWORD 'p3' VALID UNTIL '2023/01/01';

-- Attempting to add a password while the respective slot is occupied
-- results in error
ALTER ROLE u1 ADD [ [ FIRST ] PASSWORD ] 'p4' VALID UNTIL '2024/01/01';
ERROR: first password already exists

ALTER ROLE u1 ADD SECOND PASSWORD 'p4' VALID UNTIL '2024/01/01';
ERROR: second password already exists

-- Users can use this function to check whether a password slot is occupied
=> select password_exists('u1', 'first');
password_exists
-----
t

=> select password_exists('u1', 'second');
password_exists
-----
t

-- Remove all passwords from the role. Both, 'first' and 'second',
passwords are removed.
ALTER ROLE u1 DROP ALL PASSWORD;

Best regards,
Gurjeet
http://Gurje.et

On Sun, Oct 8, 2023 at 1:01 PM Gurjeet Singh <gurjeet@singh.im> wrote:
>
> On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote:
> >
> > > This way there's a notion of a 'new' and 'old' passwords.
> >
> > IIUC, you are proposing that there are exactly two slots, NEW and OLD.
> > When adding a password, OLD must be unset and it moves NEW to OLD, and
> > adds the new password in NEW. DROP only works on OLD. Is that right?
>
> Yes, that's what I was proposing. But thinking a bit more about it,
> the _implicit_ shuffling of labels 'new' and 'old' doesn't feel right
> to me. The password that used to be referred to as 'new' now
> automatically gets labeled 'old'.
>
> > It's close to the idea of deprecation, except that adding a new
> > password implicitly deprecates the existing one. I'm not sure about
> > that -- it could be confusing.
>
> +1

> I believe we should fix the _names_ of the slots the 2 passwords are
> stored in, and provide commands that manipulate those slots by
> respective names; the commands should not implicitly move the
> passwords between the slots. Additionally, we may provide functions
> that provide observability info for these slots. I propose the slot
> names FIRST and SECOND (I picked these because these keywords/tokens
> already exist in the grammar, but not yet sure if the grammar rules
> would allow their use; feel free to propose better names). FIRST
> refers to the the existing slot, namely rolpassword. SECOND refers to
> the new slot we'd add, that is, a pgauthid column named
> rolsecondpassword. The existing commands, or when neither FIRST nor
> SECOND are specified, the commands apply to the existing slot, a.k.a.
> FIRST.

Please see attached the patch that implements this proposal. The patch
is named password_rollover_v3.diff, breaking from the name
'multiple_passwords...', since this patch limits itself to address the
password-rollover feature.

The multiple_password* series of patches had removed a critical
functionality, which I believe is crucial from security perspective.
When a user does not exist, or has no passwords, or has passwords that
have expired, we must pretend to perform authentication (network
packet exchanges) as normally as possible, so that the absence of
user, or lack of (or expiration of) passwords is not revealed to an
attacker. I have restored the original behaviour in the
CheckPWChallengeAuth() function; see commit aba99df407 [2].

I looked for any existing keywords that may better fit the purpose of
naming the slots, better than FIRST and SECOND, but I could not find
any. Instead of DROP to remove the passwords, I tried DELETE and the
grammar/bison did not complain about it; so DELETE is an option, too,
but I feel DROP FIRST/SECOND/ALL PASSWORD is a better companion of ADD
FIRST/SECOND PASSWORD syntax, in the same vein as ADD/DROP COLUMN.

The doc changes are still missing, but the regression tests and the
comments therein should provide a good idea of the user interface of
this new feature. Documenting this behaviour in a succinct manner
feels difficult; so ideas welcome for how to inform the reader that
now a role is accompanied by two slots to store the passwords, and
that the old commands operate on the first slot, and to operate on the
second password slot one must use the new syntax. I guess it would be
best to start the relevant section with "To support gradual password
rollovers, Postgres provides the ability to store 2 active passwords
at the same time. The passwords are referred to as FIRST and SECOND
password. Each of these passwords can be changed independently, and
each of these can have an associated expiration time, if necessary."

Since these new commands are only available to ALTER ROLE (and not to
CREATE ROLE), the corresponding command doc page also needs to be
updated.

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

The patch progress can be followed on the Git branch
password_rollover_v3 [1]. This branch begins uses
multiple_passwords_v4 as starting point, and removes unnecessary code
(commit 326f60225f [3])

The v1 (and tombstone of v2) patches of password_rollover never
finished as the consensus changed while they were in progress, but
they exist as sibling branches of the v3 branch.

[1]: https://github.com/gurjeet/postgres/commits/password_rollover_v3
[2]: https://github.com/gurjeet/postgres/commit/aba99df407a523357db2813f0eea0b45dbeb6006
[3]: https://github.com/gurjeet/postgres/commit/326f60225f0e660338fc9c276c8728dc10db435b


Best regards,
Gurjeet
http://Gurje.et

On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh <gurjeet@singh.im> wrote:
>
> Next steps:
> - Break the patch into a series of smaller patches.

Please see attached the same v3 patch, but now split into 3 separate
patches. Each patch in the series depends on the previous patch to
have been applied. I have made sure that each patch passes `make
check` individually.

First patch adds the two new columns, rolsecondpassword and
rolsecondvaliduntil to the pg_authid shared catalog. This patch also
updates the corresponding pg_authid.dat file to set these values to
null for the rows populated during bootstrap. Finally, it adds code to
CreateRole() to set these columns' values to NULL for a role being
created.

The second patch updates the password extraction, verification
functions as well as authentication functions to honor the second
password, if any. There is more detailed description in the commit
message/body of the patch.

The third patch adds the SQL support to the ALTER ROLE command which
allows manipulation of both, the rolpassword and rolsecondpassword,
columns and their respective expiration timestamps,
rol[second]validuntil. This patch also adds regression tests for the
new SQL command, demonstrating the use of the new grammar.

v3-0001-Add-new-columns-to-pg_authid.patch
v3-0002-Update-password-verification-infrastructure-to-ha.patch
v3-0003-Added-SQL-support-for-ALTER-ROLE-to-manage-two-pa.patch



Best regards,
Gurjeet
http://Gurje.et

> On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh <gurjeet@singh.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

Greetings,

* Nathan Bossart (nathandbossart@gmail.com) wrote:
> On Wed, Oct 04, 2023 at 10:41:15PM -0700, Gurjeet Singh wrote:
> > The patches posted in this thread so far attempt to add the ability to
> > allow the user to have an arbitrary number of passwords. I believe
> > that allowing arbitrary number of passwords is not only unnecessary,
> > but the need to name passwords, the need to store them in a shared
> > catalog, etc. may actually create problems in the field. The
> > users/admins will have to choose names for passwords, which they
> > didn't have to previously. The need to name them may also lead to
> > users storing password-hints in the password names (e.g. 'mom''s
> > birthday', 'ex''s phone number', 'third password'), rendering the
> > passwords weak.
> >
> > Moreover, allowing an arbitrarily many number of passwords will
> > require us to provide additional infrastructure to solve problems like
> > observability (which passwords are currently in use, and which ones
> > have been effectively forgotten by applications), or create a nuisance
> > for admins that can create more problems than it solves.
>
> IMHO neither of these problems seems insurmountable.  Besides advising
> against using hints as names, we could also automatically generate safe
> names, or even disallow user-provided names entirely.  And adding
> observability for passwords seems worthwhile anyway.

Agreed, particularly on adding observability for password use.
Regardless of what we do, I feel pretty strongly that we need that.
That said, having this handled in a separate catalog feels like a just
generally better idea than shoving it all into pg_authid as we extend
things to include information like "last used date", "last used source
IP", etc.

Providing this observability purely through logging strikes me as a
terrible idea.

I don't find the concern about names as 'hints' to be at all compelling.
Having a way to avoid having names may have some value, but only if we
can come up with something reasonable.

> > So I propose that the feature should allow no more than 2 passwords
> > for a role, each with their own validity periods. This eliminates the
> > need to name passwords, because at any given time there are no more
> > than 2 passwords; current one, and old one. This also eliminates the
> > need for a shared catalog to hold passwords, because with the limit of
> > 2 imposed, we can store the old password and its validity period in
> > additional columns in the pg_authid table.
>
> Another approach could be to allow an abritrary number of passwords but to
> also allow administrators to limit how many passwords can be associated to
> each role.  That way, we needn't restrict this feature to 2 passwords for
> everyone.  Perhaps 2 should be the default, but in any case, IMO we
> shouldn't design to only support 2.

Agreed that it's a bad idea to design to support 2 and only 2.  If
nothing else, there's the very simple case that the user needs to do
another password rotation ... and they look and discover that the old
password is still being used and that if they took it away, things would
break, but they need time to run down which system is still using it
while still needing to perform the migration for the other systems that
are correctly being updated- boom, need 3 for that case.  There's other
use-cases that could be interesting though- presumably we'd log which
password is used to authenticate and then users could have a fleet of
web servers which each have their own password but log into the same PG
user and they could happily rotate the passwords independently for all
of those systems.

I don't propose this as some use-case just for the purpose of argument-
not sharing passwords across a bunch of systems is absolutely a good
stance when it comes to security, and due to the way permissions and
roles work in PG, being able to have both distinct passwords with
explicitly logged indication of which system used what password to log
in while not having to deal with possibly permission differences due to
using actually independent roles is valuable.  That is, each server
using a separate role to log in could lead to some servers having access
to something or other while others don't pretty easily- if they're all
logging in with the same role and just a different password, that's not
going to happen.

* Jeff Davis (pgsql@j-davis.com) wrote:
> Using a number seems weird to me because either:
>
>  (a) if the number is always increasing you'd have to look to find the
> number of the new slot to add and the old slot to remove; or
>  (b) if switched between two numbers (say 0 and 1), it would be error
> prone because you'd have to remember which is the old one that can be
> safely replaced

Yeah, a number doesn't strike me as very good either.

> Maybe a password is best described by its validity period rather than a
> name? But what about passwords that don't expire?

The validity idea is interesting but falls down when you want multiple
passwords that have the same validity period (or which all don't have
any expiration).

Giving users the option of not having to specify a name and letting the
system come up with one (similar to what we do for indexes and such)
could work out pretty decently, imv.  I'd have that be optional though-
if the user wants to specify a name, then they should be allowed to do
so.

> > And adding
> > observability for passwords seems worthwhile anyway.
>
> That might be useful just to know whether a user's password is even
> being used -- in case the admin makes a mistake and some other auth
> method is being used. Also it would help to know when a password can
> safely be removed.

Yup, +100 on this.

> Another idea: what if we introduce the notion of deprecating a
> password? To remove a password, it would have to be deprecated first,
> and maybe that would cause a LOG or WARNING message to be emitted when
> used, or show up differently in some system view. And perhaps you could
> have at most one non-deprecated password. That would give a workflow
> something like (I'm not proposing these exact keywords):

I don't see logs or warnings as being at all useful for this kind of
thing.  Making it available through a view would work- but I don't think
we need to go into the deprecation language ourselves as part of the
grammer and instead should let users write their own queries against the
views we provide to see what passwords are being used and what aren't.

* Nathan Bossart (nathandbossart@gmail.com) wrote:
> On Thu, Oct 05, 2023 at 01:09:36PM -0700, Jeff Davis wrote:
> > On Thu, 2023-10-05 at 14:04 -0500, Nathan Bossart wrote:
> >> That way, we needn't restrict this feature to 2 passwords for
> >> everyone.  Perhaps 2 should be the default, but in any case, IMO we
> >> shouldn't design to only support 2.
> >
> > Are there use cases for lots of passwords, or is it just a matter of
> > not introducing an artificial limitation?
>
> I guess it's more of the latter.  Perhaps one potential use case would be
> short-lived credentials that are created on demand.  Such a password might
> only be valid for something like 15 minutes, and many users might have the
> ability to request a password for the database role.  I don't know whether
> there is a ton of demand for such a use case, and it might already be
> solvable by just creating separate roles.  In any case, if there's general
> agreement that we only want to target the rotation use case, that's fine by
> me.

Agreed, this seems like another good example of why we shouldn't design
this for just 2.

* Jeff Davis (pgsql@j-davis.com) wrote:
> The basic problem, as I see it, is: how do we keep users from
> accidentally dropping the wrong password? Generated unique names or
> numbers don't solve that problem. Auto-incrementing or a created-at
> timestamp solves it in the sense that you can at least look at a system
> view and see if there's a newer one, but it's a little awkward. A
> validity period is a good fit if all passwords have a validity period
> and we don't change it, but gets awkward otherwise.

Providing admins a way of seeing which passwords have been recently used
seems like a clear way to minimize, at least, the risk of the wrong
password being dropped.  Allowing them to control the name feels like
it's another good way to minimize the risk since it's something they can
define and they can include in the name whatever info they need to
figure out if it's the one that's no longer being used, or not.  Forcing
the use of numbers or of validation periods seems like it'd make it
harder on users to avoid this risk, not easier.

Allowing per-password expiration is another way to address the issue of
the wrong password being removed by essentially taking the password away
and seeing what breaks while having an easy way to bring it back if
something important stops working.

> I'm also worried about two kinds of clutter:
>
> * old passwords not being garbage-collected

I'm not terribly concerned with this if the password's validity has
passed and therefore it can't be used any longer.  I could agree with
the concern about it being an issue if the passwords all stay valid
forever.  I'll point out that we arguably have this problem with roles
already today and it doesn't seem to be a huge issue- PG has no idea how
long that user is actually valid for, the admin needs to have something
external to PG to deal with this.

> * the identifier of the current password always changing (perhaps fine
> if it'a a "created at" ID?)

I'm not following what the issue is you're getting at here.

Thanks,

Stephen

On Tue, 2023-10-17 at 16:20 -0400, Stephen Frost wrote:
> Agreed that it's a bad idea to design to support 2 and only 2.

I don't disagree, but it's difficult to come up with syntax that:

 (a) supports N passwords
 (b) makes the ordinary cases simple and documentable
 (c) helps users avoid mistakes (at least in the simple cases)
 (d) makes sense passwords with and without validity period
 (e) handles the challenging cases

One challenging case is that we cannot allow the mixing of password
protocols (e.g. MD5 & SCRAM), because the authentication exchange only
gets one chance at success. If someone ends up with 7 MD5 passwords,
and they'd like to migrate to SCRAM, then we can't allow them to
migrate one password at a time (because then the other passwords would
break). I'd like to see what the SQL for doing this should look like.

>   If
> nothing else, there's the very simple case that the user needs to do
> another password rotation ... and they look and discover that the old
> password is still being used and that if they took it away, things
> would
> break, but they need time to run down which system is still using it
> while still needing to perform the migration for the other systems
> that
> are correctly being updated- boom, need 3 for that case.

That sounds like a reasonable use case. I don't know if we should make
it a requirement, but if we come up with something reasonable that
supports this case I'm fine with it. Ideally, it would still be easy to
see when you are making a mistake (e.g. forgetting to ever remove
passwords).

>   There's other
> use-cases that could be interesting though- presumably we'd log which
> password is used to authenticate and then users could have a fleet of
> web servers which each have their own password but log into the same
> PG
> user and they could happily rotate the passwords independently for
> all
> of those systems.
>
> if they're all
> logging in with the same role and just a different password, that's
> not
> going to happen.

I'm not sure this is a great idea. Can you point to some precedent
here?


> Giving users the option of not having to specify a name and letting
> the
> system come up with one (similar to what we do for indexes and such)
> could work out pretty decently, imv.  I'd have that be optional
> though-
> if the user wants to specify a name, then they should be allowed to
> do
> so.

Can you describe how some basic use cases should work with example SQL?

>
> > * the identifier of the current password always changing (perhaps
> > fine
> > if it'a a "created at" ID?)
>
> I'm not following what the issue is you're getting at here.

I just meant that when rotating passwords, you have to keep coming up
with new names, so the "current" or "primary" one would not have a
consistent name.

Regards,
    Jeff Davis

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Tue, 2023-10-17 at 16:20 -0400, Stephen Frost wrote:
> > Agreed that it's a bad idea to design to support 2 and only 2.
>
> I don't disagree, but it's difficult to come up with syntax that:
>
>  (a) supports N passwords
>  (b) makes the ordinary cases simple and documentable
>  (c) helps users avoid mistakes (at least in the simple cases)
>  (d) makes sense passwords with and without validity period
>  (e) handles the challenging cases

Undoubtably biased ... but I don't agree that this is so difficult.
What points have been raised as a downside of the originally proposed
approach, specifically?

Reading back through the thread, from a user perspective, the primary
one seems to be that passwords are expected to be named.  I'm surprised
this is being brought up as such a serious concern.  Lots and lots and
lots of things in the system require naming, after all, and the idea
that this is somehow harder or more of an issue is quite odd to me.

> One challenging case is that we cannot allow the mixing of password
> protocols (e.g. MD5 & SCRAM), because the authentication exchange only
> gets one chance at success. If someone ends up with 7 MD5 passwords,
> and they'd like to migrate to SCRAM, then we can't allow them to
> migrate one password at a time (because then the other passwords would
> break). I'd like to see what the SQL for doing this should look like.

I've got absolutely no interest in supporting md5- it's high time to rip
that out and be happy that it's gone.  We nearly did it last year and
I'm really hoping we accomplish that this year.

I'm open to the idea that we may need to support some new SCRAM version
or an alternative mechanism in the future, but it's long past time to
spend any time worrying about md5.  As for how difficult it is to deal
with supporting an alternative in the future- that's going to depend a
great deal on what that alternative is and I don't know that we can
really code to handle that as part of this effort in a sensible way, and
trying to code for "anything" is going to likely make this much more
complicated, and probably rife with security issues too.

> >   If
> > nothing else, there's the very simple case that the user needs to do
> > another password rotation ... and they look and discover that the old
> > password is still being used and that if they took it away, things
> > would
> > break, but they need time to run down which system is still using it
> > while still needing to perform the migration for the other systems
> > that
> > are correctly being updated- boom, need 3 for that case.
>
> That sounds like a reasonable use case. I don't know if we should make
> it a requirement, but if we come up with something reasonable that
> supports this case I'm fine with it. Ideally, it would still be easy to
> see when you are making a mistake (e.g. forgetting to ever remove
> passwords).

We have monitoring for many, many parts of the system and this would be
a good thing to monitor also- not just at a per-password level but also
at an overall role/user level, as you have a similar issue there and we
don't properly provide users with any way to check reasonably "hey, when
was the last time this user logged in?".  No, trawling through ancient
logs, if you even have them, isn't a proper solution to this problem.

> >   There's other
> > use-cases that could be interesting though- presumably we'd log which
> > password is used to authenticate and then users could have a fleet of
> > web servers which each have their own password but log into the same
> > PG
> > user and they could happily rotate the passwords independently for
> > all
> > of those systems.
> >
> > if they're all
> > logging in with the same role and just a different password, that's
> > not
> > going to happen.
>
> I'm not sure this is a great idea. Can you point to some precedent
> here?

It's already the case that lots and lots and lots of systems out there
log into PG using the same username/password.  With this, we're at least
offering them the ability to vary the password and keep the user the
same.  We've even seen this be asked for in other ways- the ability to
use distinct Kerberos or LDAP identifies to log into the same user in
the database, see pg_ident.conf and various suggestions for how to bring
that to LDAP too.  Other systems also support the ability to have a
group of users in LDAP, or such, be allowed to log into a specific
database user.  One big reason for this is that then you know everyong
logging into that account has exactly the same access to things- because
that's the lowest level at which access can be granted.  Having a way to
support this similar capability but for passwords is certainly useful.

> > Giving users the option of not having to specify a name and letting
> > the
> > system come up with one (similar to what we do for indexes and such)
> > could work out pretty decently, imv.  I'd have that be optional
> > though-
> > if the user wants to specify a name, then they should be allowed to
> > do
> > so.
>
> Can you describe how some basic use cases should work with example SQL?

As mentioned, we have this general idea already; a simplified CREATE
INDEX syntax can be used as an example:

CREATE INDEX [ [ IF NOT EXISTS ] name ] ON table_name ...

and so we could have:

CREATE PASS [ [ IF NOT EXISTS ] name ] FOR role ...

Giving the user the option of providing a name, but just generating one
if the user declines to include a name.  Naturally, users would have to
have some way to look up the names but that doesn't seem difficult to
provide such as through some \du command, along with appropriate views,
just like we have for indexes.

I do generally feel like the original patch was lacking when it came to
syntax and password management functions, but I don't think the answer
to that is to just throw away the whole concept of multiple passwords in
favor of only supporting a very limited number of them.  I'd think we'd
work towards improving on the syntax to support what we think users will
need to make all of this work smoothly.

> > > * the identifier of the current password always changing (perhaps
> > > fine
> > > if it'a a "created at" ID?)
> >
> > I'm not following what the issue is you're getting at here.
>
> I just meant that when rotating passwords, you have to keep coming up
> with new names, so the "current" or "primary" one would not have a
> consistent name.

Do we need a current or primary?  How is that defined, exactly?  If we
want it, we could surely create it and make it work, but we'd need to
have a good understand of just what it is and why it's the 'current' or
'primary' and I'm not sure that we've actually got a good definition for
that and I'm not convinced yet that we should.

Thanks,

Stephen

On Tue, 2023-10-17 at 22:52 -0400, Stephen Frost wrote:

> Reading back through the thread, from a user perspective, the primary
> one seems to be that passwords are expected to be named.  I'm
> surprised
> this is being brought up as such a serious concern.  Lots and lots
> and
> lots of things in the system require naming, after all, and the idea
> that this is somehow harder or more of an issue is quite odd to me.

In the simplest intended use case, the names will be arbitrary and
temporary. It's easy for me to imagine someone wondering "was I
supposed to delete 'bear' or 'lion'?". For indexes and other objects,
there's a lot more to go on, easily visible with \d.

Now, obviously that is not the end of the world, and the user could
prevent that problem a number of different ways. And we can do things
like improve the monitoring of password use, and store the password
creation time, to help users if they are confused. So I don't raise
concerns about naming as an objection to the feature overall, but
rather a concern that we aren't getting it quite right.

Maybe a name should be entirely optional, more like a comment, and the
passwords can be referenced by the salt? The salt needs to be unique
for a given user anyway.

(Aside: is the uniqueness of the salt enforced in the current patch?)

Regards,
    Jeff Davis

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Tue, 2023-10-17 at 22:52 -0400, Stephen Frost wrote:
>
> > Reading back through the thread, from a user perspective, the primary
> > one seems to be that passwords are expected to be named.  I'm
> > surprised
> > this is being brought up as such a serious concern.  Lots and lots
> > and
> > lots of things in the system require naming, after all, and the idea
> > that this is somehow harder or more of an issue is quite odd to me.
>
> In the simplest intended use case, the names will be arbitrary and
> temporary. It's easy for me to imagine someone wondering "was I
> supposed to delete 'bear' or 'lion'?". For indexes and other objects,
> there's a lot more to go on, easily visible with \d.

Sure, agreed.

> Now, obviously that is not the end of the world, and the user could
> prevent that problem a number of different ways. And we can do things
> like improve the monitoring of password use, and store the password
> creation time, to help users if they are confused. So I don't raise
> concerns about naming as an objection to the feature overall, but
> rather a concern that we aren't getting it quite right.

Right, we need more observability, agreed, but that's not strictly
necessary of this patch and could certainly be added independently.  Is
there really a need to make this observability a requirement of this
particular change?

> Maybe a name should be entirely optional, more like a comment, and the
> passwords can be referenced by the salt? The salt needs to be unique
> for a given user anyway.

I proposed an approach in the email you replied to explicitly suggesting
a way we could make the name be optional, so, sure, I'm generally on
board with that idea.  Note that it'd be optional for the user to
provide and then we'd simply generate one for them and then that name is
what would be used to refer to that password later.

> (Aside: is the uniqueness of the salt enforced in the current patch?)

Err, the salt has to be *identical* for each password of a given user,
not unique, so I'm a bit confused here.

Thanks,

Stephen

On Wed, 2023-10-18 at 14:48 -0400, Stephen Frost wrote:
> Right, we need more observability, agreed, but that's not strictly
> necessary of this patch and could certainly be added independently. 
> Is
> there really a need to make this observability a requirement of this
> particular change?

I won't draw a line in the sand, but it feels like something should be
there to help the user keep track of which password they might want to
keep. At least a "created on" date or something.

> > (Aside: is the uniqueness of the salt enforced in the current
> > patch?)
>
> Err, the salt has to be *identical* for each password of a given
> user,
> not unique, so I'm a bit confused here.

Sorry, my mistake.

If the client needs to use the same salt as existing passwords, can you
still use PQencryptPasswordConn() on the client to avoid sending the
plaintext password to the server?

Regards,
    Jeff Davis

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Wed, 2023-10-18 at 14:48 -0400, Stephen Frost wrote:
> > Right, we need more observability, agreed, but that's not strictly
> > necessary of this patch and could certainly be added independently. 
> > Is
> > there really a need to make this observability a requirement of this
> > particular change?
>
> I won't draw a line in the sand, but it feels like something should be
> there to help the user keep track of which password they might want to
> keep. At least a "created on" date or something.

Sure, no objection to adding that and seems like it should be fairly
easy ... but then again, I tend to feel that we should do that for all
of the objects in the system and we've got some strong feelings against
doing that from others.  Perhaps this case is different to them, in
which case, great, but if it's not, it'd be unfortunate for this feature
to get bogged down due to that.

> > > (Aside: is the uniqueness of the salt enforced in the current
> > > patch?)
> >
> > Err, the salt has to be *identical* for each password of a given
> > user,
> > not unique, so I'm a bit confused here.
>
> Sorry, my mistake.

Sure, no worries.

> If the client needs to use the same salt as existing passwords, can you
> still use PQencryptPasswordConn() on the client to avoid sending the
> plaintext password to the server?

Short answer, yes ... but seems that wasn't actually done yet.  Requires
a bit of additional work, since the client needs to get the existing
salt (note that as part of the SCRAM typical exchange, the client is
provided with the salt, so this isn't exposing anything new) to use to
construct what is then sent to the server to store.  We'll also need to
decide how to handle the case if the client tries to send a password
that doesn't have the same salt as the existing ones (regardless of how
many passwords we end up supporting).  Perhaps we require the user,
through the grammar, to make clear if they want to add a password, and
then error if they don't provide a matching salt, or if they want to
remove existing passwords and replace with the new one.

Thanks,

Stephen

On Tue, 10 Oct 2023 at 16:37, Gurjeet Singh <gurjeet@singh.im> wrote:
>
> > On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh <gurjeet@singh.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.

CFBot shows that the patch does not apply anymore as in [1]:
=== Applying patches on top of PostgreSQL commit ID
4d969b2f85e1fd00e860366f101fd3e3160aab41 ===
=== applying patch
./v4-0002-Update-password-verification-infrastructure-to-ha.patch
...
patching file src/backend/libpq/auth.c
Hunk #4 FAILED at 828.
Hunk #5 succeeded at 886 (offset -2 lines).
Hunk #6 succeeded at 907 (offset -2 lines).
1 out of 6 hunks FAILED -- saving rejects to file src/backend/libpq/auth.c.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_4432.log

Regards,
Vignesh

On Sat, 27 Jan 2024 at 07:18, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 10 Oct 2023 at 16:37, Gurjeet Singh <gurjeet@singh.im> wrote:
> >
> > > On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh <gurjeet@singh.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.
>
> CFBot shows that the patch does not apply anymore as in [1]:
> === Applying patches on top of PostgreSQL commit ID
> 4d969b2f85e1fd00e860366f101fd3e3160aab41 ===
> === applying patch
> ./v4-0002-Update-password-verification-infrastructure-to-ha.patch
> ...
> patching file src/backend/libpq/auth.c
> Hunk #4 FAILED at 828.
> Hunk #5 succeeded at 886 (offset -2 lines).
> Hunk #6 succeeded at 907 (offset -2 lines).
> 1 out of 6 hunks FAILED -- saving rejects to file src/backend/libpq/auth.c.rej
>
> Please post an updated version for the same.

The patch which you submitted has been awaiting your attention for
quite some time now.  As such, we have moved it to "Returned with
Feedback" and removed it from the reviewing queue. Depending on
timing, this may be reversible.  Kindly address the feedback you have
received, and resubmit the patch to the next CommitFest.

Regards,
Vignesh

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@singh.im> wrote:
>
> > On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh <gurjeet@singh.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.