security(configs): mark credential configs secret and withhold them on read (24.05)

api/api.js

@@ -124,6 +124,15 @@ plugins.connectToAllDatabases().then(function() {
         proxy_type: "https"
     });
 
+    //the outbound proxy credentials. The whole security namespace used to be
+    //serialized into the dashboard page and returned to any app admin, so these were
+    //readable well below the operator who set them. The dashboard only uses the
+    //password policy keys from this namespace, so nothing needs them.
+    plugins.setSecretConfigs("security", {
+        proxy_username: true,
+        proxy_password: true
+    });
+
     /**
     * Set tracking config
     */

frontend/express/app.js

@@ -992,9 +992,15 @@ Promise.all([plugins.dbConnection(countlyConfig), plugins.dbConnection("countly_
                     csrf_token: req.csrfToken(),
                     auth_token: req.session.auth_token,
                     member: member,
-                    config: req.config,
-                    security: plugins.getConfig("security"),
-                    tracking: plugins.getConfig("tracking"),
+                    //these namespaces are serialized into the page, so every logged in
+                    //user can read them in the source regardless of their permissions.
+                    //Secret values are dropped rather than masked: the page has no use
+                    //for a placeholder. Anything the browser genuinely needs must not
+                    //be marked secret, since masking what is already in page source
+                    //would achieve nothing.
+                    config: plugins.omitSecretConfigs("frontend", req.config),
+                    security: plugins.omitSecretConfigs("security", plugins.getConfig("security")),
+                    tracking: plugins.omitSecretConfigs("tracking", plugins.getConfig("tracking")),
                     plugins: plugins.getPlugins(),
                     pluginsFull: plugins.getPlugins(true),
                     path: countlyConfig.path || "",

plugins/pluginManager.js

@@ -26,6 +26,11 @@ var pluginDependencies = require('./pluginDependencies.js'),
     configextender = require('../api/configextender');
 var pluginConfig = {};
 
+//stands in for a secret configuration value when configuration is read by someone
+//who may know the value is set but not what it is. Deliberately not a plausible
+//value, so it is obvious in a UI and in a bug report.
+const SECRET_CONFIG_MASK = "********";
+
 /**
 * This module handles communicaton with plugins
 * @module "plugins/pluginManager"
@@ -41,6 +46,9 @@ var pluginManager = function pluginManager() {
     var defaultConfigs = {};
     var configsOnchanges = {};
     var excludeFromUI = {plugins: true};
+    //configuration values that hold credentials rather than settings, by namespace.
+    //see setSecretConfigs()
+    var secretConfigs = {};
     var finishedSyncing = true;
     var expireList = [];
     var masking = {};
@@ -388,6 +396,96 @@ var pluginManager = function pluginManager() {
         }
     };
 
+    /**
+    * Mark configuration values as secrets, so they are not handed out on read.
+    *
+    * Some configuration holds credentials rather than settings: a proxy password, an
+    * API key, the key report subscribe tokens are signed with. Those are readable by
+    * anyone who can read configuration at all, which is a lower bar than the
+    * operator who set them. Marking them here masks the value everywhere
+    * configuration is returned to a caller who is not a global admin, and keeps it
+    * out of the values exposed to the dashboard page.
+    *
+    * A value the browser needs is not a secret and must not be marked: once it is in
+    * page source, masking is theatre. tracking.self_tracking_app_key and
+    * recaptcha.site_key are deliberately not marked for that reason.
+    *
+    * getConfig() is unaffected, so server code keeps reading the real value.
+    *
+    * @param {string} namespace - namespace of configuration, usually plugin name
+    * @param {object} conf - object whose keys are secret when the value is true
+    **/
+    this.setSecretConfigs = function(namespace, conf) {
+        if (!secretConfigs[namespace]) {
+            secretConfigs[namespace] = {};
+        }
+        for (let i in conf) {
+            secretConfigs[namespace][i] = conf[i];
+        }
+    };
+
+    /**
+    * Whether a configuration value is marked secret.
+    * @param {string} namespace - namespace of configuration
+    * @param {string} key - configuration key
+    * @returns {boolean} true when the value must not be handed out
+    **/
+    this.isSecretConfig = function(namespace, key) {
+        return !!(secretConfigs[namespace] && secretConfigs[namespace][key] === true);
+    };
+
+    /**
+    * Copy of a full configuration object with every secret value replaced by a
+    * placeholder. Used for callers who may see that configuration exists but not what
+    * it is set to.
+    *
+    * The input is not modified, so the caller cannot accidentally hand a masked
+    * object to code that needs the real values.
+    *
+    * @param {object} confs - object of namespace -> configuration object
+    * @returns {object} copy with secret values masked
+    **/
+    this.maskSecretConfigs = function(confs) {
+        var masked = {};
+        for (let namespace in confs) {
+            var conf = confs[namespace];
+            if (!conf || typeof conf !== "object" || Array.isArray(conf)) {
+                masked[namespace] = conf;
+                continue;
+            }
+            masked[namespace] = {};
+            for (let key in conf) {
+                masked[namespace][key] = this.isSecretConfig(namespace, key) && conf[key] !== "" && conf[key] !== null && typeof conf[key] !== "undefined"
+                    ? SECRET_CONFIG_MASK
+                    : conf[key];
+            }
+        }
+        return masked;
+    };
+
+    /**
+    * Copy of one namespace's configuration with secret keys removed entirely, for
+    * values that are serialized into the dashboard page. Removed rather than masked:
+    * the page has no use for a placeholder, and an absent key cannot be mistaken for
+    * a real one.
+    *
+    * @param {string} namespace - namespace of configuration
+    * @param {object} conf - configuration object for that namespace
+    * @returns {object} copy without the secret keys
+    **/
+    this.omitSecretConfigs = function(namespace, conf) {
+        if (!conf || typeof conf !== "object" || Array.isArray(conf)) {
+            return conf;
+        }
+        var out = {};
+        for (let key in conf) {
+            if (!this.isSecretConfig(namespace, key)) {
+                out[key] = conf[key];
+            }
+        }
+        return out;
+    };
+
     /**
     * Get configuration from specific namespace and populate empty values with provided defaults
     * @param {string} namespace - namespace of configuration, usually plugin name

plugins/plugins/api/api.js

@@ -282,6 +282,14 @@ var plugin = {},
             plugins.loadConfigs(common.db, function() {
                 var confs = plugins.getAllConfigs();
                 delete confs.services;
+                //an app admin may see which configuration exists, and how their app
+                //differs from the server default, but not the values that are
+                //credentials: a proxy password, an API key, the key report subscribe
+                //tokens are signed with. Only a global admin can set those, so only a
+                //global admin is shown them.
+                if (!params.member.global_admin) {
+                    confs = plugins.maskSecretConfigs(confs);
+                }
                 common.returnOutput(params, confs);
             });
         });

plugins/plugins/tests.js

@@ -38,4 +38,121 @@ describe('Testing Plugins', function() {
                 done();
             });
     });
-});
+});
+
+// Configuration values that hold credentials are marked with setSecretConfigs() and
+// masked for anyone who is not a global admin. /o/configs is validateAppAdmin, so an
+// app admin can see which configuration exists and how their app differs from the
+// server default, but not the credential values. Only a global admin can set those
+// (/i/configs is validateGlobalAdmin), so only a global admin is shown them.
+//
+// reports.secretKey is the case that matters most: it self-generates, so it is set on
+// every install, and it signs the tokens that /subscribe_report accepts without
+// authentication.
+describe('Testing configs secret masking', function() {
+    var MASK = "********";
+    var API_KEY_ADMIN = "";
+    var APP_ID = "";
+    var memberApiKey = "";
+    var memberUserId = "";
+    var uniq = Date.now();
+
+    it('should read the real secret as a global admin', function(done) {
+        API_KEY_ADMIN = testUtils.get("API_KEY_ADMIN");
+        APP_ID = testUtils.get("APP_ID");
+        request
+            .get('/o/configs?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID)
+            .expect(200)
+            .end(function(err, res) {
+                if (err) {
+                    return done(err);
+                }
+                var ob = JSON.parse(res.text);
+                ob.should.have.property("reports");
+                ob.reports.should.have.property("secretKey");
+                ob.reports.secretKey.should.not.equal(MASK);
+                ob.reports.secretKey.length.should.be.above(0);
+                done();
+            });
+    });
+
+    it('should create an app admin who is not a global admin', function(done) {
+        var permission = {
+            _: {a: [APP_ID], u: [[APP_ID]]},
+            c: {},
+            r: {},
+            u: {},
+            d: {}
+        };
+        var userParams = {
+            full_name: "cfgadmin" + uniq,
+            username: "cfgadmin" + uniq,
+            password: "p4ssw0rD!",
+            email: "cfgadmin" + uniq + "@mail.test",
+            permission: permission
+        };
+        request
+            .get('/i/users/create?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify(userParams)))
+            .expect(200)
+            .end(function(err, res) {
+                if (err) {
+                    return done(err);
+                }
+                memberApiKey = res.body.api_key;
+                memberUserId = res.body._id;
+                memberApiKey.should.not.equal(API_KEY_ADMIN);
+                done();
+            });
+    });
+
+    it('should mask secrets for an app admin', function(done) {
+        request
+            .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID)
+            .expect(200)
+            .end(function(err, res) {
+                if (err) {
+                    return done(err);
+                }
+                var ob = JSON.parse(res.text);
+                ob.reports.secretKey.should.equal(MASK);
+                ob.security.should.have.property("proxy_password");
+                ob.security.proxy_password.should.not.match(/[a-z0-9]{6,}/i);
+                done();
+            });
+    });
+
+    it('should still return non-secret configuration to the app admin', function(done) {
+        request
+            .get('/o/configs?api_key=' + memberApiKey + '&app_id=' + APP_ID)
+            .expect(200)
+            .end(function(err, res) {
+                if (err) {
+                    return done(err);
+                }
+                var ob = JSON.parse(res.text);
+                // the app management screen needs these, so masking must not touch them
+                ob.should.have.property("api");
+                ob.api.should.have.property("session_duration_limit");
+                ob.api.should.have.property("event_limit");
+                ob.security.should.have.property("password_min");
+                ob.security.password_min.should.not.equal(MASK);
+                // public by design, must not be masked
+                if (ob.recaptcha) {
+                    ob.recaptcha.should.have.property("site_key");
+                    ob.recaptcha.site_key.should.not.equal(MASK);
+                }
+                done();
+            });
+    });
+
+    after(function(done) {
+        if (!memberUserId) {
+            return done();
+        }
+        request
+            .get('/i/users/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({user_ids: [memberUserId]})))
+            .end(function() {
+                done();
+            });
+    });
+});

plugins/push/api/api.js

@@ -70,6 +70,12 @@ plugins.setConfigs(FEATURE_NAME, {
     default_content_available: false, // sets content-available: 1 by default for ios
 });
 
+//outbound proxy credentials for sending pushes
+plugins.setSecretConfigs(FEATURE_NAME, {
+    proxyuser: true,
+    proxypass: true
+});
+
 plugins.internalEvents.push('[CLY]_push_sent');
 plugins.internalEvents.push('[CLY]_push_action');
 plugins.internalDrillEvents.push('[CLY]_push_action');

plugins/recaptcha/frontend/app.js

@@ -9,6 +9,12 @@ plugins.setConfigs("recaptcha", {
     secret_key: ""
 });
 
+//only secret_key. site_key is public by design: it goes to the browser to render the
+//challenge, so marking it secret would achieve nothing.
+plugins.setSecretConfigs("recaptcha", {
+    secret_key: true
+});
+
 (function(plugin) {
     plugin.init = function(app, countlyDb) {
         plugins.loadConfigs(countlyDb, function() {

plugins/reports/api/reports.js

@@ -24,6 +24,13 @@ plugins.setConfigs("reports", {
     secretKey: countlyApiConfig?.encryption?.reports_key || "Ydqa7Omkd3yhV33M3iWV1oFcOEk898h9",
 });
 
+//this key signs the subscribe/unsubscribe tokens, and /subscribe_report and
+///unsubscribe_report accept those without authentication. It self-generates, so
+//unlike the other secrets it is always set on every install.
+plugins.setSecretConfigs("reports", {
+    secretKey: true
+});
+
 versionInfo.page = (!versionInfo.title) ? "https://count.ly" : null;
 versionInfo.title = versionInfo.title || "Countly";
 var metrics = {