Thread: [HACKERS] RADIUS fallback servers
In a discussion at https://www.postgresql.org/message-id/55D51B54.4050902@joh.to we talked about adding RADIUS fallback servers. It never got to the point of it being done.
PFA a patch that implements this.
It supports multiple RADIUS servers. For all other parameters (secret, port, identifier) one can specify either the exact same number of entries, in which case each server gets it's own, or exactly one entry in which case that entry will apply to all servers. (Or zero entries for everything except secret, which will make it the default).
Each server is tried in order. If it responds positive, auth is OK. If it responds negative, auth is rejected. If it does not respond at all, we move on to the next one.
I'm wondering if in doing this we should also make the RADIUS timeout a configurable as HBA option, since it might become more important now?
Attachment
I've given an initial review of this patch. It applies cleanly and compiles without issue as of 6da9759. I'm going to continue with testing it against a set of RADIUS servers in the next few days. But in the meantime, I have a few questions (below). > It supports multiple RADIUS servers. For all other parameters (secret, port, > identifier) one can specify either the exact same number of entries, in > which case each server gets it's own, or exactly one entry in which case > that entry will apply to all servers. (Or zero entries for everything except > secret, which will make it the default). I wonder if removing the complexity of maintaining two separate lists for the server and port would be a better/less complex approach. For instance, why not go with a list of typical 'host:port' strings for 'radiusservers'? If no port is specified, then simply use the default for that specific host. Therefore, we would not have to worry about keeping the two lists in sync. Thoughts? > Each server is tried in order. If it responds positive, auth is OK. If it > responds negative, auth is rejected. If it does not respond at all, we move > on to the next one. > > I'm wondering if in doing this we should also make the RADIUS timeout a > configurable as HBA option, since it might become more important now? Yes, I think this would make sense and would be a good idea. -Adam
On Friday, March 3, 2017, Adam Brightwell <adam.brightwell@crunchydata.com> wrote:
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
I've given an initial review of this patch. It applies cleanly and
compiles without issue as of 6da9759. I'm going to continue with
testing it against a set of RADIUS servers in the next few days. But
in the meantime, I have a few questions (below).
> It supports multiple RADIUS servers. For all other parameters (secret, port,
> identifier) one can specify either the exact same number of entries, in
> which case each server gets it's own, or exactly one entry in which case
> that entry will apply to all servers. (Or zero entries for everything except
> secret, which will make it the default).
I wonder if removing the complexity of maintaining two separate lists
for the server and port would be a better/less complex approach. For
instance, why not go with a list of typical 'host:port' strings for
'radiusservers'? If no port is specified, then simply use the default
for that specific host. Therefore, we would not have to worry about
keeping the two lists in sync. Thoughts?
If we do that we should do it for all the parameters, no? So not just host:port, but something like host:port:secret:identifier? Mixing the two ways of doing it would be quite confusing I think.
And I wonder if that format wouldn't get even more confusing if you for example want to use default ports, but non-default secrets.
I can see how it would probably be easier in some of the simple cases, but I wonder if it wouldn't make it worse in a lot of other cases.
> Each server is tried in order. If it responds positive, auth is OK. If it
> responds negative, auth is rejected. If it does not respond at all, we move
> on to the next one.
>
> I'm wondering if in doing this we should also make the RADIUS timeout a
> configurable as HBA option, since it might become more important now?
Yes, I think this would make sense and would be a good idea.
//Magnus
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
>> I wonder if removing the complexity of maintaining two separate lists >> for the server and port would be a better/less complex approach. For >> instance, why not go with a list of typical 'host:port' strings for >> 'radiusservers'? If no port is specified, then simply use the default >> for that specific host. Therefore, we would not have to worry about >> keeping the two lists in sync. Thoughts? > > > If we do that we should do it for all the parameters, no? So not just > host:port, but something like host:port:secret:identifier? Mixing the two > ways of doing it would be quite confusing I think. > > And I wonder if that format wouldn't get even more confusing if you for > example want to use default ports, but non-default secrets. Yes, I agree. Such a format would be more confusing and I certainly wouldn't be in favor of it. > I can see how it would probably be easier in some of the simple cases, but I > wonder if it wouldn't make it worse in a lot of other cases. Ultimately, I think that it would be better off in a separate configuration file. Something to the effect of each line representing a server, something like: '<server> <port> <secret> <identifier>' With 'radiusservers' simply being the path to that file and 'radiusserver', etc. would remain as is. Where only one or the other could be provided, but not both. Though, that's perhaps would be beyond the scope of this patch. At any rate, I'm going to continue moving forward with testing this patch as is. -Adam
On Mon, Mar 6, 2017 at 10:24 AM, Adam Brightwell <adam.brightwell@crunchydata.com> wrote: >>> I wonder if removing the complexity of maintaining two separate lists >>> for the server and port would be a better/less complex approach. For >>> instance, why not go with a list of typical 'host:port' strings for >>> 'radiusservers'? If no port is specified, then simply use the default >>> for that specific host. Therefore, we would not have to worry about >>> keeping the two lists in sync. Thoughts? >> >> >> If we do that we should do it for all the parameters, no? So not just >> host:port, but something like host:port:secret:identifier? Mixing the two >> ways of doing it would be quite confusing I think. >> >> And I wonder if that format wouldn't get even more confusing if you for >> example want to use default ports, but non-default secrets. > > Yes, I agree. Such a format would be more confusing and I certainly > wouldn't be in favor of it. > >> I can see how it would probably be easier in some of the simple cases, but I >> wonder if it wouldn't make it worse in a lot of other cases. > > Ultimately, I think that it would be better off in a separate > configuration file. Something to the effect of each line representing > a server, something like: > > '<server> <port> <secret> <identifier>' > > With 'radiusservers' simply being the path to that file and > 'radiusserver', etc. would remain as is. Where only one or the other > could be provided, but not both. Though, that's perhaps would be > beyond the scope of this patch. > > At any rate, I'm going to continue moving forward with testing this patch as is. I have run through testing this patch against a small set of RADIUS servers. This testing included both single server and multiple server configurations. All seems to work as expected. -Adam
On Mon, Mar 6, 2017 at 8:14 PM, Adam Brightwell <adam.brightwell@crunchydata. com> wrote:
On Mon, Mar 6, 2017 at 10:24 AM, Adam Brightwell
<adam.brightwell@crunchydata.com> wrote:
>>> I wonder if removing the complexity of maintaining two separate lists
>>> for the server and port would be a better/less complex approach. For
>>> instance, why not go with a list of typical 'host:port' strings for
>>> 'radiusservers'? If no port is specified, then simply use the default
>>> for that specific host. Therefore, we would not have to worry about
>>> keeping the two lists in sync. Thoughts?
>>
>>
>> If we do that we should do it for all the parameters, no? So not just
>> host:port, but something like host:port:secret:identifier? Mixing the two
>> ways of doing it would be quite confusing I think.
>>
>> And I wonder if that format wouldn't get even more confusing if you for
>> example want to use default ports, but non-default secrets.
>
> Yes, I agree. Such a format would be more confusing and I certainly
> wouldn't be in favor of it.
>
>> I can see how it would probably be easier in some of the simple cases, but I
>> wonder if it wouldn't make it worse in a lot of other cases.
>
> Ultimately, I think that it would be better off in a separate
> configuration file. Something to the effect of each line representing
> a server, something like:
>
> '<server> <port> <secret> <identifier>'
>
> With 'radiusservers' simply being the path to that file and
> 'radiusserver', etc. would remain as is. Where only one or the other
> could be provided, but not both. Though, that's perhaps would be
> beyond the scope of this patch.
I think it is. If we want to go there I don't think we should do that as a RADIUS thing -- we should rethink it within the context of *all* the different authentication methods we have.
I have run through testing this patch against a small set of RADIUS>
> At any rate, I'm going to continue moving forward with testing this patch as is.
servers. This testing included both single server and multiple server
configurations. All seems to work as expected.
Seeps I forgot about this one.
Thanks for the review -- I've applied the patch.
I'll look into the timeout thing as a separate patch later.