From 4efca2d681708b624de6aa668156b23e669726c7 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Thu, 5 May 2022 20:04:12 -0600 Subject: [PATCH] Fixed issue that prevented the /api/v1/system/package endpoint from successfully correctly installing pfSense packages. --- .../etc/inc/api/framework/APIResponse.inc | 16 +- .../inc/api/models/APISystemPackageCreate.inc | 154 +++--------------- .../inc/api/models/APISystemPackageDelete.inc | 13 +- .../local/www/api/documentation/openapi.yml | 16 +- tests/test_api_v1_system_package.py | 77 +-------- 5 files changed, 28 insertions(+), 248 deletions(-) diff --git a/pfSense-pkg-API/files/etc/inc/api/framework/APIResponse.inc b/pfSense-pkg-API/files/etc/inc/api/framework/APIResponse.inc index 5f077b51a..98dc613a3 100644 --- a/pfSense-pkg-API/files/etc/inc/api/framework/APIResponse.inc +++ b/pfSense-pkg-API/files/etc/inc/api/framework/APIResponse.inc @@ -569,21 +569,9 @@ function get($id, $data=[], $all=false) { ], 1077 => [ "status" => "bad request", - "code" => 400, - "return" => $id, - "message" => "Remote URL does not contain a pfSense package" - ], - 1078 => [ - "status" => "bad request", - "code" => 504, - "return" => $id, - "message" => "System package installation exceeded timeout" - ], - 1079 => [ - "status" => "bad request", - "code" => 400, + "code" => 500, "return" => $id, - "message" => "System package installation timeout cannot be greater than 120 seconds" + "message" => "System package failed to install" ], 1080 => [ "status" => "bad request", diff --git a/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageCreate.inc b/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageCreate.inc index b1712516d..fac55cc2b 100644 --- a/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageCreate.inc +++ b/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageCreate.inc @@ -18,8 +18,6 @@ require_once("api/framework/APIResponse.inc"); class APISystemPackageCreate extends APIModel { - public $pkg_mode; - # Create our method constructor public function __construct() { parent::__construct(); @@ -28,32 +26,35 @@ class APISystemPackageCreate extends APIModel { } public function action() { - return APIResponse\get($this->install_pkg()); + # Force the action to be logged in the configuration history if our API response code is success + if (pkg_install($this->validated_data["name"])) { + return APIResponse\get(0); + } else { + return APIResponse\get(1077); + } } public function validate_payload(){ - $this->__validate_force(); // Must run before __validate_name $this->__validate_name(); - $this->__validate_timeout(); } - # Attempts to install the package specified in $this->validated_data["name"] - public function install_pkg() { - # Format and execute our pkg command. Enforce a timeout to prevent gateway timeouts. - $pkg_force = ($this->validated_data["force"]) ? " -f" : ""; - $pkg_y = ($this->pkg_mode === "install") ? " -y" : ""; - $pkg_cmd = "pkg ".$this->pkg_mode.$pkg_y.$pkg_force." ".$this->validated_data["name"]." 2>&1"; - exec("timeout ".$this->validated_data["timeout"]." ".$pkg_cmd, $pkg_out, $pkg_rc); - - # Check for known errors - $api_rc = $this->__check_pkg_install($pkg_out, $pkg_rc); - - # Force the action to be logged in the configuration history if our API response code is success - if ($api_rc === 0) { - $this->write_config(); + private function __validate_name() { + # Check for our required name input + if (isset($this->initial_data["name"])) { + # Ensure this package exists in pfSense's repos + if ($this->is_pkg_in_repo($this->initial_data["name"])) { + # Ensure package is not already installed + if (!is_pkg_installed($this->initial_data["name"])) { + $this->validated_data["name"] = $this->initial_data["name"]; + } else { + $this->errors[] = APIResponse\get(1076); + } + } else { + $this->errors[] = APIResponse\get(1075); + } + } else { + $this->errors[] = APIResponse\get(1073); } - - return $api_rc; } # Checks pfSense's upstream package repo for a package by name. Requires upstream internet connection. @@ -74,115 +75,4 @@ class APISystemPackageCreate extends APIModel { return false; } - - private function __validate_name() { - # Check for our required name input - if (isset($this->initial_data["name"])) { - # Check if this is a remote/third-party package to be installed by URL - if (filter_var($this->initial_data["name"], FILTER_VALIDATE_URL)) { - # Set pkg add to install package from URL - $this->pkg_mode = "add"; - $this->validated_data["name"] = filter_var($this->initial_data["name"], FILTER_VALIDATE_URL); - } - # Otherwise, we will assume this package exists in pfSense's package repos - else { - # Set pkg install to install the package from pfSense's repos - $this->pkg_mode = "install"; - - # Ensure this package exists in pfSense's repos - if ($this->is_pkg_in_repo($this->initial_data["name"])) { - # Ensure package is not already installed - if (!is_pkg_installed($this->initial_data["name"]) or $this->validated_data["force"]) { - $this->validated_data["name"] = $this->initial_data["name"]; - } else { - $this->errors[] = APIResponse\get(1076); - } - } else { - $this->errors[] = APIResponse\get(1075); - } - - } - } else { - $this->errors[] = APIResponse\get(1073); - } - } - - private function __validate_force() { - # Check for our optional force input - if ($this->initial_data["force"] === true) { - $this->validated_data["force"] = true; - } - } - - private function __validate_timeout() { - # Check for our optional timeout input - if (isset($this->initial_data["timeout"])) { - # Require timeout value to be 120 seconds or less - if (is_numeric($this->initial_data["timeout"]) and intval($this->initial_data["timeout"]) <= 120) { - # Force timeouts less than 5 to minimum of 5 seconds - if (intval($this->initial_data["timeout"]) < 5) { - $this->initial_data["timeout"] = 5; - } - - $this->validated_data["timeout"] = intval($this->initial_data["timeout"]); - } else { - $this->errors[] = APIResponse\get(1079); - } - } else { - $this->validated_data["timeout"] = 90; - } - } - - # This function is intended to take the output of our pkg add or install command and check for failures. - # Returns the corresponding API response ID. - # TODO: matching text based error messages is prone to breaking as the pkg cli tool changes. As of now, pkg does - # TODO: not return unique return codes for specific errors. Re-evaluate as time goes and refactor when a better way - # TODO: is made available. - private function __check_pkg_install($pkg_out, $pkg_rc) { - # Check if our package installation timed out - if ($pkg_rc === 124) { - return 1078; - } - - # Loop through each line of the pkg output and check for known error messages - foreach ($pkg_out as $pkg_line) { - # Check for 'pkg install' no matching package in repository error - if (APITools\str_starts_with("pkg: No packages available to install matching", $pkg_line)) { - return 1075; - } - # Check for 'pkg install' most recent version is already installed error - elseif (APITools\str_starts_with("The most recent versions of packages are already installed", $pkg_line)) { - return 1076; - } - # Check for 'pkg add' most recent version is already installed error - elseif (APITools\str_starts_with("the most recent version of", $pkg_line)) { - return 1076; - } - # Check for 'pkg add' no package file found at URL error - elseif (APITools\str_ends_with(": Not Found", $pkg_line)) { - return 1075; - } - # Check for 'pkg add' URL does not contain package file error - elseif (APITools\str_ends_with(": Unrecognized archive format", $pkg_line)) { - return 1077; - } - # Check for 'pkg add' DNS resolution error - elseif (APITools\str_ends_with(": No address record", $pkg_line)) { - return 13; - } - # Check for 'pkg add' URL unreachable error - elseif (APITools\str_ends_with(": Network is unreachable", $pkg_line)) { - return 13; - } - } - - # When no known error messages were matched, but the pkg command still failed, return unexpected error - if ($pkg_rc !== 0) { - return 1; - } - # Otherwise, our package installation appears to be successful - else { - return 0; - } - } } diff --git a/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageDelete.inc b/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageDelete.inc index 7ec48f9b0..f6e42d956 100644 --- a/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageDelete.inc +++ b/pfSense-pkg-API/files/etc/inc/api/models/APISystemPackageDelete.inc @@ -26,7 +26,8 @@ class APISystemPackageDelete extends APIModel { } public function action() { - $this->delete_pkg(); + uninstall_package($this->validated_data["name"]); + $this->write_config(); return APIResponse\get(0); } @@ -34,16 +35,6 @@ class APISystemPackageDelete extends APIModel { $this->__validate_name(); } - # Deletes the package specified in $this->validated_data["name"] - public function delete_pkg() { - # Remove the requested package and clean up dependencies afterwards - pkg_call("delete -y " . $this->validated_data["name"]); - pkg_call("autoremove -y"); - - # Force the action to be logged in the configuration history - $this->write_config(); - } - private function __validate_name() { # Check for our required name input if (isset($this->initial_data["name"])) { 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 6c3d37b8c..f36b110c8 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 @@ -8470,24 +8470,10 @@ paths: application/json: schema: properties: - force: - default: false - description: Force the installation of the package. Forced installs - will always force the package to re-install and bypasses certain - warnings. Use extreme caution when using forced installs. - type: boolean name: description: Name of pfSense package to install. This must be the - pfSense package internal name including the `pfSense-pkg-` prefix - or a URL to an external package. + pfSense package internal name including the `pfSense-pkg-` prefix. type: string - timeout: - default: 90 - description: Amount of time (in seconds) to allow the package installation - to take before timing out. - maximum: 120 - minimum: 5 - type: integer required: - name type: object diff --git a/tests/test_api_v1_system_package.py b/tests/test_api_v1_system_package.py index 9218cf662..61c092f69 100644 --- a/tests/test_api_v1_system_package.py +++ b/tests/test_api_v1_system_package.py @@ -12,36 +12,13 @@ class APIE2ETestSystemPackage(e2e_test_framework.APIE2ETest): "name": "pfSense-pkg-nmap" } }, - { - "name": "Check install of package via URL", - "resp_time": 30, - "payload": { - "name": "https://github.com/jaredhendrickson13/pfsense-saml2-auth/releases/latest/download/pfSense-2.5-pkg-saml2-auth.txz" - } - }, - { - "name": "Check forced install of pfSense repo package", - "resp_time": 30, - "payload": { - "name": "pfSense-pkg-nmap", - "force": True - } - }, - { - "name": "Check forced install of package via URL", - "resp_time": 30, - "payload": { - "name": "https://github.com/jaredhendrickson13/pfsense-saml2-auth/releases/latest/download/pfSense-2.5-pkg-saml2-auth.txz", - "force": True - } - }, { "name": "Check package name required constraint", "status": 400, "return": 1073 }, { - "name": "Check inability to install already installed package when not forced", + "name": "Check inability to install already installed package", "status": 400, "return": 1076, "resp_time": 30, @@ -49,51 +26,6 @@ class APIE2ETestSystemPackage(e2e_test_framework.APIE2ETest): "name": "pfSense-pkg-nmap" } }, - { - "name": "Check inability to install already installed package via URL when not forced", - "status": 400, - "return": 1076, - "resp_time": 30, - "payload": { - "name": "https://github.com/jaredhendrickson13/pfsense-saml2-auth/releases/latest/download/pfSense-2.5-pkg-saml2-auth.txz" - } - }, - { - "name": "Check package install timeout enforcement", - "status": 504, - "return": 1078, - "resp_time": 8, - "payload": { - "name": "https://1.2.3.4/", - "timeout": 5 - } - }, - { - "name": "Check package install timeout maximum constraint", - "status": 400, - "return": 1079, - "payload": { - "name": "https://1.2.3.4/", - "timeout": 121 - } - }, - { - "name": "Check invalid package file at URL", - "status": 400, - "return": 1077, - "payload": { - "name": "https://example.com/index.html" - } - }, - { - "name": "Check package install via URL with unresolvable DNS name", - "status": 502, - "return": 13, - "resp_time": 5, - "payload": { - "name": "https://doesnotexist.jaredhendrickson.com" - } - }, { "name": "Check install package from pfSense repo that doesn't exist", "status": 400, @@ -113,13 +45,6 @@ class APIE2ETestSystemPackage(e2e_test_framework.APIE2ETest): "name": "pfSense-pkg-nmap" } }, - { - "name": "Test deletion of URL installed package", - "resp_time": 30, - "payload": { - "name": "pfSense-pkg-saml2-auth" - } - }, { "name": "Check package name required constraint", "status": 400,