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

2532 Avoid exposing environment variables while deploying on Digital Ocean #2535

Closed
wants to merge 7 commits into from

Conversation

arjxn-py
Copy link
Contributor

@arjxn-py arjxn-py commented Jun 24, 2024

Should fix #2532

This PR improves the environment variable validation function in utils.py by adding functionality to mask the values of environment variables if they are set, while ensuring missing variables are correctly identified & are not exposed.

What does this implement/fix?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

@arjxn-py
Copy link
Contributor Author

The ValueError that's being thrown now looks like:

Value error, Set environment variables are: {'DIGITALOCEAN_TOKEN': '******oken'}

Missing the following required environment variables: {'SPACES_ACCESS_KEY_ID', 'SPACES_SECRET_ACCESS_KEY'}

Please see the documentation for more information: https://www.nebari.dev/docs/how-tos/nebari-do

@viniciusdc
Copy link
Contributor

This looks great @arjxn-py, I am a bit conflicted though if we should showcase the settled values at all in the error message... Maybe some rewording of the final error message would help. What do you think?

@arjxn-py
Copy link
Contributor Author

What do you think?

When i added the line to showcase the settled values the motive was to help users quickly identify which variables are set and which are missing.

Hence I believe it depends upon how we prioritize things, i.e. :

  • If we prioritize ease of debugging and transparency we can show those masked values
  • If security is the highest priority and even minimal exposure is unacceptable, then we can avoid showing them.

You can check either of the above to let me know which way should I go.
In addition to that, if we'd not want to show the masked values, I believe that it makes the mask_values function redundant plus I think that the previous error message is minimal & good but in case you'd want to reword that too, you can let me know. Thanks 🙂💐

@arjxn-py
Copy link
Contributor Author

I'm not sure how much significance does this PR hold after nebari-dev/governance#50, but I've made some changes so that the raised ValueError looks like this now -

Value error, Missing the following required environment variables: {'SPACES_ACCESS_KEY_ID', 'SPACES_SECRET_ACCESS_KEY'}

Please see the documentation for more information: https://www.nebari.dev/docs/how-tos/nebari-do

I've also removed the mask_value method as that is not being used now.
Tagging @viniciusdc as a gentle follow up. Thanks.

@arjxn-py arjxn-py changed the title 2532 Mask the exposed environment variables while deploying on Digital Ocean 2532 Avoid exposing environment variables while deploying on Digital Ocean Jun 27, 2024
@arjxn-py
Copy link
Contributor Author

I don't see failing integration tests related to these changes. But if I can do anything about them, please let me know :)

@viniciusdc
Copy link
Contributor

Hi @arjxn-py, thanks for the follow-ups; it was a bit of a wild week. Yes, we will probably decommission DO soon, but I don't see any issues merging this as is. We will review this during our next internal meeting on Monday to see if we can include it in the Jully release. Thanks for the contribution !!

@viniciusdc
Copy link
Contributor

viniciusdc commented Jul 2, 2024

Hi @arjxn-py, thanks for showing up at the community meeting today!!! We hope to see you in the next one as well, and feel free to ask any questions during those meetings as well 🚀

Regarding this PR, though, I am inclined to ignore any changes added to it since we will probably remove these files when DO is deprecated. But I don't want to waste your efforts, so I would suggest you double-check how we are handling these errors with the other cloud providers (I assume we just copied the same logic over there), and then you can refactor this PR to change things over there instead 😄

@viniciusdc
Copy link
Contributor

you might not need to create accounts to the other providers, I think you might be able to workaround it by just set any random thing as the values for the variables (just make sure the name of the variables match what are in our docs) e.g

export AWS_ACCESS_KEY_ID="Access Key ID - add some random thing"
export AWS_SECRET_ACCESS_KEY="Secret Access Key - same"

try it out, then you don't need to create any accounts to test it (deploy will obviously not work though, keep that in mind)

@arjxn-py
Copy link
Contributor Author

arjxn-py commented Jul 5, 2024

Thanks for the further instructions @viniciusdc, I checked the same for other cloud providers as well and below are the results.

AWS

Seems to handle it well as we're creating boto3 session directly and handling client error nicely:

except ClientError as e:
if "AuthFailure" in str(e):
print(
"Please double-check that the AWS credentials are valid and have the correct permissions.",
"If you're deploying into a non-standard partition (e.g. AWS GovCloud), please ensure the region specified exists in that partition.",
)

nebari init aws --project projectname    --domain domain    --auth-provider password    
Unable to locate your Amazon Web Services credentials, refer to this guide on how to generate them:

        https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_access-keys.html


Paste your AWS_ACCESS_KEY_ID:
Paste your AWS_SECRET_ACCESS_KEY:
Defaulting to `us-east-1` region.
Please double-check that the AWS credentials are valid and have the correct permissions. If you're deploying into a non-standard partition (e.g. AWS GovCloud), please ensure the region specified exists in that partition.

GCP

I believe correct credentials should create a file with a name gcloud hence raising a FileNotFoundError, but I cannot see credentials being exposed

nebari init gcp --project projectname    --domain domain    --auth-provider password    
Unable to locate your Google Cloud Provider credentials, refer to this guide on how to generate them:

        https://cloud.google.com/iam/docs/creating-managing-service-accounts


Paste your GOOGLE_CREDENTIALS:
Paste your PROJECT_ID:
Defaulting to region:`us-central1`.

FileNotFoundError: [Errno 2] No such file or directory: 'gcloud'

Azure

Throwing ClientAuthenticationError & no leakage of credentials on fake credentials

Unable to locate your Azure credentials, refer to this guide on how to generate them:

        https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/guides/service_principal_client_secret#creating-a-service-principal-in-the-azure-por
tal


Paste your ARM_CLIENT_ID:
Paste your ARM_SUBSCRIPTION_ID:
Paste your ARM_TENANT_ID:
Paste your ARM_CLIENT_SECRET:
Defaulting to region:`Central US`.

ClientAuthenticationError: DefaultAzureCredential failed to retrieve a token from the included credentials.
Attempted credentials:
        EnvironmentCredential: EnvironmentCredential authentication unavailable. Environment variables are not fully configured.
Visit https://aka.ms/azsdk/python/identity/environmentcredential/troubleshoot to troubleshoot.this issue.
        ManagedIdentityCredential: ManagedIdentityCredential authentication unavailable, no response from the IMDS endpoint.
        SharedTokenCacheCredential: SharedTokenCacheCredential authentication unavailable. No accounts were found in the cache.
        AzureCliCredential: Azure CLI not found on path
        AzurePowerShellCredential: PowerShell is not installed
To mitigate this issue, please refer to the troubleshooting guidelines here at https://aka.ms/azsdk/python/identity/defaultazurecredential/troubleshoot.

@viniciusdc
Copy link
Contributor

Uhm.. I see, so it seems that the other providers were already fixed in the past, and only DO was left behind, primarily due to previous discussions on deprecation, though this also means we don't have anything else to change there... @arjxn-py thanks for having a look and inspecting the other providers. I think you can close this one since we will be removing the DO files soon

@arjxn-py
Copy link
Contributor Author

Closing xref: nebari-dev/governance#50

@arjxn-py arjxn-py closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

[ENH] Digital Ocean exposes deploy token when validator error occours
2 participants