fix(secrets): fix an issue with secret server listening on IPv6 (#134)

* fix(secrets): fix an issue with secret server listening on IPv6

* ci: bump version

* feat(secrets): use precise address in `updateMasks()`

* refactor(secrets): replace `request` with `got`

* fix(secrets): store server address on fs

* test(secrets): add tests for helpers

* build: upgrade eslint, clean up dev env

* build: upgrade dependencies

* fix(addNewMask): fail by default

* tests: fix Logger tests

Author: masontikhonov

.dockerignore

@@ -1,6 +1,10 @@
 .git
 .gitignore
+.github
 node_modules
 logs/*.log
 lib/state.json
-*.md
+*.md
+.eslintrc.json
+test
+.eslintignore

.eslintrc renamed to .eslintrc.json

@@ -1,21 +1,20 @@
 {
-  "extends": "airbnb",
-  "ignorePatterns":[
-    "node_modules"
-  ],
+  "extends": "airbnb-base",
+
+  "env": {
+    "node": true,
+    "mocha": true,
+    "es6": true
+  },
+
   "parserOptions": {
-    "ecmaVersion": 2018,
+    "ecmaVersion": 2021,
     "sourceType": "script",
     "ecmaFeatures": {
       "impliedStrict": true
     }
   },
-
-  "env": {
-    "node": true,
-    "mocha": true
-  },
-
+  
   "plugins": [
     "chai-friendly",
     "import",
@@ -78,7 +77,6 @@
     "quote-props": ["error", "consistent"],
 
     "promise/catch-or-return": ["error", { "allowThen": true }],
-    "promise/no-native": "error",
 
     "mocha/no-exclusive-tests": "error",
 
@@ -91,25 +89,5 @@
     "node/no-deprecated-api": "warn",
     "no-useless-constructor": "warn",
     "no-return-await": "off"
-  },
-  "overrides": [
-    {
-      "plugins": ["jest"],
-      "env": {
-        "jest": true
-      },
-      "files": [
-        "**/__tests__/**/*.[jt]s?(x)",
-        "__mocks__/**/*.js",
-        "**/__mocks__/**/*.js"
-      ],
-      "rules": {
-        "jest/no-disabled-tests": "warn",
-        "jest/no-focused-tests": "error",
-        "jest/no-identical-title": "error",
-        "jest/prefer-to-have-length": "warn",
-        "jest/valid-expect": "error"
-      }
-    }
-  ]
+  }
 }

lib/addNewMask.js

@@ -1,41 +1,61 @@
-const rp = require('request-promise');
+const { getServerAddress } = require('./helpers');
 
-function updateMasks(secret) {
-    const port = process.env.PORT || 8080;
-    const host = process.env.HOST || 'localhost';
+const exitCodes = {
+    success: 0,
+    error: 1,
+    missingArguments: 2,
+    unexpectedSuccess: 3,
+};
 
-    const opt = {
-        uri: `http://${host}:${port}/secrets`,
-        method: 'POST',
-        json: true,
-        body: secret,
-        resolveWithFullResponse: true,
-    };
+/**
+ * Unexpected exit with code 0 can lead to the leakage of secrets in the build logs.
+ * The exit should never be successful unless the secret was successfully masked.
+ */
+let exitWithError = true;
+const exitHandler = (exitCode) => {
+    if ((!exitCode || !process.exitCode) && exitWithError) {
+        console.warn(`Unexpected exit with code 0. Exiting with ${exitCodes.unexpectedSuccess} instead`);
+        process.exitCode = exitCodes.unexpectedSuccess;
+    }
+};
+process.on('exit', exitHandler);
 
-    rp(opt)
-        .then((res) => {
-            if (res.statusCode >= 400) {
-                console.log(`could not create mask for secret: ${secret.key}, because server responded with: ${res.statusCode}\n\n${res.body}`);
-                process.exit(1);
-            }
-            console.log(`successfully updated masks with secret: ${secret.key}`);
-            process.exit(0);
-        })
-        .catch((err) => {
-            console.log(`could not create mask for secret: ${secret.key}, due to error: ${err}`);
-            process.exit(1);
+async function updateMasks(secret) {
+    try {
+        const serverAddress = await getServerAddress();
+        console.debug(`server address: ${serverAddress}`);
+        const url = new URL('secrets', serverAddress);
+
+        // eslint-disable-next-line import/no-unresolved
+        const { default: httpClient } = await import('got');
+        const response = await httpClient.post(url, {
+            json: secret,
+            throwHttpErrors: false,
         });
+
+        if (response.statusCode === 201) {
+            console.log(`successfully updated masks with secret: ${secret.key}`);
+            exitWithError = false;
+            process.exit(exitCodes.success);
+        } else {
+            console.error(`could not create mask for secret: ${secret.key}. Server responded with: ${response.statusCode}\n\n${response.body}`);
+            process.exit(exitCodes.error);
+        }
+    } catch (error) {
+        console.error(`could not create mask for secret: ${secret.key}. Error: ${error}`);
+        process.exit(exitCodes.error);
+    }
 }
 
 if (require.main === module) {
     // first argument is the secret key second argument is the secret value
     if (process.argv.length < 4) {
         console.log('not enough arguments, need secret key and secret value');
-        process.exit(2);
+        process.exit(exitCodes.missingArguments);
     }
     const key = process.argv[2];
     const value = process.argv[3];
     updateMasks({ key, value });
 } else {
-    module.exports = updateMasks;
+    module.exports = { updateMasks, exitHandler };
 }

lib/const.js

@@ -0,0 +1,8 @@
+const { tmpdir } = require('node:os');
+const { resolve } = require('node:path');
+
+const SERVER_ADDRESS_PATH = resolve(tmpdir(), 'LOGGER_SERVER_ADDRESS');
+
+module.exports = {
+    SERVER_ADDRESS_PATH,
+};

lib/helpers.js

@@ -1,8 +1,9 @@
-const Q = require('q');
-const { stat } = require('fs/promises');
+const { stat, writeFile, readFile } = require('node:fs/promises');
 const path = require('path');
 const logger = require('cf-logs').Logger('codefresh:containerLogger');
+const getPromiseWithResolvers = require('core-js-pure/es/promise/with-resolvers');
 const { BuildFinishedSignalFilename } = require('./enums');
+const { SERVER_ADDRESS_PATH } = require('./const');
 
 const checkFileInterval = 1000;
 
@@ -27,13 +28,38 @@ function _watchForBuildFinishedSignal(deferred) {
 }
 
 function watchForBuildFinishedSignal() {
-    const deferred = Q.defer();
+    const deferred = getPromiseWithResolvers();
 
     _watchForBuildFinishedSignal(deferred);
 
     return deferred.promise;
 }
 
+const saveServerAddress = async (serverAddress) => {
+    try {
+        await writeFile(SERVER_ADDRESS_PATH, serverAddress, { encoding: 'utf8' });
+    } catch (error) {
+        logger.error(`Failed to save server address: ${error}`);
+        throw error;
+    }
+};
+
+const getServerAddress = async () => {
+    try {
+        return await readFile(SERVER_ADDRESS_PATH, { encoding: 'utf8' });
+    } catch (error) {
+        logger.error(`Failed to read server address: ${error}`);
+        throw error;
+    }
+};
+
 module.exports = {
+    /**
+     * Polyfill of `Promise.withResolvers`, TC39 Stage 4 proposal.
+     * @see https://github.com/tc39/proposal-promise-with-resolvers
+     */
+    getPromiseWithResolvers,
     watchForBuildFinishedSignal,
+    saveServerAddress,
+    getServerAddress,
 };

lib/index.js

@@ -1,4 +1,4 @@
-const path = require('path');
+const path = require('node:path');
 const cflogs = require('cf-logs');
 
 const loggerOptions = {

lib/logger.js

@@ -1,18 +1,17 @@
 const fs = require('fs');
 const { EventEmitter } = require('events');
 const _ = require('lodash');
-const Q = require('q');
 const Docker = require('dockerode');
 const DockerEvents = require('docker-events');
-const bodyParser = require('body-parser');
 const CFError = require('cf-errors');
 const logger = require('cf-logs').Logger('codefresh:containerLogger');
 const { TaskLogger } = require('@codefresh-io/task-logger');
-const express = require('express');
+const fastify = require('fastify');
 const { ContainerStatus } = require('./enums');
 const { LoggerStrategy } = require('./enums');
 const { ContainerHandlingStatus } = require('./enums');
 const ContainerLogger = require('./ContainerLogger');
+const { getPromiseWithResolvers, saveServerAddress } = require('./helpers');
 
 const initialState = {
     pid: process.pid, status: 'init', lastLogsDate: new Date(), failedHealthChecks: [], restartCounter: 0, containers: {}
@@ -35,7 +34,7 @@ class Logger {
         this.containerLoggers = [];
         this.totalLogSize = 0;
         this.taskLogger = undefined;
-        this.buildFinishedPromise = buildFinishedPromise || Q.resolve();
+        this.buildFinishedPromise = buildFinishedPromise || Promise.resolve();
         this.finishedContainers = 0;
         this.finishedContainersEmitter = new EventEmitter();
         this.showProgress = showProgress;
@@ -77,7 +76,7 @@ class Logger {
      * will attach it self to all existing containers if requested
      * the container label should be 'io.codefresh.loggerId'
      */
-    start() {
+    async start() {
 
         logger.info(`Logging container created for logger id: ${this.loggerId}`);
 
@@ -124,7 +123,7 @@ class Logger {
 
             });
 
-        this._listenForEngineUpdates();
+        await this._listenForEngineUpdates();
     }
 
     _readState() {
@@ -188,9 +187,11 @@ class Logger {
         const receivedLoggerId = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.logger.id'];
         const runCreationLogic = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.runCreationLogic'];
         const stepName = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.logger.stepName'];
-        const receivedLogSizeLimit = _.get(container,
+        const receivedLogSizeLimit = _.get(
+            container,
             'Labels',
-            _.get(container, 'Actor.Attributes'))['io.codefresh.logger.logSizeLimit'];
+            _.get(container, 'Actor.Attributes')
+        )['io.codefresh.logger.logSizeLimit'];
         const loggerStrategy = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.logger.strategy'];
 
         if (!containerId) {
@@ -350,31 +351,45 @@ class Logger {
         });
     }
 
-    _listenForEngineUpdates() {
-        const app = express();
-        this._app = app;
-        const port = process.env.PORT || 8080;
-        const host = process.env.HOST || 'localhost';
-
-        app.use(bodyParser.json());
-
-        app.post('/secrets', (req, res) => {
-            try {
-                const secret = req.body;
-                logger.info(`got request to add new mask: ${JSON.stringify(secret)}`);
-
-                // secret must have { key, value } structure
-                this.taskLogger.addNewMask(secret);
-                res.status(201).end('secret added');
-            } catch (err) {
-                logger.info(`could not create new mask due to error: ${err}`);
-                res.status(400).end(err);
-            }
-        });
+    async _listenForEngineUpdates() {
+        try {
+            const port = +(process.env.PORT || 8080);
+            const host = process.env.HOST || '0.0.0.0';
+
+            const server = fastify();
+            const secretsOptions = {
+                schema: {
+                    body: {
+                        type: 'object',
+                        required: ['key', 'value'],
+                        properties: {
+                            key: { type: 'string' },
+                            value: { type: 'string' },
+                        },
+                    },
+                },
+            };
+            server.post('/secrets', secretsOptions, async (request, reply) => {
+                try {
+                    const { body: secret } = request;
+                    logger.info(`got request to add new mask: ${secret.key}`);
+                    this.taskLogger.addNewMask(secret);
+                    reply.code(201);
+                    return 'secret added';
+                } catch (err) {
+                    logger.info(`could not create new mask for due to error: ${err}`);
+                    reply.code(500);
+                    throw err;
+                }
+            });
 
-        app.listen(port, host, () => {
-            logger.info(`listening for engine updates on ${host}:${port}`);
-        });
+            const address = await server.listen({ host, port });
+            await saveServerAddress(address);
+            logger.info(`listening for engine updates on ${address}`);
+        } catch (error) {
+            logger.error(`could not start server for engine updates due to error: ${error}`);
+            throw error;
+        }
     }
 
     _handleContainerStreamEnd(containerId) {
@@ -385,7 +400,7 @@ class Logger {
 
     // do not call before build is finished
     _awaitAllStreamsClosed() {
-        const deferred = Q.defer();
+        const deferred = getPromiseWithResolvers();
         this._checkAllStreamsClosed(deferred);
         this.finishedContainersEmitter.on('end', this._checkAllStreamsClosed.bind(this, deferred));
         return deferred.promise;