Re: [PoC] Let libpq reject unexpected authentication requests

On 10/5/22 06:33, Peter Eisentraut wrote:
> I think it would be good to put some provisions in place here, even if
> they are elementary. Otherwise, there will be a significant burden on
> the person who implements the next SASL method (i.e., you ;-) ) to
> figure that out then.

Sounds good, I'll work on that. v10 does not yet make changes in this area.

> I think you could just stick a string list of allowed SASL methods into
> PGconn.
>
> By the way, I'm not sure all the bit fiddling is really worth it. An
> array of integers (or unsigned char or whatever) would work just as
> well. Especially if you are going to have a string list for SASL
> anyway. You're not really saving any bits or bytes either way in the
> normal case.

Yeah, with the SASL case added in, the bitmasks might not be long for
this world. It is nice to be able to invert the whole thing, but a
separate boolean saying "invert the list" could accomplish the same goal
and I think we'll need to have that for the SASL mechanism list anyway.

> Minor comments:
>
> Pasting together error messages like with auth_description() isn't going
> to work. You either need to expand the whole message in
> check_expected_areq(), or perhaps rephrase the message like
>
> libpq_gettext("auth method \"%s\" required, but server requested \"%s\"\n"),
> conn->require_auth,
> auth_description(areq)
>
> and make auth_description() just return a single word not subject to
> translation.

Right. Michael tried to warn me about that upthread, but I only ended up
fixing one of the two error cases for some reason. I've merged the two
into one code path for v10.

Quick error messaging bikeshed: do you prefer

auth method "!password,!md5" requirement failed: ...

or

auth method requirement "!password,!md5" failed: ...

?

> spurious whitespace change in fe-secure-openssl.c

Fixed.

> whitespace error in patch:
>
> .git/rebase-apply/patch:109: tab in indent.
> via TLS, nor GSS authentication via its
> encrypted transport.)

Fixed.

> In the 0002 patch, the configure test needs to be added to meson.build.

Added.

Thanks,
--Jacob