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 | CLI | List Accounts When Decrypt Access Keys Fails #8781

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Feb 10, 2025

Explain the changes

  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.

Issues:

  1. It is a partial fix related to issue NC | NSFS | CLI | Separate Bucket and Account List Functions #8747 - this fix only handles the list account CLI with decrypt issue.

Testing Instructions:

Automatic Tests:

Please run:

  • sudo npx jest test_nc_account_invalid_mkm_integration.test.js -t 'cli with renamed master key file'
  • npx jest test_config_fs.test.js -t 'remove_encrypted_secret_key'

Manual Tests

  1. Create an account with the CLI: sudo node src/cmd/manage_nsfs account add --name <account-name> --new_buckets_path /Users/buckets/ --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 /Users/buckets/.
  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 (should not fail, but you will see the property encrypted_secret_key and decryption_err).
  4. Account status with decryption: sudo node src/cmd/manage_nsfs account status --name <account-name> --show_secrets (will fail - we didn't handle this case).

Before this fix we had this error (added partial output):

  "error": {
    "code": "InvalidMasterKey",
    "message": "Master key manager had issues loading master key, can not decrypt/encrypt secrets.",
    "cause": "Error: master key id is missing in master_keys_by_id\n    at NCMasterKeysManager._validate_master_key_manager ...
...
  }
  • Doc added/updated
  • Tests added

@shirady shirady force-pushed the nc-list-accounts-missing-master-key branch from 9e9c365 to a35f4f9 Compare February 16, 2025 07:44
@shirady shirady requested a review from naveenpaul1 February 16, 2025 11:41
@shirady shirady force-pushed the nc-list-accounts-missing-master-key branch 2 times, most recently from f4dc8ff to 6b029e9 Compare February 17, 2025 08:33
@shirady shirady requested a review from romayalon February 17, 2025 11:14
@shirady shirady force-pushed the nc-list-accounts-missing-master-key branch from 6b029e9 to bfa4dba Compare February 17, 2025 13:12
@shirady shirady requested a review from romayalon February 17, 2025 13:46
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 <[email protected]>
@shirady shirady force-pushed the nc-list-accounts-missing-master-key branch from bfa4dba to 6f63da5 Compare February 17, 2025 15:14
@shirady shirady merged commit ea0b9cd into noobaa:master Feb 17, 2025
11 checks passed
@shirady shirady deleted the nc-list-accounts-missing-master-key branch February 17, 2025 16:12
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.

3 participants