NC | CLI | List accounts when decrypt access keys fails

shirady · shirady · commit 6f63da546a85 · 2025-02-17T17:13:53.000+02:00
1. In account list add additional option of return_encrypted_on_decryption_error and we will neither return the secret_key nor the encrypted_secret_key in case of failure of access keys decryption.
2. In the function get_identity_config_data add a case in try-catch clause for the decryption failure and add the property of decryption_err in case there was failure of access keys decryption that is related to INVALID_MASTER_KEY and remove the encrypted_secret_key property (added a helper function for that).
3. Small refactors in file test_nc_account_invalid_mkm_integration.test.js so we can add additional account and rename the file master_keys.json for the test.

Signed-off-by: shirady &lt;57721533+shirady@users.noreply.github.com&gt;
diff --git a/src/cmd/manage_nsfs.js b/src/cmd/manage_nsfs.js
@@ -597,6 +597,10 @@ async function list_account_config_files(wide, show_secrets, filters = {}) {
     const options = {
         show_secrets: show_secrets || should_filter,
         decrypt_secret_key: show_secrets,
+        // in case we have an error on secret_key decryption 
+        // we will neither return the secret_key nor the encrypted_secret_key
+        // and add the property of decryption_err with the error we had
+        return_on_decryption_error: true,
         silent_if_missing: true
     };
 
diff --git a/src/sdk/config_fs.js b/src/sdk/config_fs.js
@@ -231,24 +231,35 @@ class ConfigFS {
      * and decrypts the account's secret_key if decrypt_secret_key is true
      * if silent_if_missing is true -      
      *   if the config file was deleted (encounter ENOENT error) - continue (returns undefined)
+     * if decrypt_secret_key is true and the decryption failed with rpc error of INVALID_MASTER_KEY
+     *    and return_on_decryption_error is true - 
+     *        add a property decryption_err with the error we've got
+     *        if  return_on_decryption_error is false - throw the error (as it was before)
      * @param {string} config_file_path
-     * @param {{show_secrets?: boolean, decrypt_secret_key?: boolean, silent_if_missing?: boolean}} [options]
+     * @param {{show_secrets?: boolean, decrypt_secret_key?: boolean, silent_if_missing?: boolean,
+     *          return_on_decryption_error?: boolean}} [options]
      * @returns {Promise<Object>}
      */
     async get_identity_config_data(config_file_path, options = {}) {
         const { show_secrets = false, decrypt_secret_key = false, silent_if_missing = false } = options;
+        let config_data;
         try {
             const data = await this.get_config_data(config_file_path, options);
             if (!data && silent_if_missing) return;
-            const config_data = _.omit(data, show_secrets ? [] : ['access_keys']);
+            config_data = _.omit(data, show_secrets ? [] : ['access_keys']);
             if (decrypt_secret_key) config_data.access_keys = await nc_mkm.decrypt_access_keys(config_data);
             return config_data;
         } catch (err) {
             dbg.warn('get_identity_config_data: with config_file_path', config_file_path, 'got an error', err);
+            if (err.rpc_code === 'INVALID_MASTER_KEY' && options.return_on_decryption_error) {
+                config_data.decryption_err = err.message;
+                return this.remove_encrypted_secret_key(config_data);
+            }
             if (err.code === 'ENOENT' && silent_if_missing) return;
             throw err;
         }
     }
+
     /**
      * get_config_data reads a config file and returns its content
      * @param {string} config_file_path
@@ -820,7 +831,6 @@ class ConfigFS {
         await native_fs_utils.folder_delete(account_dir_path, this.fs_context, undefined, true);
     }
 
-
     /**
      * _prepare_for_account_schema processes account data before writing it to the config dir and does the following -
      * 1. encrypts its access keys
@@ -839,6 +849,19 @@ class ConfigFS {
         return { parsed_account_data, string_account_data };
     }
 
+    /**
+     * remove_encrypted_secret_key will remove the encrypted_secret_key property from an identity
+     * @param {object} config_data
+     * @returns {object}
+     */
+    remove_encrypted_secret_key(config_data) {
+        const size = config_data.access_keys.length;
+        for (let index = 0; index < size; index++) {
+            config_data.access_keys[index] = _.omit(config_data.access_keys[index], ['encrypted_secret_key']);
+        }
+        return config_data;
+    }
+
     /////////////////////////////////////
     //////   ACCOUNT NAME INDEX    //////
     /////////////////////////////////////
diff --git a/src/test/unit_tests/jest_tests/test_config_fs.test.js b/src/test/unit_tests/jest_tests/test_config_fs.test.js
@@ -1,6 +1,7 @@
 /* Copyright (C) 2024 NooBaa */
 'use strict';
 
+const _ = require('lodash');
 const os = require('os');
 const path = require('path');
 const config = require('../../../../config');
@@ -71,3 +72,45 @@ describe('compare_host_and_config_dir_version', () => {
                 `mentioned in system.json=${system_config_dir_version}, any updates to the config directory are blocked until the source code upgrade`);
     });
 });
+
+describe('remove_encrypted_secret_key', () => {
+    const account_data = {
+        _id: '6784bccb9c05f2fb04c38dd5',
+        name: 'account-1',
+        email: 'account-1',
+        creation_date: '2025-01-13T07:12:11.145Z',
+        access_keys: [ {access_key: 'GIGiFBmjaaE7OKD5N7hA', encrypted_secret_key: 'jrE1UT9AKtqn2g57GlAAjNttqeKBtEyy4uIl4rjfqHSJ22gvt9dflw==EXAMPLE'} ],
+        nsfs_account_config: {uid: 1001, gid: 1001, new_buckets_path: '/User/buckets'},
+        allow_bucket_creation: true,
+        master_key_id: '6767ff7b12869117a8221e62EXAMPLE',
+        decryption_err: 'master key id is missing in master_keys_by_id',
+    };
+
+    it('remove_encrypted_secret_key on account with 1 pair of access keys', () => {
+        const account_data_without_encrypted_secret_key = config_fs.remove_encrypted_secret_key(account_data);
+        expect(account_data_without_encrypted_secret_key.length).toEqual(account_data.length);
+        expect(Array.isArray(account_data_without_encrypted_secret_key.access_keys)).toBe(true);
+        expect(account_data_without_encrypted_secret_key.access_keys[0].encrypted_secret_key).toBeUndefined();
+    });
+
+    it('remove_encrypted_secret_key on account with 0 pairs of access keys', () => {
+        const account_data_no_access_keys = _.cloneDeep(account_data);
+        account_data_no_access_keys.access_keys = [];
+        const account_data_without_encrypted_secret_key = config_fs.remove_encrypted_secret_key(account_data_no_access_keys);
+        expect(account_data_without_encrypted_secret_key.length).toEqual(account_data.length);
+        expect(Array.isArray(account_data_without_encrypted_secret_key.access_keys)).toBe(true);
+        expect(account_data_without_encrypted_secret_key.access_keys.length).toBe(0);
+    });
+
+    it('remove_encrypted_secret_key on account with 2 pairs of access keys', () => {
+        const account_data_with_2_pair_access_keys = _.cloneDeep(account_data);
+        account_data_with_2_pair_access_keys.access_keys[1] = {
+            access_key: 'GIGiFBmjaaE7OKD5N8kB',
+            encrypted_secret_key: 'jrE1UT9AKtqn2g57GlAAjNttqeKBtEyy4uIl4rjfqHSJ22gvt9ddeu==EXAMPLE'
+        };
+        const account_data_without_encrypted_secret_key = config_fs.remove_encrypted_secret_key(account_data_with_2_pair_access_keys);
+        expect(account_data_without_encrypted_secret_key.length).toEqual(account_data.length);
+        expect(Array.isArray(account_data_without_encrypted_secret_key.access_keys)).toBe(true);
+        expect(account_data_without_encrypted_secret_key.access_keys.length).toBe(2);
+    });
+});
diff --git a/src/test/unit_tests/jest_tests/test_nc_account_invalid_mkm_integration.test.js b/src/test/unit_tests/jest_tests/test_nc_account_invalid_mkm_integration.test.js
@@ -11,20 +11,30 @@ const { exec_manage_cli, set_path_permissions_and_owner, TMP_PATH, set_nc_config
 const { TYPES, ACTIONS } = require('../../../manage_nsfs/manage_nsfs_constants');
 const ManageCLIError = require('../../../manage_nsfs/manage_nsfs_cli_errors').ManageCLIError;
 const ManageCLIResponse = require('../../../manage_nsfs/manage_nsfs_cli_responses').ManageCLIResponse;
+const { get_process_fs_context } = require('../../../util/native_fs_utils');
+const nb_native = require('../../../util/nb_native');
 
 const tmp_fs_path = path.join(TMP_PATH, 'test_nc_invalid_mkm_integration');
 const config_root = path.join(tmp_fs_path, 'config_root_account_mkm_integration');
 const root_path = path.join(tmp_fs_path, 'root_path_account_mkm_integration/');
-const defaults = {
-    _id: 'account1',
+const defaults_account1 = {
     type: TYPES.ACCOUNT,
     name: 'account1',
-    new_buckets_path: `${root_path}new_buckets_path_mkm_integration/`,
-    uid: 999,
-    gid: 999,
+    new_buckets_path: `${root_path}new_buckets_path_mkm_integration_account1/`,
+    uid: 1001,
+    gid: 1001,
     access_key: 'GIGiFAnjaaE7OKD5N7hA',
     secret_key: 'U2AYaMpU3zRDcRFWmvzgQr9MoHIAsD+3oEXAMPLE',
 };
+const defaults_account2 = {
+    type: TYPES.ACCOUNT,
+    name: 'account2',
+    new_buckets_path: `${root_path}new_buckets_path_mkm_integration_account2/`,
+    uid: 1002,
+    gid: 1002,
+    access_key: 'HIHiFAnjaaE7OKD5N7hA',
+    secret_key: 'U3BYaMpU3zRDcRFWmvzgQr9MoHIAsD+3oEXAMPLE',
+};
 
 describe('manage nsfs cli account flow + fauly master key flow', () => {
     describe('cli account ops - master key is missing', () => {
@@ -49,13 +59,13 @@ describe('manage nsfs cli account flow + fauly master key flow', () => {
         });
 
         it('cli account list', async () => {
-            const { name } = defaults;
+            const { name } = defaults_account1;
             const list_res = await list_account_flow();
             expect(list_res.response.reply[0].name).toBe(name);
         });
 
         it('cli account status', async () => {
-            const { name, uid, gid, new_buckets_path } = defaults;
+            const { name, uid, gid, new_buckets_path } = defaults_account1;
             const status_res = await status_account();
             expect(status_res.response.reply.name).toBe(name);
             expect(status_res.response.reply.email).toBe(name);
@@ -99,7 +109,7 @@ describe('manage nsfs cli account flow + fauly master key flow', () => {
 
         it('should fail | cli create account', async () => {
             try {
-                await create_account({ ...defaults, name: 'account_corrupted_mk' });
+                await create_account({ ...defaults_account1, name: 'account_corrupted_mk' });
                 fail('should have failed with InvalidMasterKey');
             } catch (err) {
                 expect(JSON.parse(err.stdout).error.code).toBe(ManageCLIError.InvalidMasterKey.code);
@@ -116,13 +126,13 @@ describe('manage nsfs cli account flow + fauly master key flow', () => {
         });
 
         it('cli account list', async () => {
-            const { name } = defaults;
+            const { name } = defaults_account1;
             const list_res = await list_account_flow();
             expect(list_res.response.reply[0].name).toBe(name);
         });
 
         it('cli account status', async () => {
-            const { name, uid, gid, new_buckets_path } = defaults;
+            const { name, uid, gid, new_buckets_path } = defaults_account1;
             const status_res = await status_account();
             expect(status_res.response.reply.name).toBe(name);
             expect(status_res.response.reply.email).toBe(name);
@@ -165,7 +175,7 @@ describe('manage nsfs cli account flow + fauly master key flow', () => {
 
         it('should fail | cli create account', async () => {
             try {
-                await create_account({ ...defaults, name: 'account_corrupted_mk' });
+                await create_account({ ...defaults_account1, name: 'account_corrupted_mk' });
                 fail('should have failed with InvalidMasterKey');
             } catch (err) {
                 expect(JSON.parse(err.stdout).error.code).toBe(ManageCLIError.InvalidMasterKey.code);
@@ -182,13 +192,13 @@ describe('manage nsfs cli account flow + fauly master key flow', () => {
         });
 
         it('cli account list', async () => {
-            const { name } = defaults;
+            const { name } = defaults_account1;
             const list_res = await list_account_flow();
             expect(list_res.response.reply[0].name).toBe(name);
         });
 
         it('cli account status', async () => {
-            const { name, uid, gid, new_buckets_path } = defaults;
+            const { name, uid, gid, new_buckets_path } = defaults_account1;
             const status_res = await status_account();
             expect(status_res.response.reply.name).toBe(name);
             expect(status_res.response.reply.email).toBe(name);
@@ -211,6 +221,34 @@ describe('manage nsfs cli account flow + fauly master key flow', () => {
             expect(delete_res.response.code).toBe(ManageCLIResponse.AccountDeleted.code);
         });
     });
+
+    describe('cli with renamed master key file (invalid)', () => {
+        const type = TYPES.ACCOUNT;
+
+        beforeEach(async () => {
+            await setup_nc_system_and_first_account();
+            await setup_account(defaults_account2);
+            await master_key_file_rename(true);
+        });
+
+        afterEach(async () => {
+            await master_key_file_rename(false);
+            await fs_utils.folder_delete(`${config_root}`);
+            await fs_utils.folder_delete(`${root_path}`);
+        });
+
+        it('cli list with wide and show_secrets flags (will show encrypted_secret_key and warning property)', async () => {
+            const action = ACTIONS.LIST;
+            const account_options = { config_root, wide: true, show_secrets: true };
+            const res = await exec_manage_cli(type, action, account_options);
+            const account_array_res = JSON.parse(res).response.reply;
+            for (const account_res of account_array_res) {
+                expect(account_res.access_keys[0].encrypted_secret_key).toBeUndefined();
+                expect(account_res.access_keys[0].secret_key).toBeUndefined();
+                expect(account_res.decryption_err).toBeDefined();
+            }
+        });
+    });
 });
 
 async function create_account(account_options) {
@@ -224,7 +262,7 @@ async function create_account(account_options) {
 
 async function update_account() {
     const action = ACTIONS.UPDATE;
-    const { type, name, new_buckets_path } = defaults;
+    const { type, name, new_buckets_path } = defaults_account1;
     const new_uid = '1111';
     const account_options = { config_root, name, new_buckets_path, uid: new_uid };
     const res = await exec_manage_cli(type, action, account_options);
@@ -234,7 +272,7 @@ async function update_account() {
 
 async function status_account(show_secrets) {
     const action = ACTIONS.STATUS;
-    const { type, name } = defaults;
+    const { type, name } = defaults_account1;
     const account_options = { config_root, name, show_secrets };
     const res = await exec_manage_cli(type, action, account_options);
     const parsed_res = JSON.parse(res);
@@ -243,7 +281,7 @@ async function status_account(show_secrets) {
 
 async function list_account_flow() {
     const action = ACTIONS.LIST;
-    const { type } = defaults;
+    const { type } = defaults_account1;
     const account_options = { config_root };
     const res = await exec_manage_cli(type, action, account_options);
     const parsed_res = JSON.parse(res);
@@ -252,7 +290,7 @@ async function list_account_flow() {
 
 async function delete_account_flow() {
     const action = ACTIONS.DELETE;
-    const { type, name } = defaults;
+    const { type, name } = defaults_account1;
     const account_options = { config_root, name };
     const res = await exec_manage_cli(type, action, account_options);
     const parsed_res = JSON.parse(res);
@@ -269,11 +307,34 @@ function fail(reason) {
 async function setup_nc_system_and_first_account() {
     await fs_utils.create_fresh_path(root_path);
     set_nc_config_dir_in_config(config_root);
+    await setup_account(defaults_account1);
+}
+
+async function setup_account(account_defaults) {
     const action = ACTIONS.ADD;
-    const { type, name, new_buckets_path, uid, gid } = defaults;
+    const { type, name, new_buckets_path, uid, gid } = account_defaults;
     const account_options = { config_root, name, new_buckets_path, uid, gid };
     await fs_utils.create_fresh_path(new_buckets_path);
     await fs_utils.file_must_exist(new_buckets_path);
     await set_path_permissions_and_owner(new_buckets_path, account_options, 0o700);
     await exec_manage_cli(type, action, account_options);
 }
+
+
+/**
+ * master_key_file_rename will rename the master_keys.json file
+ * to mock a situation where master_key_id points to a missing master key
+ * use the to_rename_temp false to rename it back (after the test)
+ * @param {boolean} to_rename_temp
+ */
+async function master_key_file_rename(to_rename_temp) {
+    const default_fs_config = get_process_fs_context();
+    const source_path = path.join(config_root, 'master_keys.json');
+    const dest_path = path.join(config_root, 'temp_master_keys.json');
+    // eliminate the master key file by renaming it
+    if (to_rename_temp) {
+        await nb_native().fs.rename(default_fs_config, source_path, dest_path);
+    } else {
+        await nb_native().fs.rename(default_fs_config, dest_path, source_path);
+    }
+}
