Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Raise fixed token-length limit in hba.c.

  2. Eliminate fixed token-length limit in hba.c.

  1. Removing the fixed-size buffer restriction in hba.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-24T17:53:28Z

    We got a complaint at [1] about how a not-so-unreasonable LDAP
configuration can hit the "authentication file token too long,
skipping" error case in hba.c's next_token().  I think we've
seen similar complaints before, although a desultory archives
search didn't turn one up.

A minimum-change response would be to increase the MAX_TOKEN
constant from 256 to (say) 1K or 10K.  But it wouldn't be all
that hard to replace the fixed-size buffer with a StringInfo,
as attached.

Given the infrequency of complaints, I'm inclined to apply
the more thorough fix only in HEAD, and to just raise MAX_TOKEN
in the back branches.  Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/PH0PR04MB8294A4C5A65D9D492CBBD349C002A%40PH0PR04MB8294.namprd04.prod.outlook.com

  2. Re: Removing the fixed-size buffer restriction in hba.c

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2023-07-24T18:07:07Z

    On Mon, Jul 24, 2023 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> We got a complaint at [1] about how a not-so-unreasonable LDAP
> configuration can hit the "authentication file token too long,
> skipping" error case in hba.c's next_token().  I think we've
> seen similar complaints before, although a desultory archives
> search didn't turn one up.
>
> A minimum-change response would be to increase the MAX_TOKEN
> constant from 256 to (say) 1K or 10K.  But it wouldn't be all
> that hard to replace the fixed-size buffer with a StringInfo,
> as attached.
>

+1 for replacing it with StringInfo. And the patch LGTM!


>
> Given the infrequency of complaints, I'm inclined to apply
> the more thorough fix only in HEAD, and to just raise MAX_TOKEN
> in the back branches.  Thoughts?
>

It makes sense to change it only in HEAD.

Regards,

--
Fabrízio de Royes Mello

  3. Re: Removing the fixed-size buffer restriction in hba.c

    Nathan Bossart <nathandbossart@gmail.com> — 2023-07-24T22:58:31Z

    On Mon, Jul 24, 2023 at 03:07:07PM -0300, Fabrízio de Royes Mello wrote:
> On Mon, Jul 24, 2023 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> We got a complaint at [1] about how a not-so-unreasonable LDAP
>> configuration can hit the "authentication file token too long,
>> skipping" error case in hba.c's next_token().  I think we've
>> seen similar complaints before, although a desultory archives
>> search didn't turn one up.
>>
>> A minimum-change response would be to increase the MAX_TOKEN
>> constant from 256 to (say) 1K or 10K.  But it wouldn't be all
>> that hard to replace the fixed-size buffer with a StringInfo,
>> as attached.
>>
> 
> +1 for replacing it with StringInfo. And the patch LGTM!

I'd suggest noting that next_token() clears buf, but otherwise it looks
reasonable to me, too.

>> Given the infrequency of complaints, I'm inclined to apply
>> the more thorough fix only in HEAD, and to just raise MAX_TOKEN
>> in the back branches.  Thoughts?
>>
> 
> It makes sense to change it only in HEAD.

I wouldn't be opposed to back-patching the more thorough fix, but I don't
feel too strongly about it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  4. Re: Removing the fixed-size buffer restriction in hba.c

    Michael Paquier <michael@paquier.xyz> — 2023-07-25T00:14:54Z

    On Mon, Jul 24, 2023 at 03:58:31PM -0700, Nathan Bossart wrote:
> On Mon, Jul 24, 2023 at 03:07:07PM -0300, Fabrízio de Royes Mello wrote:
>>> Given the infrequency of complaints, I'm inclined to apply
>>> the more thorough fix only in HEAD, and to just raise MAX_TOKEN
>>> in the back branches.  Thoughts?
>>>
>> 
>> It makes sense to change it only in HEAD.
> 
> I wouldn't be opposed to back-patching the more thorough fix, but I don't
> feel too strongly about it.

- * The token, if any, is returned at *buf (a buffer of size bufsz), and
+ * The token, if any, is returned into *buf, and
  * *lineptr is advanced past the token.

The comment indentation is a bit off here.

  * In event of an error, log a message at ereport level elevel, and also
- * set *err_msg to a string describing the error.  Currently the only
- * possible error is token too long for buf.
+ * set *err_msg to a string describing the error.  Currently no error
+ * conditions are defined.

I find the choice to keep err_msg in next_token() a bit confusing in
next_field_expand().  If no errors are possible, why not just get rid
of it?

FWIW, I don't feel strongly about backpatching that either, so for the
back branches I'd choose to bump up the token size limit and call it a
day.
--
Michael

  5. Re: Removing the fixed-size buffer restriction in hba.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-25T00:21:54Z

    Michael Paquier <michael@paquier.xyz> writes:
> I find the choice to keep err_msg in next_token() a bit confusing in
> next_field_expand().  If no errors are possible, why not just get rid
> of it?

Yeah, I dithered about that.  I felt like removing it only to put it
back later would be silly, but then again maybe there won't ever be
a need to put it back.  I'm OK with removing it if there's not
objections.

			regards, tom lane

  6. Re: Removing the fixed-size buffer restriction in hba.c

    Michael Paquier <michael@paquier.xyz> — 2023-07-25T00:45:06Z

    On Mon, Jul 24, 2023 at 08:21:54PM -0400, Tom Lane wrote:
> Yeah, I dithered about that.  I felt like removing it only to put it
> back later would be silly, but then again maybe there won't ever be
> a need to put it back.  I'm OK with removing it if there's not
> objections.

Seeing what this code does, the odds of needing an err_msg seem rather
low to me, so I would just remove it for now for the sake of clarity.
It can always be added later if need be.
--
Michael

  7. Re: Removing the fixed-size buffer restriction in hba.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-27T16:11:38Z

    Michael Paquier <michael@paquier.xyz> writes:
> Seeing what this code does, the odds of needing an err_msg seem rather
> low to me, so I would just remove it for now for the sake of clarity.
> It can always be added later if need be.

Done that way.  Thanks for looking at it!

			regards, tom lane

  8. Re: Removing the fixed-size buffer restriction in hba.c

    Michael Paquier <michael@paquier.xyz> — 2023-07-27T22:42:07Z

    On Thu, Jul 27, 2023 at 12:11:38PM -0400, Tom Lane wrote:
> Done that way.  Thanks for looking at it!

Thanks for applying!
--
Michael