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

modify node key script to use AWS kms #358

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Oct 29, 2024

PR Type

enhancement


Description

  • Migrated key management from Hashicorp Vault to AWS KMS in the generate_node_keys.sh script.
  • Updated the add_to_vault function to add_to_kms and modified it to use AWS KMS for key encryption.
  • Changed the restore_node_keys.sh script to retrieve and decrypt keys using AWS KMS.
  • Enhanced the scripts to store key-value pairs in the .env file after decryption.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Enhancement
generate_node_keys.sh
Migrate key management from Hashicorp Vault to AWS KMS     

scripts/generate_node_keys.sh

  • Changed key management from Hashicorp Vault to AWS KMS.
  • Updated function names and comments to reflect AWS KMS usage.
  • Modified API calls to use AWS KMS for encryption.
  • Adjusted echo statements for better readability.
  • +10/-10 
    restore_node_keys.sh
    Update key retrieval to use AWS KMS                                           

    scripts/restore_node_keys.sh

  • Updated function to retrieve keys from AWS KMS instead of Vault.
  • Added decryption process using AWS KMS.
  • Modified storage process in .env file with AWS KMS data.
  • +9/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Hardcoded Values
    The AWS KMS key ID and region are hardcoded in the script. This might not be suitable for different environments or security practices.

    Incorrect Encryption Call
    The script incorrectly uses the encryption command instead of decryption when retrieving the key-value pair from AWS KMS.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the plaintext format in the AWS KMS encryption command

    Ensure that the plaintext format for the AWS KMS encryption command is correctly
    structured to avoid potential issues with key-value parsing.

    scripts/generate_node_keys.sh [37]

    -aws kms encrypt --key-id ${kms_key_id} --plaintext "key=${key_id} value=${key_value}" --region ${aws_region}
    +aws kms encrypt --key-id ${kms_key_id} --plaintext "{\"key\":\"${key_id}\", \"value\":\"${key_value}\"}" --region ${aws_region}
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a potential bug by ensuring the plaintext format is correctly structured for AWS KMS encryption, which is crucial for proper key-value parsing and avoiding errors.

    9
    Correct the handling of encrypted data to ensure it is properly stored and accessed

    Ensure that the encrypted key-value is stored in a file before attempting to decrypt
    it, as the current command assumes the output is a file path.

    scripts/restore_node_keys.sh [35-36]

    -local encrypted_key_value=$(aws kms encrypt --key-id ${key_id} --plaintext "key=${key_id} value=${key_value}" --region us-east-2)
    -local decrypted_key_value=$(aws kms decrypt --ciphertext-blob fileb://${encrypted_key_value} --output text --query Plaintext --region us-east-2)
    +local encrypted_key_value=$(aws kms encrypt --key-id ${key_id} --plaintext "key=${key_id} value=${key_value}" --region us-east-2 --output text --query CiphertextBlob)
    +echo "${encrypted_key_value}" > encrypted_key_value.bin
    +local decrypted_key_value=$(aws kms decrypt --ciphertext-blob fileb://encrypted_key_value.bin --output text --query Plaintext --region us-east-2)
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a critical issue by ensuring that encrypted data is stored in a file before decryption, aligning with the expected input format and preventing runtime errors.

    8
    Best practice
    Replace hard-coded values with configurable environment variables

    Replace the hard-coded AWS KMS key ID and region with variables to allow for
    flexibility and configuration changes without modifying the script.

    scripts/generate_node_keys.sh [34-35]

    -local kms_key_id="arn:aws:kms:us-west-2:123456789012:key/abcd1234-a123-1234-a123-123456789012" # Replace with your KMS key ID
    -local aws_region="us-east-2"
    +local kms_key_id=${AWS_KMS_KEY_ID}
    +local aws_region=${AWS_REGION}
    Suggestion importance[1-10]: 8

    Why: This suggestion improves flexibility and maintainability by replacing hard-coded values with environment variables, allowing for easier configuration changes without modifying the script directly.

    8
    Possible issue
    Add error handling to the AWS KMS decryption command

    Modify the decryption command to handle errors and check for successful decryption
    before proceeding.

    scripts/restore_node_keys.sh [36]

     local decrypted_key_value=$(aws kms decrypt --ciphertext-blob fileb://${encrypted_key_value} --output text --query Plaintext --region us-east-2)
    +if [ $? -ne 0 ]; then
    +  echo "Decryption failed"
    +  exit 1
    +fi
    Suggestion importance[1-10]: 7

    Why: Adding error handling improves the robustness of the script by ensuring that decryption failures are caught and handled appropriately, preventing potential issues downstream.

    7

    @DaMandal0rian DaMandal0rian merged commit 727d75f into main Oct 29, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the feat/kms-key-management branch October 29, 2024 11:56
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant