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