vHosts Package #369

sthames42 · 2017-07-01T00:04:45Z

This is a port of the vhosts package from pre-2.3. It now uses nginx.
I have been using it on my production router for months running 2.3.3-P1.
This is my first PR so any input is welcome.

…ports into vhosts-package Rebasing from devel.

rbgarga · 2017-07-28T17:18:26Z

www/pfSense-pkg-vHosts/Makefile

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#*******************************************************************************


Makefile doesn't need to have all this license text, usually we keep them as clean as FreeBSD's ones

I don't want to be difficult but there is nothing in the style guide about makefiles.

rbgarga · 2017-07-28T17:18:35Z

www/pfSense-pkg-vHosts/Makefile

+
+PORTNAME=	pfSense-pkg-vHosts
+PORTVERSION=	1.0.0
+#PORTREVISION=	1


Remove commented out line

The purpose of keeping the line in the file is that, while there is no need for a revision now, there may be in the future. Setting PORTREVISION=0 produces a version string of "1.0.0_0" which is not appropriate. This simply makes it easier to add a revision number without having to try and remember PORTREVISION is the variable to set.

What's more, there is commented out code all through the pfSense source code. What is your objection to this?

rbgarga · 2017-07-28T17:19:26Z

www/pfSense-pkg-vHosts/Makefile

+
+do-install:
+	${MKDIR} ${STAGEDIR}/etc/inc/priv
+	${MKDIR} ${STAGEDIR}${PREFIX}/pkg


This line can be removed, next line will create /pkg subdirectory since ${MKDIR} uses -p parameter

Ok, but the development that comes behind me will probably not know that. I didn't. So what is the harm in having both lines?

rbgarga · 2017-07-28T17:20:18Z

www/pfSense-pkg-vHosts/Makefile

+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/pkg/shortcuts/vhosts.inc      ${STAGEDIR}${PREFIX}/pkg/shortcuts
+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/www/packages/vhosts.php       ${STAGEDIR}${PREFIX}/www/packages
+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/www/packages/vhosts_certs.php ${STAGEDIR}${PREFIX}/www/packages
+	${INSTALL_DATA} ${FILESDIR}${DATADIR}/info.xml                     ${STAGEDIR}${DATADIR}


Avoid using more than 80 columns, use a \ to split command in multiline

The advent of 80 column lines in code is a holdover of punch cards which were how programs were written up through the 1970's. When cards gave way to monitors, the early monitors were limited to 80 column displays without scrolling.
What on earth is the purpose of limiting lines to 80 columns now? It certainly does not make things more readable.

rbgarga · 2017-07-28T17:20:50Z

www/pfSense-pkg-vHosts/Makefile

+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/pkg/vhosts.inc                ${STAGEDIR}${PREFIX}/pkg
+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/pkg/shortcuts/vhosts.inc      ${STAGEDIR}${PREFIX}/pkg/shortcuts
+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/www/packages/vhosts.php       ${STAGEDIR}${PREFIX}/www/packages
+	${INSTALL_DATA} ${FILESDIR}${PREFIX}/www/packages/vhosts_certs.php ${STAGEDIR}${PREFIX}/www/packages


IMO these files should be installed in www/ instead of www/packages

A number of packages store their files in www/packages. Since there is no documentation on the structure of the package environment, I used pfSense-pkg-Cron as my example. I would be happy to make this change later but now the application is working and I hesitate to change it. Can we wait until the package is accepted into the repository so at least it can be installed from the UI instead of manually from the command line?

rbgarga · 2017-07-28T17:22:00Z

www/pfSense-pkg-vHosts/README.md

@@ -0,0 +1,58 @@
+# vHosts


Please do not add other files like this in root port directory. This information can be moved as a comment in the source code

Ok, this one has me baffled. README.md is not included in the build and only serves as a help page for someone that might want to have a look at some documentation. The help page facility for packages is broken which is why there is no help for many packages. In the future, I'm hoping to fix that and use this file to create the actual help page. In the meantime, what is the objection to putting this file in the root given it's not included in the build?

sthames42 · 2017-07-29T21:26:49Z

I have redone a lot of my code to adhere to the pfSense Developer Style Guide. I have no great objection to that as I understand the need for standards. But none of the changes requested are mentioned in the style guide and none of them have anything to do with the writing or function of the code.

I love pfSense. It's the best open-source router software I've seen. I've used it for many years and believe I have something to contribute. This package is very useful and is only an example of what I can do. But if you don't want me involved because I insist on putting in README files or refuse to break lines at 80 columns, I guess that's up to you.

I apologize for my tone. I worked on this package very hard and, given the total lack of documentation for package development, thought I did a damn good job. The nature of your change requests caught me off guard.

rbgarga · 2017-07-31T16:10:28Z

@sthames42 I understand. For FreeBSD-ports repository we try to follow upstream rules to make softwares used to validate ports (e.g. portlint) happy. On FreeBSD ports there are such rules related to what I've asked you to change.

You can read more about FreeBSD Ports standards in the FreeBSD Porters Handbook [1] and also if you want to validate the port before submit, you can install portlint on a FreeBSD system and run portlint -CN inside the root directory of the port you are developing. Additionally you can run a make -DNO_DEPENDS check-plist to make sure pkg-plist is correct.

Let me know if you have other questions regarding ports infrastructure. I'll be glad to help.

[1] https://www.freebsd.org/doc/en/books/porters-handbook/index.html

sthames42 · 2017-07-31T22:10:58Z

@rbgarga thank you for responding so quickly.

I did know about portlint and the package conforms with no fatals. There are a few warnings:

Makefile: new ports should not set PORTREVISION.
pkg-install: possible use of absolute pathname "/usr/local/bin/php".
pkg-install: possible direct use of "/usr/local" found. if so, use ${PREFIX} or ${LOCALBASE}, as appropriate.
pkg-deinstall: possible use of absolute pathname "/usr/local/bin/php".
pkg-deinstall: possible direct use of "/usr/local" found. if so, use ${PREFIX} or ${LOCALBASE}, as appropriate.

I uncommented PORTREVISION in the Makefile and set it to 0. The handbook says (and I tested it to be sure) this will not result in a version string of "1.0.0_0" as I feared. Leaving it at zero is a clear reminder to bump it when a change is made. Removing it would result in this step being forgotten. Since it's only a warning and it will only show up as long as the port is new, is there any real harm in leaving it in?

I assume correction of warnings is not mandatory especially given that none of the other packages I looked at seem concerned with these other errors. Also, the handbook says clearly not to blindly follow portlint.

In make -DNO_DEPENDS check-plist, I'm getting this:

Error: Orphaned: @dir /etc/inc/priv
Error: Orphaned: @dir /etc/inc

I can find no reference to this error. The handbook says to use @dir when you need to create empty folders so they can be cleaned up by deinstall but I can find no reason for the error. Any suggestions?

I have removed the extraneous comments from the Makefile but I find nothing regarding a line length limit anywhere in the porter's handbook or anywhere else in FreeBSD standards for that matter. Unless there is a compelling reason, I tend not be put continuation lines in makefiles because if a following space is accidentally left in, the results are unpredictable. I have run in to this with makefiles many times.

The handbook does mention readme files in several places but there is nothing about NOT including one. Nor is there anything regarding extra files in the root folder. Readme.md files are standard in every other project I've seen on Github. I did remove the file "notes.md" which was simply notes to myself, anyway.

After review of most other packages, I moved the files in www/packages to www as you requested.

sthames42 · 2017-07-31T22:11:25Z

Sorry. Hit the wrong button.

jim-p

Mostly minor security-based fixes needed, but overall looks OK.

The only part I really do not like is that it redundantly and unnecessarily reimplements parts of the Certificate Manager. That is not needed, and the page should be removed. If this page has a feature that is not in the Certificate Manager, it needs to be put into the base system Certificate Manager and not as a part of a package.

We are finally getting to the point where almost, if not all, of the packages are tied back into the Certificate Manager rather than having their own certificate code, I don't want to take a step backward here.

jim-p · 2017-08-01T13:04:50Z

www/pfSense-pkg-vHosts/files/usr/local/pkg/vhosts.inc

+        ssl_session_timeout                     10m;
+        keepalive_timeout                       70;
+        ssl_session_cache                       shared:SSL:10m;
+        ssl_protocols                           TLSv1 TLSv1.1 TLSv1.2;


Consider making TLSv1 an option that defaults to off. If not now, in a future revision.

This configuration is taken directly from the nginx config for the WebConfigurator. Your suggestion is reasonable and I would even suggest the list of protocols supported might be selectable by the user in the future. However, I would hesitate to give this tool enough facility such that users in the future might consider using it as a robust, web application server. IMHO, a router is the wrong place for this.

True, but by that logic TLSv1 should be off by default here. If they need to support older/insecure clients they can use a proper web server.

I disagree. The primary purpose of this tool was to provide a way to put up a server down page for a server behind the router undergoing maintenance. By turning off TLSv1, I would be restricting that server to TLSv1.1 or higher. That is not my decision to make. But to disable it in the future, with an option to enable, is certainly reasonable.

It could go either way is my point. Given the choice, since it's running on a firewall I always err on the side of security and safety.

jim-p · 2017-08-01T13:07:27Z

www/pfSense-pkg-vHosts/files/usr/local/pkg/vhosts.inc

+	/* Remove all existing configurations. */
+	/*-------------------------------------*/
+	conf_mount_rw();
+	exec("/bin/rm -f $conf_file_prefix*");


I know it's one you set manually as a global, but please make sure $conf_file_prefix contains a sane value before passing it to things like rm -f with a glob. Also, when using variables inside strings, surround the variable with braces to make the separation clear, e.g. /bin/rm -f {$conf_file_prefix}* -- Otherwise, especially in cases like this, it is easy to overlook the *.

In this specific instance, using our unlink_if_exists() function would be best, it can accept a glob as a parameter and doesn't require a shell exec.

This is a good suggestion. I'm changing to unlink_if_exists() now. I don't pollute my code with sanity checks for the developer (generally me). If I am stupid enough to leave a constant value empty, the result should be damaging enough so I know clearly it has happened and learn well from the mistake. As a developer, I don't want to be protected from my own stupidity. It creates better discipline. Something I have to relearn whenever I go back to working in scripted rather than compiled languages.

Sanity checks are almost never "pollution". It's safer, especially when making changes via glob, to make sure the code cannot possibly destroy more than intended. It's not about how intelligent or careful a developer is. Accidents happen. Ounce of prevention is worth a pound of cure.

How should this error be reported? What is the mechanism for a global error like this?

That is not as important as simply skipping the unlink/rm if the variable is empty.

You could use file_notice() or log_error() to produce an error the user can see if you want, but it's not about warning it's about stopping a coding problem from wiping a bunch of files from the disk unintentionally.

Easy, killer. I will put in a check that throws a 500 error in this case. I didn't miss your point but I think you missed mine. Suppose there was a package that would leave a hole in the firewall if a group of files was not removed and the file path was provided by a constant. Would you have the same suggestion? Just skip the 'rm' if the value is not set?

I didn't miss your point. It wasn't valid. Nothing you've said can justify deliberately omitting a safety check and acting accordingly. Making it throw a 500 error is excessive, given the context. But if you feel that's necessary, go ahead. I'd still rather it never delete files even by accident.

My point wasn't valid? Wow. I wouldn't have said that do you, at all. I thought we were having a difference of opinion. I've reconsidered, anyway. There is no point in the check. If the constant is not set, the package will not work, at all.

I'm getting you're a "my way or the highway" guy, Jim, and clearly you get nasty when someone doesn't agree with you. I don't treat people like that and won't be part of it.

I'd rather not skirt the issue and pretend it was a discussion. I'll tell you straight and won't BS you about it. It's not disrespect, it's the opposite.

I'm only a "my way or the highway" type when it comes to safety and security. On those points, there is no room for debate. Being a security product, we have to be strict in certain places.

Leaving out a security check because you believe it's safe to is exactly the time you need them. That's the point of them. To protect against the unexpected.

If you'd used constants, you might have a point. But you used globals/variables which do not behave like constants.

Jim, from your lack of civility, I assumed you were a kid with no respect for those of us that have been here from the beginning. Looks like you made the same assumption about me. Following your last insults, I was pissed and stomped off like a child so nobody's behavior was sterling. But I know who you are, now. We're both long in the tooth in this business and would probably have a lot to talk about. You're wrong Jim. You're arguments are idiotic. Oh, I'll make this stupid change "full stop" because you're the gatekeeper and that appears matter to you. But I'd love it if you could give me a single example of how, without this "sanity check", there would be any danger to the router anywhere but on the test bench.

jim-p · 2017-08-01T13:12:30Z

www/pfSense-pkg-vHosts/files/usr/local/pkg/vhosts.inc

+		safe_mkdir($root_folder);
+
+		if (!file_exists("$root_folder/index.php")) {
+			file_put_contents("$root_folder/index.php", "<?php\necho phpinfo();\n?>\n");


Putting phpinfo in the default index page seems a little risky if someone has this open to the world. Maybe just use a simple "Hello world" or "It works!" style page.

This was a holdover from the original package. I like it because it lets me know PHP is working on that server. But, again, this is a case of protecting the IT director from himself. This page lets him know the server is up. He should absolutely disable the vhost until valid index page is implemented. (He/him are general pronouns, here, and not intended to be gender specific. :))

Knowing that PHP is working is a fine goal. The same effect can be achieved with something like <?php echo "Hello World!"; ?> for example without the risk of exposing the wealth of info that you get from phpinfo(), however.

Making this change.

jim-p · 2017-08-01T13:17:22Z

www/pfSense-pkg-vHosts/files/usr/local/pkg/vhosts.inc

+*******************************************************************************/
+function vhosts_pre_deinstall() {
+	global $vhosts_g;
+	exec("/bin/rm -f {$vhosts_g['conf_file_prefix']}*");


As mentioned previously, test to make sure the value is sane and use unlink_if_exists() here instead of a shell exec.

jim-p · 2017-08-01T13:25:28Z

www/pfSense-pkg-vHosts/files/usr/local/www/vhosts.php

+*******************************************************************************/
+$a_vhosts = &$config['installedpackages']['vhosts']['config'];
+$act      = $_GET['act'];
+$id       = $_GET['id'];


Make sure this $id value is sane (e.g. test against is_numericint()) or unset it here. The way it gets used later which could be a potential XSS vector.

jim-p · 2017-08-01T13:39:44Z