Skip to content

Commit 86a6645

Browse files
masontikhonovmasontikhonov
authored
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
1 parent 0ac8a9b commit 86a6645

14 files changed

+2606
-1511
lines changed

‎.dockerignore

+5-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
.git
22
.gitignore
3+
.github
34
node_modules
45
logs/*.log
56
lib/state.json
6-
*.md
7+
*.md
8+
.eslintrc.json
9+
test
10+
.eslintignore

‎.eslintrc ‎.eslintrc.json

+11-33
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,20 @@
11
{
2-
"extends": "airbnb",
3-
"ignorePatterns":[
4-
"node_modules"
5-
],
2+
"extends": "airbnb-base",
3+
4+
"env": {
5+
"node": true,
6+
"mocha": true,
7+
"es6": true
8+
},
9+
610
"parserOptions": {
7-
"ecmaVersion": 2018,
11+
"ecmaVersion": 2021,
812
"sourceType": "script",
913
"ecmaFeatures": {
1014
"impliedStrict": true
1115
}
1216
},
13-
14-
"env": {
15-
"node": true,
16-
"mocha": true
17-
},
18-
17+
1918
"plugins": [
2019
"chai-friendly",
2120
"import",
@@ -78,7 +77,6 @@
7877
"quote-props": ["error", "consistent"],
7978

8079
"promise/catch-or-return": ["error", { "allowThen": true }],
81-
"promise/no-native": "error",
8280

8381
"mocha/no-exclusive-tests": "error",
8482

@@ -91,25 +89,5 @@
9189
"node/no-deprecated-api": "warn",
9290
"no-useless-constructor": "warn",
9391
"no-return-await": "off"
94-
},
95-
"overrides": [
96-
{
97-
"plugins": ["jest"],
98-
"env": {
99-
"jest": true
100-
},
101-
"files": [
102-
"**/__tests__/**/*.[jt]s?(x)",
103-
"__mocks__/**/*.js",
104-
"**/__mocks__/**/*.js"
105-
],
106-
"rules": {
107-
"jest/no-disabled-tests": "warn",
108-
"jest/no-focused-tests": "error",
109-
"jest/no-identical-title": "error",
110-
"jest/prefer-to-have-length": "warn",
111-
"jest/valid-expect": "error"
112-
}
113-
}
114-
]
92+
}
11593
}

‎lib/addNewMask.js

+45-25
Original file line numberDiff line numberDiff line change
@@ -1,41 +1,61 @@
1-
const rp = require('request-promise');
1+
const { getServerAddress } = require('./helpers');
22

3-
function updateMasks(secret) {
4-
const port = process.env.PORT || 8080;
5-
const host = process.env.HOST || 'localhost';
3+
const exitCodes = {
4+
success: 0,
5+
error: 1,
6+
missingArguments: 2,
7+
unexpectedSuccess: 3,
8+
};
69

7-
const opt = {
8-
uri: `http://${host}:${port}/secrets`,
9-
method: 'POST',
10-
json: true,
11-
body: secret,
12-
resolveWithFullResponse: true,
13-
};
10+
/**
11+
* Unexpected exit with code 0 can lead to the leakage of secrets in the build logs.
12+
* The exit should never be successful unless the secret was successfully masked.
13+
*/
14+
let exitWithError = true;
15+
const exitHandler = (exitCode) => {
16+
if ((!exitCode || !process.exitCode) && exitWithError) {
17+
console.warn(`Unexpected exit with code 0. Exiting with ${exitCodes.unexpectedSuccess} instead`);
18+
process.exitCode = exitCodes.unexpectedSuccess;
19+
}
20+
};
21+
process.on('exit', exitHandler);
1422

15-
rp(opt)
16-
.then((res) => {
17-
if (res.statusCode >= 400) {
18-
console.log(`could not create mask for secret: ${secret.key}, because server responded with: ${res.statusCode}\n\n${res.body}`);
19-
process.exit(1);
20-
}
21-
console.log(`successfully updated masks with secret: ${secret.key}`);
22-
process.exit(0);
23-
})
24-
.catch((err) => {
25-
console.log(`could not create mask for secret: ${secret.key}, due to error: ${err}`);
26-
process.exit(1);
23+
async function updateMasks(secret) {
24+
try {
25+
const serverAddress = await getServerAddress();
26+
console.debug(`server address: ${serverAddress}`);
27+
const url = new URL('secrets', serverAddress);
28+
29+
// eslint-disable-next-line import/no-unresolved
30+
const { default: httpClient } = await import('got');
31+
const response = await httpClient.post(url, {
32+
json: secret,
33+
throwHttpErrors: false,
2734
});
35+
36+
if (response.statusCode === 201) {
37+
console.log(`successfully updated masks with secret: ${secret.key}`);
38+
exitWithError = false;
39+
process.exit(exitCodes.success);
40+
} else {
41+
console.error(`could not create mask for secret: ${secret.key}. Server responded with: ${response.statusCode}\n\n${response.body}`);
42+
process.exit(exitCodes.error);
43+
}
44+
} catch (error) {
45+
console.error(`could not create mask for secret: ${secret.key}. Error: ${error}`);
46+
process.exit(exitCodes.error);
47+
}
2848
}
2949

3050
if (require.main === module) {
3151
// first argument is the secret key second argument is the secret value
3252
if (process.argv.length < 4) {
3353
console.log('not enough arguments, need secret key and secret value');
34-
process.exit(2);
54+
process.exit(exitCodes.missingArguments);
3555
}
3656
const key = process.argv[2];
3757
const value = process.argv[3];
3858
updateMasks({ key, value });
3959
} else {
40-
module.exports = updateMasks;
60+
module.exports = { updateMasks, exitHandler };
4161
}

‎lib/const.js

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
const { tmpdir } = require('node:os');
2+
const { resolve } = require('node:path');
3+
4+
const SERVER_ADDRESS_PATH = resolve(tmpdir(), 'LOGGER_SERVER_ADDRESS');
5+
6+
module.exports = {
7+
SERVER_ADDRESS_PATH,
8+
};

‎lib/helpers.js

+29-3
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
const Q = require('q');
2-
const { stat } = require('fs/promises');
1+
const { stat, writeFile, readFile } = require('node:fs/promises');
32
const path = require('path');
43
const logger = require('cf-logs').Logger('codefresh:containerLogger');
4+
const getPromiseWithResolvers = require('core-js-pure/es/promise/with-resolvers');
55
const { BuildFinishedSignalFilename } = require('./enums');
6+
const { SERVER_ADDRESS_PATH } = require('./const');
67

78
const checkFileInterval = 1000;
89

@@ -27,13 +28,38 @@ function _watchForBuildFinishedSignal(deferred) {
2728
}
2829

2930
function watchForBuildFinishedSignal() {
30-
const deferred = Q.defer();
31+
const deferred = getPromiseWithResolvers();
3132

3233
_watchForBuildFinishedSignal(deferred);
3334

3435
return deferred.promise;
3536
}
3637

38+
const saveServerAddress = async (serverAddress) => {
39+
try {
40+
await writeFile(SERVER_ADDRESS_PATH, serverAddress, { encoding: 'utf8' });
41+
} catch (error) {
42+
logger.error(`Failed to save server address: ${error}`);
43+
throw error;
44+
}
45+
};
46+
47+
const getServerAddress = async () => {
48+
try {
49+
return await readFile(SERVER_ADDRESS_PATH, { encoding: 'utf8' });
50+
} catch (error) {
51+
logger.error(`Failed to read server address: ${error}`);
52+
throw error;
53+
}
54+
};
55+
3756
module.exports = {
57+
/**
58+
* Polyfill of `Promise.withResolvers`, TC39 Stage 4 proposal.
59+
* @see https://github.com/tc39/proposal-promise-with-resolvers
60+
*/
61+
getPromiseWithResolvers,
3862
watchForBuildFinishedSignal,
63+
saveServerAddress,
64+
getServerAddress,
3965
};

‎lib/index.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const path = require('path');
1+
const path = require('node:path');
22
const cflogs = require('cf-logs');
33

44
const loggerOptions = {

‎lib/logger.js

+48-33
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,17 @@
11
const fs = require('fs');
22
const { EventEmitter } = require('events');
33
const _ = require('lodash');
4-
const Q = require('q');
54
const Docker = require('dockerode');
65
const DockerEvents = require('docker-events');
7-
const bodyParser = require('body-parser');
86
const CFError = require('cf-errors');
97
const logger = require('cf-logs').Logger('codefresh:containerLogger');
108
const { TaskLogger } = require('@codefresh-io/task-logger');
11-
const express = require('express');
9+
const fastify = require('fastify');
1210
const { ContainerStatus } = require('./enums');
1311
const { LoggerStrategy } = require('./enums');
1412
const { ContainerHandlingStatus } = require('./enums');
1513
const ContainerLogger = require('./ContainerLogger');
14+
const { getPromiseWithResolvers, saveServerAddress } = require('./helpers');
1615

1716
const initialState = {
1817
pid: process.pid, status: 'init', lastLogsDate: new Date(), failedHealthChecks: [], restartCounter: 0, containers: {}
@@ -35,7 +34,7 @@ class Logger {
3534
this.containerLoggers = [];
3635
this.totalLogSize = 0;
3736
this.taskLogger = undefined;
38-
this.buildFinishedPromise = buildFinishedPromise || Q.resolve();
37+
this.buildFinishedPromise = buildFinishedPromise || Promise.resolve();
3938
this.finishedContainers = 0;
4039
this.finishedContainersEmitter = new EventEmitter();
4140
this.showProgress = showProgress;
@@ -77,7 +76,7 @@ class Logger {
7776
* will attach it self to all existing containers if requested
7877
* the container label should be 'io.codefresh.loggerId'
7978
*/
80-
start() {
79+
async start() {
8180

8281
logger.info(`Logging container created for logger id: ${this.loggerId}`);
8382

@@ -124,7 +123,7 @@ class Logger {
124123

125124
});
126125

127-
this._listenForEngineUpdates();
126+
await this._listenForEngineUpdates();
128127
}
129128

130129
_readState() {
@@ -188,9 +187,11 @@ class Logger {
188187
const receivedLoggerId = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.logger.id'];
189188
const runCreationLogic = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.runCreationLogic'];
190189
const stepName = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.logger.stepName'];
191-
const receivedLogSizeLimit = _.get(container,
190+
const receivedLogSizeLimit = _.get(
191+
container,
192192
'Labels',
193-
_.get(container, 'Actor.Attributes'))['io.codefresh.logger.logSizeLimit'];
193+
_.get(container, 'Actor.Attributes')
194+
)['io.codefresh.logger.logSizeLimit'];
194195
const loggerStrategy = _.get(container, 'Labels', _.get(container, 'Actor.Attributes'))['io.codefresh.logger.strategy'];
195196

196197
if (!containerId) {
@@ -350,31 +351,45 @@ class Logger {
350351
});
351352
}
352353

353-
_listenForEngineUpdates() {
354-
const app = express();
355-
this._app = app;
356-
const port = process.env.PORT || 8080;
357-
const host = process.env.HOST || 'localhost';
358-
359-
app.use(bodyParser.json());
360-
361-
app.post('/secrets', (req, res) => {
362-
try {
363-
const secret = req.body;
364-
logger.info(`got request to add new mask: ${JSON.stringify(secret)}`);
365-
366-
// secret must have { key, value } structure
367-
this.taskLogger.addNewMask(secret);
368-
res.status(201).end('secret added');
369-
} catch (err) {
370-
logger.info(`could not create new mask due to error: ${err}`);
371-
res.status(400).end(err);
372-
}
373-
});
354+
async _listenForEngineUpdates() {
355+
try {
356+
const port = +(process.env.PORT || 8080);
357+
const host = process.env.HOST || '0.0.0.0';
358+
359+
const server = fastify();
360+
const secretsOptions = {
361+
schema: {
362+
body: {
363+
type: 'object',
364+
required: ['key', 'value'],
365+
properties: {
366+
key: { type: 'string' },
367+
value: { type: 'string' },
368+
},
369+
},
370+
},
371+
};
372+
server.post('/secrets', secretsOptions, async (request, reply) => {
373+
try {
374+
const { body: secret } = request;
375+
logger.info(`got request to add new mask: ${secret.key}`);
376+
this.taskLogger.addNewMask(secret);
377+
reply.code(201);
378+
return 'secret added';
379+
} catch (err) {
380+
logger.info(`could not create new mask for due to error: ${err}`);
381+
reply.code(500);
382+
throw err;
383+
}
384+
});
374385

375-
app.listen(port, host, () => {
376-
logger.info(`listening for engine updates on ${host}:${port}`);
377-
});
386+
const address = await server.listen({ host, port });
387+
await saveServerAddress(address);
388+
logger.info(`listening for engine updates on ${address}`);
389+
} catch (error) {
390+
logger.error(`could not start server for engine updates due to error: ${error}`);
391+
throw error;
392+
}
378393
}
379394

380395
_handleContainerStreamEnd(containerId) {
@@ -385,7 +400,7 @@ class Logger {
385400

386401
// do not call before build is finished
387402
_awaitAllStreamsClosed() {
388-
const deferred = Q.defer();
403+
const deferred = getPromiseWithResolvers();
389404
this._checkAllStreamsClosed(deferred);
390405
this.finishedContainersEmitter.on('end', this._checkAllStreamsClosed.bind(this, deferred));
391406
return deferred.promise;

‎no-onlys.sh

-8
This file was deleted.

0 commit comments

Comments
 (0)