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.

Sign up for GitHub

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

Jump to bottom

Support AWS IMDSv2 #28

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Support AWS IMDSv2 #28

merged 6 commits into from
Jan 31, 2024

Conversation

arossert
Copy link
Contributor

Added support for IMDSv2
#27

@kshivakumar kshivakumar self-requested a review January 29, 2024 11:30
kshivakumar
kshivakumar reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@kshivakumar kshivakumar left a comment

Choose a reason for hiding this comment

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

@arossert changes look fine. Since IMDSv2 is supposed to be more secure than v1, it's good to implement it now.
Have you tested it in both Linux and Windows EC2 instances?
I couldn't test it myself as I am having some issues with my AWS account, haven't used it in long time.

@kshivakumar
Copy link
Collaborator

@arossert Can you call the two APIs concurrently using asyncio.gather.
As per the documentation, each instance is user-configured to use only one of the APIs. It seems in your case, your Windows EC2 instances are configured to use v2.

@arossert
Copy link
Contributor Author

@kshivakumar yes I tested it on both Windows and Linux, what I don't know to tell is if V2 can be disabled and api/token call can fail.

If this is the case we can try to do what you suggested and do both, one to try with V1 (without token) and the other with.

Let me know if you think it will work better

@kshivakumar
Copy link
Collaborator

I tested it on both Windows and Linux...

Great!
@arossert Yeah, please go ahead with concurrently calling both the APIs.

@arossert
Copy link
Contributor Author

@kshivakumar I ended up keeping the old _get_metadata flow and added _get_metadata_v2 that generates a token and calls the _get_metadata with the needed header.

I removed the try...catch block and just ignored the exceptions with the return_exceptions flag.
I will run some more tests on this new changes and report back

Let me know if this looks good to you.

kshivakumar
kshivakumar requested changes Jan 30, 2024
View reviewed changes
cloud_detect/providers/aws_provider.py Outdated Show resolved Hide resolved
cloud_detect/providers/aws_provider.py Outdated Show resolved Hide resolved
@kshivakumar kshivakumar changed the title Support imd sv2 Support AWS IMDSv2 Jan 31, 2024
@kshivakumar
Copy link
Collaborator

@arossert Please fix the "DCO" check.

@amirrossert
Get token before accessing document API
ce95c33 
Signed-off-by: Amir Rossert <[email protected]>
@amirrossert
Fix tests
90f627b 
Signed-off-by: Amir Rossert <[email protected]>
@amirrossert
Check for v1 and v2 APIs
0bb4b8d 
Signed-off-by: Amir Rossert <[email protected]>
@amirrossert
And try block
dd10b63 
Remove response.raise_for_status()
Change TTL to 60 seconds

Signed-off-by: Amir Rossert <[email protected]>
@amirrossert
Refactor
245b983 
Signed-off-by: Amir Rossert <[email protected]>
@amirrossert
Use contextlib.suppress
fc11d7b 
Signed-off-by: Amir Rossert <[email protected]>
@arossert
Copy link
Contributor Author

@arossert Please fix the "DCO" check.

Fixed

@kshivakumar kshivakumar merged commit ee34d0d into dgzlopes:master Jan 31, 2024
7 checks passed
@kshivakumar kshivakumar mentioned this pull request Jan 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kshivakumar kshivakumar kshivakumar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arossert @kshivakumar @amirrossert