feat: add support for accessing AWS Neptune using assume role credentials #818
Conversation
|
@ssheladiya Thank you for the contribution. I took a quick glance at the code and it is very straightforward. I don't see any major blockers for this. I'll need to carve out some time to give this a proper test and double check the code. But overall, this looks great. |
|
I've added a related issue that points back to the discussion thread for traceability. |
|
@ssheladiya One small thought looking at the screenshot in the description, I think we should move the assume role input down one position. My thinking is that the region and service type fields are required, but the assumed role is optional. Making it last makes a bit more sense. |
sure, I can make that change |
ssheladiya
force-pushed
the
aws-assume-role-support
branch
from
a82094b to
04899a6
Compare
|
@kmcginnes Fixed all merge conflicts and changes. |
|
I did some testing using an assumed IAM role that had Neptune read only access. It looks good from that perspective. I need to verify with some internal folks to make sure there's no security concerns with this feature. This might take a bit of time (hopefully measured in days, but I don't want to promise anything). |
| // Function to check if the credentials are valid. | ||
| function areCredentialsValid(creds: AwsCredentials): boolean { | ||
| return creds.expiration | ||
| ? new Date(creds.expiration).getTime() - Date.now() > 5 * 60 * 1000 |
There was a problem hiding this comment.
Is this time range something that is consistent for all users and situations? Perhaps there is a better way to check for expiration instead of checking against a hard coded timespan?
There was a problem hiding this comment.
Usually credentials do come with the expiration time and we would need to check against that time if the credentials are valid or not. One other way I could think of is it let it fail and then retry with the new credentials. But I think that is not very efficient approach. Thoughts?
There was a problem hiding this comment.
That's a good question. My immediate thought would be
It surely complicates the logic, but I think it will yield the best outcome.
| sessionToken: Credentials?.SessionToken, | ||
| }), | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Is it possible this catch will apply to more than the STSClient. For example, I think it is possible that the aws4.sign() function could throw. If that happens the error message below does not apply.
It might be worth combining the fromNodeProviderChain() credential path and the new STSClient credential path in to a separate function. That allows you to put the try/catch in there.
Then this function becomes something more like (pseudo code):
function getIAMHeaders() {
const creds = getCredentials();
if (!creds) {
throw new Error();
}
return aws4.sign(options, {
accessKeyId: creds.AccessKeyId,
secretAccessKey: creds.SecretAccessKey,
...(creds.SessionToken && {
sessionToken: creds.SessionToken,
}),
});
}There was a problem hiding this comment.
That makes sense, I can make that change.
There was a problem hiding this comment.
fixed in - dfc34dc
There was a problem hiding this comment.
Nice. I'll take another look at this soon.
|
@ssheladiya Sorry for all the conflicting merges lately. You can keep the branch up to date each time if you like, or you can wait until I get the approval to move forward with this change. |
No worries. I can understand, these things take time :) |
|
@kmcginnes Now that I think of this, do you think it makes sense to abstract the assume role arn from the UI? I think if we can pass the assume role ARN settings through env vars, that would be better rather than adding the ARN on the UI. Users who wish to enable this feature can configure the proxy with necessary env vars. That would simplify the change and possibly would not require changes on the UI. WDYT? |
Honestly, I like that idea better. I think anything that is a field or checkbox in the UI is something a user is going to look at and question what their configuration should be. Since the use of assumed roles is not a common pattern for deploying graph-explorer (from what I've seen), I think it would be best to leave as an env var and have a user configure that upon container deployment. |
|
I 100% agree on making this an environment value that only the proxy server consumes. It makes the configuration more manual, but it also makes it potentially more secure. |
|
@ssheladiya Now that you've brought up this wonderful feature, we have some ideas on how to use it. Which means we'd like to get this merged in soon-ish (read within the next month). Would you like me to take over this branch and finish up the work, or would you like to finish it up? I don't want to step on any toes. |
One question, though: if we keep the assume-role ARN as an environment variable, won’t that restrict each Graph Explorer instance to a single role? My goal is to run one Graph Explorer in our management account and have it connect to Neptune clusters in multiple account |
|
@guyelia It certainly would. And you are correct, that is limiting. We will have to consider the options with that use case in mind. This will take a bit more thought. |
Description
Allows to connect to AWS Neptune DB/Graph via assume role credentials. A lot of companies use IAM role chaining as an extra layer of protecting to allow fine grained privileges to AWS resources. This PR allows to assume a specific role mentioned creating a connection.
Validation
Related Issues
Check List
license.
pnpm checksto ensure code compiles and meets standards.pnpm testto check if all tests are passing.Changelog.md.