IP Whitelist Check for Creating Credentials

[spiridonovpolytechnic]

Dear Benjamin, I have added a whitelist functionality for creating credentials. It is an additional (optional) layer of security and also allows a simple method of authentication (without logging in) by limiting creation of credentials to specific IP addresses. The summary of additions are as follows: config.php:

• added a boolean flag to check against whitelist and added a whitelist array for CIDR IP ranges

security.php:

• added a function to check an IP address against an array CIDR ranges

pw.php:

• added a check of client IP against whitelist if the whitelist flag is set
• navbar is now only displayed to whitelisted IP addresses
• null URL argument branch of the code is now only available to whitelisted IP addresses
• post URL argument branch of the code is now only available to whitelisted IP addresses

edit: cleaned up text formatting

[bemosior]

Thanks for contributing this! I'll take a look momentarily.

[bemosior]

Overall, this looks really good. I'd like to make two requests before merging this in:

1. We should default $checkCreatorIpWhitelist to false in config.ini (just like we do with $requireApacheAuth) so folks setting it up for the first time have fewer things that could be misconfigured.

2. I see what you were going for with conditionally displaying the nav menu, but I think it might be best to take advantage of the logic already present for handling Apache-based authentication for sake of consistency. When unauthenticated, the existing logic will display the nav, will not display the form, and will display an authentication error at the top. Try out the changes below and see if you agree with this approach. We could certainly make additional adjustments to improve the user-friendliness.

diff --git a/pwpusher_public/pw.php b/pwpusher_public/pw.php
index 7f88103..574f03f 100644
--- a/pwpusher_public/pw.php
+++ b/pwpusher_public/pw.php
@@ -27,11 +27,7 @@ print getHeader();

 //Print the navbar
 /** @noinspection PhpToStringImplementationInspection */
-if ($creatorIpOk)
-{
-    print getNavBar();
-}
-
+print getNavBar();

 //Find user arguments, if any.
 $arguments = getArguments();
@@ -50,7 +46,7 @@ if ($requireCASAuth) {
 }

 //If the form function argument doesn't exist, print the form for the user.
-if ($arguments['func'] == 'none' || $arguments == false && $creatorIpOk) {
+if ($arguments['func'] == 'none' || $arguments == false) {

     //Force CAS Authentication in order to load the form
     if ($requireCASAuth) {
@@ -64,7 +60,7 @@ if ($arguments['func'] == 'none' || $arguments == false && $creatorIpOk) {
         }

         //Fail Apache Authentication if configured but not successful
-    } elseif ($requireApacheAuth && empty($_SERVER['PHP_AUTH_USER'])) {
+    } elseif ($requireApacheAuth && empty($_SERVER['PHP_AUTH_USER']) || $checkCreatorIpWhitelist && !$creatorIpOk) {
         //This section is a courtesy check; PHP_AUTH_USER can possibly be spoofed
         //if web auth isn't configured.
         /** @noinspection PhpToStringImplementationInspection */
@@ -77,7 +73,7 @@ if ($arguments['func'] == 'none' || $arguments == false && $creatorIpOk) {
     //Get form elements
     print getFormElements();

-} elseif ($arguments['func'] == 'post' && $creatorIpOk) {
+} elseif ($arguments['func'] == 'post') {

     //Force CAS Authentication in order to post the form
     if ($requireCASAuth) {
@@ -88,7 +84,7 @@ if ($arguments['func'] == 'none' || $arguments == false && $creatorIpOk) {
             $_SERVER['PHP_AUTH_NAME'] = $attributes[$casSamlNameAttribute];
         }

-    } elseif ($requireApacheAuth && empty($_SERVER['PHP_AUTH_USER'])) {
+    } elseif ($requireApacheAuth && empty($_SERVER['PHP_AUTH_USER']) || $checkCreatorIpWhitelist && !$creatorIpOk) {
         //This section is a courtesy check; PHP_AUTH_USER can possibly be spoofed
         //if web auth isn't configured.
         /** @noinspection PhpToStringImplementationInspection */

[spiridonovpolytechnic]

Agreed on the default of having the whitelist disabled. The reason for my implementation is that there are people who want to have the simplified authentication theme of "your IP is your authentication. " This way everyone on the intranet (either physically or via VPN) can use the utility without having to authenticate and everyone outside can only retrieve. I'll review the changes at lunchtime or when I get home.

[bemosior]

Yes, understood! The changes I suggest should accomplish the same, just using pre-existing constructs that (I think) provide a slightly more consistent and user-friendly experience. No rush!