From 8c9d2745e2e82e69d6dd3b7750fca387c3b5ad41 Mon Sep 17 00:00:00 2001 From: Jamie Curnow Date: Wed, 20 Aug 2025 09:53:13 +1000 Subject: [PATCH] Fix remote execution bug where email address can contain malicious code also convert almost all cmd execs for certificates to properly escape arguments --- backend/internal/access-list.js | 58 +-- backend/internal/certificate.js | 336 ++++++++++-------- backend/internal/nginx.js | 62 ++-- backend/lib/utils.js | 12 +- backend/routes/nginx/certificates.js | 4 +- backend/schema/common.json | 5 + .../schema/components/certificate-object.json | 2 +- backend/setup.js | 24 +- test/cypress/e2e/api/Certificates.cy.js | 24 ++ 9 files changed, 305 insertions(+), 222 deletions(-) diff --git a/backend/internal/access-list.js b/backend/internal/access-list.js index f6043e18..2407a0ac 100644 --- a/backend/internal/access-list.js +++ b/backend/internal/access-list.js @@ -1,5 +1,5 @@ const _ = require('lodash'); -const fs = require('fs'); +const fs = require('node:fs'); const batchflow = require('batchflow'); const logger = require('../logger').access; const error = require('../lib/error'); @@ -38,7 +38,7 @@ const internalAccessList = { .then((row) => { data.id = row.id; - let promises = []; + const promises = []; // Now add the items data.items.map((item) => { @@ -116,7 +116,7 @@ const internalAccessList = { .then((row) => { if (row.id !== data.id) { // Sanity check that something crazy hasn't happened - throw new error.InternalValidationError('Access List could not be updated, IDs do not match: ' + row.id + ' !== ' + data.id); + throw new error.InternalValidationError(`Access List could not be updated, IDs do not match: ${row.id} !== ${data.id}`); } }) .then(() => { @@ -135,10 +135,10 @@ const internalAccessList = { .then(() => { // Check for items and add/update/remove them if (typeof data.items !== 'undefined' && data.items) { - let promises = []; - let items_to_keep = []; + const promises = []; + const items_to_keep = []; - data.items.map(function (item) { + data.items.map((item) => { if (item.password) { promises.push(accessListAuthModel .query() @@ -154,7 +154,7 @@ const internalAccessList = { } }); - let query = accessListAuthModel + const query = accessListAuthModel .query() .delete() .where('access_list_id', data.id); @@ -175,9 +175,9 @@ const internalAccessList = { .then(() => { // Check for clients and add/update/remove them if (typeof data.clients !== 'undefined' && data.clients) { - let promises = []; + const promises = []; - data.clients.map(function (client) { + data.clients.map((client) => { if (client.address) { promises.push(accessListClientModel .query() @@ -190,7 +190,7 @@ const internalAccessList = { } }); - let query = accessListClientModel + const query = accessListClientModel .query() .delete() .where('access_list_id', data.id); @@ -249,7 +249,7 @@ const internalAccessList = { return access.can('access_lists:get', data.id) .then((access_data) => { - let query = accessListModel + const query = accessListModel .query() .select('access_list.*', accessListModel.raw('COUNT(proxy_host.id) as proxy_host_count')) .leftJoin('proxy_host', function() { @@ -267,7 +267,7 @@ const internalAccessList = { } if (typeof data.expand !== 'undefined' && data.expand !== null) { - query.withGraphFetched('[' + data.expand.join(', ') + ']'); + query.withGraphFetched(`[${data.expand.join(', ')}]`); } return query.then(utils.omitRow(omissions())); @@ -327,7 +327,7 @@ const internalAccessList = { // 3. reconfigure those hosts, then reload nginx // set the access_list_id to zero for these items - row.proxy_hosts.map(function (val, idx) { + row.proxy_hosts.map((_val, idx) => { row.proxy_hosts[idx].access_list_id = 0; }); @@ -340,11 +340,11 @@ const internalAccessList = { }) .then(() => { // delete the htpasswd file - let htpasswd_file = internalAccessList.getFilename(row); + const htpasswd_file = internalAccessList.getFilename(row); try { fs.unlinkSync(htpasswd_file); - } catch (err) { + } catch (_err) { // do nothing } }) @@ -374,7 +374,7 @@ const internalAccessList = { getAll: (access, expand, search_query) => { return access.can('access_lists:list') .then((access_data) => { - let query = accessListModel + const query = accessListModel .query() .select('access_list.*', accessListModel.raw('COUNT(proxy_host.id) as proxy_host_count')) .leftJoin('proxy_host', function() { @@ -393,19 +393,19 @@ const internalAccessList = { // Query is used for searching if (typeof search_query === 'string') { query.where(function () { - this.where('name', 'like', '%' + search_query + '%'); + this.where('name', 'like', `%${search_query}%`); }); } if (typeof expand !== 'undefined' && expand !== null) { - query.withGraphFetched('[' + expand.join(', ') + ']'); + query.withGraphFetched(`[${expand.join(', ')}]`); } return query.then(utils.omitRows(omissions())); }) .then((rows) => { if (rows) { - rows.map(function (row, idx) { + rows.map((row, idx) => { if (typeof row.items !== 'undefined' && row.items) { rows[idx] = internalAccessList.maskItems(row); } @@ -424,7 +424,7 @@ const internalAccessList = { * @returns {Promise} */ getCount: (user_id, visibility) => { - let query = accessListModel + const query = accessListModel .query() .count('id as count') .where('is_deleted', 0); @@ -445,7 +445,7 @@ const internalAccessList = { */ maskItems: (list) => { if (list && typeof list.items !== 'undefined') { - list.items.map(function (val, idx) { + list.items.map((val, idx) => { let repeat_for = 8; let first_char = '*'; @@ -468,7 +468,7 @@ const internalAccessList = { * @returns {String} */ getFilename: (list) => { - return '/data/access/' + list.id; + return `/data/access/${list.id}`; }, /** @@ -479,15 +479,15 @@ const internalAccessList = { * @returns {Promise} */ build: (list) => { - logger.info('Building Access file #' + list.id + ' for: ' + list.name); + logger.info(`Building Access file #${list.id} for: ${list.name}`); return new Promise((resolve, reject) => { - let htpasswd_file = internalAccessList.getFilename(list); + const htpasswd_file = internalAccessList.getFilename(list); // 1. remove any existing access file try { fs.unlinkSync(htpasswd_file); - } catch (err) { + } catch (_err) { // do nothing } @@ -504,14 +504,14 @@ const internalAccessList = { if (list.items.length) { return new Promise((resolve, reject) => { batchflow(list.items).sequential() - .each((i, item, next) => { + .each((_i, item, next) => { if (typeof item.password !== 'undefined' && item.password.length) { - logger.info('Adding: ' + item.username); + logger.info(`Adding: ${item.username}`); utils.execFile('openssl', ['passwd', '-apr1', item.password]) .then((res) => { try { - fs.appendFileSync(htpasswd_file, item.username + ':' + res + '\n', {encoding: 'utf8'}); + fs.appendFileSync(htpasswd_file, `${item.username}:${res}\n`, {encoding: 'utf8'}); } catch (err) { reject(err); } @@ -528,7 +528,7 @@ const internalAccessList = { reject(err); }) .end((results) => { - logger.success('Built Access file #' + list.id + ' for: ' + list.name); + logger.success(`Built Access file #${list.id} for: ${list.name}`); resolve(results); }); }); diff --git a/backend/internal/certificate.js b/backend/internal/certificate.js index f2e845a2..55e74c3e 100644 --- a/backend/internal/certificate.js +++ b/backend/internal/certificate.js @@ -1,6 +1,6 @@ const _ = require('lodash'); -const fs = require('fs'); -const https = require('https'); +const fs = require('node:fs'); +const https = require('node:https'); const tempWrite = require('temp-write'); const moment = require('moment'); const archiver = require('archiver'); @@ -49,7 +49,7 @@ const internalCertificate = { processExpiringHosts: () => { if (!internalCertificate.intervalProcessing) { internalCertificate.intervalProcessing = true; - logger.info('Renewing SSL certs expiring within ' + internalCertificate.renewBeforeExpirationBy[0] + ' ' + internalCertificate.renewBeforeExpirationBy[1] + ' ...'); + logger.info(`Renewing SSL certs expiring within ${internalCertificate.renewBeforeExpirationBy[0]} ${internalCertificate.renewBeforeExpirationBy[1]} ...`); const expirationThreshold = moment().add(internalCertificate.renewBeforeExpirationBy[0], internalCertificate.renewBeforeExpirationBy[1]).format('YYYY-MM-DD HH:mm:ss'); @@ -70,7 +70,7 @@ const internalCertificate = { */ let sequence = Promise.resolve(); - certificates.forEach(function (certificate) { + certificates.forEach((certificate) => { sequence = sequence.then(() => internalCertificate .renew( @@ -202,7 +202,7 @@ const internalCertificate = { .then(() => { // At this point, the letsencrypt cert should exist on disk. // Lets get the expiry date from the file and update the row silently - return internalCertificate.getCertificateInfoFromFile('/etc/letsencrypt/live/npm-' + certificate.id + '/fullchain.pem') + return internalCertificate.getCertificateInfoFromFile(`${internalCertificate.getLiveCertPath(certificate.id)}/fullchain.pem`) .then((cert_info) => { return certificateModel .query() @@ -263,7 +263,7 @@ const internalCertificate = { .then((row) => { if (row.id !== data.id) { // Sanity check that something crazy hasn't happened - throw new error.InternalValidationError('Certificate could not be updated, IDs do not match: ' + row.id + ' !== ' + data.id); + throw new error.InternalValidationError(`Certificate could not be updated, IDs do not match: ${row.id} !== ${data.id}`); } return certificateModel @@ -308,7 +308,7 @@ const internalCertificate = { return access.can('certificates:get', data.id) .then((access_data) => { - let query = certificateModel + const query = certificateModel .query() .where('is_deleted', 0) .andWhere('id', data.id) @@ -323,7 +323,7 @@ const internalCertificate = { } if (typeof data.expand !== 'undefined' && data.expand !== null) { - query.withGraphFetched('[' + data.expand.join(', ') + ']'); + query.withGraphFetched(`[${data.expand.join(', ')}]`); } return query.then(utils.omitRow(omissions())); @@ -354,17 +354,17 @@ const internalCertificate = { }) .then((certificate) => { if (certificate.provider === 'letsencrypt') { - const zipDirectory = '/etc/letsencrypt/live/npm-' + data.id; + const zipDirectory = internalCertificate.getLiveCertPath(data.id); if (!fs.existsSync(zipDirectory)) { - throw new error.ItemNotFoundError('Certificate ' + certificate.nice_name + ' does not exists'); + throw new error.ItemNotFoundError(`Certificate ${certificate.nice_name} does not exists`); } - let certFiles = fs.readdirSync(zipDirectory) + const certFiles = fs.readdirSync(zipDirectory) .filter((fn) => fn.endsWith('.pem')) .map((fn) => fs.realpathSync(path.join(zipDirectory, fn))); - const downloadName = 'npm-' + data.id + '-' + `${Date.now()}.zip`; - const opName = '/tmp/' + downloadName; + const downloadName = `npm-${data.id}-${Date.now()}.zip`; + const opName = `/tmp/${downloadName}`; internalCertificate.zipFiles(certFiles, opName) .then(() => { logger.debug('zip completed : ', opName); @@ -392,7 +392,7 @@ const internalCertificate = { return new Promise((resolve, reject) => { source .map((fl) => { - let fileName = path.basename(fl); + const fileName = path.basename(fl); logger.debug(fl, 'added to certificate zip'); archive.file(fl, { name: fileName }); }); @@ -462,7 +462,7 @@ const internalCertificate = { getAll: (access, expand, search_query) => { return access.can('certificates:list') .then((access_data) => { - let query = certificateModel + const query = certificateModel .query() .where('is_deleted', 0) .groupBy('id') @@ -479,12 +479,12 @@ const internalCertificate = { // Query is used for searching if (typeof search_query === 'string') { query.where(function () { - this.where('nice_name', 'like', '%' + search_query + '%'); + this.where('nice_name', 'like', `%${search_query}%`); }); } if (typeof expand !== 'undefined' && expand !== null) { - query.withGraphFetched('[' + expand.join(', ') + ']'); + query.withGraphFetched(`[${expand.join(', ')}]`); } return query.then(utils.omitRows(omissions())); @@ -499,7 +499,7 @@ const internalCertificate = { * @returns {Promise} */ getCount: (user_id, visibility) => { - let query = certificateModel + const query = certificateModel .query() .count('id as count') .where('is_deleted', 0); @@ -521,7 +521,7 @@ const internalCertificate = { writeCustomCert: (certificate) => { logger.info('Writing Custom Certificate:', certificate); - const dir = '/data/custom_ssl/npm-' + certificate.id; + const dir = `/data/custom_ssl/npm-${certificate.id}`; return new Promise((resolve, reject) => { if (certificate.provider === 'letsencrypt') { @@ -531,7 +531,7 @@ const internalCertificate = { let certData = certificate.meta.certificate; if (typeof certificate.meta.intermediate_certificate !== 'undefined') { - certData = certData + '\n' + certificate.meta.intermediate_certificate; + certData = `${certData}\n${certificate.meta.intermediate_certificate}`; } try { @@ -543,7 +543,7 @@ const internalCertificate = { return; } - fs.writeFile(dir + '/fullchain.pem', certData, function (err) { + fs.writeFile(`${dir}/fullchain.pem`, certData, (err) => { if (err) { reject(err); } else { @@ -553,7 +553,7 @@ const internalCertificate = { }) .then(() => { return new Promise((resolve, reject) => { - fs.writeFile(dir + '/privkey.pem', certificate.meta.certificate_key, function (err) { + fs.writeFile(`${dir}/privkey.pem`, certificate.meta.certificate_key, (err) => { if (err) { reject(err); } else { @@ -591,7 +591,7 @@ const internalCertificate = { validate: (data) => { return new Promise((resolve) => { // Put file contents into an object - let files = {}; + const files = {}; _.map(data.files, (file, name) => { if (internalCertificate.allowedSslFiles.indexOf(name) !== -1) { files[name] = file.data.toString(); @@ -603,7 +603,7 @@ const internalCertificate = { .then((files) => { // For each file, create a temp file and write the contents to it // Then test it depending on the file type - let promises = []; + const promises = []; _.map(files, (content, type) => { promises.push(new Promise((resolve) => { if (type === 'certificate_key') { @@ -688,11 +688,11 @@ const internalCertificate = { reject(new error.ValidationError('Result Validation Error: Validation timed out. This could be due to the key being passphrase-protected.')); }, 10000); utils - .exec('openssl pkey -in ' + filepath + ' -check -noout 2>&1 ') + .exec(`openssl pkey -in ${filepath} -check -noout 2>&1 `) .then((result) => { clearTimeout(failTimeout); if (!result.toLowerCase().includes('key is valid')) { - reject(new error.ValidationError('Result Validation Error: ' + result)); + reject(new error.ValidationError(`Result Validation Error: ${result}`)); } fs.unlinkSync(filepath); resolve(true); @@ -700,7 +700,7 @@ const internalCertificate = { .catch((err) => { clearTimeout(failTimeout); fs.unlinkSync(filepath); - reject(new error.ValidationError('Certificate Key is not valid (' + err.message + ')', err)); + reject(new error.ValidationError(`Certificate Key is not valid (${err.message})`, err)); }); }); }); @@ -735,9 +735,9 @@ const internalCertificate = { * @param {Boolean} [throw_expired] Throw when the certificate is out of date */ getCertificateInfoFromFile: (certificate_file, throw_expired) => { - let certData = {}; + const certData = {}; - return utils.exec('openssl x509 -in ' + certificate_file + ' -subject -noout') + return utils.execFile('openssl', ['x509', '-in', certificate_file, '-subject', '-noout']) .then((result) => { // Examples: // subject=CN = *.jc21.com @@ -745,11 +745,11 @@ const internalCertificate = { const regex = /(?:subject=)?[^=]+=\s+(\S+)/gim; const match = regex.exec(result); if (match && typeof match[1] !== 'undefined') { - certData['cn'] = match[1]; + certData.cn = match[1]; } }) .then(() => { - return utils.exec('openssl x509 -in ' + certificate_file + ' -issuer -noout'); + return utils.execFile('openssl', ['x509', '-in', certificate_file, '-issuer', '-noout']); }) .then((result) => { @@ -760,11 +760,11 @@ const internalCertificate = { const regex = /^(?:issuer=)?(.*)$/gim; const match = regex.exec(result); if (match && typeof match[1] !== 'undefined') { - certData['issuer'] = match[1]; + certData.issuer = match[1]; } }) .then(() => { - return utils.exec('openssl x509 -in ' + certificate_file + ' -dates -noout'); + return utils.execFile('openssl', ['x509', '-in', certificate_file, '-dates', '-noout']); }) .then((result) => { // notBefore=Jul 14 04:04:29 2018 GMT @@ -773,7 +773,7 @@ const internalCertificate = { let validTo = null; const lines = result.split('\n'); - lines.map(function (str) { + lines.map((str) => { const regex = /^(\S+)=(.*)$/gim; const match = regex.exec(str.trim()); @@ -789,21 +789,21 @@ const internalCertificate = { }); if (!validFrom || !validTo) { - throw new error.ValidationError('Could not determine dates from certificate: ' + result); + throw new error.ValidationError(`Could not determine dates from certificate: ${result}`); } if (throw_expired && validTo < parseInt(moment().format('X'), 10)) { throw new error.ValidationError('Certificate has expired'); } - certData['dates'] = { + certData.dates = { from: validFrom, to: validTo }; return certData; }).catch((err) => { - throw new error.ValidationError('Certificate is not valid (' + err.message + ')', err); + throw new error.ValidationError(`Certificate is not valid (${err.message})`, err); }); }, @@ -814,7 +814,7 @@ const internalCertificate = { * @param {Boolean} [remove] * @returns {Object} */ - cleanMeta: function (meta, remove) { + cleanMeta: (meta, remove) => { internalCertificate.allowedSslFiles.map((key) => { if (typeof meta[key] !== 'undefined' && meta[key]) { if (remove) { @@ -834,24 +834,35 @@ const internalCertificate = { * @returns {Promise} */ requestLetsEncryptSsl: (certificate) => { - logger.info('Requesting Let\'sEncrypt certificates for Cert #' + certificate.id + ': ' + certificate.domain_names.join(', ')); + logger.info(`Requesting LetsEncrypt certificates for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); - const cmd = `${certbotCommand} certonly ` + - `--config '${letsencryptConfig}' ` + - '--work-dir "/tmp/letsencrypt-lib" ' + - '--logs-dir "/tmp/letsencrypt-log" ' + - `--cert-name "npm-${certificate.id}" ` + - '--agree-tos ' + - '--authenticator webroot ' + - `--email '${certificate.meta.letsencrypt_email}' ` + - '--preferred-challenges "dns,http" ' + - `--domains "${certificate.domain_names.join(',')}" ` + - (letsencryptServer !== null ? `--server '${letsencryptServer}' ` : '') + - (letsencryptStaging && letsencryptServer === null ? '--staging ' : ''); + const args = [ + 'certonly', + '--config', + letsencryptConfig, + '--work-dir', + '/tmp/letsencrypt-lib', + '--logs-dir', + '/tmp/letsencrypt-log', + '--cert-name', + `npm-${certificate.id}`, + '--agree-tos', + '--authenticator', + 'webroot', + '--email', + certificate.meta.letsencrypt_email, + '--preferred-challenges', + 'dns,http', + '--domains', + certificate.domain_names.join(','), + ]; - logger.info('Command:', cmd); + const adds = internalCertificate.getAdditionalCertbotArgs(certificate.id); + args.push(...adds.args); - return utils.exec(cmd) + logger.info(`Command: ${certbotCommand} ${args ? args.join(' ') : ''}`); + + return utils.execFile(certbotCommand, args, adds.opts) .then((result) => { logger.success(result); return result; @@ -868,50 +879,48 @@ const internalCertificate = { requestLetsEncryptSslWithDnsChallenge: async (certificate) => { await certbot.installPlugin(certificate.meta.dns_provider); const dnsPlugin = dnsPlugins[certificate.meta.dns_provider]; - logger.info(`Requesting Let'sEncrypt certificates via ${dnsPlugin.name} for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); + logger.info(`Requesting LetsEncrypt certificates via ${dnsPlugin.name} for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); - const credentialsLocation = '/etc/letsencrypt/credentials/credentials-' + certificate.id; + const credentialsLocation = `/etc/letsencrypt/credentials/credentials-${certificate.id}`; fs.mkdirSync('/etc/letsencrypt/credentials', { recursive: true }); fs.writeFileSync(credentialsLocation, certificate.meta.dns_provider_credentials, {mode: 0o600}); // Whether the plugin has a ---credentials argument const hasConfigArg = certificate.meta.dns_provider !== 'route53'; - let mainCmd = certbotCommand + ' certonly ' + - `--config '${letsencryptConfig}' ` + - '--work-dir "/tmp/letsencrypt-lib" ' + - '--logs-dir "/tmp/letsencrypt-log" ' + - `--cert-name 'npm-${certificate.id}' ` + - '--agree-tos ' + - `--email '${certificate.meta.letsencrypt_email}' ` + - `--domains '${certificate.domain_names.join(',')}' ` + - `--authenticator '${dnsPlugin.full_plugin_name}' ` + - ( - hasConfigArg - ? `--${dnsPlugin.full_plugin_name}-credentials '${credentialsLocation}' ` - : '' - ) + - ( - certificate.meta.propagation_seconds !== undefined - ? `--${dnsPlugin.full_plugin_name}-propagation-seconds '${certificate.meta.propagation_seconds}' ` - : '' - ) + - (letsencryptServer !== null ? `--server '${letsencryptServer}' ` : '') + - (letsencryptStaging && letsencryptServer === null ? '--staging ' : ''); + const args = [ + 'certonly', + '--config', + letsencryptConfig, + '--work-dir', + '/tmp/letsencrypt-lib', + '--logs-dir', + '/tmp/letsencrypt-log', + '--cert-name', + `npm-${certificate.id}`, + '--agree-tos', + '--email', + certificate.meta.letsencrypt_email, + '--domains', + certificate.domain_names.join(','), + '--authenticator', + dnsPlugin.full_plugin_name, + ]; - // Prepend the path to the credentials file as an environment variable - if (certificate.meta.dns_provider === 'route53') { - mainCmd = 'AWS_CONFIG_FILE=\'' + credentialsLocation + '\' ' + mainCmd; + if (hasConfigArg) { + args.push(`--${dnsPlugin.full_plugin_name}-credentials`, credentialsLocation); + } + if (certificate.meta.propagation_seconds !== undefined) { + args.push(`--${dnsPlugin.full_plugin_name}-propagation-seconds`, certificate.meta.propagation_seconds.toString()); } - if (certificate.meta.dns_provider === 'duckdns') { - mainCmd = mainCmd + ' --dns-duckdns-no-txt-restore'; - } + const adds = internalCertificate.getAdditionalCertbotArgs(certificate.id, certificate.meta.dns_provider); + args.push(...adds.args); - logger.info('Command:', mainCmd); + logger.info(`Command: ${certbotCommand} ${args ? args.join(' ') : ''}`); try { - const result = await utils.exec(mainCmd); + const result = await utils.execFile(certbotCommand, args, adds.opts); logger.info(result); return result; } catch (err) { @@ -939,7 +948,7 @@ const internalCertificate = { return renewMethod(certificate) .then(() => { - return internalCertificate.getCertificateInfoFromFile('/etc/letsencrypt/live/npm-' + certificate.id + '/fullchain.pem'); + return internalCertificate.getCertificateInfoFromFile(`${internalCertificate.getLiveCertPath(certificate.id)}/fullchain.pem`); }) .then((cert_info) => { return certificateModel @@ -971,22 +980,31 @@ const internalCertificate = { * @returns {Promise} */ renewLetsEncryptSsl: (certificate) => { - logger.info('Renewing Let\'sEncrypt certificates for Cert #' + certificate.id + ': ' + certificate.domain_names.join(', ')); + logger.info(`Renewing LetsEncrypt certificates for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); - const cmd = certbotCommand + ' renew --force-renewal ' + - `--config '${letsencryptConfig}' ` + - '--work-dir "/tmp/letsencrypt-lib" ' + - '--logs-dir "/tmp/letsencrypt-log" ' + - `--cert-name 'npm-${certificate.id}' ` + - '--preferred-challenges "dns,http" ' + - '--no-random-sleep-on-renew ' + - '--disable-hook-validation ' + - (letsencryptServer !== null ? `--server '${letsencryptServer}' ` : '') + - (letsencryptStaging && letsencryptServer === null ? '--staging ' : ''); + const args = [ + 'renew', + '--force-renewal', + '--config', + letsencryptConfig, + '--work-dir', + '/tmp/letsencrypt-lib', + '--logs-dir', + '/tmp/letsencrypt-log', + '--cert-name', + `npm-${certificate.id}`, + '--preferred-challenges', + 'dns,http', + '--no-random-sleep-on-renew', + '--disable-hook-validation', + ]; - logger.info('Command:', cmd); + const adds = internalCertificate.getAdditionalCertbotArgs(certificate.id, certificate.meta.dns_provider); + args.push(...adds.args); - return utils.exec(cmd) + logger.info(`Command: ${certbotCommand} ${args ? args.join(' ') : ''}`); + + return utils.execFile(certbotCommand, args, adds.opts) .then((result) => { logger.info(result); return result; @@ -1004,27 +1022,29 @@ const internalCertificate = { throw Error(`Unknown DNS provider '${certificate.meta.dns_provider}'`); } - logger.info(`Renewing Let'sEncrypt certificates via ${dnsPlugin.name} for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); + logger.info(`Renewing LetsEncrypt certificates via ${dnsPlugin.name} for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); - let mainCmd = certbotCommand + ' renew --force-renewal ' + - `--config "${letsencryptConfig}" ` + - '--work-dir "/tmp/letsencrypt-lib" ' + - '--logs-dir "/tmp/letsencrypt-log" ' + - `--cert-name 'npm-${certificate.id}' ` + - '--disable-hook-validation ' + - '--no-random-sleep-on-renew ' + - (letsencryptServer !== null ? `--server '${letsencryptServer}' ` : '') + - (letsencryptStaging && letsencryptServer === null ? '--staging ' : ''); + const args = [ + 'renew', + '--force-renewal', + '--config', + letsencryptConfig, + '--work-dir', + '/tmp/letsencrypt-lib', + '--logs-dir', + '/tmp/letsencrypt-log', + '--cert-name', + `npm-${certificate.id}`, + '--disable-hook-validation', + '--no-random-sleep-on-renew', + ]; - // Prepend the path to the credentials file as an environment variable - if (certificate.meta.dns_provider === 'route53') { - const credentialsLocation = '/etc/letsencrypt/credentials/credentials-' + certificate.id; - mainCmd = 'AWS_CONFIG_FILE=\'' + credentialsLocation + '\' ' + mainCmd; - } + const adds = internalCertificate.getAdditionalCertbotArgs(certificate.id, certificate.meta.dns_provider); + args.push(...adds.args); - logger.info('Command:', mainCmd); + logger.info(`Command: ${certbotCommand} ${args ? args.join(' ') : ''}`); - return utils.exec(mainCmd) + return utils.execFile(certbotCommand, args, adds.opts) .then(async (result) => { logger.info(result); return result; @@ -1037,25 +1057,29 @@ const internalCertificate = { * @returns {Promise} */ revokeLetsEncryptSsl: (certificate, throw_errors) => { - logger.info('Revoking Let\'sEncrypt certificates for Cert #' + certificate.id + ': ' + certificate.domain_names.join(', ')); + logger.info(`Revoking LetsEncrypt certificates for Cert #${certificate.id}: ${certificate.domain_names.join(', ')}`); - const mainCmd = certbotCommand + ' revoke ' + - `--config '${letsencryptConfig}' ` + - '--work-dir "/tmp/letsencrypt-lib" ' + - '--logs-dir "/tmp/letsencrypt-log" ' + - `--cert-path '/etc/letsencrypt/live/npm-${certificate.id}/fullchain.pem' ` + - '--delete-after-revoke ' + - (letsencryptServer !== null ? `--server '${letsencryptServer}' ` : '') + - (letsencryptStaging && letsencryptServer === null ? '--staging ' : ''); + const args = [ + 'revoke', + '--config', + letsencryptConfig, + '--work-dir', + '/tmp/letsencrypt-lib', + '--logs-dir', + '/tmp/letsencrypt-log', + '--cert-path', + `${internalCertificate.getLiveCertPath(certificate.id)}/fullchain.pem`, + '--delete-after-revoke', + ]; - // Don't fail command if file does not exist - const delete_credentialsCmd = `rm -f '/etc/letsencrypt/credentials/credentials-${certificate.id}' || true`; + const adds = internalCertificate.getAdditionalCertbotArgs(certificate.id); + args.push(...adds.args); - logger.info('Command:', mainCmd + '; ' + delete_credentialsCmd); + logger.info(`Command: ${certbotCommand} ${args ? args.join(' ') : ''}`); - return utils.exec(mainCmd) + return utils.execFile(certbotCommand, args, adds.opts) .then(async (result) => { - await utils.exec(delete_credentialsCmd); + await utils.exec(`rm -f '/etc/letsencrypt/credentials/credentials-${certificate.id}' || true`); logger.info(result); return result; }) @@ -1073,9 +1097,8 @@ const internalCertificate = { * @returns {Boolean} */ hasLetsEncryptSslCerts: (certificate) => { - const letsencryptPath = '/etc/letsencrypt/live/npm-' + certificate.id; - - return fs.existsSync(letsencryptPath + '/fullchain.pem') && fs.existsSync(letsencryptPath + '/privkey.pem'); + const letsencryptPath = internalCertificate.getLiveCertPath(certificate.id); + return fs.existsSync(`${letsencryptPath}/fullchain.pem`) && fs.existsSync(`${letsencryptPath}/privkey.pem`); }, /** @@ -1087,7 +1110,7 @@ const internalCertificate = { */ disableInUseHosts: (in_use_result) => { if (in_use_result.total_count) { - let promises = []; + const promises = []; if (in_use_result.proxy_hosts.length) { promises.push(internalNginx.bulkDeleteConfigs('proxy_host', in_use_result.proxy_hosts)); @@ -1117,7 +1140,7 @@ const internalCertificate = { */ enableInUseHosts: (in_use_result) => { if (in_use_result.total_count) { - let promises = []; + const promises = []; if (in_use_result.proxy_hosts.length) { promises.push(internalNginx.bulkGenerateConfigs('proxy_host', in_use_result.proxy_hosts)); @@ -1150,12 +1173,12 @@ const internalCertificate = { // Create a test challenge file const testChallengeDir = '/data/letsencrypt-acme-challenge/.well-known/acme-challenge'; - const testChallengeFile = testChallengeDir + '/test-challenge'; + const testChallengeFile = `${testChallengeDir}/test-challenge`; fs.mkdirSync(testChallengeDir, {recursive: true}); fs.writeFileSync(testChallengeFile, 'Success', {encoding: 'utf8'}); async function performTestForDomain (domain) { - logger.info('Testing http challenge for ' + domain); + logger.info(`Testing http challenge for ${domain}`); const url = `http://${domain}/.well-known/acme-challenge/test-challenge`; const formBody = `method=G&url=${encodeURI(url)}&bodytype=T&requestbody=&headername=User-Agent&headervalue=None&locationid=1&ch=false&cc=false`; const options = { @@ -1169,13 +1192,16 @@ const internalCertificate = { const result = await new Promise((resolve) => { - const req = https.request('https://www.site24x7.com/tools/restapi-tester', options, function (res) { + const req = https.request('https://www.site24x7.com/tools/restapi-tester', options, (res) => { let responseBody = ''; - res.on('data', (chunk) => responseBody = responseBody + chunk); - res.on('end', function () { + res.on('data', (chunk) => { + responseBody = responseBody + chunk; + }); + + res.on('end', () => { try { - const parsedBody = JSON.parse(responseBody + ''); + const parsedBody = JSON.parse(`${responseBody}`); if (res.statusCode !== 200) { logger.warn(`Failed to test HTTP challenge for domain ${domain} because HTTP status code ${res.statusCode} was returned: ${parsedBody.message}`); resolve(undefined); @@ -1196,7 +1222,7 @@ const internalCertificate = { // Make sure to write the request body. req.write(formBody); req.end(); - req.on('error', function (e) { logger.warn(`Failed to test HTTP challenge for domain ${domain}`, e); + req.on('error', (e) => { logger.warn(`Failed to test HTTP challenge for domain ${domain}`, e); resolve(undefined); }); }); @@ -1238,6 +1264,34 @@ const internalCertificate = { fs.unlinkSync(testChallengeFile); return results; + }, + + getAdditionalCertbotArgs: (certificate_id, dns_provider) => { + const args = []; + if (letsencryptServer !== null) { + args.push('--server', letsencryptServer); + } + if (letsencryptStaging && letsencryptServer === null) { + args.push('--staging'); + } + + // For route53, add the credentials file as an environment variable, + // inheriting the process env + const opts = {}; + if (certificate_id && dns_provider === 'route53') { + opts.env = process.env; + opts.env.AWS_CONFIG_FILE = `/etc/letsencrypt/credentials/credentials-${certificate_id}`; + } + + if (dns_provider === 'duckdns') { + args.push('--dns-duckdns-no-txt-restore'); + } + + return {args: args, opts: opts}; + }, + + getLiveCertPath: (certificate_id) => { + return `/etc/letsencrypt/live/npm-${certificate_id}`; } }; diff --git a/backend/internal/nginx.js b/backend/internal/nginx.js index 5f802c00..59694d3c 100644 --- a/backend/internal/nginx.js +++ b/backend/internal/nginx.js @@ -1,5 +1,5 @@ const _ = require('lodash'); -const fs = require('fs'); +const fs = require('node:fs'); const logger = require('../logger').nginx; const config = require('../lib/config'); const utils = require('../lib/utils'); @@ -57,9 +57,9 @@ const internalNginx = { // It will always look like this: // nginx: [alert] could not open error log file: open() "/var/log/nginx/error.log" failed (6: No such device or address) - let valid_lines = []; - let err_lines = err.message.split('\n'); - err_lines.map(function (line) { + const valid_lines = []; + const err_lines = err.message.split('\n'); + err_lines.map((line) => { if (line.indexOf('/var/log/nginx/error.log') === -1) { valid_lines.push(line); } @@ -105,7 +105,7 @@ const internalNginx = { logger.info('Testing Nginx configuration'); } - return utils.exec('/usr/sbin/nginx -t -g "error_log off;"'); + return utils.execFile('/usr/sbin/nginx', ['-t', '-g', 'error_log off;']); }, /** @@ -115,7 +115,7 @@ const internalNginx = { return internalNginx.test() .then(() => { logger.info('Reloading Nginx'); - return utils.exec('/usr/sbin/nginx -s reload'); + return utils.execFile('/usr/sbin/nginx', ['-s', 'reload']); }); }, @@ -128,7 +128,7 @@ const internalNginx = { if (host_type === 'default') { return '/data/nginx/default_host/site.conf'; } - return '/data/nginx/' + internalNginx.getFileFriendlyHostType(host_type) + '/' + host_id + '.conf'; + return `/data/nginx/${internalNginx.getFileFriendlyHostType(host_type)}/${host_id}.conf`; }, /** @@ -141,7 +141,7 @@ const internalNginx = { let template; try { - template = fs.readFileSync(__dirname + '/../templates/_location.conf', {encoding: 'utf8'}); + template = fs.readFileSync(`${__dirname}/../templates/_location.conf`, {encoding: 'utf8'}); } catch (err) { reject(new error.ConfigurationError(err.message)); return; @@ -152,7 +152,7 @@ const internalNginx = { const locationRendering = async () => { for (let i = 0; i < host.locations.length; i++) { - let locationCopy = Object.assign({}, {access_list_id: host.access_list_id}, {certificate_id: host.certificate_id}, + const locationCopy = Object.assign({}, {access_list_id: host.access_list_id}, {certificate_id: host.certificate_id}, {ssl_forced: host.ssl_forced}, {caching_enabled: host.caching_enabled}, {block_exploits: host.block_exploits}, {allow_websocket_upgrade: host.allow_websocket_upgrade}, {http2_support: host.http2_support}, {hsts_enabled: host.hsts_enabled}, {hsts_subdomains: host.hsts_subdomains}, {access_list: host.access_list}, @@ -183,21 +183,21 @@ const internalNginx = { */ generateConfig: (host_type, host_row) => { // Prevent modifying the original object: - let host = JSON.parse(JSON.stringify(host_row)); + const host = JSON.parse(JSON.stringify(host_row)); const nice_host_type = internalNginx.getFileFriendlyHostType(host_type); if (config.debug()) { - logger.info('Generating ' + nice_host_type + ' Config:', JSON.stringify(host, null, 2)); + logger.info(`Generating ${nice_host_type} Config:`, JSON.stringify(host, null, 2)); } const renderEngine = utils.getRenderEngine(); return new Promise((resolve, reject) => { - let template = null; - let filename = internalNginx.getConfigName(nice_host_type, host.id); + let template = null; + const filename = internalNginx.getConfigName(nice_host_type, host.id); try { - template = fs.readFileSync(__dirname + '/../templates/' + nice_host_type + '.conf', {encoding: 'utf8'}); + template = fs.readFileSync(`${__dirname}/../templates/${nice_host_type}.conf`, {encoding: 'utf8'}); } catch (err) { reject(new error.ConfigurationError(err.message)); return; @@ -252,7 +252,7 @@ const internalNginx = { }) .catch((err) => { if (config.debug()) { - logger.warn('Could not write ' + filename + ':', err.message); + logger.warn(`Could not write ${filename}:`, err.message); } reject(new error.ConfigurationError(err.message)); @@ -277,11 +277,11 @@ const internalNginx = { const renderEngine = utils.getRenderEngine(); return new Promise((resolve, reject) => { - let template = null; - let filename = '/data/nginx/temp/letsencrypt_' + certificate.id + '.conf'; + let template = null; + const filename = `/data/nginx/temp/letsencrypt_${certificate.id}.conf`; try { - template = fs.readFileSync(__dirname + '/../templates/letsencrypt-request.conf', {encoding: 'utf8'}); + template = fs.readFileSync(`${__dirname}/../templates/letsencrypt-request.conf`, {encoding: 'utf8'}); } catch (err) { reject(new error.ConfigurationError(err.message)); return; @@ -302,7 +302,7 @@ const internalNginx = { }) .catch((err) => { if (config.debug()) { - logger.warn('Could not write ' + filename + ':', err.message); + logger.warn(`Could not write ${filename}:`, err.message); } reject(new error.ConfigurationError(err.message)); @@ -316,7 +316,7 @@ const internalNginx = { * @param {String} filename */ deleteFile: (filename) => { - logger.debug('Deleting file: ' + filename); + logger.debug(`Deleting file: ${filename}`); try { fs.unlinkSync(filename); } catch (err) { @@ -330,7 +330,7 @@ const internalNginx = { * @returns String */ getFileFriendlyHostType: (host_type) => { - return host_type.replace(new RegExp('-', 'g'), '_'); + return host_type.replace(/-/g, '_'); }, /** @@ -340,7 +340,7 @@ const internalNginx = { * @returns {Promise} */ deleteLetsEncryptRequestConfig: (certificate) => { - const config_file = '/data/nginx/temp/letsencrypt_' + certificate.id + '.conf'; + const config_file = `/data/nginx/temp/letsencrypt_${certificate.id}.conf`; return new Promise((resolve/*, reject*/) => { internalNginx.deleteFile(config_file); resolve(); @@ -355,7 +355,7 @@ const internalNginx = { */ deleteConfig: (host_type, host, delete_err_file) => { const config_file = internalNginx.getConfigName(internalNginx.getFileFriendlyHostType(host_type), typeof host === 'undefined' ? 0 : host.id); - const config_file_err = config_file + '.err'; + const config_file_err = `${config_file}.err`; return new Promise((resolve/*, reject*/) => { internalNginx.deleteFile(config_file); @@ -373,7 +373,7 @@ const internalNginx = { */ renameConfigAsError: (host_type, host) => { const config_file = internalNginx.getConfigName(internalNginx.getFileFriendlyHostType(host_type), typeof host === 'undefined' ? 0 : host.id); - const config_file_err = config_file + '.err'; + const config_file_err = `${config_file}.err`; return new Promise((resolve/*, reject*/) => { fs.unlink(config_file, () => { @@ -392,8 +392,8 @@ const internalNginx = { * @returns {Promise} */ bulkGenerateConfigs: (host_type, hosts) => { - let promises = []; - hosts.map(function (host) { + const promises = []; + hosts.map((host) => { promises.push(internalNginx.generateConfig(host_type, host)); }); @@ -406,8 +406,8 @@ const internalNginx = { * @returns {Promise} */ bulkDeleteConfigs: (host_type, hosts) => { - let promises = []; - hosts.map(function (host) { + const promises = []; + hosts.map((host) => { promises.push(internalNginx.deleteConfig(host_type, host, true)); }); @@ -418,14 +418,12 @@ const internalNginx = { * @param {string} config * @returns {boolean} */ - advancedConfigHasDefaultLocation: function (cfg) { - return !!cfg.match(/^(?:.*;)?\s*?location\s*?\/\s*?{/im); - }, + advancedConfigHasDefaultLocation: (cfg) => !!cfg.match(/^(?:.*;)?\s*?location\s*?\/\s*?{/im), /** * @returns {boolean} */ - ipv6Enabled: function () { + ipv6Enabled: () => { if (typeof process.env.DISABLE_IPV6 !== 'undefined') { const disabled = process.env.DISABLE_IPV6.toLowerCase(); return !(disabled === 'on' || disabled === 'true' || disabled === '1' || disabled === 'yes'); diff --git a/backend/lib/utils.js b/backend/lib/utils.js index 66f2dfd9..e2d60778 100644 --- a/backend/lib/utils.js +++ b/backend/lib/utils.js @@ -29,15 +29,19 @@ module.exports = { /** * @param {String} cmd * @param {Array} args + * @param {Object|undefined} options * @returns {Promise} */ - execFile: (cmd, args) => { - // logger.debug('CMD: ' + cmd + ' ' + (args ? args.join(' ') : '')); + execFile: (cmd, args, options) => { + logger.debug(`CMD: ${cmd} ${args ? args.join(' ') : ''}`); + if (typeof options === 'undefined') { + options = {}; + } return new Promise((resolve, reject) => { - execFile(cmd, args, (err, stdout, /*stderr*/) => { + execFile(cmd, args, options, (err, stdout, stderr) => { if (err && typeof err === 'object') { - reject(err); + reject(new error.CommandError(stderr, 1, err)); } else { resolve(stdout.trim()); } diff --git a/backend/routes/nginx/certificates.js b/backend/routes/nginx/certificates.js index bf47c03f..4b10d137 100644 --- a/backend/routes/nginx/certificates.js +++ b/backend/routes/nginx/certificates.js @@ -6,7 +6,7 @@ const apiValidator = require('../../lib/validator/api'); const internalCertificate = require('../../internal/certificate'); const schema = require('../../schema'); -let router = express.Router({ +const router = express.Router({ caseSensitive: true, strict: true, mergeParams: true @@ -231,7 +231,7 @@ router */ router .route('/:certificate_id/download') - .options((req, res) => { + .options((_req, res) => { res.sendStatus(204); }) .all(jwtdecode()) diff --git a/backend/schema/common.json b/backend/schema/common.json index 83de0143..343f125e 100644 --- a/backend/schema/common.json +++ b/backend/schema/common.json @@ -110,6 +110,11 @@ "caching_enabled": { "description": "Should we cache assets", "type": "boolean" + }, + "email": { + "description": "Email address", + "type": "string", + "pattern": "^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,}$" } } } diff --git a/backend/schema/components/certificate-object.json b/backend/schema/components/certificate-object.json index b75dcf61..dcc2a834 100644 --- a/backend/schema/components/certificate-object.json +++ b/backend/schema/components/certificate-object.json @@ -69,7 +69,7 @@ "type": "object" }, "letsencrypt_email": { - "type": "string" + "$ref": "../common.json#/properties/email" }, "propagation_seconds": { "type": "integer", diff --git a/backend/setup.js b/backend/setup.js index 6b9b8e78..fcb4f59c 100644 --- a/backend/setup.js +++ b/backend/setup.js @@ -24,7 +24,7 @@ const setupDefaultUser = () => { const email = process.env.INITIAL_ADMIN_EMAIL || 'admin@example.com'; const password = process.env.INITIAL_ADMIN_PASSWORD || 'changeme'; - logger.info('Creating a new user: ' + email + ' with password: ' + password); + logger.info(`Creating a new user: ${email} with password: ${password}`); const data = { is_deleted: 0, @@ -113,20 +113,20 @@ const setupCertbotPlugins = () => { .andWhere('provider', 'letsencrypt') .then((certificates) => { if (certificates && certificates.length) { - let plugins = []; - let promises = []; + const plugins = []; + const promises = []; - certificates.map(function (certificate) { + certificates.map((certificate) => { if (certificate.meta && certificate.meta.dns_challenge === true) { if (plugins.indexOf(certificate.meta.dns_provider) === -1) { plugins.push(certificate.meta.dns_provider); } // Make sure credentials file exists - const credentials_loc = '/etc/letsencrypt/credentials/credentials-' + certificate.id; + const credentials_loc = `/etc/letsencrypt/credentials/credentials-${certificate.id}`; // Escape single quotes and backslashes const escapedCredentials = certificate.meta.dns_provider_credentials.replaceAll('\'', '\\\'').replaceAll('\\', '\\\\'); - const credentials_cmd = '[ -f \'' + credentials_loc + '\' ] || { mkdir -p /etc/letsencrypt/credentials 2> /dev/null; echo \'' + escapedCredentials + '\' > \'' + credentials_loc + '\' && chmod 600 \'' + credentials_loc + '\'; }'; + const credentials_cmd = `[ -f '${credentials_loc}' ] || { mkdir -p /etc/letsencrypt/credentials 2> /dev/null; echo '${escapedCredentials}' > '${credentials_loc}' && chmod 600 '${credentials_loc}'; }`; promises.push(utils.exec(credentials_cmd)); } }); @@ -136,7 +136,7 @@ const setupCertbotPlugins = () => { if (promises.length) { return Promise.all(promises) .then(() => { - logger.info('Added Certbot plugins ' + plugins.join(', ')); + logger.info(`Added Certbot plugins ${plugins.join(', ')}`); }); } }); @@ -165,9 +165,7 @@ const setupLogrotation = () => { return runLogrotate(); }; -module.exports = function () { - return setupDefaultUser() - .then(setupDefaultSettings) - .then(setupCertbotPlugins) - .then(setupLogrotation); -}; +module.exports = () => setupDefaultUser() + .then(setupDefaultSettings) + .then(setupCertbotPlugins) + .then(setupLogrotation); diff --git a/test/cypress/e2e/api/Certificates.cy.js b/test/cypress/e2e/api/Certificates.cy.js index 9f47edcb..837bde96 100644 --- a/test/cypress/e2e/api/Certificates.cy.js +++ b/test/cypress/e2e/api/Certificates.cy.js @@ -96,4 +96,28 @@ describe('Certificates endpoints', () => { expect(data.error.message).to.contain('data/domain_names/0 must match pattern'); }); }); + + it('Request Certificate - LE Email Escaped', () => { + cy.task('backendApiPost', { + token: token, + path: '/api/nginx/certificates', + data: { + domain_names: ['test.com"||echo hello-world||\\\\n test.com"'], + meta: { + dns_challenge: false, + letsencrypt_agree: true, + letsencrypt_email: "admin@example.com' --version;echo hello-world", + }, + provider: 'letsencrypt', + }, + returnOnError: true, + }).then((data) => { + cy.validateSwaggerSchema('post', 400, '/nginx/certificates', data); + expect(data).to.have.property('error'); + expect(data.error).to.have.property('message'); + expect(data.error).to.have.property('code'); + expect(data.error.code).to.equal(400); + expect(data.error.message).to.contain('data/meta/letsencrypt_email must match pattern'); + }); + }); });