freeradius code style and other fixes #272

doktornotor · 2017-01-28T15:52:10Z

Code style/other fixes

Fix code style, whitespace, indentation
Make things much more readable by leaving out the middle part of the?: ternary operator
Replace exec() with mwexec()
Use native PHP functions where possible/suitable
Replace horrible code using echo redirection from shell with file_put_contents()
Use openvpn_create_dhparams() to create DH params file
Do not use code like exec("cd $foo; bar")
Use full paths to executables in mwexec()
Improve comments
Etc. ...

Others:

Add prominent deprecation warnings to 'Certificates' and 'EAP' tabs regarding the redundant FreeRADIUS Cert Manager feature. See https://redmine.pfsense.org/issues/7170.
Fix Bug #6928 - "Access-Reject" not logged in MySQL radpostauth table

…precation warning here See https://redmine.pfsense.org/issues/7170

…deprecation warning about FreeRADIUS Cert Manager here See https://redmine.pfsense.org/issues/7170

rbgarga · 2017-01-30T10:20:47Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+	}
+	if (file_exists(FREERADIUS_ETC . "/raddb/sites-enabled/inner-tunnel")) {
+		unlink(FREERADIUS_ETC . "/raddb/sites-enabled/inner-tunnel");
+	}


Replace these if statements by a simple call to unlink_if_exists() or @Unlink()

rbgarga · 2017-01-30T10:24:31Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

-// Deletes virtual-server coa by default. Will be re-enabled if there is an interface-type "coa"
-exec("rm -f " . FREERADIUS_ETC . "/raddb/sites-enabled/coa");
+	// Deletes virtual-server coa by default. Will be re-enabled if there is an interface-type "coa"
+	mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/sites-enabled/coa");


Use unlink_if_exists() instead of rm -f

rbgarga · 2017-01-30T10:28:06Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

 	// This is to fix the mysqlclient.so which gets lost after reboot
-	exec("ldconfig -m /usr/local/lib/mysql");
+	// XXX: Probably some PBI leftover, remove?
+	mwexec("/sbin/ldconfig -m /usr/local/lib/mysql");


I strongly believe this could be removed. MySQL package registers itself to run when /etc/rc.d/ldconfig is called by rc script

rbgarga · 2017-01-30T10:29:18Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc


-$arrusers = $config['installedpackages']['freeradius']['config'];
+		// Variables for users file defined parameters


Looks like this block is not correctly indented

rbgarga · 2017-01-30T10:30:34Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+		foreach ($arrmacs as $macs) {
+
+		// Variables for authorized_macs file defined parameters
+		$varmacsaddress = $macs['varmacsaddress'];


Not correctly indented

rbgarga · 2017-01-30T10:32:08Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/client.crt");
+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/client.key");
+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/client.pem");
+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/client.tar");


Use unlink_if_exists()

rbgarga · 2017-01-30T10:32:34Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+				mwexec("/usr/bin/tar -C " . FREERADIUS_ETC . "/raddb/certs -cf " . FREERADIUS_ETC . "/raddb/certs/client.tar client.crt client.csr client.key ca.der client.pem");
+
+				// Make all files in certs folder read/write only for root
+				mwexec("/bin/chmod -R 0600 " . FREERADIUS_ETC . "/raddb/certs/");


Use chmod()

rbgarga · 2017-01-30T10:32:49Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/index*");
+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/dh");
+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/random");
+				mwexec("/bin/rm -f " . FREERADIUS_ETC . "/raddb/certs/client.tar");


Use unlink_if_exists()

rbgarga · 2017-01-30T10:33:04Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+				// this command deletes the pfsense_cert_mgr checkfile so when we change back to pfsense cert manager a new DH + random file will be created
+				if (file_exists(FREERADIUS_ETC . "/raddb/certs/pfsense_cert_mgr")) {
+					unlink(FREERADIUS_ETC . "/raddb/certs/pfsense_cert_mgr");
+				}


Use unlink_if_exists()

rbgarga · 2017-01-30T10:33:18Z

net/pfSense-pkg-freeradius2/files/usr/local/pkg/freeradius.inc

+
+				// tar client-cert files
+				mwexec("/usr/bin/tar -C " . FREERADIUS_ETC . "/raddb/certs -cf " . FREERADIUS_ETC . "/raddb/certs/client.tar client.crt client.csr client.key ca.der client.pem");
+				mwexec("/bin/chmod -R 0600 " . FREERADIUS_ETC . "/raddb/certs/");


Use chmod()

doktornotor · 2017-01-30T10:58:08Z

As for the "use chmod()" - it needs to be recursive where chmod -R is being used, not really an option without complicating the code even further. Roger otherwise. Thanks for taking time to read this monster :)

rbgarga · 2017-01-30T11:10:58Z

@doktornotor it won't complicate too much because certs directory is supposed to contain only files and not subdirectories. You can do it like:

$certs = glob(FREERADIUS_ETC . "/raddb/certs/*");
array_walk($certs, function($cert_file) {
	chmod($cert_file, 0600);
});

doktornotor · 2017-01-30T11:13:10Z

I'll see what I can do with that. Other than that, unlink_if_exists() does handle globbing correctly, right? (So that all the rm -f junk can be replaced.)

rbgarga · 2017-01-30T11:14:13Z

Yes, it handles globbing. But anyway, -f is for force removal and not recursive option

doktornotor · 2017-01-30T11:17:37Z

Well yeah, but the (unreadable) stuff is terminated with * in bunch of places, such as this:
/var/log/radacct/datacounter/$varusersmaxtotaloctetstimerange/used-octets-$varusersusername*
(extremely easy to miss)

Another WTF:
Do you think the recursive chown on $frlib (line 128) really serves any useful purpose now? NFC about how it got there in the first place and why.

doktornotor · 2017-01-30T12:15:49Z

All done minus chmod(). The array_walk() thing does not work like this (syntax error, unexpected '}') and I'd really rather leave that thing alone, just makes the code unreadable.

rbgarga · 2017-01-30T13:07:59Z

@doktornotor I've tested array_walk before send it to you, it works. You probably had some copy/paste issue. It's a common PHP usage and also in pfSense you can see it being used at https://github.com/pfsense/pfsense/blob/master/src/usr/local/www/widgets/widgets/dyn_dns_status.widget.php#L59

doktornotor · 2017-01-30T13:22:31Z

Done. (Note: Pasting from browser can screw newlines and the PHP error is not helpful at all.) So we now have 8 lines of PHP instead of two simple ones. Oh well...

rbgarga · 2017-01-30T13:29:25Z

Thanks! It's better to have more PHP lines than spawning shell sessions to run system binaries

Confirmed working by a user in https://redmine.pfsense.org/issues/6928#note-5

doktornotor · 2017-01-30T14:36:01Z

Added a tiny patch for https://redmine.pfsense.org/issues/6928 (verified by a user) - and that's really all here unless some other changes are requested.

ChangeLog: [decof/du/tru64] Remove support because the os is no longer updated for more than 10 years [openstep/nextstep] Remove support because the os is no longer updated for more than 20 years Add experimental build system based on Autotools (#270) Fixed LTsock testing on darwin (#272) Remove NEW and OLD folders (#6) Fix FreeBSD testcases (#271) Rewrite documentation and publish at https://lsof.readthedocs.io/

2.2.1 2023-01-20 Detect tables used in the query of a PREPARE statement (#273) Expose recursive walk functionality via walk! (#268) Retain schema in name when parsing out functions (#272) https://github.com/pganalyze/pg_query/blob/main/CHANGELOG.md

doktornotor added 6 commits January 28, 2017 11:02

freeradius code style fixes - freeradius.inc (part 1/5)

3bd672c

freeradius code style fixes - freeradius.inc (part 2/5)

7afef1d

freeradius code style fixes - freeradius.inc (part 3/5)

f6e7b0e

freeradius code style fixes - freeradius.inc (part 4/5)

c4caee7

freeradius code style fixes - freeradius.inc (part 5/5)

aeb67c6

Bump port version

4220d12

netgate-git-updates added the CLA label Jan 28, 2017

doktornotor added 10 commits January 29, 2017 11:27

freeradius code style fixes - freeradiusauthorizedmacs.xml

3b07548

freeradius code style fixes - freeradiuscerts.xml; add a prominent de…

7c52f39

…precation warning here See https://redmine.pfsense.org/issues/7170

freeradius code style fixes - freeradiusclients.xml

5dccd50

freeradius code style fixes - freeradiuseapconf.xml, add a prominent …

1a41b58

…deprecation warning about FreeRADIUS Cert Manager here See https://redmine.pfsense.org/issues/7170

freeradius code style fixes - freeradiusinterfaces.xml

9ab4017

freeradius code style fixes - freeradiusmodulesldap.xml

4a9b6b7

freeradius code style fixes - freeradiussettings.xml

f4546fd

freeradius code style fixes - freeradiussqlconf.xml

d94f855

freeradius code style fixes - freeradius.xml

2aabea2

freeradius code style fixes - freeradius_view_config.php

47ed950

rbgarga requested changes Jan 30, 2017

View reviewed changes

rbgarga added the Changes requested label Jan 30, 2017

doktornotor added 2 commits January 30, 2017 12:42

Use unlink_if_exists() instead of "rm -f"; remove useless ldconfig call

7bb2712

Fix indentation

a4737dc

Use chmod() to set permissions

bb9b1ba

rbgarga removed the Changes requested label Jan 30, 2017

rbgarga approved these changes Jan 30, 2017

View reviewed changes

rbgarga requested a review from jim-p January 30, 2017 13:29

Fix Bug #6928 - "Access-Reject" not logged in MySQL radpostauth table

620ad3d

Confirmed working by a user in https://redmine.pfsense.org/issues/6928#note-5

netgate-git-updates merged commit 620ad3d into pfsense:devel Jan 31, 2017

netgate-git-updates pushed a commit that referenced this pull request Jan 31, 2017

Merge pull request #272 from doktornotor/patch-4

6e084bb

doktornotor deleted the patch-4 branch January 31, 2017 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

freeradius code style and other fixes #272

freeradius code style and other fixes #272

doktornotor commented Jan 28, 2017 •

edited

Loading

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

rbgarga Jan 30, 2017

doktornotor commented Jan 30, 2017 •

edited

Loading

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

doktornotor commented Jan 30, 2017

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017


		$arrusers = $config['installedpackages']['freeradius']['config'];
		// Variables for users file defined parameters

freeradius code style and other fixes #272

freeradius code style and other fixes #272

Conversation

doktornotor commented Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor commented Jan 30, 2017 • edited Loading

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

doktornotor commented Jan 30, 2017

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

rbgarga commented Jan 30, 2017

doktornotor commented Jan 30, 2017

doktornotor commented Jan 28, 2017 •

edited

Loading

doktornotor commented Jan 30, 2017 •

edited

Loading