Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the servdata YAML more complete #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ghalse
Copy link
Contributor

@ghalse ghalse commented Aug 8, 2017

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

Extend the YAML output from the servdata command to include the two
missing pieces of data from the current model: proto and addr_type. The
latter is useful when one wants to provide the option for IPv6 support
in a RADIUS server that supports it.
If the addr_type is available in the YAML, we can extend this template
to correctly set the IPv4Only and IPv6Only options depending on the info
in the database.

This change introduces a backwards incompatibility - previously if the
addr_type was set to `any` this was interpreted as setting IPv4Only
@zmousm
Copy link
Contributor

zmousm commented Sep 29, 2017

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
Copy link
Contributor Author

ghalse commented Oct 9, 2017

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
Copy link
Contributor

zmousm commented Oct 9, 2017

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
Copy link
Contributor Author

ghalse commented Jun 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants