Thread: Supporting fallback RADIUS server(s)

Hi,

We use RADIUS authentication at $WORK, and it has one major flaw (well, 
two, but I already fixed the other one this week): it only supports 
specifying a single server, which as we might know, is bad for high 
availability.  So I'm developing a patch to fix this issue, but I'm not 
exactly sure what the configuration should look like.  I see multiple 
options, but the one I like the best is the following:

Add two new HBA configuration options: radiusfallbackservers and 
radiusfallbackports; both lists parsed with SplitIdentifierString (à la 
listen_addresses).  If radiusfallbackservers is set, it's the list of 
servers to try after "radiusserver" has timed out (but ONLY if it timed 
out, obviously).  If radiusfallbackports is set, it must be of equal 
length with radiusfallbackservers, and the corresponding port entry is 
used for each fallback server.  If it's not set, radiusport is used as 
the port for all fallback servers, or finally the default port (1812) if 
no port options have been specified in pg_hba.conf.

This interface is both flexible, simple to use in the common case (YMMV) 
and fully backwards compatible.

I'm planning two submit two patches: one to refactor CheckRADIUSAuth() a 
bit to make it easier to try the authentication against more than one 
server, and then another one to add the two new configuration options.

Does anyone have a problem with this plan?  Want to comment on the 
interface or get anything else off their chest?


.m

Marko Tiikkaja <marko@joh.to> writes:
> So I'm developing a patch to fix this issue, but I'm not 
> exactly sure what the configuration should look like.  I see multiple 
> options, but the one I like the best is the following:

> Add two new HBA configuration options: radiusfallbackservers and 
> radiusfallbackports; both lists parsed with SplitIdentifierString (à la 
> listen_addresses).

Why add new GUCs for that?  Can't we just redefine radiusserver as a list
of servers to try in sequence, and similarly split radiusport into a list?

Or maybe better, rename it radius_servers.  But what you have here seems
weird, and it certainly doesn't follow the precedent of what we did when
we replaced listen_address with listen_addresses.
        regards, tom lane

On 2015-08-20 02:29, Tom Lane wrote:
> Marko Tiikkaja <marko@joh.to> writes:
>> So I'm developing a patch to fix this issue, but I'm not
>> exactly sure what the configuration should look like.  I see multiple
>> options, but the one I like the best is the following:
>
>> Add two new HBA configuration options: radiusfallbackservers and
>> radiusfallbackports; both lists parsed with SplitIdentifierString (Ã  la
>> listen_addresses).
>
> Why add new GUCs for that?  Can't we just redefine radiusserver as a list
> of servers to try in sequence, and similarly split radiusport into a list?
>
> Or maybe better, rename it radius_servers.  But what you have here seems
> weird, and it certainly doesn't follow the precedent of what we did when
> we replaced listen_address with listen_addresses.

If we were adding RADIUS support today, this would be the best option. 
But seeing that we still only support one server today, this seems like 
a backwards incompatibility which practically 100% of users won't 
benefit from.  But I don't care much either way.  If we think breaking 
compatibility here is acceptable, I'd say we go for radius_servers and 
radius_ports.


.m

On 8/20/15 12:53 PM, Magnus Hagander wrote:
> We could change it to radius_servers and radius_ports, and deprecate but
> keep accepting the old parameters for a release or two.

That's one option..

> To make it easy, we
> make sure that both parameter names accepts the same format parameter, so
> it's easy enough to just replace it once deprecated.

I'm not sure I understand what you mean by this.


.m

On 8/20/15 12:57 PM, Magnus Hagander wrote:
> I mean that you could write radius_server=foo or radius_servers=foo as well
> as radius_server=foo,bar and radius_servers=foo,bar. As long as you don't
> specify both radius_server and radius_servers, either one of them should
> accept either one server or multiple servers.

I think if we do it this way, I'd prefer if "radiusserver" was kept 
fully backwards compatible.  So either specify "radiusserver" or 
"radius_servers", but only the second one accepts a list.


.m

Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Aug 20, 2015 at 2:36 AM, Marko Tiikkaja <marko@joh.to> wrote:
>> On 2015-08-20 02:29, Tom Lane wrote:
>>> Why add new GUCs for that?  Can't we just redefine radiusserver as a list
>>> of servers to try in sequence, and similarly split radiusport into a list?

> We could change it to radius_servers and radius_ports, and deprecate but
> keep accepting the old parameters for a release or two. To make it easy, we
> make sure that both parameter names accepts the same format parameter, so
> it's easy enough to just replace it once deprecated.

Considering that we did no such thing for listen_address, which is of
concern to probably two or three orders-of-magnitude more users than
radiusserver, I don't really see why we'd do it for the RADIUS settings.

Our expectations about forward compatibility for postgresql.conf entries
have always been pretty low; even more so for not-widely-used settings.

In any case, wouldn't what you describe simply put off the pain for awhile
(replacing it with confusion in the short term)?  Eventually we're going
to break it.
        regards, tom lane

<p dir="ltr"><br /> On Aug 20, 2015 4:16 PM, "Tom Lane" <<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:<br /> ><br /> > Magnus Hagander <<a
href="mailto:magnus@hagander.net">magnus@hagander.net</a>>writes:<br /> > > On Thu, Aug 20, 2015 at 2:36 AM,
MarkoTiikkaja <<a href="mailto:marko@joh.to">marko@joh.to</a>> wrote:<br /> > >> On 2015-08-20 02:29,
TomLane wrote:<br /> > >>> Why add new GUCs for that?  Can't we just redefine radiusserver as a list<br />
>>>> of servers to try in sequence, and similarly split radiusport into a list?<br /> ><br /> > >
Wecould change it to radius_servers and radius_ports, and deprecate but<br /> > > keep accepting the old
parametersfor a release or two. To make it easy, we<br /> > > make sure that both parameter names accepts the
sameformat parameter, so<br /> > > it's easy enough to just replace it once deprecated.<br /> ><br /> >
Consideringthat we did no such thing for listen_address, which is of<br /> > concern to probably two or three
orders-of-magnitudemore users than<br /> > radiusserver, I don't really see why we'd do it for the RADIUS
settings.<br/> ><br /> > Our expectations about forward compatibility for postgresql.conf entries<br /> > have
alwaysbeen pretty low; even more so for not-widely-used settings.<br /> ><br /> > In any case, wouldn't what you
describesimply put off the pain for awhile<br /> > (replacing it with confusion in the short term)?  Eventually
we'regoing<br /> > to break it.<br /> ><br /><p dir="ltr">Well, that's true of any depreciation. And if we throw
alog message then people will know about it... <p dir="ltr">That said, I'm not against a clean break with compatibility
either.As long as it's clean - like renaming the parameters. <p dir="ltr">/Magnus <br /> 

On 8/20/15 4:25 PM, Magnus Hagander wrote:
> On Aug 20, 2015 4:16 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> Our expectations about forward compatibility for postgresql.conf entries
>> have always been pretty low; even more so for not-widely-used settings.
>>
>> In any case, wouldn't what you describe simply put off the pain for awhile
>> (replacing it with confusion in the short term)?  Eventually we're going
>> to break it.
>>
>
> Well, that's true of any depreciation. And if we throw a log message then
> people will know about it...
>
> That said, I'm not against a clean break with compatibility either. As long
> as it's clean - like renaming the parameters.

All right.  I'll submit a patch for that.

Thanks for the input, both.


.m