[PoC/RFC] Multiple passwords, interval expirations

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.

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

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 who are also interested, and
may be able to bring someone else to help work on it too but that might be
more August / September time frame).

Thanks,

Stephen

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

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.

Greetings,

On Fri, Jul 1, 2022 at 10:51 Brindle, Joshua <joshuqbr@amazon.com> wrote:

On 6/30/22 8:20 PM, Stephen Frost wrote:

* 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

Hi!

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.

Perhaps though would certainly like this to patch to be useful for the
use-cases that we had discussed, naturally. :)

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.

Wasn’t too bad.. needs more clean-up, there was some white space issues and
some simple re-base stuff, but then the support for “md5” pg_hba option was
broken for users with SCRAM passwords because we weren’t checking if there
was a SCRAM pw stored and upgrading to SCRAM in that case. That’s the main
case that I fixed. We will need to document this though, of course. The
patch I submitted should basically do:

pg_hba md5 + md5-only pws -> md5 auth used
pg_hba md5 + scram-only pws -> scram
pg_hba md5 + md5 and scram pws -> scram
pg_hba scram -> scram

Not sure if we need to try and do something to make it possible to have
pg_hba md5 + mixed pws and have md5 used but it’s tricky as we would have
to know on the server side early on if that’s what we want to do. We could
add an option to md5 to say “only do md5” maybe but I’m also inclined to
not bother and tell people to just get moved to scram already.

For my 2c, I’d also like to move to having a separate column for the PW
type from the actual secret but that’s largely an independent change.

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 support for “md5” pg_hba option was broken for users with SCRAM passwords because we weren’t checking if there was a SCRAM pw stored and upgrading to SCRAM in that case. That’s the main case that I fixed. We will need to document this though, of course. The patch I submitted should basically do:

pg_hba md5 + md5-only pws -> md5 auth used
pg_hba md5 + scram-only pws -> scram
pg_hba md5 + md5 and scram pws -> scram
pg_hba scram -> scram

Not sure if we need to try and do something to make it possible to have pg_hba md5 + mixed pws and have md5 used but it’s tricky as we would have to know on the server side early on if that’s what we want to do. We could add an option to md5 to say “only do md5” maybe but I’m also inclined to not bother and tell people to just get moved to scram already.

For my 2c, I’d also like to move to having a separate column for the PW type from the actual secret but that’s largely an independent change.

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

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

The changes since V3 are (compare [1]v3 patch, applied to a contemporary commit on master branch https://github.com/gurjeet/postgres/commits/multiple_passwords_v3 vs. [2]main development branch, patch rebased to current master branch, followed by many changes https://github.com/gurjeet/postgres/commits/multiple_passwords, 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
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
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]https://en.wikipedia.org/wiki/Zero_one_infinity_rule, 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