Content-Type: multipart/mixed;
	boundary="Apple-Mail=_AC9386FB-3D15-404D-B745-0269504E3C77"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\))
Subject: Re: [PATCH] Fix bug when calling strncmp in check_authmethod_valid
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <tencent_22238742F7B6A675CC1DF0D2A90E7C8C1E08@qq.com>
Date: Mon, 13 May 2024 10:34:48 +0200
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Michael Paquier <michael@paquier.xyz>
Message-Id: <A75E69B9-436D-409B-BBB8-E94076146E5D@yesql.se>
References: <CAMbWs4-mWCGbbE_bne5=AfqjYGDaUZmjCw2+soLjrdNA0xUDFw@mail.gmail.com>
 <Zi8pC2WkIL56cHmj@paquier.xyz> <1068977.1714367460@sss.pgh.pa.us>
 <Zi8vNgPIaeUP5-iM@paquier.xyz> <1076837.1714368760@sss.pgh.pa.us>
 <Zi9E9h-S2cRkumqb@paquier.xyz>
 <CAMbWs48uTLZT14aaBaEg7cpB6ercU5wm9+WfGLGC3VoP28gfCA@mail.gmail.com>
 <1222343.1714398316@sss.pgh.pa.us> <ZjAibGF5tvKWjJDs@paquier.xyz>
 <CAMbWs48adH1Am=tszAqyq3UtEhDCsFL22U8Jzq0EpPAA6tOd9A@mail.gmail.com>
 <1540702.1714441693@sss.pgh.pa.us>
 <tencent_29731C7C7E6A2F9FB807C3A1DC3D81293C06@qq.com>
 <FDDCEFB7-B808-44CE-B165-617A6FBC6F7A@yesql.se>
 <tencent_22238742F7B6A675CC1DF0D2A90E7C8C1E08@qq.com>
To: Jingxian Li <aqktjcm@qq.com>
Archived-At: <https://www.postgresql.org/message-id/A75E69B9-436D-409B-BBB8-E94076146E5D%40yesql.se>
Precedence: bulk


--Apple-Mail=_AC9386FB-3D15-404D-B745-0269504E3C77
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

> On 7 May 2024, at 06:46, Jingxian Li <aqktjcm@qq.com> wrote:

>> But, since it's been broken in all supported versions of postgres and =
has
>> AFAICT never been documented to exist, should we fix it or just =
remove it?  We
>> don't support auth-options for any other methods, like clientcert to =
cert for
>> example.  If we fix it we should also document that it works IMHO.
>=20
> You mentioned that auth-options are not supported for auth methods =
except pam,
> but I found that  some methods (such as  ldap and radius etc.) also =
requires aut-options,
> and there are no corresponding auth methods ending with space (such as =
 "ldap " and=20
> radius ") present in auth_methods_host and auth_methods_local arrays.

Correct, only pam and ident were ever supported (yet not documented) and =
ident
was removed a long time ago.

Searching the archives I was unable to find any complaints, and this has =
been
broken for the entire window of supported releases, so I propose we =
remove it
as per the attached patch.  If anyone is keen on making this work again =
for all
the types where it makes sense, it can be resurrected (probably with a =
better
implementation).

Any objections to fixing this in 17 by removing it? (cc:ing Michael from =
the RMT)

--
Daniel Gustafsson


--Apple-Mail=_AC9386FB-3D15-404D-B745-0269504E3C77
Content-Disposition: attachment;
	filename=v2-0001-Remove-auth-options-support-from-initdb.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v2-0001-Remove-auth-options-support-from-initdb.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=2017cc0811b7eebd93f8e063a5b025180c95badf56=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20Daniel=20Gustafsson=20=
<dgustafsson@postgresql.org>=0ADate:=20Mon,=2013=20May=202024=2009:55:48=20=
+0200=0ASubject:=20[PATCH=20v2]=20Remove=20auth-options=20support=20from=20=
initdb=0A=0AWhen=20--auth=20was=20added=20to=20initdb=20in=20commit=20=
e7029b212755=20it=20had=20support=0Afor=20auth=20options=20separated=20=
by=20space=20from=20the=20auth=20type,=20like:=0A=0A=20=20=20=20--auth=20=
pam=20<servicename>=0A=20=20=20=20--auth=20ident=20sameuser=0A=0APassing=20=
an=20option=20to=20the=20ident=20auth=20type=20was=20removed=20in=20=
01c1a12a5bb4=0Awhich=20left=20the=20pam=20auth-options=20support=20in=20=
place.=208a02339e9ba3=20broke=0Athis=20by=20inverting=20a=20calculation=20=
in=20the=20strncmp=20arguments,=20which=20went=0Aunnoticed=20for=20a=20=
long=20time.=20=20The=20ability=20to=20pass=20options=20to=20the=20auth=0A=
type=20was=20never=20documented.=0A=0ARather=20than=20fixing=20the=20=
support=20for=20an=20undocumented=20feature=20which=20has=0Abeen=20=
broken=20for=20all=20supported=20versions,=20and=20which=20only=20=
supports=20one=0Aout=20of=20many=20auth=20types=20which=20can=20take=20=
options,=20it=20is=20removed.=0A=0AReported-by:=20Jingxian=20Li=20=
<aqktjcm@qq.com>=0ADiscussion:=20=
https://postgr.es/m/tencent_29731C7C7E6A2F9FB807C3A1DC3D81293C06@qq.com=0A=
---=0A=20src/bin/initdb/initdb.c=20|=208=20++------=0A=201=20file=20=
changed,=202=20insertions(+),=206=20deletions(-)=0A=0Adiff=20--git=20=
a/src/bin/initdb/initdb.c=20b/src/bin/initdb/initdb.c=0Aindex=20=
30e17bd1d1..5e89b3c8e8=20100644=0A---=20a/src/bin/initdb/initdb.c=0A+++=20=
b/src/bin/initdb/initdb.c=0A@@=20-102,7=20+102,7=20@@=20static=20const=20=
char=20*const=20auth_methods_host[]=20=3D=20{=0A=20=09"sspi",=0A=20=
#endif=0A=20#ifdef=20USE_PAM=0A-=09"pam",=20"pam=20",=0A+=09"pam",=0A=20=
#endif=0A=20#ifdef=20USE_BSD_AUTH=0A=20=09"bsd",=0A@@=20-118,7=20+118,7=20=
@@=20static=20const=20char=20*const=20auth_methods_host[]=20=3D=20{=0A=20=
static=20const=20char=20*const=20auth_methods_local[]=20=3D=20{=0A=20=09=
"trust",=20"reject",=20"scram-sha-256",=20"md5",=20"password",=20"peer",=20=
"radius",=0A=20#ifdef=20USE_PAM=0A-=09"pam",=20"pam=20",=0A+=09"pam",=0A=20=
#endif=0A=20#ifdef=20USE_BSD_AUTH=0A=20=09"bsd",=0A@@=20-2512,10=20=
+2512,6=20@@=20check_authmethod_valid(const=20char=20*authmethod,=20=
const=20char=20*const=20*valid_methods,=0A=20=09{=0A=20=09=09if=20=
(strcmp(authmethod,=20*p)=20=3D=3D=200)=0A=20=09=09=09return;=0A-=09=09=
/*=20with=20space=20=3D=20param=20*/=0A-=09=09if=20(strchr(authmethod,=20=
'=20'))=0A-=09=09=09if=20(strncmp(authmethod,=20*p,=20(authmethod=20-=20=
strchr(authmethod,=20'=20')))=20=3D=3D=200)=0A-=09=09=09=09return;=0A=20=09=
}=0A=20=0A=20=09pg_fatal("invalid=20authentication=20method=20\"%s\"=20=
for=20\"%s\"=20connections",=0A--=20=0A2.39.3=20(Apple=20Git-146)=0A=0A=

--Apple-Mail=_AC9386FB-3D15-404D-B745-0269504E3C77--