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

Set region parameter to be used for STS only on AWS secrets engine #22726

Merged
merged 8 commits into from
Oct 4, 2024

Conversation

GBT55
Copy link
Contributor

@GBT55 GBT55 commented Sep 1, 2023

This PR aims to fix the following issue: #22602

We thought that having the sts_region parameter might be the solution. However, we have opted for a different approach and with this change now, the AWS engine region parameter will only affect STS API requests, and not IAM requests such as rotating the root IAM credential (which was our main problem).

Also, the IAM endpoint on AWS is global, so it doesn't make a lot of sense to use a specific region on the request because it will most likely fail, causing the above issue we described.

@GBT55 GBT55 requested a review from a team as a code owner September 1, 2023 09:20
@hashicorp-cla
Copy link

hashicorp-cla commented Sep 1, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, without iam_endpoint and sts_endpoint set, there is no problem rotating root credentials with the region parameter set. I understand that IAM is a global service, but I'm worried that this has the potential to change behavior for most of our users.

Would you mind clarifying whether #22602 is a bug or feature request? If it's a bug, reproduction steps would be needed. I don't want to risk behavior changes for the rest of our users for what seems like an uncommon configuration (with the proxying of all STS traffic).

@@ -36,22 +36,21 @@ func getRootConfig(ctx context.Context, s logical.Storage, clientType string, lo

credsConfig.AccessKey = config.AccessKey
credsConfig.SecretKey = config.SecretKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it would change behavior for configurations that explicitly set the region and don't set iam_endpoint or sts_endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uploaded the following commit to address this: 51aee8e

@schavis schavis added the needs-eng-review Community PR waiting for ENG review label Sep 12, 2023
@GBT55
Copy link
Contributor Author

GBT55 commented Sep 14, 2023

Today, without iam_endpoint and sts_endpoint set, there is no problem rotating root credentials with the region parameter set. I understand that IAM is a global service, but I'm worried that this has the potential to change behavior for most of our users.

Would you mind clarifying whether #22602 is a bug or feature request? If it's a bug, reproduction steps would be needed. I don't want to risk behavior changes for the rest of our users for what seems like an uncommon configuration (with the proxying of all STS traffic).

Thank you @austingebauer.
I think this might be a bug, reproduction steps ahead:

  1. Launch Vault in a non aws vm or environment:
  2. Enable the aws secrets engine and write the following data to the AWS secrets engine root config
{
  "access_key": "xxxxxxx",
  "secret_key": "xxxxxxx",
  "iam_endpoint": "https://iam.example.proxy.com",
  "sts_endpoint": "https://sts.example.proxy.com",
  "region": "eu-west-1"
}
  1. Then create a role on this aws secrets engine, e.g.:
{
  "credential_type": "assumed_role",
  "role_arns": "arn:aws:iam::xxxxxxx:role/xxxxxx"
}
  1. Now try to get credentials from this role, everything should work fine and you should get the credentials
curl \
    --header "X-Vault-Token: $VAULT_TOKEN" \
    --request POST \
    $VAULT_ADDR/v1/aws/creds/<roleName>
  1. Now try to rotate the root IAM credentials
curl -sX POST --header "X-Vault-Token: ${VAULT_TOKEN}" $VAULT_ADDR/v1/aws/config/rotate-root

You will get this error:

{"errors":["1 error occurred:\n\t* error calling GetUser: SignatureDoesNotMatch: Credential should be scoped to a valid region.\n\tstatus code: 403, request id: xxxxxxxxxxxxx\n\n"]}

Why?

As we can see here, the "eu-west-1" region is being used when you call the sts service with "aws/creds/" which is fine. However, it is also being used when calling the iam service with "aws/config/rotate-root"; this is incorrect because iam is a global service which (in this case) means the same as using the "us-east-1" region, which is clearly not the same as "eu-west-1" and therefore causes an error.

A custom region can be used for sts calls but not for iam calls.

Furthermore, if we try it the other way around, i.e. leaving the region empty, this happens:

  1. Write the following data to the aws root config:
{
  "access_key": "xxxxxxx",
  "secret_key": "xxxxxxx",
  "iam_endpoint": "https://iam.example.proxy.com",
  "sts_endpoint": "https://sts.example.proxy.com"
}
  1. Now try to get credentials from this role, you will get an error.
{"errors":["Error assuming role: SignatureDoesNotMatch: Credential should be scoped to a valid region.\n\tstatus code: 403, request id: xxxxxxxxxxxxxxxxxx"]}
  1. But, try now to rotate the key and you will see it works fine:
{"request_id":"xxxxxxxxxxxxxxxxx","lease_id":"","renewable":false,"lease_duration":0,"data":{"access_key":"xxxxxxxxx"},"wrap_info":null,"warnings":null,"auth":null}

Now it is happening the other way around, the region is defaulting to "us-east-1" (same as global) and therefore the iam service works, but our sts service which is located in "eu-west-1" does not.

@austingebauer
Copy link
Member

Hi @GBT55 - Taking a look at this again and have some questions.

A custom region can be used for sts calls but not for iam calls.

This is not the behavior that I'm seeing when iam_endpoint is not set. Are you able to explain why I'm able to use a custom region, say us-west-2, for IAM calls to successfully rotate root when iam_endpoint is not set?

For example, like:

vault write aws/config/root \
    access_key="xxx" \
    secret_key="xxx" \
    region="us-west-2"

The SignatureDoesNotMatch is leading me to wonder if the IAM request is signed with region-specific information (in your case, eu-west-1) but is somehow being routed incorrectly via the proxying setup. Can you give more details on what the proxy is doing? Have you tried to set the region="aws-global" (looking at valid region values)?

@GBT55
Copy link
Contributor Author

GBT55 commented Nov 15, 2023

Hi @austingebauer we have been investigating further if it was our proxy's fault due to incorrect configuration, but we don't think so.

Are you able to explain why I'm able to use a custom region, say us-west-2, for IAM calls to successfully rotate root when iam_endpoint is not set?

We have tested it and we are also able to rotate the credentials successfully. The problem comes when specifying ANY endpoint AND a region other than us-east-1. From our local development machines, with direct connection to the internet, we have encountered the following cases:

Vault Parameters Case 1 Case 2 Case 3 Case 4
iam_endpoint https://iam.amazonaws.com https://iam.amazonaws.com https://iam.amazonaws.com nil
sts_endpoint https://sts.amazonaws.com https://sts.amazonaws.com https://sts.eu-west-1.amazonaws.com nil
region us-east-1 / nil eu-west-1 eu-west-1 eu-west-1
Rotate root credentials (IAM)
Generate credentials (STS)

This behaviour is exactly the same on our VMs that go through our proxy

Why is Case 4 working but Case 2 isn't, both should be the same right?
Looking at the source code we can see that in the file builtin/logical/aws/client.go, no matter if you use IAM or STS endpoint, the region will be override by the region parameter. Which is not ideal because, as said, IAM is not in the eu-west-1 region.

credsConfig.AccessKey = config.AccessKey
credsConfig.SecretKey = config.SecretKey
credsConfig.Region = config.Region
maxRetries = config.MaxRetries
switch {
case clientType == "iam" && config.IAMEndpoint != "":
	endpoint = *aws.String(config.IAMEndpoint)
case clientType == "sts" && config.STSEndpoint != "":
	endpoint = *aws.String(config.STSEndpoint)
}

We have found a possible solution. We understand that more people may be using the region parameter and changing it may result in unexpected behavior, so we propose to create a new parameter for aws/config/root called sts_region, similar to the one existing on aws auth backend. This parameter will override the region, if set, so that it can continue to work as it does now and people (like us) who use a regional STS endpoint will be able to specify the sts_region for it. Here is quick peek at this solution:

credsConfig.AccessKey = config.AccessKey
credsConfig.SecretKey = config.SecretKey
credsConfig.Region = config.Region
maxRetries = config.MaxRetries
switch {
case clientType == "iam" && config.IAMEndpoint != "":
	endpoint = *aws.String(config.IAMEndpoint)
case clientType == "sts" && config.STSEndpoint != "": 
	endpoint = *aws.String(config.STSEndpoint)
	if config.STSRegion != "" {
		credsConfig.Region = config.STSRegion
	}
}

We have already tested and compiled Vault with this change and all 4 cases are working correctly. If you give me the ok I will commit the changes to this PR.
Thanks.

@austingebauer
Copy link
Member

@GBT55 - Thanks for doing the detailed analysis! The table is really helpful. I'm good on moving forward with the sts_region solution. It also helps that we have precedent for this in the AWS auth method. If you PR the changes then I'll review them.

@GBT55
Copy link
Contributor Author

GBT55 commented Dec 12, 2023

Hi @austingebauer! Sorry for the delay in replying. I just uploaded the commit with the changes I told you about, just so you know.
Thank you very much.

Copy link
Member

@robmonte robmonte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, sorry for the delay since the last follow-up!

@tomcf-hcp tomcf-hcp dismissed austingebauer’s stale review October 4, 2024 15:47

Resolved separately.

@robmonte robmonte requested a review from a team as a code owner October 4, 2024 15:49
@robmonte robmonte enabled auto-merge (squash) October 4, 2024 18:19
@robmonte robmonte merged commit aeca0cd into hashicorp:main Oct 4, 2024
66 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-eng-review Community PR waiting for ENG review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants