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

[batch] Use default credentials for the Azure SAS token test #13981

Conversation

daniel-goldstein
Copy link
Contributor

@daniel-goldstein daniel-goldstein commented Nov 6, 2023

A very small PR but here's the background and context behind this change. When talking to either GCP or Azure, hail chooses credentials in the following order from highest priority to lowest priority:

  1. An explicit credential_file argument passed to the relevant credentials class
  2. An environment variable containing the path to the credentials (GOOGLE_APPLICATION_CREDENTIALS or AZURE_APPLICATION_CREDENTIALS) (from this you can see why the code that was here is totally redundant)
  3. The latent credentials present on the machine. This might be gcloud or az credentials, or the metadata server if you're on a cloud VM.

I'm trying to rid the codebase of most explicit providing of credentials file paths, for two reasons:

  • Quality of life. I'm already signed into the cloud with gcloud and az. I shouldn't need to download some file and provide AZURE_APPLICATION_CREDENTIALS to run this test. It should just use the latent credentials.
  • We are trying to phase out credentials files altogether for security reasons. These files are long-lived secrets that you really don't want to leak and are currently exposed to users in Batch jobs, so they can be easily exfiltrated. Using the latent credentials on a cloud VM (the metadata server) has the benefit of only issuing short-lived access tokens which last for hours not months, so it's basically always better to use the latent credentials when possible.

@danking danking merged commit 8b498aa into hail-is:main Nov 21, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants