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

FreeRADIUS plugin #190

Merged
merged 223 commits into from
Jul 6, 2017
Merged

FreeRADIUS plugin #190

merged 223 commits into from
Jul 6, 2017

Conversation

mimugmail
Copy link
Member

Hi,

here's a first start for a new Freeradius3 plugin.
Building and installing the package works but it's not getting installed under services (need help @fichtner ).

At the moment it's more or less a copy of Quagga / General from @fabianfrz and only PoC-quality.
I'm gonna add clients.conf and users for basic configuration, but it would be nice to check functionality via UI.

Any hints for linking the package?

Thanks!

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

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

PLUGIN_NAME= freeradius3
PLUGIN_VERSION= 0.0.1
PLUGIN_COMMENT= Freeradius3
PLUGIN_DEPENDS= talloc python2 gdbm
Copy link
Member

Choose a reason for hiding this comment

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

without reading on, curious why the depends?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, these were the dependencies when installing freeradius, don't know if it's required here also

Copy link
Member

Choose a reason for hiding this comment

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

well, for one it should have "freeradius3", the rest is automatic...

Copy link
Member

Choose a reason for hiding this comment

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

(you're building package os-freeradius, which depends on package freeradius3)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok, fixed!

@@ -0,0 +1,7 @@
PLUGIN_NAME= freeradius3
Copy link
Member

Choose a reason for hiding this comment

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

probably prefer "freeradius", which might be some work...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can rename it in the code. Grabbed the naming from pfSense, perhaps some enthusiast wants to implement also Freeradius2. :)

@@ -0,0 +1,15 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

note to self: check against freebsd freeradius3 pkg-plist

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied it, is there some action required from my side?

Copy link
Member

Choose a reason for hiding this comment

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

no, I'll take care of this

@@ -0,0 +1,9 @@
FreeRADIUS includes a RADIUS server, a BSD licensed client library, a PAM library, and an Apache module. In most cases, the word FreeRADIUS refers to the RADIUS server.
Copy link
Member

Choose a reason for hiding this comment

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

Line breaks for 70ish characters please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@fichtner fichtner self-assigned this Jun 30, 2017
@@ -0,0 +1,5 @@
<menu>
<Routing cssClass="fa fa-map-signs" order="45">
Copy link
Member

Choose a reason for hiding this comment

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

<Services>

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, stupid copy+past :)

@mimugmail
Copy link
Member Author

I did some final newline fixes which resulted from copy+paste and added a button to "General" for VLAN assignment.

So for now it supports:

  • General Radius features for authentication (like any application which supports Radius e.g. OpenVPN)
  • WIFI WPA-Enterprise authentication
  • 802.1X switchport authentication and dynamic VLAN assignment

These should be the most common scenarios for Radius so I'd like to let the userbase test and ask them for their use cases to add their wishes.

Whats missing:

  • Usage of OPNsense created CA and certificates to allow EAP-TLS, but this is a bigger change and I'd first pull this one to master.

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

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

No static etc/ files yet? In general this is ready to merge so I approve, but several style notes and questions for your consideration. Great work again, thank you!

Let me know when this is ready from your perspective.

@@ -0,0 +1,7 @@
PLUGIN_NAME= freeradius
PLUGIN_VERSION= 0.3.0
Copy link
Member

Choose a reason for hiding this comment

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

just FYI: as long as the pull request is open you don't have to necessarily bump the version more than once. we also have PLUGIN_REVISION= which can be set to a numeric value for bugfixes, that's why most plugins only have X.Y, not X.Y.Z. But @fabianfrz likes three version numbers and that's perfectly fine here... only want to explain :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with both, let's keep it this way.

Copy link
Member

Choose a reason for hiding this comment

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

ok 👍

PLUGIN_NAME= freeradius
PLUGIN_VERSION= 0.3.0
PLUGIN_COMMENT= RADIUS Authentication, Authorization and Accounting Server
PLUGIN_DEPENDS= freeradius3
Copy link
Member

Choose a reason for hiding this comment

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

also FYI, freeradius3 has been bumped to 3.0.14 in the ports tree two days ago


if (isset($config['OPNsense']['freeradius']['general']['enabled']) && $config['OPNsense']['freeradius']['general']['enabled'] == 1) {
$services[] = array(
'description' => gettext('FreeRADIUS lets you run a local radius service instance.'),
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 this is too long, simply "FreeRADIUS" is ok as it is shown in the dashboard service widget.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

{
if ($this->request->isPost()) {
$backend = new Backend();
$response = $backend->configdRun("freeradius start", true);
Copy link
Member

Choose a reason for hiding this comment

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

why detach here? better not to even if it takes a few seconds

Copy link
Member Author

Choose a reason for hiding this comment

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

What exactly does that mean? You are speaking to a copy+paster :)

Copy link
Member

Choose a reason for hiding this comment

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

"true" means start the service asynchronously as a background job, the controller will return immediately but the service status may not be ready yet. if copy+paste, try without it.

Copy link
Member Author

Choose a reason for hiding this comment

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

cd plugins && fgrep configdRun -r * | grep start

returns always lines with "true" when there's start. Keep it so? :)

Copy link
Member

Choose a reason for hiding this comment

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

weird, but fine by me

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps some more pkg maintainers than me do just copy+paste :P

{
public function indexAction()
{
$this->view->title = gettext("FreeRADIUS-Clients");
Copy link
Member

Choose a reason for hiding this comment

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

No hyphen "-" in composite string

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

<id>user.description</id>
<label>Description</label>
<type>text</type>
<help>Surname, Lastname, or anything you need to describe.</help>
Copy link
Member

Choose a reason for hiding this comment

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

Surname, and "last name" are the same. Vorname is "given name" if you meant that. No capitalisation of works in English beyond the first word mostly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

<id>user.vlan</id>
<label>VLAN ID</label>
<type>text</type>
<help>VLAN ID the user get's in, e.g. for 802.1X, leave empty if you don't know what it is.</help>
Copy link
Member

Choose a reason for hiding this comment

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

"get's in" -> receives

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

<id>general.vlanassign</id>
<label>Enable VLAN assignment</label>
<type>checkbox</type>
<help>This allows you to dynamically assign VLANs on your physical switchports.</help>
Copy link
Member

Choose a reason for hiding this comment

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

"switch ports"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

<menu>
<Services>
<FreeRADIUS VisibleName="FreeRADIUS" cssClass="fa fa-address-book-o">
<General VisibleName="General" cssClass="fa fa-address-book-o fa-fw" url="/ui/freeradius/general/index" order="10"/>
Copy link
Member

Choose a reason for hiding this comment

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

No cssClass for sub-sub-nav :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,15 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: I will slightly change this after merge if it needs adjustment.

@@ -0,0 +1,9 @@
<menu>
<Services>
<FreeRADIUS VisibleName="FreeRADIUS" cssClass="fa fa-address-book-o">
Copy link
Member

Choose a reason for hiding this comment

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

PS: VisibleName is automatic if it matches the node name, so you don't need it at all here.

@mimugmail
Copy link
Member Author

@fichtner Is there something missing in Menu.xml since there's no icon displayed?

@fichtner
Copy link
Member

fichtner commented Jul 6, 2017

symbol missing from our font awesome version? I can take a look after merge if you want

@mimugmail
Copy link
Member Author

Possibly, yes.

For me it's mergeable now. Thanks for taking the time to review @fichtner and @fabianfrz 👍

@fichtner fichtner merged commit 1391330 into opnsense:master Jul 6, 2017
@fichtner
Copy link
Member

fichtner commented Jul 6, 2017

@mimugmail thanks again! 👍

@fichtner
Copy link
Member

fichtner commented Jul 6, 2017

@mimugmail we don't have font awesome 4.7 yet...

@fichtner
Copy link
Member

fichtner commented Jul 6, 2017

icon fixed via opnsense/core@92d1886

@rout3rx
Copy link

rout3rx commented Aug 6, 2017

is there anyone to told me how to install radius on opnsense as you develops?

@fichtner
Copy link
Member

fichtner commented Aug 6, 2017

it's easily available from the firmware: plugin menu in 17.7 :)

@rout3rx
Copy link

rout3rx commented Aug 6, 2017

thanks for your worthwhile effort.
i hope opnsense team tried to have : best reporting for every modules and Antivirus and Antispam for a complete UTM.

@fabianfrz
Copy link
Member

@rout3rx @mimugmail may be working on AV integration (c-icap/av_scan/clamav) but anti spam is currently not planned.

@rout3rx
Copy link

rout3rx commented Aug 6, 2017

sorry, excuse me i can ask a small question in this board? how can i apply bandwidth limit or user traffic limitation on users in freeradius?

@mimugmail
Copy link
Member Author

mimugmail commented Aug 6, 2017 via email

@rout3rx
Copy link

rout3rx commented Aug 6, 2017

when i want create share internet via PPTP or PPPoE connections, i want to apply some restriction like time of connection, bandwidth and traffic quota.

@fichtner
Copy link
Member

fichtner commented Aug 6, 2017

This requires accounting setup and such, we maybe getting a bit ahead of our schedule in providing one step after another in RADIUS integration.

But it's perfectly possible to set this up manually with the package (not the plugin).

@rout3rx
Copy link

rout3rx commented Aug 6, 2017

is it possible to add? or can i add more option in freeradius config file?

@mimugmail
Copy link
Member Author

Only when managed without the plugin.

Just do a pkg install freeradius3 via CLI and go ahead in /usr/local/etc/raddb

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

Successfully merging this pull request may close these issues.

4 participants