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

On Sun, Oct 8, 2023 at 1:01 PM Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
>
> On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis <pgsql(at)j-davis(dot)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