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

make JSON_CREDENTIALS optional #79

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

ezra-risq
Copy link

@ezra-risq ezra-risq commented Apr 8, 2023

The JSON_CREDENTIALS value should be optional.

  • The google client libraries support alternate authentication methods, so users shouldn't be forced to only use the json credentials file method
  • My company's security policy forbids the use of json key files stored on compute instances in production, so the json credentials file method is not an option for me
  • Simply making the JSON_CREDENTIALS option optional allows the underlying Google libraries to just do the right thing as regards the ADC search order (see https://cloud.google.com/docs/authentication/application-default-credentials )
  • This PR will look for the JSON_CREDENTIALS file if that option is present, and just lets ADC do its thing if not.

@meeb
Copy link
Owner

meeb commented Apr 9, 2023

Thanks! I've not used anything Google-y for some years so I'm not surprised this needs some maintenance. Your patch looks fine to me if the application credentials detection works properly and as you describe now. Could you just update the readme with a # JSON_CREDENTIALS are optional if you need to override the default credentials detection note or similar comment and I'll merge this into the next release.

(Also, totally unrelated, your SSL cert for https://www.risq.io/ expired in January if you hadn't noticed!).

@ezra-risq
Copy link
Author

Great-- I just updated the README.

(Also, totally unrelated, your SSL cert for https://www.risq.io/ expired in January if you hadn't noticed!).

Yeah, my company got acquired and that's not my job anymore-- I have to figure out whose it is; they really should just redirect our old site.

@meeb meeb merged commit 91ddf03 into meeb:master Apr 11, 2023
@meeb
Copy link
Owner

meeb commented Apr 11, 2023

Perfect, thanks!

@meeb
Copy link
Owner

meeb commented Apr 11, 2023

This has been rolled into the latest release, v3.1.3 and is available now.

@micahlagrange
Copy link

This needs done for AWS too, allow us to use instance profiles instead of specifying keys/secrets.

@meeb
Copy link
Owner

meeb commented Mar 11, 2024

Got an example of using instance profiles with boto? Also that's probably a different issue. I don't personally publish with distill to S3 so I'm happy to accept PRs for that as well.

@micahlagrange
Copy link

micahlagrange commented Mar 11, 2024 via email

@meeb
Copy link
Owner

meeb commented Mar 11, 2024

So you can just call boto3.client('s3') with no other arguments and it'll, I assume, pull in the access credentials from the environment?

@micahlagrange
Copy link

micahlagrange commented Mar 11, 2024 via email

@meeb
Copy link
Owner

meeb commented Mar 12, 2024

I've created #88 to track this as it's a different issue.

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