On 06/08/2024 03:58, Thomas Munro wrote:
> On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> What if the message contains multiple attribute of the same type? If
>> there's a duplicate Message-Authenticator, we should surely reject the
>> packet. I don't know if duplicate attributes are legal in general.
>
> Duplicate attributes are legal in general per RFC 2865, which has a
> table of attributes and their possible quantity; unfortunately this
> one is an extension from RFC 2869, and I didn't find where it pins
> that down. I suppose we could try to detect an unexpected duplicate,
> which might have the side benefit of checking the rest of the
> attributes for well-formedness (though in practice there aren't any).
> Is it worth bothering with that?
Hmm, it does feel sloppy to not verify the format of the rest of the
attributes. That's not new with this patch though. Maybe have a separate
function to verify the packet's format, and call that before
radius_find_attribute().
> I suppose if we wanted to be extra fastidious, we could also test with
> a gallery of malformed packets crafted by a Perl script, but that
> feels like overkill. On the other hand it would be bad if you could
> crash a server by lobbing UDP packets at it because of some dumb
> thinko.
This would also be a easy target for fuzz testing. I'm not too worried
though, the packet format is pretty simple. Still, bugs happen. (Not a
requirement for this patch in any case)
> +my $radius_port = PostgreSQL::Test::Cluster::get_free_port();
This isn't quite right because get_free_port() finds a free TCP port,
while radius uses UDP.
> +#else
> + ereport(elevel,
> + (errcode(ERRCODE_CONFIG_FILE_ERROR),
> + errmsg("this build does not support radiusrequirema=1"),
> + errcontext("line %d of configuration file \"%s\"",
> + line_num, file_name)));
> +#endif
Maybe something like:
errmsg("radiusrequirema=1 is not supported because the server was
built without OpenSSL")
to give the user a hint what they need to do to enable it.
Other than those little things, looks good to me.
--
Heikki Linnakangas
Neon (https://neon.tech)
