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

IAM role support #362

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jpiper
Copy link
Contributor

@jpiper jpiper commented Mar 28, 2016

I noticed #344 and thought I would get the ball rolling. Here's an implementation that will fall back to using an IAM role when no other credentials are found.

@jpiper jpiper force-pushed the jpiper/develop-iam-support branch 2 times, most recently from 29b5c78 to 56ad18e Compare March 28, 2016 11:02
@jpiper jpiper force-pushed the jpiper/develop-iam-support branch from 56ad18e to 482efe1 Compare March 28, 2016 11:08
@jmarshall
Copy link
Member

Apologies for not getting back to you on this sooner after getting the 1.3.1 release out the door.

As discussed on #344, if we do include this functionality, it needs to be something that is actively enabled by the user rather than something that might take non-EC2 users by surprise. Also it would be preferable to avoid burning Amazon IP numbers and paths (http://169.254.169.254/latest/meta-data/iam/security-credentials/) into htslib.

I've dusted off and committed to develop (see 1cc7d12) a curl_kput() function that does the same thing as your curl_as_string(), but writes to a kstring_t as those already have the code to manage a memory buffer.

I would accept a pull request that looked for a new key iam_role_url (we can workshop the key name further…) in the .aws/credentials file:

iam_role_url = http://169.254.169.254/latest/meta-data/iam/security-credentials/

and, in the absence of other credentials, used the presence of that to trigger code to curl_kput-fetch those two URLs and extract AccessKeyId/SecretAccessKey/Token from the resulting JSON.

I'd rather the JSON was parsed using string functions rather than regexps (htslib currently doesn't #include <regex.h> and would like to stay that way). But actually htslib may eventually need basic JSON reading capabilities for some other things too, so don't worry about that too much: I may be adding something akin to parse_ini's interface for simple JSON reading needs, which would suffice for parsing this.

@jpiper
Copy link
Contributor Author

jpiper commented May 17, 2016

To me, rather than conflating the .aws/credentials file, using IAM to retrieve the security credentials could be handled via an env var (likewise, we could bikeshed on the name, but something like HTSLIB_IAM_FALLBACK or something ).

With regards to burning in this IP address - this shouldn't be a problem as this URL has always been used for AWS metadata, and the 169.254* IP block is reserved for link local see RFC3927 and here so will never traverse past the local network.

I completely agree on the regex parsing though - this was a very dirty hack and a better solution could definitely be found.

@jmarshall
Copy link
Member

People in coffee shops do not have the liberty of trusting their local network.

@jpiper
Copy link
Contributor Author

jpiper commented May 17, 2016

True - I guess my point here is that given that the user will always have to manually enable IAM support, is there a difference in potential security vulnerabilities between using a simple TRUE/FALSE flag, or having to copy and paste in an IP address that's always going to be the same?

It results in the same behaviour, it's just one is easier to activate than the other.

@jmarshall
Copy link
Member

True, the explicit switch is what protects people with miscreants on their local networks. So the reason for specifying the URL in the switch is (1) somewhat tenuously, there may be a different equivalent for other S3-alike providers; (2) we are not going to burn Amazon-specific IP numbers and paths into HTSlib.

I would prefer a .aws/credentials key to a random environment variable, as that is more self-contained. However I have not verified that other .aws/credentials parsing tools (notably Amazon's!) quietly accept keys that they do not recognise. But cf #346.

@jpiper
Copy link
Contributor Author

jpiper commented May 30, 2016

Noted. How about using KSON (from klib) for JSON parsing? I'd be happy to cook up a new PR based on your develop branch (with curl_kput) with your aforementioned requirements (not storing the AWS metadata url in the code).

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.

3 participants