Skip to content

NC | NSFS | CLI | Make Update Account Tolerance to Missing Master Key #8236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

NC | NSFS | CLI | Make Update Account Tolerance to Missing Master Key #8236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/cmd/manage_nsfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,11 @@ async function fetch_account_data(action, user_input) {
// @ts-ignore
data = _.omitBy(data, _.isUndefined);
const decrypt_secret_key = action === ACTIONS.UPDATE;
data = await fetch_existing_account_data(action, data, decrypt_secret_key);
const regenerate = get_boolean_or_string_value(user_input.regenerate);
const allow_update_when_master_key_fails = decrypt_secret_key && (regenerate ||
(user_input.access_key && user_input.secret_key));
data = await fetch_existing_account_data(action, data, decrypt_secret_key,
allow_update_when_master_key_fails);
}

// override values
Expand Down Expand Up @@ -362,8 +366,18 @@ async function fetch_account_data(action, user_input) {
return data;
}

async function fetch_existing_account_data(action, target, decrypt_secret_key) {
/**
* fetch_existing_account_data is used to combine the data the we have saved in the config file\
* with the user input (used in delete and update actions only)
* @param {string} action
* @param {object} target
* @param {boolean} decrypt_secret_key
* @param {boolean} allow_update_when_master_key_fails
*/
async function fetch_existing_account_data(action, target, decrypt_secret_key,
allow_update_when_master_key_fails) {
const options = { show_secrets: true, decrypt_secret_key };
if (decrypt_secret_key) options.return_encrypted_if_decryption_fails = allow_update_when_master_key_fails;
let source;
try {
source = await config_fs.get_account_by_name(target.name, options);
Expand Down
15 changes: 10 additions & 5 deletions src/sdk/config_fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,26 +187,30 @@ class ConfigFS {
}

/**
* get_config_data reads a config file and returns its content
* get_identity_config_data reads a config file and returns its content
* while omitting secrets if show_secrets flag was not provided
* 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)
* @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_encrypted_if_decryption_fails?: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;
const { show_secrets = false, decrypt_secret_key = false, silent_if_missing = false,
return_encrypted_if_decryption_fails = 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.code === 'ENOENT' && silent_if_missing) return;
if (return_encrypted_if_decryption_fails && err.rpc_code === 'INVALID_MASTER_KEY') return config_data;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed,
This solution creates a new master key under the hood, which means it tries to solve a big issue (master key disappeared) without letting the customer know about it. I don't think this is how we should do that, let's take some time for design.
I'm worried about temporary failures that we will create a new master key instead of fixing the issue.

throw err;
}
}
Expand Down Expand Up @@ -456,7 +460,8 @@ class ConfigFS {
* as a part of iterating the file, therefore we continue
* (not throwing this error and return undefined)
* @param {string} account_name
* @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_encrypted_if_decryption_fails?:boolean}} [options]
* @returns {Promise<Object>}
*/
async get_account_by_name(account_name, options = {}) {
Expand Down
113 changes: 95 additions & 18 deletions src/test/unit_tests/jest_tests/test_nc_account_invalid_mkm_integration.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_nsfs_account_cli');
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', () => {
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -211,6 +221,50 @@ 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('should fail - cli update uid (without master key id - only allowed to update access keys)',
async () => {
try {
const account_options = { config_root, name: defaults_account1.name, uid: 2050 };
const action = ACTIONS.UPDATE;
await exec_manage_cli(type, action, account_options);
} catch (err) {
expect(JSON.parse(err.stdout).error.code).toBe(ManageCLIError.InvalidMasterKey.code);
}
});

it('cli update access keys with regenerate', async () => {
const account_options = { config_root, name: defaults_account1.name, regenerate: true };
const action = ACTIONS.UPDATE;
const res = await exec_manage_cli(type, action, account_options);
expect(JSON.parse(res).response.code).toEqual(ManageCLIResponse.AccountUpdated.code);
});

it('cli update access keys with access_key and secret_key flags', async () => {
const new_access_key = 'BIBiFAnjaaE7OKD5N7hZ';
const new_secret_key = 'ZIBYaMpU3zRDcRFWmvzgQr9MoHIAsD+3oEXAMPLE';
const account_options = { config_root, name: defaults_account1.name,
access_key: new_access_key, secret_key: new_secret_key };
const action = ACTIONS.UPDATE;
const res = await exec_manage_cli(type, action, account_options);
expect(JSON.parse(res).response.code).toEqual(ManageCLIResponse.AccountUpdated.code);
});
});
});

async function create_account(account_options) {
Expand All @@ -224,7 +278,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);
Expand All @@ -234,7 +288,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);
Expand All @@ -243,7 +297,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);
Expand All @@ -252,7 +306,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);
Expand All @@ -269,11 +323,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);
}
}
4 changes: 3 additions & 1 deletion src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_cli.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,7 @@ describe('manage nsfs cli bucket flow', () => {

});

describe('cli list bucket', () => {
describe('cli list buckets', () => {
const config_root = path.join(tmp_fs_path, 'config_root_manage_nsfs5');
const root_path = path.join(tmp_fs_path, 'root_path_manage_nsfs5/');
const bucket_storage_path = path.join(tmp_fs_path, 'root_path_manage_nsfs5', 'bucket1');
Expand Down Expand Up @@ -896,6 +896,7 @@ describe('manage nsfs cli bucket flow', () => {
const bucket_options = { config_root, name: 'bucket2' };
const action = ACTIONS.LIST;
const res = await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
expect(Array.isArray(JSON.parse(res).response.reply)).toBe(true);
expect(JSON.parse(res).response.reply.map(item => item.name))
.toEqual(expect.arrayContaining([]));
});
Expand All @@ -904,6 +905,7 @@ describe('manage nsfs cli bucket flow', () => {
const bucket_options = { config_root, name: 'bucket1' };
const action = ACTIONS.LIST;
const res = await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
expect(Array.isArray(JSON.parse(res).response.reply)).toBe(true);
expect(JSON.parse(res).response.reply.map(item => item.name))
.toEqual(expect.arrayContaining(['bucket1']));
});
Expand Down