From f7837735ede0fd4c377a8c18cefe550ccc32bea9 Mon Sep 17 00:00:00 2001 From: Frank Wall Date: Sat, 6 Jan 2024 16:36:53 +0100 Subject: [PATCH 1/2] security/acme-client: use common function to run shell commands --- .../library/OPNsense/AcmeClient/LeAccount.php | 64 +++++-------------- .../OPNsense/AcmeClient/LeAutomation/Base.php | 26 ++------ .../OPNsense/AcmeClient/LeCertificate.php | 55 ++++------------ .../library/OPNsense/AcmeClient/LeUtils.php | 6 +- .../OPNsense/AcmeClient/LeValidation/Base.php | 29 ++------- 5 files changed, 40 insertions(+), 140 deletions(-) diff --git a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAccount.php b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAccount.php index 94a91008d..1186dbccb 100644 --- a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAccount.php +++ b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAccount.php @@ -1,7 +1,7 @@ config->name, $this->debug); - // Preparation to run acme client - $proc_env = $this->acme_env; // env variables for proc_open() - $proc_env['PATH'] = $this::ACME_ENV_PATH; - $proc_desc = array( // descriptor array for proc_open() - 0 => array("pipe", "r"), // stdin - 1 => array("pipe", "w"), // stdout - 2 => array("pipe", "w") // stderr - ); - $proc_pipes = array(); - // Run acme client to generate a account key + // Preparation to run acme client + $proc_env = $this->acme_env; // add env variables + $proc_env['PATH'] = $this::ACME_ENV_PATH; + + // Prepare acme.sh command to generate a account key $acmecmd = '/usr/local/sbin/acme.sh ' . '--createAccountKey ' . implode(' ', $this->acme_args) . ' ' . LeUtils::execSafe('--accountkeylength %s', self::ACME_ACCOUNT_KEY_LENGTH) . ' ' . LeUtils::execSafe('--accountconf %s', $account_conf_file); LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug); - $proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env); - // Make sure the resource could be setup properly - if (is_resource($proc)) { - // Close all pipes - fclose($proc_pipes[0]); - fclose($proc_pipes[1]); - fclose($proc_pipes[2]); - // Get exit code - $result = proc_close($proc); - } else { - LeUtils::log_error('unable to start acme client process'); - $this->setStatus(500); - return false; - } + // Run acme.sh command + $result = LeUtils::run_shell_command($acmecmd, $proc_env); - // Check exit code + // Check acme.sh result if ($result) { LeUtils::log_error('failed to create a new account key for ' . (string)$this->config->name); $this->setStatus(300); @@ -220,38 +204,20 @@ class LeAccount extends LeCommon } // Preparation to run acme client - $proc_env = $this->acme_env; // env variables for proc_open() + $proc_env = $this->acme_env; // add env variables $proc_env['PATH'] = $this::ACME_ENV_PATH; - $proc_desc = array( // descriptor array for proc_open() - 0 => array("pipe", "r"), // stdin - 1 => array("pipe", "w"), // stdout - 2 => array("pipe", "w") // stderr - ); - $proc_pipes = array(); - // Run acme client + // Prepare acme.sh command to register an account $acmecmd = '/usr/local/sbin/acme.sh ' . '--registeraccount ' . implode(' ', $this->acme_args) . ' ' . LeUtils::execSafe('--accountconf %s', $this->account_conf_file); LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug); - $proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env); - // Make sure the resource could be setup properly - if (is_resource($proc)) { - // Close all pipes - fclose($proc_pipes[0]); - fclose($proc_pipes[1]); - fclose($proc_pipes[2]); - // Get exit code - $result = proc_close($proc); - } else { - LeUtils::log_error('unable to start acme client process'); - $this->setStatus(500); - return false; - } + // Run acme.sh command + $result = LeUtils::run_shell_command($acmecmd, $proc_env); - // Check validation result + // Check acme.sh result if ($result) { LeUtils::log_error('account registration failed for ' . $this->config->name); $this->setStatus(400); diff --git a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAutomation/Base.php b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAutomation/Base.php index 4f79aa471..fc3119580 100644 --- a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAutomation/Base.php +++ b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeAutomation/Base.php @@ -1,7 +1,7 @@ * All rights reserved. @@ -120,16 +120,10 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon LeUtils::log('running automation (acme.sh): ' . $this->config->name); // Preparation to run acme client - $proc_env = $this->acme_env; // env variables for proc_open() + $proc_env = $this->acme_env; // add env variables $proc_env['PATH'] = $this::ACME_ENV_PATH; - $proc_desc = array( // descriptor array for proc_open() - 0 => array("pipe", "r"), // stdin - 1 => array("pipe", "w"), // stdout - 2 => array("pipe", "w") // stderr - ); - $proc_pipes = array(); - // Run acme client + // Prepare acme.sh command to run a deploy hook $acmecmd = self::ACME_CMD . ' ' . '--deploy ' @@ -137,18 +131,8 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug); $proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env); - // Make sure the resource could be setup properly - if (is_resource($proc)) { - // Close all pipes - fclose($proc_pipes[0]); - fclose($proc_pipes[1]); - fclose($proc_pipes[2]); - // Get exit code - $result = proc_close($proc); - } else { - LeUtils::log_error('unable to start acme client process'); - return false; - } + // Run acme.sh command + $result = LeUtils::run_shell_command($acmecmd, $proc_env); // acme.sh records the last used deploy hook and would automatically // use it on the next run. This information must be removed from the diff --git a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeCertificate.php b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeCertificate.php index 06e406b57..bf16c5fb5 100644 --- a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeCertificate.php +++ b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeCertificate.php @@ -1,7 +1,7 @@ config->name); // Preparation to run acme client - $proc_env = $this->acme_env; // env variables for proc_open() + $proc_env = $this->acme_env; // add env variables $proc_env['PATH'] = $this::ACME_ENV_PATH; - $proc_desc = array( // descriptor array for proc_open() - 0 => array("pipe", "r"), // stdin - 1 => array("pipe", "w"), // stdout - 2 => array("pipe", "w") // stderr - ); - $proc_pipes = array(); - // Run acme client to remove certificate and related config + // Prepare acme.sh command to remove certificate and related config $acmecmd = '/usr/local/sbin/acme.sh ' . '--remove ' . implode(' ', $this->acme_args) . ' ' . LeUtils::execSafe('--domain %s', (string)$this->config->name); LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug); - $proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env); - // Make sure the resource could be setup properly - if (is_resource($proc)) { - // Close all pipes - fclose($proc_pipes[0]); - fclose($proc_pipes[1]); - fclose($proc_pipes[2]); - // Get exit code - $result = proc_close($proc); - } else { - LeUtils::log_error('unable to start acme client process'); - return false; - } + // Run acme.sh command + $result = LeUtils::run_shell_command($acmecmd, $proc_env); - // Check exit code + // Check acme.sh result if ($result) { LeUtils::log_error('error removing certificate ' . (string)$this->config->name); return false; @@ -565,36 +549,19 @@ class LeCertificate extends LeCommon $account_conf_file = $account_conf_dir . '/account.conf'; // Preparation to run acme client - $proc_env = $this->acme_env; // env variables for proc_open() + $proc_env = $this->acme_env; // add env variables $proc_env['PATH'] = $this::ACME_ENV_PATH; - $proc_desc = array( // descriptor array for proc_open() - 0 => array("pipe", "r"), // stdin - 1 => array("pipe", "w"), // stdout - 2 => array("pipe", "w") // stderr - ); - $proc_pipes = array(); - // Run acme client to revoke certificate + // Prepare acme.sh command to revoke certificate $acmecmd = '/usr/local/sbin/acme.sh ' . '--revoke ' . implode(' ', $this->acme_args) . ' ' . LeUtils::execSafe('--domain %s', (string)$this->config->name) . ' ' . LeUtils::execSafe('--accountconf %s', $account_conf_file); LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug); - $proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env); - // Make sure the resource could be setup properly - if (is_resource($proc)) { - // Close all pipes - fclose($proc_pipes[0]); - fclose($proc_pipes[1]); - fclose($proc_pipes[2]); - // Get exit code - $result = proc_close($proc); - } else { - LeUtils::log_error('unable to start acme client process'); - return false; - } + // Run acme.sh command + $result = LeUtils::run_shell_command($acmecmd, $proc_env); // Check exit code if ($result) { diff --git a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php index ab4d46c85..f7c497c78 100644 --- a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php +++ b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php @@ -1,7 +1,7 @@ * Copyright (C) 2008 Shrew Soft Inc. @@ -200,11 +200,11 @@ class LeUtils fclose($proc_pipes[2]); // Get exit code $result = proc_close($proc); - log_error(sprintf("AcmeClient: The shell command '%s' returned exit code '%d'", $proc_cmd, $result)); + log_error(sprintf("AcmeClient: The shell command returned exit code '%d': '%s'", $result, $proc_cmd)); return($result); } else { log_error(sprintf("AcmeClient: Unable to prepare shell command '%s'", $proc_cmd)); - return false; + return(-999); } } } diff --git a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeValidation/Base.php b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeValidation/Base.php index b516343d6..1af7c8dfe 100644 --- a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeValidation/Base.php +++ b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeValidation/Base.php @@ -1,7 +1,7 @@ * All rights reserved. @@ -154,16 +154,10 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon $account_conf_file = $account_conf_dir . '/account.conf'; // Preparation to run acme client - $proc_env = $this->acme_env; // env variables for proc_open() + $proc_env = $this->acme_env; // add env variables $proc_env['PATH'] = $this::ACME_ENV_PATH; - $proc_desc = array( // descriptor array for proc_open() - 0 => array("pipe", "r"), // stdin - 1 => array("pipe", "w"), // stdout - 2 => array("pipe", "w") // stderr - ); - $proc_pipes = array(); - // Run acme client + // Prepare acme.sh command // NOTE: We "export" certificates to our own directory, so we don't have to deal // with domain names in filesystem, but instead can use the ID of our certObj, which // will never change. @@ -173,25 +167,14 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon . implode(' ', $this->acme_args) . ' ' . LeUtils::execSafe('--accountconf %s', $account_conf_file); LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug); - $proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env); - // Make sure the resource could be setup properly - if (is_resource($proc)) { - // Close all pipes - fclose($proc_pipes[0]); - fclose($proc_pipes[1]); - fclose($proc_pipes[2]); - // Get exit code - $result = proc_close($proc); - } else { - LeUtils::log_error('unable to start acme client process'); - return false; - } + // Run acme.sh command + $result = LeUtils::run_shell_command($acmecmd, $proc_env); // Run optional cleanup tasks. $this->cleanup(); - // Check validation result + // Check acme.sh result if ($result) { LeUtils::log_error('domain validation failed (' . $this->getMethod() . ')'); return false; From e6742e3169ff269120ca39abff7ea2758218312a Mon Sep 17 00:00:00 2001 From: Frank Wall Date: Sat, 6 Jan 2024 17:40:41 +0100 Subject: [PATCH 2/2] security/acme-client: fix command failure with gcloud, refs #3745 --- security/acme-client/pkg-descr | 3 +++ .../mvc/app/library/OPNsense/AcmeClient/LeUtils.php | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/security/acme-client/pkg-descr b/security/acme-client/pkg-descr index 41b68f69d..bfec3433c 100644 --- a/security/acme-client/pkg-descr +++ b/security/acme-client/pkg-descr @@ -8,6 +8,9 @@ WWW: https://github.com/acmesh-official/acme.sh Plugin Changelog ================ +Fixed: +* fix sporadic command failure with gcloud DNS API (#3745) + 3.20 Added: diff --git a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php index f7c497c78..cca7ce55e 100644 --- a/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php +++ b/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeUtils.php @@ -194,10 +194,19 @@ class LeUtils // Make sure the resource could be setup properly if (is_resource($proc)) { - // Close all pipes + // This workaround ensures that the accurate return code + // is reliably returned. fclose($proc_pipes[0]); + $output = array(); + while (!feof($proc_pipes[1])) { + $output[] = rtrim(fgets($proc_pipes[1], 1024), "\n"); + } fclose($proc_pipes[1]); + while (!feof($proc_pipes[2])) { + $output[] = rtrim(fgets($proc_pipes[2], 1024), "\n"); + } fclose($proc_pipes[2]); + // Get exit code $result = proc_close($proc); log_error(sprintf("AcmeClient: The shell command returned exit code '%d': '%s'", $result, $proc_cmd));