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

Support IMDSv2 HttpTokens #63067

Merged
merged 2 commits into from
Dec 15, 2022
Merged

Support IMDSv2 HttpTokens #63067

merged 2 commits into from
Dec 15, 2022

Conversation

SteffeyDev
Copy link

@SteffeyDev SteffeyDev commented Nov 15, 2022

What does this PR do?

This PR allows salt-cloud to connect to IMDSv2 using tokens, while still supporting IMDSv1.

I'm running salt-cloud on an EC2 instance that requires tokens for IMDS, which caused salt-cloud to fail to authenticate with EC2 when using use-instance-role-credentials in the cloud provider. My changes allow salt-cloud to authenticate successfully by using IMDSv2 tokens to get the security-credentials.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Not sure if any documentation changes are needed, as I didn't see anything in the documentation about it not supporting IMDSv2 already. Let me know if any documentation changes are needed.

Commits signed with GPG?

No

@SteffeyDev SteffeyDev requested a review from a team as a code owner November 15, 2022 21:42
@SteffeyDev SteffeyDev requested review from dwoz and removed request for a team November 15, 2022 21:42
@welcome
Copy link

welcome bot commented Nov 15, 2022

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@cmcmarrow cmcmarrow added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Nov 16, 2022
@cmcmarrow
Copy link
Contributor

@SteffeyDev can we get some tests and a change log? Thank your for contributing to salt. I see you kept backwards compatibility in mind, thanks for that!

@SteffeyDev
Copy link
Author

@SteffeyDev can we get some tests and a change log? Thank your for contributing to salt. I see you kept backwards compatibility in mind, thanks for that!

Sure. As I said in the initial comment, I don't see any existing tests for this file, so I'm not sure the best way to test just this change independently. Can you provide some pointers for that?

@cmcmarrow
Copy link
Contributor

@SteffeyDev you could write some mock tests for get_metadata. You would pass the old way args for 1 test and check results is right. You would then do the same thing with the new args and make sure you get the right results again. You could mock requests.get

@SteffeyDev
Copy link
Author

Ok @cmcmarrow I added some tests, can you please review?

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

just a clarifying question

salt/utils/aws.py Show resolved Hide resolved
@SteffeyDev SteffeyDev requested review from Ch3LL and removed request for dwoz November 30, 2022 18:47
@Ch3LL Ch3LL merged commit 581b759 into saltstack:master Dec 15, 2022
@lorengordon
Copy link
Contributor

@lorengordon
Copy link
Contributor

I see two or three issues related to support for IMDSv2, and could possibly be closed by this patch. Can anyone testing confirm?

@Ch3LL I did some testing last week, and believe IMDSv2 support is indeed working in salt 3006. It might be worth following up a bit on the three tickets I linked and seeing if they can be closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants