From d91c00e1b4123943d72248bbfde6aa3c2b72d049 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Thu, 6 Oct 2022 20:18:19 -0600 Subject: [PATCH 1/7] feat: implement login protection adds the enable_login_protection field to the api ui page and /api/v1/system/api. this field toggles enabling login protection for api authentication. this will help prevent brute force attacks against api endpoints --- .../files/etc/inc/api/framework/APIAuth.inc | 22 ++++++++++++++++++- .../etc/inc/api/models/APISystemAPIUpdate.inc | 10 +++++++++ .../local/www/api/documentation/openapi.yml | 3 +++ .../files/usr/local/www/api/index.php | 17 ++++++++++++++ 4 files changed, 51 insertions(+), 1 deletion(-) diff --git a/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc b/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc index 44885ee97..ba5569b68 100644 --- a/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc +++ b/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc @@ -118,8 +118,9 @@ class APIAuth { $resp = false; } - # Set our class is_authenticated attribute to our authentication resp and return the resp + # Set our object is_authenticated attribute to our authentication resp, log auth, and return the resp $this->is_authenticated = $resp; + $this->__log_authentication($this->is_authenticated); return $this->is_authenticated; } @@ -174,4 +175,23 @@ class APIAuth { return true; } } + + # Logs the authentication attempt if login protection is enabled for the API + private function __log_authentication($authenticated) { + # Log authentication attempts if enabled + if (isset($this->api_config["enable_login_protection"])) { + # Log successful authentication + if ($authenticated) { + log_auth( + gettext("Successful login for user '{$this->username}' from: {$this->ip_address} (Local Database)") + ); + } + # Log failed authentication + else { + log_auth( + gettext("webConfigurator authentication error for user '{$this->username}' from: {$this->ip_address}") + ); + } + } + } } diff --git a/pfSense-pkg-API/files/etc/inc/api/models/APISystemAPIUpdate.inc b/pfSense-pkg-API/files/etc/inc/api/models/APISystemAPIUpdate.inc index 921d36bf8..1078343da 100644 --- a/pfSense-pkg-API/files/etc/inc/api/models/APISystemAPIUpdate.inc +++ b/pfSense-pkg-API/files/etc/inc/api/models/APISystemAPIUpdate.inc @@ -67,6 +67,15 @@ class APISystemAPIUpdate extends APIModel { } } + private function __validate_enable_login_protection() { + # Check for our optional 'enable_login_protection' payload value + if ($this->initial_data['enable_login_protection'] === true) { + $this->validated_data["enable_login_protection"] = ""; + } elseif ($this->initial_data['enable_login_protection'] === false) { + unset($this->validated_data["enable_login_protection"]); + } + } + private function __validate_hasync() { # Check for our optional 'hasync' payload value if ($this->initial_data['hasync'] === true) { @@ -259,6 +268,7 @@ class APISystemAPIUpdate extends APIModel { $this->__validate_keybytes(); $this->__validate_custom_headers(); $this->__validate_access_list(); + $this->__validate_enable_login_protection(); $this->__validate_hasync(); $this->__validate_hasync_hosts(); $this->__validate_hasync_username(); diff --git a/pfSense-pkg-API/files/usr/local/www/api/documentation/openapi.yml b/pfSense-pkg-API/files/usr/local/www/api/documentation/openapi.yml index 92a52db2e..db26fc3c6 100644 --- a/pfSense-pkg-API/files/usr/local/www/api/documentation/openapi.yml +++ b/pfSense-pkg-API/files/usr/local/www/api/documentation/openapi.yml @@ -12073,6 +12073,9 @@ paths: will be disabled and you will no longer be able to API requests until the API enabled via the webConfigurator. type: boolean + enable_login_protection: + description: Enable or disable Login Protection for API authentication requests. + type: boolean hasync: description: Enable or disable HA sync for API configurations. type: boolean diff --git a/pfSense-pkg-API/files/usr/local/www/api/index.php b/pfSense-pkg-API/files/usr/local/www/api/index.php index 1499fbcb9..48d394202 100644 --- a/pfSense-pkg-API/files/usr/local/www/api/index.php +++ b/pfSense-pkg-API/files/usr/local/www/api/index.php @@ -158,6 +158,13 @@ $pkg_config["access_list"] = ""; } + # Validate login protection settings + if ($_POST["enable_login_protection"]) { + $pkg_config["enable_login_protection"] = ""; + } else { + unset($pkg_config["enable_login_protection"]); + } + # Validate HA Sync settings if enabled if (!empty($_POST["hasync"])) { $pkg_config["hasync"] = ""; @@ -319,6 +326,16 @@ ### Populate the ADVANCED section of the UI form $advanced_section->addClass("hide-api-advanced-settings"); +$advanced_section->addInput(new Form_Checkbox( + 'enable_login_protection', + 'Login Protection', + 'Enable API Login Protection', + isset($pkg_config["enable_login_protection"]) +))->setHelp( + "Include API authentication in pfSense's Login Protection feature. When enabled, all API authentication requests + will be logged and monitored for brute force attacks. Login Protection can be configured in + System > Advanced" +); $advanced_section->addInput(new Form_Checkbox( 'hasync', 'Sync API Configuration', From ba5689ef4e65536cca77d396409b46f5ffc24dff Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Thu, 6 Oct 2022 22:19:29 -0600 Subject: [PATCH 2/7] fix: check if enable_login_protection is empty on the api ui page, we were evaluating a base variable which erroneously set or unset the variable --- pfSense-pkg-API/files/usr/local/www/api/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pfSense-pkg-API/files/usr/local/www/api/index.php b/pfSense-pkg-API/files/usr/local/www/api/index.php index 48d394202..e4607071a 100644 --- a/pfSense-pkg-API/files/usr/local/www/api/index.php +++ b/pfSense-pkg-API/files/usr/local/www/api/index.php @@ -159,7 +159,7 @@ } # Validate login protection settings - if ($_POST["enable_login_protection"]) { + if (!empty($_POST["enable_login_protection"])) { $pkg_config["enable_login_protection"] = ""; } else { unset($pkg_config["enable_login_protection"]); From dce751e782dd93ac36430329c074c1699d9d8b97 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Thu, 6 Oct 2022 22:20:28 -0600 Subject: [PATCH 3/7] fix: default auth logs username login protection requires the username to be set in order to work. this sets the default username to 'unknown' if no username could be found --- pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc b/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc index ba5569b68..a4c5f7bbd 100644 --- a/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc +++ b/pfSense-pkg-API/files/etc/inc/api/framework/APIAuth.inc @@ -178,18 +178,22 @@ class APIAuth { # Logs the authentication attempt if login protection is enabled for the API private function __log_authentication($authenticated) { + # Variables + $username = ($this->username) ?: "unknown"; + $ip_address = $this->ip_address; + # Log authentication attempts if enabled if (isset($this->api_config["enable_login_protection"])) { # Log successful authentication if ($authenticated) { log_auth( - gettext("Successful login for user '{$this->username}' from: {$this->ip_address} (Local Database)") + gettext("Successful login for user '{$username}' from: {$ip_address} (Local Database)") ); } # Log failed authentication else { log_auth( - gettext("webConfigurator authentication error for user '{$this->username}' from: {$this->ip_address}") + gettext("webConfigurator authentication error for user '{$username}' from: {$ip_address}") ); } } From b9dc95b5055b25d34886875bef9bfe9914809233 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Fri, 7 Oct 2022 01:03:04 -0600 Subject: [PATCH 4/7] chore: enable login protection by default --- pfSense-pkg-API/files/usr/local/share/pfSense-pkg-API/info.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/pfSense-pkg-API/files/usr/local/share/pfSense-pkg-API/info.xml b/pfSense-pkg-API/files/usr/local/share/pfSense-pkg-API/info.xml index 1128b031e..6ed49468e 100644 --- a/pfSense-pkg-API/files/usr/local/share/pfSense-pkg-API/info.xml +++ b/pfSense-pkg-API/files/usr/local/share/pfSense-pkg-API/info.xml @@ -20,6 +20,7 @@ sha256 16 + \ No newline at end of file From 4d324358870f26daa43c4433b2c4741b70f294b3 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Fri, 7 Oct 2022 01:38:52 -0600 Subject: [PATCH 5/7] tests: added login protection test --- tests/e2e_test_framework/__init__.py | 4 +++ tests/test_login_protection.py | 39 ++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 tests/test_login_protection.py diff --git a/tests/e2e_test_framework/__init__.py b/tests/e2e_test_framework/__init__.py index 66f38c48b..eae0ca7e9 100644 --- a/tests/e2e_test_framework/__init__.py +++ b/tests/e2e_test_framework/__init__.py @@ -54,6 +54,7 @@ def __init__(self): self.get() self.put() self.delete() + self.custom_tests() sys.exit(self.exit_code) except KeyboardInterrupt: sys.exit(1) @@ -106,6 +107,9 @@ def delete(self): if test_params.get("status", 200) == 200: time.sleep(self.time_delay) + def custom_tests(self): + """Allows child classes to specify custom tests. This is inteded to be overwritten by the child class.""" + # PRE/POST REQUEST METHODS. These are intended to be overwritten by a child class. def pre_post(self): """ diff --git a/tests/test_login_protection.py b/tests/test_login_protection.py new file mode 100644 index 000000000..f11013f21 --- /dev/null +++ b/tests/test_login_protection.py @@ -0,0 +1,39 @@ +"""Script used to test the login protection API integration""" +import e2e_test_framework +import requests +import sys + + +class APIE2ETestLoginProtection(e2e_test_framework.APIE2ETest): + """Class used to test the login protection API integration.""" + def custom_tests(self): + self.test_login_protection() + + def test_login_protection(self): + """Custom test method to ensure login protection locks out too many failed auth attempts.""" + # Variables + test_params = {"name": "Ensure login protection blocks many failed auth attempts"} + + # Fail authentication 3 times to initiate the lockout + for _ in range(0, 3): + requests.get( + self.format_url("/api/v1/system/api"), + auth=("bad_username", "bad_password"), + verify=False + ) + + # Try another request and ensure it times out as expected + try: + requests.get( + self.format_url("/api/v1/system/api"), + auth=("bad_username", "bad_password"), + verify=False, + timeout=(5, 5) + ) + print(self.__format_msg__("GET", test_params, "Expected lockout for too many failed requests")) + sys.exit(1) + except (requests.exceptions.ReadTimeout, requests.exceptions.ConnectTimeout): + print(self.__format_msg__("GET", test_params, "Response is valid", mode="ok")) + + +APIE2ETestLoginProtection() From 82d479a2bbefd0feade3489b989eea462d7ced39 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Fri, 7 Oct 2022 01:40:41 -0600 Subject: [PATCH 6/7] lint: fixed import order for tests --- tests/test_login_protection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_login_protection.py b/tests/test_login_protection.py index f11013f21..c0a90eb6b 100644 --- a/tests/test_login_protection.py +++ b/tests/test_login_protection.py @@ -1,7 +1,7 @@ """Script used to test the login protection API integration""" -import e2e_test_framework -import requests import sys +import requests +import e2e_test_framework class APIE2ETestLoginProtection(e2e_test_framework.APIE2ETest): From d3b28da21e83af9089ff46e331af8aab46e4301b Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Fri, 7 Oct 2022 08:25:25 -0600 Subject: [PATCH 7/7] tests: just expect login protection timeout before the test_login_protection test expected requests to start failing at a specific point, but this does not work with all login protection configs and situations. it now just expects the requests to start to timeout after many failed auth attempts --- tests/test_login_protection.py | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/tests/test_login_protection.py b/tests/test_login_protection.py index c0a90eb6b..dee48882d 100644 --- a/tests/test_login_protection.py +++ b/tests/test_login_protection.py @@ -14,26 +14,22 @@ def test_login_protection(self): # Variables test_params = {"name": "Ensure login protection blocks many failed auth attempts"} - # Fail authentication 3 times to initiate the lockout - for _ in range(0, 3): - requests.get( - self.format_url("/api/v1/system/api"), - auth=("bad_username", "bad_password"), - verify=False - ) + # Fail authentication many times to initiate the lockout + for _ in range(0, 5): + try: + requests.get( + self.format_url("/api/v1/system/api"), + auth=("bad_username", "bad_password"), + verify=False, + timeout=(5, 5) + ) + except (requests.exceptions.ReadTimeout, requests.exceptions.ConnectTimeout): + print(self.__format_msg__("GET", test_params, "Response is valid", mode="ok")) + sys.exit(0) - # Try another request and ensure it times out as expected - try: - requests.get( - self.format_url("/api/v1/system/api"), - auth=("bad_username", "bad_password"), - verify=False, - timeout=(5, 5) - ) - print(self.__format_msg__("GET", test_params, "Expected lockout for too many failed requests")) - sys.exit(1) - except (requests.exceptions.ReadTimeout, requests.exceptions.ConnectTimeout): - print(self.__format_msg__("GET", test_params, "Response is valid", mode="ok")) + # Test fails if we were not locked out + print(self.__format_msg__("GET", test_params, "Expected lockout for too many failed requests")) + sys.exit(1) APIE2ETestLoginProtection()