search

grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

Make the servdata YAML more complete #53

Open ghalse opened 7 years ago

ghalse commented 7 years ago

The current YAML output from the servdata command does not include two pieces of information current model: proto and addr_type. Whilst the former is currently not really used in DjNRO, the latter is useful when one wants to provide the option for IPv6 support in a RADIUS server that supports it.

This pull request includes two commits.

The first adds the two missing pieces of data to the YAML output (both are included for completeness). This should be backwards compatible since it's only adding additional information to the YAML which existing implementations should ignore.

The second updates the existing radsecproxy.tpl example template to make use of the addr_type entry to control the generation of the IPv4Only and IPv6Only flags. This may have backwards compatability issues if anyone depends on this template, since the current version generates IPv4Only on for all entries regardless of the addr_type, and the new version will leave it out for addr_type any (so permitting IPv6).

zmousm commented 6 years ago

Hi there, sorry for the very very very very long delay.

No objection whatsoever to add RADIUS protocol and IP version to servdata.

However, apart from the changes in your patch, it is also necessary to amend RADPROTOS and ADDRTYPES in edumanage/models.py; the additional options are already there, but commented out. Such tuples are used for field choices; changing the options will usually lead to creating a (perhaps spurious) database schema migration (see #22 for example).

As for the hard-coded IPv4Only in radsecproxy.tpl, your observation is correct, however it should probably have never been that way, so I think it's OK. The field addr_type defaults to ipv4 anyway and it is otherwise only used for form input validation. Aside: accepting hostnames vs. IP addresses for servers is a topic that deserves separate discussion (and perhaps reconsideration); for example it is typically a bad idea to use hostnames for RADIUS clients definitions.

ghalse commented 6 years ago

I knew about RADPROTS and ADDRTYPES, but hadn't included them to avoid changes to the UI. I have that as a local patch, but can add it to the pull request if you'd like.

I also personally agree about hostnames vs IPs, but the underlying RADIUS server will accept hostnames so my take was let people make their own choice about addr_type any.

zmousm commented 6 years ago

Perhaps we could move such tuples to settings (and use get_choices_from_settings()). This would allow opt-in for the new options, rather than introduce them unconditionally for everyone. Right?

I would prefer to push such changes through a separate PR, so as to avoid changing the context for this one (and adjusting the description etc).

ghalse commented 2 months ago

The more relevant of these changes (enabling TLS) is now in @vladimir-mencl-eresearch's addition to #92