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

[feature] Added openwisp_radius installation #223

Closed
wants to merge 38 commits into from
Closed

Conversation

atb00ker
Copy link
Member

@atb00ker atb00ker commented Nov 21, 2020

Todo:

  • test celery-beat tasks
  • Check docs

Implements #198.

Copy link
Member Author

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started with openwisp_radius ansible setup. 😄

molecule/resources/converge.yml Outdated Show resolved Hide resolved
tasks/apt.yml Outdated Show resolved Hide resolved
tasks/freeradius.yml Outdated Show resolved Hide resolved
templates/openwisp2/settings.py Outdated Show resolved Hide resolved
templates/openwisp2/settings.py Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting this Ajay, great progress, I left some comments, more review and testing will be needed but it's surely going well.

defaults/main.yml Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
templates/openwisp2/urls.py Outdated Show resolved Hide resolved
@atb00ker atb00ker force-pushed the openwisp_radius branch 3 times, most recently from 3e2dee8 to b898ca9 Compare December 5, 2020 10:21
@atb00ker atb00ker changed the title {WIP}[feature] Added openwisp_radius installation [feature] Added openwisp_radius installation Dec 5, 2020
@atb00ker atb00ker marked this pull request as ready for review December 5, 2020 10:35
Copy link
Member Author

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pandafy @nemesisdesign, please find the comments below, I think this out of the oven, a second pair eyes would be very helpful! 😄
New error found, working on it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
templates/openwisp2/urls.py Outdated Show resolved Hide resolved
templates/openwisp2/settings.py Outdated Show resolved Hide resolved
@atb00ker atb00ker marked this pull request as ready for review December 5, 2020 13:17
@atb00ker atb00ker force-pushed the openwisp_radius branch 4 times, most recently from b615612 to 6193334 Compare December 5, 2020 13:50
@atb00ker
Copy link
Member Author

atb00ker commented Dec 5, 2020

It fails on molecule but works on my system install.
Need to investigate.

@atb00ker
Copy link
Member Author

atb00ker commented Dec 8, 2020

@pandafy, stuck at #223 (comment), if you have time, please look into it.
It works only device but running on molecule fails, looks like a django / dependency related thing (might need to version lock something).

@nemesifier
Copy link
Member

nemesifier commented Dec 9, 2020

I wonder why we have a task that installs cryptography in this role, cryptography should be a dependency of django-x509 and I don't think we have to explicitly install it.

I am releasing a new minor version of django-x509 to ensure the cryptography dependency is bound: openwisp/django-x509#108
Then I will remove the pip install task from this role (#239).

@pandafy
Copy link
Member

pandafy commented Dec 10, 2020

@atb00ker this is occurring becausue openwisp/openwisp-radius@0819e35 was added after version 0.1.0. This should not occur if you test with the latest master. I guess it will require issuing 0.2.0 release of openwisp-radius

@atb00ker
Copy link
Member Author

@pandafy got it, since radius 2 release is so close, then let's wait for it! 😄

@nemesifier
Copy link
Member

@pandafy got it, since radius 2 release is so close, then let's wait for it!

@atb00ker openwisp-radius 0.2 is out! 😁
https://github.com/openwisp/openwisp-radius/releases/tag/0.2.0

@@ -64,3 +64,22 @@
- name: Show OpenWisp log
debug:
var: openwisp_log

- name: Check Freeradius
# TODO: This test should work when openwisp-radius 0.3.0 is released!
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remains a blocker to merge this PR.
We would need to release the next version of openwisp-radius to enable testing here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we're getting near to that 👍

@atb00ker atb00ker force-pushed the openwisp_radius branch 4 times, most recently from 77a25a9 to 301e123 Compare April 28, 2021 16:53

- name: Inner tunnel
template:
src: freeradius/openwisp_site.j2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atb00ker isn't this wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was one of the causes of build failure,fixed it in the latest commit! 😄

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically ready!
OpenWISP RADIUS 0.3 milestone issues: https://github.com/openwisp/openwisp-radius/milestone/1

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an issue that prevents the group configurations (radius group check, radius group reply) from working when using registration with phone number because the plus sign gets escaped. This is breaking group authorization rules.

To fix it we need to add a configurable variable called openwisp2_freeradius_safe_characters defaulting to +@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /.

Then in the configuration of the SQL dialect in use has to be changed, I think we have to replace the line which contains safe_characters =, because it seems the default is different depending on the DB:

The value we put should be:

safe_characters = "{{ openwisp2_freeradius_safe_characters }}"

cron_delete_old_users: "'hour': 0, 'minute': 10"
cron_cleanup_stale_radacct: "'hour': 0, 'minute': 20"
cron_delete_old_postauth: "'hour': 0, 'minute': 30"
cron_delete_old_radacct: "'hour': 1, 'minute': 30"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should prefix these cron variables with openwisp2 for consistency, since it's configuration related to openwisp.

@nemesifier
Copy link
Member

I found an issue that prevents the group configurations (radius group check, radius group reply) from working when using registration with phone number because the plus sign gets escaped. This is breaking group authorization rules.

Commit 8bebc28 should fix it. I tested it. WIll keep an eye on the CI.

@nemesifier
Copy link
Member

I have merged this manually in the dev branch, I will keep this open until dev is ready to be recommended as a way to install OpenWISP RADIUS, something I'm working on with @pandafy.

@nemesifier
Copy link
Member

Merged manually in the dev branch, this can be closed, thank you very much @atb00ker! 👍 👍

@nemesifier nemesifier closed this Oct 8, 2021
@atb00ker
Copy link
Member Author

@nemesisdesign awesome, big win! 😄

@nemesifier nemesifier deleted the openwisp_radius branch October 18, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants