Re: BUG #16106: Patch - Radius secrets always gets lowercased

On 11/11/2019, 20:24, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> > I'm using radius authentication in pg_hba.conf and I've run into the
> > following issue.
> > The radiussecrets is always getting lowercased even if I start it with
> > double quotes. Seems the double quotes are removed by the tokenization
> > process and then the secret gets lowercased by
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_postgres_postgres_blob_REL-5F12-5FSTABLE_src_backend_utils_adt_varlena.c-23L3652&d=DwIFAg&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=RfiS9YBgBDc5Zhy12-AtR1hy_bcIbiTRHiI6ByZ4K0g&m=WfTxWV_TdwLYGd3LXR1CA-JOl_p-ykuP6TwfdpvoXzA&s=IoON5fYePjnpuFN1eX2piFEkT8LfV-g4D7jA-evG_Ew&e=
> > I'm attaching a patch for this since I don't think the secrets should ever
> > be lowercased.
>
> Hm. I know zip about RADIUS but this seems like generally a sane
> change to make. The other very-dubious-in-this-context assumption
> that is embedded in SplitIdentifierString is that the strings should
> be truncated at NAMEDATALEN.
>
> Why did you not change the parsing for all four RADIUS options?
> Probably case-folding wouldn't matter for the server names,
> but the length limitation could.
>
> (Hmm ... on the same principle, PostmasterMain probably shouldn't
> be using this function for parsing ListenAddresses.)
>
> I'm hesitant to back-patch a change like this, because in theory
> it could change a working configuration into a non-working one.
> But it'd be sensible to do in HEAD.
>
> regards, tom lane
>

Hi Tom,

Thanks for taking the time to review this.
I'm resubmitting the patch with changes to all fields.

Should I send the patch to pgsql-hackers?

The issue here is that this is currently broken, the setup won't work as described in the documentation since the secret sent to Radius won't match (unless it's already lowercased)
We only noticed this because we were upgrading from 9.6 and it seems this bug was introduced in 10 in this commit: https://github.com/postgres/postgres/commit/6b76f1bb58f53aec25cfec76391270ea36ad1170

I don't think patch would break anything in current configs since the secret would currently need to be lowercased anyway for the radius auth to work.

Kind regards,
Marcos David

--- src/backend/libpq/hba.copy.c 2019-11-12 14:29:12.000000000 +0000
+++ src/backend/libpq/hba.c 2019-11-12 14:17:29.000000000 +0000
@@ -1927,7 +1927,7 @@

REQUIRE_AUTH_OPTION(uaRADIUS, "radiusservers", "radius");

- if (!SplitIdentifierString(dupval, ',', &parsed_servers))
+ if (!SplitGUCList(dupval, ',', &parsed_servers))
{
/* syntax error in list */
ereport(elevel,
@@ -1976,7 +1976,7 @@

REQUIRE_AUTH_OPTION(uaRADIUS, "radiusports", "radius");

- if (!SplitIdentifierString(dupval, ',', &parsed_ports))
+ if (!SplitGUCList(dupval, ',', &parsed_ports))
{
ereport(elevel,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
@@ -2011,7 +2011,7 @@

REQUIRE_AUTH_OPTION(uaRADIUS, "radiussecrets", "radius");

- if (!SplitIdentifierString(dupval, ',', &parsed_secrets))
+ if (!SplitGUCList(dupval, ',', &parsed_secrets))
{
/* syntax error in list */
ereport(elevel,
@@ -2033,7 +2033,7 @@

REQUIRE_AUTH_OPTION(uaRADIUS, "radiusidentifiers", "radius");

- if (!SplitIdentifierString(dupval, ',', &parsed_identifiers))
+ if (!SplitGUCList(dupval, ',', &parsed_identifiers))
{
/* syntax error in list */
ereport(elevel,