-
Notifications
You must be signed in to change notification settings - Fork 143
Use Instance profile credential #8
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
Comments
Would this be the |
A very important enhancement request actually. It is considered to be very insecure to save a permanent AWS_ACCESS_KEY and AWS_SECRET_KEY somewhere. It's even forbidden in a lot of enterprise environments. Instead roles should be used to allow AWS components to access services they need. So let's assume this nginx-s3-gateway would run on a AWS EC2 instance, with the right role applied to the instances it would be allowed to access the S3 bucket with temporary keys which can be retrieved from the EC2 metadata, as described here: https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2.html That would be a huge security improvement for the nginx-s3-gateway, so I clearly vote up for this, new feature request. |
"Retrieve security credentials from EC2 instance metadata or ECS task metadata endpoint" ? https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html#instance-metadata-security-credentials |
Depending on how you are running it, if nginx is running on ec2 then instance if ecs then task. |
Hi, |
This feature would also be hugely beneficial to my team. |
I would like some help from the community in designing this feature. I've got the following questions: Do the security credentials need to be refreshed at a periodic interval? If so, it will be difficult to implement the feature purely within nginx. I can imagine a few implementations:
Does anyone have other ideas about how to implement this? |
Appreciate your response! I had looked into implementing the IAM role feature, and this is the path that I had initially considered. Though the other approaches seem interesting as well. I'll highlight some of the information that I've found in my research so far.
Hopefully this is helpful. I'll continue to explore this on my own as time permits and I'm happy to help do any research that might get this feature implemented quickly! |
I don't know if this is accurate. If you click on the |
I'm a complete newbie to how this project works :) However, I like it 👍 The expiration of credentials for a role of an ECS task running on the fargate compute type is, by default and currently, 6 hours, iirc. That means, we could hold a copy of these credentials for quite a long time actually. The response from the endpoints mentioned by @ajschmidt8 are also containing the date and time when these credentials will expire. At least for EC2 (I assume it's the same for ECS), credentials are automatically renewed by the endpoints at least 5 minutes before the current ones expire. While looking at the Compatibility of the njs module, I came to a quick idea of how this could be implemented:
By doing so, nginx fetches the credentials asynchronously to actual requests (except for the first one, but even that could be worked around by adding a What do you think? Would that be possible, or did I miss an important details? :) |
While IMDSv1 is still possible to use, one can disable v1 on a per-instance configuration. Also, v2 protects against some possible attacks which were possible with v1 (https://aws.amazon.com/blogs/security/defense-in-depth-open-firewalls-reverse-proxies-ssrf-vulnerabilities-ec2-instance-metadata-service/). Because of that, implementing a new feature, which requires v1, might be difficult to justify. There might even be environments, where the v1 must be disabled and not be used for security or compliance reasons. I also think that, with an async approach, using v1 doesn't really give a big benefit anymore (compared to on a per-request basis). |
I spiked something together :D It's not nearly in a state to be in a PR but maybe it is already enough to get a discussion started about my proposed solution: https://github.com/nginxinc/nginx-s3-gateway/compare/master...FlorianSW:issue/8?expand=1 (I hope you can see this diff)
I'll explore a bit more in the next days or weeks whenever I find a bit of time. However, any feedback is welcome :) (especially int he njs area, as this is completely new to me). |
i used my changes I mentioned yesterday and deployed them to ECS. A bit as expected it did not work. After trying to figure out what is going on, I think I now face two fundamental problems with njs (which are by design as far as I know), which, until now, I did not find any solution for (nor a good one):
|
@FlorianSW, just wanted to say that I appreciate the effort here! Hopefully you and @dekobon can come up with something that works. |
Hi @FlorianSW,
Yes, you are right. Currently, all requests are totally independent by design.
Take a look at the following example.
The quick, but not so elegant solution would be implementing
not implemented yet.
One way to do this is to refresh credentials once they are detected to be stale. |
Yeah, I kind of expected such an answer :/
I actually do the example of "generating a JWT". Means:
Based on that it seems I'm doing something wrong 😅
I thought about that already as well. Basically, we "just" need to save three values (they even do not need to be persistent) somewhere. The environment was something I was thinking about as well, as the values would reside there anyway, if one does not use the instance credentials. If I see it correctly (and reading that into your "would be implementing Giving this a bit more thought, I wouldn't call it "not so elegant" anymore to be honest. Like I said, our requirements for persistency and even synchronicity between worker processes are really low (basically: no need for any of that), as these credentials can simply be retrieved again when they are not present (anymore).
Is there any plan on having something like that? I mean, right now, it would not be a big issue, if that is not present as a feature (given we can work around the other things, the first request might get impacted in performance, however, that should be ok every 6 hours or so).
But that would be in the scope of an actual request, right? Meaning, if a request is served and the credentials are found to be expired, they would be refreshed before the request can be served, right? |
Unfortunately, The limitation is that nginx is not able to wait for a variable evaluation asynchronously. (WebCrypto async methods are working right now, because they can be executed synchronously in a JS call.) But, as I mentioned above, the
BTW, this is not as bad as it may sound. nginx is reading from FS all the time. In most cases, frequently accessed files are always in a file memory cache, so reading such a file is usually fast. (The FS solution is definitely suboptimal when the file content is changed often and not in atomic fashion, but the credential example is different).
yes, setenv is not implemented.
Yes, this is will be global (process level) change, visible to other requests.
I remembered the good (nginx) way to handle expired data (like credentials). |
just to clarify. As of now, unlike |
Alright, I played around a bit in the last days with what inputs @xeioex gave in the recent comments, and it seems I have a working proof of concept, at least :) For the setup:
The current flow looks like this:
Caveats:
I'll update the code in my branch in the evening, if I do not forget it 🙈 |
I just made the last changes I wanted to make, which now also includes refreshing credentials, when the cached ones reach their expiration time. This should make sure, that always fresh credentials are used to access the respective S3 bucket. As the default expiration in ECS is 6 hours, I wasn't able to test out the refresh directly, however, it's the same logic as if no credentials are there. Assuming that the As I'm more or less happy with my code, I opened a PR (#18) for all of you to review :) There is also the open points that needs testing:
After all: Every review, suggestion and feedback is welcome :) |
This is huge! Thank you for all of your hard work on this @FlorianSW. I will try to find some time this weekend to test this out. |
I'd like to move discussion regarding instance profile support to github issues rather than the PR because there is more to discuss than what can be contained in a single PR. First off, I've made a number of changes in the branch version.
Next, I see that we still need to do the following before integrating:
Any and all help is welcome! |
Fixed in 0df77d6 |
Is it possible to use temporary credential retrieved from instance profile/role?
The text was updated successfully, but these errors were encountered: