On 06/08/2024 03:58, Thomas Munro wrote:
On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinn...@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)
