On Mon, Mar 6, 2017 at 8:14 PM, Adam Brightwell < adam.brightw...@crunchydata.com> wrote:
> On Mon, Mar 6, 2017 at 10:24 AM, Adam Brightwell > <adam.brightw...@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. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
