search

ex-aws / ex_aws_sts

13 stars 31 forks source link

Assume role web identity adapter #15

Closed tjsousa closed 4 years ago

tjsousa commented 4 years ago

This introduces support for using web identity tokens when assuming a role for authentication.

Currently supports ENV vars but since it hooks into the adapter mechanism that uses configuration files it needs to bring the same dependencies (as ex_aws tries to read the config file before dispatching to the adapter). This is fine for now but should be revisited if the interface to the adapters changes in the future to be more flexible.

It also forces dummy values for authentication since every request to ex_aws will try to use an authentication mechanism (to create an Authorization header) even if in this case the request doesn't actually need one.

This also adds unit tests to the existing adapter.

vanetix commented 4 years ago

Hey @tjsousa I'll set aside some time this week and review this. Thank you!

tjsousa commented 4 years ago

Thanks @vanetix. I was wondering if we should make sweet_xml part of the runtime dependencies as the AssumeRole operations need the respective parser, even though this package can be used without the adapters.

In this PR I preferred to remove it from :test to simplify testing, but I am not sure what's the best course of action. Maybe review the approach in a different PR? Or just update the README for people not to forget to include it? What do you think?

vanetix commented 4 years ago

Also in regards to the sweet_xml question - I personally think this ex_aws module should require it since its not much use without the parser (unless you're using another xml parser). Maybe it could be nice to take the approach of similar to the json_codec option?

tjsousa commented 4 years ago

Hi @vanetix! Thanks for reviewing, I've pushed the requested change along with a unit test and rebased with the master branch. Should we wait for @koozdra's take?

We've been using the fork in production for a few weeks and it is looking good. I agree the overhaul of xml parsing but think it should be tackle separately, do you agree?

vanetix commented 4 years ago

Yep, I think we can handle the SweetXML parsing stuff in another thread / PR. 👍

I'll wait for @koozdra to take a look as well, but we can get this merged by the end of the week regardless. Thanks!

tjsousa commented 4 years ago

Thank you 🙌

Do you have plans to release of a new version of the package soon?

vanetix commented 4 years ago

For sure, we're going to get with @benwilson512 to see if we can get it released or added as hex.pm maintainers.

tchamblee commented 4 years ago

Any chance we could get an update pushed to hex.pm @benwilson512 ? We've been waiting on this for a while too.

ckreiling commented 4 years ago

Currently referencing the commit SHA as well - a hex.pm release would be fantastic!