From b912768923a02349a312e54b84bf3e5fc79054da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Mon, 13 Apr 2020 22:38:46 +0200 Subject: [PATCH] feat: move secrets to settings (#577) * feat: move secrets to settings * feat: Add better support for detailed db options. Added db field in options to allow better control of db-options. Especially important to allow special chars in database password which might lead to an invaid url when defined as a database-url. * fix: integrate logger with knex logger * fix: remove secret option from all examples * fix: more options.js unit tests * fix: added settings-store e2e tests --- docs/getting-started.md | 1 - docs/securing-unleash.md | 5 +- examples/basic-auth-unleash.js | 1 - examples/google-auth-unleash.js | 1 - examples/keycloak-auth-unleash.js | 1 - lib/db/db-pool.js | 18 ++- lib/db/index.js | 2 + lib/db/setting-store.js | 52 ++++++++ lib/options.js | 40 +++--- lib/options.test.js | 122 ++++++++++++++++--- lib/server-impl.js | 3 + lib/server-impl.test.js | 6 + migrations/20200329191251-settings-secret.js | 21 ++++ migrator.js | 6 +- package.json | 3 +- server-dev.js | 15 +++ test/e2e/helpers/database-config.js | 15 ++- test/e2e/helpers/database-init.js | 2 +- test/e2e/stores/setting-store.e2e.test.js | 28 +++++ test/fixtures/fake-setting-store.js | 6 + test/fixtures/store.js | 2 + yarn.lock | 5 + 22 files changed, 295 insertions(+), 60 deletions(-) create mode 100644 lib/db/setting-store.js create mode 100644 migrations/20200329191251-settings-secret.js create mode 100644 server-dev.js create mode 100644 test/e2e/stores/setting-store.e2e.test.js create mode 100644 test/fixtures/fake-setting-store.js diff --git a/docs/getting-started.md b/docs/getting-started.md index 5c4607c162..c1599ebd51 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -52,7 +52,6 @@ Available unleash options include: - **serverMetrics** (boolean) - use this option to turn on/off prometheus metrics. - **preHook** (function) - this is a hook if you need to provide any middlewares to express before `unleash` adds any. Express app instance is injected as first argument. - **preRouterHook** (function) - use this to register custom express middlewares before the `unleash` specific routers are added. This is typically how you would register custom middlewares to handle authentication. -- **secret** (string) - set this when you want to secure unleash. Used to encrypt the user session. - **adminAuthentication** (string) - use this when implementing custom admin authentication [securing-unleash](./securing-unleash.md). Possible values are: - `none` - will disable authentication altogether - `unsecure` - (default) will use simple cookie based authentication. UI will require the user to specify an email in order to use unleash. diff --git a/docs/securing-unleash.md b/docs/securing-unleash.md index 1c5e7a845e..52e06a3507 100644 --- a/docs/securing-unleash.md +++ b/docs/securing-unleash.md @@ -7,11 +7,11 @@ The Unleash API is split into two different paths: `/api/client` and `/api/admin ## General settings -Unleash uses an encrypted cookie to maintain a user session. This allows users to be logged in across multiple instances of Unleash. To protect this cookie, you should specify the `secret` option when starting Unleash. +Unleash uses an encrypted cookie to maintain a user session. This allows users to be logged in across multiple instances of Unleash. To protect this cookie, Unleash will automatically generate a secure token the first time you start Unleash. ## Securing the Admin API -To secure the Admin API, you have to tell Unleash that you are using a custom admin authentication and implement your authentication logic as a preHook. You should also set the secret option to a protected secret in your system. +To secure the Admin API, you have to tell Unleash that you are using a custom admin authentication and implement your authentication logic as a preHook. ```javascript const unleash = require('unleash-server'); @@ -20,7 +20,6 @@ const myCustomAdminAuth = require('./auth-hook'); unleash .start({ databaseUrl: 'postgres://unleash_user:passord@localhost:5432/unleash', - secret: 'super-duper-secret', adminAuthentication: 'custom', preRouterHook: myCustomAdminAuth, }) diff --git a/examples/basic-auth-unleash.js b/examples/basic-auth-unleash.js index 0b525d87c6..84c15f7c4a 100644 --- a/examples/basic-auth-unleash.js +++ b/examples/basic-auth-unleash.js @@ -8,7 +8,6 @@ const basicAuth = require('./basic-auth-hook'); unleash .start({ databaseUrl: 'postgres://unleash_user:passord@localhost:5432/unleash', - secret: 'super-duper-secret', adminAuthentication: 'custom', preRouterHook: basicAuth, }) diff --git a/examples/google-auth-unleash.js b/examples/google-auth-unleash.js index 7b0e2a26da..12d0c2b157 100644 --- a/examples/google-auth-unleash.js +++ b/examples/google-auth-unleash.js @@ -8,7 +8,6 @@ const enableGoogleOauth = require('./google-auth-hook'); unleash .start({ databaseUrl: 'postgres://unleash_user:passord@localhost:5432/unleash', - secret: 'super-duper-secret', adminAuthentication: 'custom', preRouterHook: enableGoogleOauth, }) diff --git a/examples/keycloak-auth-unleash.js b/examples/keycloak-auth-unleash.js index 0e2e8ec5c7..5ac6b14005 100644 --- a/examples/keycloak-auth-unleash.js +++ b/examples/keycloak-auth-unleash.js @@ -8,7 +8,6 @@ const enableKeycloak = require('./keycloak-auth-hook'); unleash .start({ databaseUrl: 'postgres://unleash_user:passord@localhost:5432/unleash', - secret: 'super-duper-secret', adminAuthentication: 'custom', preRouterHook: enableKeycloak, }) diff --git a/lib/db/db-pool.js b/lib/db/db-pool.js index fca93aafe4..3b73b1d7ea 100644 --- a/lib/db/db-pool.js +++ b/lib/db/db-pool.js @@ -3,17 +3,23 @@ const knex = require('knex'); module.exports.createDb = function({ - databaseUrl, + db, poolMin = 2, poolMax = 20, - databaseSchema = 'public', + databaseSchema, + getLogger, }) { - const db = knex({ + const logger = getLogger('db-pool.js'); + return knex({ client: 'pg', - connection: databaseUrl, + connection: db, pool: { min: poolMin, max: poolMax }, searchPath: databaseSchema, + log: { + debug: msg => logger.debug(msg), + info: msg => logger.info(msg), + warn: msg => logger.warn(msg), + error: msg => logger.error(msg), + }, }); - - return db; }; diff --git a/lib/db/index.js b/lib/db/index.js index e782adf4f5..c1d0f33808 100644 --- a/lib/db/index.js +++ b/lib/db/index.js @@ -9,6 +9,7 @@ const ClientMetricsDb = require('./client-metrics-db'); const ClientMetricsStore = require('./client-metrics-store'); const ClientApplicationsStore = require('./client-applications-store'); const ContextFieldStore = require('./context-field-store'); +const SettingStore = require('./setting-store'); module.exports.createStores = (config, eventBus) => { const getLogger = config.getLogger; @@ -38,5 +39,6 @@ module.exports.createStores = (config, eventBus) => { eventStore, getLogger ), + settingStore: new SettingStore(db, getLogger), }; }; diff --git a/lib/db/setting-store.js b/lib/db/setting-store.js new file mode 100644 index 0000000000..db07b9da70 --- /dev/null +++ b/lib/db/setting-store.js @@ -0,0 +1,52 @@ +/* eslint camelcase: "off" */ +'use strict'; + +const TABLE = 'settings'; + +class SettingStore { + constructor(db, getLogger) { + this.db = db; + this.logger = getLogger('settings-store.js'); + } + + updateRow(name, content) { + return this.db(TABLE) + .where('name', name) + .update({ + content: JSON.stringify(content), + }); + } + + insertNewRow(name, content) { + return this.db(TABLE).insert({ name, content }); + } + + insert(name, content) { + return this.db(TABLE) + .count('*') + .where('name', name) + .map(row => ({ count: row.count })) + .then(rows => { + if (rows[0].count > 0) { + return this.updateRow(name, content); + } else { + return this.insertNewRow(name, content); + } + }); + } + + async get(name) { + const result = await this.db + .select() + .from(TABLE) + .where('name', name); + + if (result.length > 0) { + return result[0].content; + } else { + return undefined; + } + } +} + +module.exports = SettingStore; diff --git a/lib/options.js b/lib/options.js index 8d1652fb83..ac48e86e95 100644 --- a/lib/options.js +++ b/lib/options.js @@ -1,16 +1,26 @@ 'use strict'; +const { readFileSync } = require('fs'); +const parseDbUrl = require('parse-database-url'); +const merge = require('deepmerge'); const { publicFolder } = require('unleash-frontend'); const { defaultLogProvider, validateLogProvider } = require('./logger'); -const fs = require('fs'); -const isDev = () => process.env.NODE_ENV === 'development'; const THIRTY_DAYS = 30 * 24 * 60 * 60 * 1000; function defaultOptions() { return { databaseUrl: defaultDatabaseUrl(), databaseSchema: 'public', + db: { + user: process.env.DATABASE_USERNAME, + password: process.env.DATABASE_PASSWORD, + host: process.env.DATABASE_HOST, + port: process.env.DATABASE_PORT || 5432, + database: process.env.DATABASE_NAME || 'unleash', + ssl: process.env.DATABASE_SSL, + driver: 'postgres', + }, port: process.env.HTTP_PORT || process.env.PORT || 4242, host: process.env.HTTP_HOST, pipe: undefined, @@ -19,8 +29,7 @@ function defaultOptions() { enableLegacyRoutes: true, extendedPermissions: false, publicFolder, - enableRequestLogger: isDev(), - secret: 'UNLEASH-SECRET', + enableRequestLogger: false, sessionAge: THIRTY_DAYS, adminAuthentication: 'unsecure', ui: {}, @@ -34,29 +43,26 @@ function defaultOptions() { function defaultDatabaseUrl() { if (process.env.DATABASE_URL_FILE) { - return fs.readFileSync(process.env.DATABASE_URL_FILE, 'utf8'); + return readFileSync(process.env.DATABASE_URL_FILE, 'utf8'); } else if (process.env.DATABASE_URL) { return process.env.DATABASE_URL; - } else if (isDev() || process.env.DATABASE_HOST) { - const dbUsername = process.env.DATABASE_USERNAME || 'unleash_user'; - const dbPassword = process.env.DATABASE_PASSWORD || 'passord'; - const dbHost = process.env.DATABASE_HOST || 'localhost'; - const dbPort = process.env.DATABASE_PORT || 5432; - const dbName = process.env.DATABASE_NAME || 'unleash'; - const sslSupport = process.env.DATABASE_SSL || 'true'; - return `postgres://${dbUsername}:${dbPassword}@${dbHost}:${dbPort}/${dbName}?ssl=${sslSupport}`; } else { return undefined; } } module.exports = { - createOptions: opts => { - const options = Object.assign({}, defaultOptions(), opts); + createOptions: (opts = {}) => { + const options = merge(defaultOptions(), opts); - if (!options.databaseUrl) { + // Use DATABASE_URL when 'db' not defined. + if (!opts.db && options.databaseUrl) { + options.db = parseDbUrl(options.databaseUrl); + } + + if (!options.db.host) { throw new Error( - 'You must either pass databaseUrl option or set environment variable DATABASE_URL || (DATABASE_HOST, DATABASE_PORT, DATABASE_USERNAME, DATABASE_PASSWORD, DATABASE_NAME)' + `Unleash requires database details to start. See https://unleash.github.io/docs/getting_started` ); } diff --git a/lib/options.test.js b/lib/options.test.js index f5d38f13a8..474126f0a0 100644 --- a/lib/options.test.js +++ b/lib/options.test.js @@ -13,19 +13,6 @@ test('should require DATABASE_URI', t => { }); }); -test('should set default databaseUrl for development', t => { - delete process.env.NODE_ENV; - process.env.NODE_ENV = 'development'; - const { createOptions } = require('./options'); - - const options = createOptions({}); - - t.true( - options.databaseUrl === - 'postgres://unleash_user:passord@localhost:5432/unleash?ssl=true' - ); -}); - test('should use DATABASE_URL from env', t => { const databaseUrl = 'postgres://u:p@localhost:5432/name'; delete process.env.NODE_ENV; @@ -60,12 +47,115 @@ test('should use databaseUrl from options', t => { }); test('should not override provided options', t => { - process.env.DATABASE_URL = 'test'; + process.env.DATABASE_URL = 'postgres://test:5432/name'; process.env.NODE_ENV = 'production'; const { createOptions } = require('./options'); - const options = createOptions({ databaseUrl: 'test', port: 1111 }); + const options = createOptions({ + databaseUrl: 'postgres://test:5432/name', + port: 1111, + }); - t.true(options.databaseUrl === 'test'); + t.true(options.databaseUrl === 'postgres://test:5432/name'); t.true(options.port === 1111); }); + +test('should add listen options from host and port', t => { + const { createOptions } = require('./options'); + const options = createOptions({ + databaseUrl: 'postgres://test:5432/name', + port: 1111, + host: 'localhost', + }); + + t.deepEqual(options.listen, { port: 1111, host: 'localhost' }); +}); + +test('should use pipe to path', t => { + const { createOptions } = require('./options'); + const options = createOptions({ + databaseUrl: 'postgres://test:5432/name', + port: 1111, + host: 'localhost', + pipe: '\\\\?\\pipe', + }); + + t.deepEqual(options.listen, { path: options.pipe }); +}); + +test('should prefer databaseUrl from options', t => { + process.env.DATABASE_URL = 'postgres://test:5432/name'; + const databaseUrl = 'postgres://u:p@localhost:5432/options'; + const { createOptions } = require('./options'); + + const options = createOptions({ databaseUrl }); + + t.deepEqual(options.databaseUrl, databaseUrl); +}); + +test('should expand databaseUrl from options', t => { + process.env.DATABASE_URL = 'postgres://test:5432/name'; + const databaseUrl = 'postgres://u:p@localhost:5432/options'; + const { createOptions } = require('./options'); + + const options = createOptions({ databaseUrl }); + + t.deepEqual(options.db, { + database: 'options', + driver: 'postgres', + host: 'localhost', + password: 'p', + port: '5432', + user: 'u', + }); +}); + +test('should validate getLogger', t => { + const databaseUrl = 'postgres://u:p@localhost:5432/options'; + const getLogger = () => {}; + const { createOptions } = require('./options'); + + t.throws(() => { + createOptions({ databaseUrl, getLogger }); + }); +}); + +test('should accept custome log-provider', t => { + const databaseUrl = 'postgres://u:p@localhost:5432/options'; + const getLogger = () => ({ + debug: console.log, + info: console.log, + warn: console.log, + error: console.log, + }); + const { createOptions } = require('./options'); + const options = createOptions({ databaseUrl, getLogger }); + + t.deepEqual(options.getLogger, getLogger); +}); + +test('should prefer custom db connection options', t => { + const databaseUrl = 'postgres://u:p@localhost:5432/options'; + const db = { + user: 'db_user', + password: 'db_password', + host: 'db-host', + port: 3232, + database: 'db_database', + ssl: false, + driver: 'postgres', + }; + const { createOptions } = require('./options'); + const options = createOptions({ databaseUrl, db }); + + t.deepEqual(options.db, db); +}); + +test('should baseUriPath', t => { + const databaseUrl = 'postgres://u:p@localhost:5432/options'; + const baseUriPath = 'some'; + const { createOptions } = require('./options'); + const options = createOptions({ databaseUrl, baseUriPath }); + + t.deepEqual(options.baseUriPath, baseUriPath); +}); diff --git a/lib/server-impl.js b/lib/server-impl.js index 018655caaf..e6e1414d33 100644 --- a/lib/server-impl.js +++ b/lib/server-impl.js @@ -18,11 +18,13 @@ async function createApp(options) { const logger = options.getLogger('server-impl.js'); const eventBus = new EventEmitter(); const stores = createStores(options, eventBus); + const secret = await stores.settingStore.get('unleash.secret'); const config = Object.assign( { stores, eventBus, + secret, logFactory: options.getLogger, // TODO: remove in v4.x }, options @@ -59,6 +61,7 @@ async function createApp(options) { server.on('listening', () => resolve({ app, + stores, server, eventBus, stateService, diff --git a/lib/server-impl.test.js b/lib/server-impl.test.js index adde6d2367..ebcf06af88 100644 --- a/lib/server-impl.test.js +++ b/lib/server-impl.test.js @@ -15,6 +15,11 @@ const getApp = proxyquire('./app', { }); const eventStore = new EventEmitter(); +const settingStore = { + get: () => { + Promise.resolve('secret'); + }, +}; const serverImpl = proxyquire('./server-impl', { './app': getApp, @@ -27,6 +32,7 @@ const serverImpl = proxyquire('./server-impl', { createStores() { return { eventStore, + settingStore, }; }, }, diff --git a/migrations/20200329191251-settings-secret.js b/migrations/20200329191251-settings-secret.js new file mode 100644 index 0000000000..069ae2c792 --- /dev/null +++ b/migrations/20200329191251-settings-secret.js @@ -0,0 +1,21 @@ +/* eslint camelcase: "off" */ +'use strict'; + +const crypto = require('crypto'); + +const settingsName = 'unleash.secret'; + +exports.up = function(db, cb) { + const secret = crypto.randomBytes(20).toString('hex'); + + db.runSql( + ` + INSERT INTO settings(name, content) + VALUES('${settingsName}', '${JSON.stringify(secret)}')`, + cb + ); +}; + +exports.down = function(db, cb) { + db.runSql(`DELETE FROM settings WHERE name = '${settingsName}'`, cb); +}; diff --git a/migrator.js b/migrator.js index 8d14f9c12d..4eaf116c5b 100644 --- a/migrator.js +++ b/migrator.js @@ -3,11 +3,9 @@ require('db-migrate-shared').log.setLogLevel('error'); const { getInstance } = require('db-migrate'); -const parseDbUrl = require('parse-database-url'); -function migrateDb({ databaseUrl, databaseSchema = 'public' }) { - const custom = parseDbUrl(databaseUrl); - custom.schema = databaseSchema; +function migrateDb({ db, databaseSchema = 'public' }) { + const custom = Object.assign({}, db, { schema: databaseSchema }); const dbmigrate = getInstance(true, { cwd: __dirname, config: { custom }, diff --git a/package.json b/package.json index 85205134ff..ea2194cc2b 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "scripts": { "start": "node server.js", "start:google": "node examples/google-auth-unleash.js", - "start:dev": "NODE_ENV=development supervisor --ignore ./node_modules/,website server.js", + "start:dev": "NODE_ENV=development supervisor --ignore ./node_modules/,website server-dev.js", "start:dev:pg": "pg_virtualenv npm run start:dev:pg-chain", "start:dev:pg-chain": "export DATABASE_URL=postgres://$PGUSER:$PGPASSWORD@localhost:$PGPORT/postgres ; db-migrate up && npm run start:dev", "db-migrate": "db-migrate", @@ -70,6 +70,7 @@ "db-migrate": "^0.11.6", "db-migrate-pg": "^1.0.0", "deep-diff": "^1.0.2", + "deepmerge": "^4.2.2", "errorhandler": "^1.5.1", "express": "^4.17.1", "gravatar-url": "^3.1.0", diff --git a/server-dev.js b/server-dev.js new file mode 100644 index 0000000000..b22747ea4a --- /dev/null +++ b/server-dev.js @@ -0,0 +1,15 @@ +'use strict'; + +const unleash = require('./lib/server-impl'); + +unleash.start({ + db: { + user: 'unleash_user', + password: 'passord', + host: 'localhost', + port: 5432, + database: 'unleash', + ssl: true, + }, + enableRequestLogger: true, +}); diff --git a/test/e2e/helpers/database-config.js b/test/e2e/helpers/database-config.js index 800ce52331..4f97aef62a 100644 --- a/test/e2e/helpers/database-config.js +++ b/test/e2e/helpers/database-config.js @@ -1,13 +1,12 @@ 'use strict'; -function getDatabaseUrl() { - if (process.env.TEST_DATABASE_URL) { - return process.env.TEST_DATABASE_URL; - } else { - return 'postgres://unleash_user:passord@localhost:5432/unleash_test'; - } -} +const parseDbUrl = require('parse-database-url'); module.exports = { - getDatabaseUrl, + getDb: () => { + const url = + process.env.TEST_DATABASE_URL || + 'postgres://unleash_user:passord@localhost:5432/unleash_test'; + return parseDbUrl(url); + }, }; diff --git a/test/e2e/helpers/database-init.js b/test/e2e/helpers/database-init.js index 015fdf475b..cae9ed6cb8 100644 --- a/test/e2e/helpers/database-init.js +++ b/test/e2e/helpers/database-init.js @@ -58,7 +58,7 @@ function createFeatures(store) { module.exports = async function init(databaseSchema = 'test', getLogger) { const options = { - databaseUrl: require('./database-config').getDatabaseUrl(), + db: require('./database-config').getDb(), databaseSchema, minPool: 1, maxPool: 1, diff --git a/test/e2e/stores/setting-store.e2e.test.js b/test/e2e/stores/setting-store.e2e.test.js new file mode 100644 index 0000000000..104e330284 --- /dev/null +++ b/test/e2e/stores/setting-store.e2e.test.js @@ -0,0 +1,28 @@ +'use strict'; + +const test = require('ava'); +const dbInit = require('../helpers/database-init'); +const getLogger = require('../../fixtures/no-logger'); + +let stores; + +test.before(async () => { + const db = await dbInit('setting_store_serial', getLogger); + stores = db.stores; +}); + +test.after(async () => { + await stores.db.destroy(); +}); + +test.serial('should have api secret stored', async t => { + const secret = await stores.settingStore.get('unleash.secret'); + t.assert(secret); +}); + +test.serial('should insert arbitarty value', async t => { + const value = { b: 'hello' }; + await stores.settingStore.insert('unleash.custom', value); + const ret = await stores.settingStore.get('unleash.custom'); + t.deepEqual(ret, value); +}); diff --git a/test/fixtures/fake-setting-store.js b/test/fixtures/fake-setting-store.js new file mode 100644 index 0000000000..e84ddcabc3 --- /dev/null +++ b/test/fixtures/fake-setting-store.js @@ -0,0 +1,6 @@ +'use strict'; + +module.exports = () => ({ + insert: () => Promise.resolve(), + get: () => Promise.resolve(), +}); diff --git a/test/fixtures/store.js b/test/fixtures/store.js index a1a1a46377..c9a78f65d2 100644 --- a/test/fixtures/store.js +++ b/test/fixtures/store.js @@ -7,6 +7,7 @@ const featureToggleStore = require('./fake-feature-toggle-store'); const eventStore = require('./fake-event-store'); const strategyStore = require('./fake-strategies-store'); const contextFieldStore = require('./fake-context-store'); +const settingStore = require('./fake-setting-store'); module.exports = { createStores: () => { @@ -25,6 +26,7 @@ module.exports = { eventStore: eventStore(), strategyStore: strategyStore(), contextFieldStore: contextFieldStore(), + settingStore: settingStore(), }; }, }; diff --git a/yarn.lock b/yarn.lock index d74a93cb86..c123d0a899 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1725,6 +1725,11 @@ deep-is@~0.1.3: version "0.1.3" resolved "https://registry.yarnpkg.com/deep-is/-/deep-is-0.1.3.tgz#b369d6fb5dbc13eecf524f91b070feedc357cf34" +deepmerge@^4.2.2: + version "4.2.2" + resolved "https://registry.yarnpkg.com/deepmerge/-/deepmerge-4.2.2.tgz#44d2ea3679b8f4d4ffba33f03d865fc1e7bf4955" + integrity sha512-FJ3UgI4gIl+PHZm53knsuSFpE+nESMr7M4v9QcgB7S63Kj/6WqMiFQJpBBYz1Pt+66bZpP3Q7Lye0Oo9MPKEdg== + default-require-extensions@^3.0.0: version "3.0.0" resolved "https://registry.yarnpkg.com/default-require-extensions/-/default-require-extensions-3.0.0.tgz#e03f93aac9b2b6443fc52e5e4a37b3ad9ad8df96"