Skip to content
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.

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

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

Closed
wants to merge 1 commit into from

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Jul 29, 2024

Explain the changes

  1. Allow account update (only regenerate and access_key and secret_key) when we cannot decrypt the current access keys.
  2. Edit the bucket list tests to validate that we receive an array in the response (as we did in PR NC | NSFS | CLI | Improve Performance of List with Name Filter #8272).
  3. In config_fs fix JSDoc of the function get_identity_config_data.
  4. In mamage_nsfs add JSDoc to the function fetch_existing_account_data.

Issues: Fixed partially #8104

  1. Currently, if an account's master_key_id points to a missing master key, the commands: account update and account status in noobaa-cli (manage_nsfs) will fail with an error.
    Note: A missing master key can happen when the master keys that were previously used cannot be recovered due to some issue.
  2. GAP: in this PR we didn't make list accounts tolerance to missing master key and list buckets/accounts with Invalid JSON (it was in past of this PR but due to code changes that happened this code fix will be added separately).

Testing Instructions:

Unit Tests

Please run:

  1. sudo npx jest test_nc_nsfs_bucket_cli.test.js (for the change in point number 2 in the changes above).
  2. sudo npx jest test_nc_account_invalid_mkm_integration.test.js

Manual Testing

A. Missing master key

  1. Create an account with the CLI: sudo node src/cmd/manage_nsfs account add --name shira-1001 --new_buckets_path /tmp/nsfs_root1 --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid>
    Note: before creating the account need to give permission to the new_buckets_path: chmod 777 /tmp/nsfs_root1.
  2. Make the master key invalid by renaming the file master_key.json: sudo mv /etc/noobaa.conf.d/master_keys.json /etc/noobaa.conf.d/temp_master_keys.json
  3. Account list with decryption: sudo node src/cmd/manage_nsfs account list --wide --show_secrets (doesn't fail, but you will see the property encrypted_secret_key).
  4. Account status with decryption: sudo node src/cmd/manage_nsfs account status --name shira-1001 --show_secrets (will fail).
  5. Account update - can update the access keys (other update options will result in an error): sudo node src/cmd/manage_nsfs account update --name shira-1001 --regenerate or sudo node src/cmd/manage_nsfs account update --access_key <access-key-id> --secret_key <secret-key>.
  • Doc added/updated
  • Tests added

@shirady shirady changed the title NC | NSFS | CLI | Make list accounts and update account tolerance to missing master key NC | NSFS | CLI | Make List Accounts and Update Account Tolerance to Missing Master Key Jul 29, 2024
@shirady shirady force-pushed the nsfs-nc-missing-master-key branch 2 times, most recently from aebeaab to 646b644 Compare July 29, 2024 12:57
@shirady shirady self-assigned this Jul 29, 2024
@shirady shirady marked this pull request as draft August 8, 2024 07:47
@shirady shirady force-pushed the nsfs-nc-missing-master-key branch 3 times, most recently from 3bbee87 to c429ba5 Compare August 11, 2024 09:48
@shirady shirady marked this pull request as ready for review August 11, 2024 10:27
@shirady
Copy link
Contributor Author

shirady commented Aug 14, 2024

@romayalon I added the suggested changes.
I was wondering if you also want to add it in the docs somewhere (the file NooBaaCLI.md is very general).

@shirady shirady changed the title NC | NSFS | CLI | Make List Accounts and Update Account Tolerance to Missing Master Key NC | NSFS | CLI | Make List Accounts and Update Account Tolerance to Missing Master Key and List Buckets/Accounts with Invalid JSON Aug 14, 2024
@shirady shirady force-pushed the nsfs-nc-missing-master-key branch 4 times, most recently from 5cb88ef to 3952711 Compare August 20, 2024 07:24
@shirady shirady marked this pull request as draft September 8, 2024 13:51
@shirady
Copy link
Contributor Author

shirady commented Sep 8, 2024

I Converted it to draft since there was an issue after rebase with the health tests.

@shirady shirady changed the title NC | NSFS | CLI | Make List Accounts and Update Account Tolerance to Missing Master Key and List Buckets/Accounts with Invalid JSON NC | NSFS | CLI | Make Update Account Tolerance to Missing Master Key Oct 13, 2024
@shirady shirady marked this pull request as ready for review October 13, 2024 11:36
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.

@shirady
Copy link
Contributor Author

shirady commented Oct 14, 2024

Closing for now - would need design (see this comment).

@shirady shirady closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants