fix: Remove SystemCredentials source check during AWS role assume#22859
fix: Remove SystemCredentials source check during AWS role assume#22859shortstacked merged 2 commits inton8n-io:masterfrom
Conversation
6eef6c3 to
eca2c8a
Compare
|
|
||
| type Resolvers = 'environment' | 'podIdentity' | 'containerMetadata' | 'instanceMetadata'; | ||
| type RetrunData = { | ||
| type ReturnData = { |
There was a problem hiding this comment.
While stepping through understanding, noticed this misspelling. This type is not exported and only used below.
| ); | ||
| }); | ||
|
|
||
| it('should successfully assume role using system credentials by instanceMetadata', async () => { |
There was a problem hiding this comment.
Ultimately, this could (should?) iterate through each source type and ensure it behaves as intended.
|
Hey @onyxraven, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: Regarding new nodes: If your node integrates with an AI service that you own or represent, please email [email protected] and we will be happy to discuss the best approach. About review timelines: Thank you again for contributing to n8n. |
eca2c8a to
6e5223d
Compare
|
This has been rebased against n8n master as of today.
|
|
Hello @onyxraven I habe a problem with EKS Pod Identity. All my conf is OK and I can assume role on the pod using CLI but not on n8n (Failed to assume role: System AWS credentials are required for role assumption. Please ensure AWS credentials are available via environment variables, instance metadata, or container role.) Logs in the pod-identity-agent is : For validate assume role is OK in CLI I do : Your pull request can fix my problem ? |
This looks like a different issue. This PR would handle the way n8n uses the credentials, where this error is the system failing to get the initial system credentials. I don't have access to a pod identity cluster currently so I can't reproduce. |
|
I'm still cherry-picking the commit from this PR into our docker builds as of v2.1.2 and it's still working great in production. This PR really is a low-impact merge that very clearly only affects a completely broken and undocumented Credential type, so seems like they should just merge it, IMO. |
|
hey @onyxraven i have a quick question, i was facing an issue when trying to enable AWS IAM Role assume, the error is Couldn’t connect with these settings I think your PR solves this? |
Hello @admirationmr I think you need to set env var "N8N_AWS_SYSTEM_CREDENTIALS_ACCESS_ENABLED" to true |
As @darkgoldxp said, you do need to set that environment variable, but even with that variable added, the credential will still not work with system credentials without this PR merged. |
@marty-sullivan Hey, thanks for the reply. I just enabled system credentials in the pod configuration, and now the error is slightly different: Failed to assume role: System AWS credentials are required for role assumption. Please ensure AWS credentials are available via environment variables, instance metadata, or container role. I’m guessing this means it won’t work until the PR is merged. |
Possibly. Depends on which flow you're expecting to use. IRSA is handled in this PR. AFAIK, the other strategies (pod identity, EC2 Instance via KIAM, etc) should work. The code first has to acquire the running-container's credentials, and then use those sign an STS request to assume the provided role and get those credentials (which requires the execution role and the role-to-assume to have mutual sts:assumeRole permissions. I think one of the previous comments may have been a missing policy permission.). That second step (the sts assume) is what this PR helps fix. Without it, you will likely not see this error, but an error when attempting to use the credentials in a node which will reference the container's execution role (something like: As far as a container, we're considering it. I'd rather not have a unofficial patch/fork that must be maintained if we can avoid it. |
6e5223d to
267f5a0
Compare
|
Glad to see a review is requested. I can confirm this PR is still working in our production self-hosted environment as of v2.6.3 |
ShireenMissi
left a comment
There was a problem hiding this comment.
Tested locally and it works as expected ✅
|
@onyxraven Thank you for your PR 🙏 |
Done! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
It failed the PR name check - should this be "AWS Node"? (Bummer is it passed previously?) |
|
Got released with |
Summary
This removes the 'source === "environment"' check done during the role assume for AWS SystemCredentials.
In every case of the SystemCredentials usage for AWS, it is supposed to assume the role required during credential setup (with the given externalId as well). The previous conditional would only do that assume if it was via environment. This left out the (recently added) pod, container, and instance sources, and those would return the system credentials instead (unintended, I think, and a potential security risk). These sources should act the same way.
I added a test using one of the other sources, and red-green tested. I've also tested this "in practice" along with my other PR (#22316) in our environment.
Related Linear tickets, Github issues, and Community forum posts
Fixes #21961
(https://linear.app/n8n/issue/GHC-5527)
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)