Re: 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(at)joh(dot)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(at)joh(dot)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 Thu, Aug 20, 2015 at 2:36 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> On 2015-08-20 02:29, Tom Lane wrote:
>
>> Marko Tiikkaja <marko(at)joh(dot)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.

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.

And we could consider throwing a WARNING or at least a LOG when the old
name is used, indicating that it's deprecated.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

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 Thu, Aug 20, 2015 at 12:55 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> 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.

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.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

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(at)hagander(dot)net> writes:
> On Thu, Aug 20, 2015 at 2:36 AM, Marko Tiikkaja <marko(at)joh(dot)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

On Aug 20, 2015 4:16 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > On Thu, Aug 20, 2015 at 2:36 AM, Marko Tiikkaja <marko(at)joh(dot)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.
>

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.

/Magnus

On 8/20/15 4:25 PM, Magnus Hagander wrote:
> On Aug 20, 2015 4:16 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)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