Обсуждение: Remove last traces of SCM credential auth from libpq?
Hi all, libpq has kept some code related to the support of authentication with SCM credentials for some time now, code dead in the backend since 9.1. Wouldn't it be time to let it go and remove this code entirely, erroring in libpq if attempting to connect to a server that supports that? Hard to say if this is actually working these days. Opinions or thoughts? -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> libpq has kept some code related to the support of authentication with
> SCM credentials for some time now, code dead in the backend since
> 9.1. Wouldn't it be time to let it go and remove this code entirely,
> erroring in libpq if attempting to connect to a server that supports
> that?
+1. Since that's only used on Unix-domain sockets, it could only be
useful if you were using current libpq while talking to a pre-9.1
server on the same machine. That seems fairly unlikely --- and if
you did have to do that, you could still connect, just not with peer
auth. You'd be suffering other quality-of-life problems too,
because we removed support for such old servers from psql and pg_dump
awhile ago.
> Hard to say if this is actually working these days.
I didn't trace the old discussions, but the commit that removed the
server-side support (be4585b1c) mentions something about portability
issues with that code ... so it's rather likely that it didn't work
anyway.
In addition to the changes here, it looks like you could drop the
configure/meson probes that set HAVE_STRUCT_CMSGCRED.
Also, in pg_fe_sendauth, couldn't you just let the default: case
handle it instead of adding a bespoke error message? We're not
really expecting that anyone is ever going to hit this, so I'm
not convinced it's worth the translation burden.
regards, tom lane
On 3/16/23 10:49 AM, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> libpq has kept some code related to the support of authentication with >> SCM credentials for some time now, code dead in the backend since >> 9.1. Wouldn't it be time to let it go and remove this code entirely, >> erroring in libpq if attempting to connect to a server that supports >> that? > > +1. Since that's only used on Unix-domain sockets, it could only be > useful if you were using current libpq while talking to a pre-9.1 > server on the same machine. +1. > Also, in pg_fe_sendauth, couldn't you just let the default: case > handle it instead of adding a bespoke error message? We're not > really expecting that anyone is ever going to hit this, so I'm > not convinced it's worth the translation burden. +1 to this, that was my thought as well. That would let us remove the "AUTH_REQ_SCM_CREDS" constant too. It looks like in the po files there are a bunch of "SCM_CRED authentication method not supported" messages that can also be removed. Thanks, Jonathan
Вложения
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> It looks like in the po files there are a bunch of "SCM_CRED
> authentication method not supported" messages that can also be removed.
Those will go away in the normal course of translation maintenance,
there's no need to remove them by hand. (Generally speaking, there
is no need to ever touch the .po files except when new versions get
imported from the translation repo.)
regards, tom lane
On Thu, Mar 16, 2023 at 10:49:45AM -0400, Tom Lane wrote: > In addition to the changes here, it looks like you could drop the > configure/meson probes that set HAVE_STRUCT_CMSGCRED. Right, done. > Also, in pg_fe_sendauth, couldn't you just let the default: case > handle it instead of adding a bespoke error message? We're not > really expecting that anyone is ever going to hit this, so I'm > not convinced it's worth the translation burden. Yes, I was wondering if that's worth keeping or not, so I chose consistency with AUTH_REQ_KRB4 and AUTH_REQ_KRB5. Would it be better to hold on this patch for 17~? I have just noticed that while looking at Jacob's patch for require_auth, so the timing is not good. Honestly, I don't see a reason to wait a few extra month to remove that, particularly now that pg_dump and pg_upgrade go down to 9.2.. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Mar 16, 2023 at 10:49:45AM -0400, Tom Lane wrote:
>> Also, in pg_fe_sendauth, couldn't you just let the default: case
>> handle it instead of adding a bespoke error message? We're not
>> really expecting that anyone is ever going to hit this, so I'm
>> not convinced it's worth the translation burden.
> Yes, I was wondering if that's worth keeping or not, so I chose
> consistency with AUTH_REQ_KRB4 and AUTH_REQ_KRB5.
Maybe flush those special messages too? I'm not sure how long
they've been obsolete, though.
> Would it be better to hold on this patch for 17~?
Nah, I see no reason to wait. We already dropped the higher-level
client support (psql/pg_dump) for these server versions in v15.
regards, tom lane
On Thu, Mar 16, 2023 at 08:10:12PM -0400, Tom Lane wrote: > Maybe flush those special messages too? I'm not sure how long > they've been obsolete, though. KRB4 was switched in a159ad3 back in 2005, and KRB5 in 98de86e back in 2014 (deprecated in 8.3, so that's even older than creds). So yes, that could be removed as well, I guess, falling back to the default error message. > Nah, I see no reason to wait. We already dropped the higher-level > client support (psql/pg_dump) for these server versions in v15. Okay. I'll clean up this part today, then. -- Michael
Вложения
On Fri, Mar 17, 2023 at 09:30:32AM +0900, Michael Paquier wrote: > KRB4 was switched in a159ad3 back in 2005, and KRB5 in 98de86e back in > 2014 (deprecated in 8.3, so that's even older than creds). So yes, > that could be removed as well, I guess, falling back to the default > error message. This seems like something worth a thread of its own, will send a patch. >> Nah, I see no reason to wait. We already dropped the higher-level >> client support (psql/pg_dump) for these server versions in v15. > > Okay. I'll clean up this part today, then. I got around to do that with 98ae2c8. -- Michael