On Sun, Oct 8, 2023 at 1:01 PM Gurjeet Singh <gurj...@singh.im> wrote: > > On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis <pg...@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
password_rollover_v3.diff
Description: Binary data
