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

IMDSv2 support #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

IMDSv2 support #11

wants to merge 3 commits into from

Conversation

condour
Copy link

@condour condour commented Jun 15, 2022

Please ensure the following before submitting a PR:

  • if suggesting code changes or improvements, open an issue first
  • for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION
  • for all but trivial changes (e.g., typo fixes), documentation your change in NEWS.md with a parenthetical reference to the issue number being addressed
  • if changing documentation, edit files in /R not /man and run devtools::document() to update documentation
  • add code or new test files to /tests for any new functionality or bug fix
  • make sure R CMD check runs without error before submitting the PR

@condour condour marked this pull request as ready for review July 14, 2022 16:25
@condour condour changed the title rough draft IMDSv2 support Jul 21, 2022
Copy link

@laurenkahn laurenkahn left a comment

Choose a reason for hiding this comment

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

Test 1: EC2 using IMDSv1, Sys.setenv(USE_IMDS_TOKEN="TRUE")
Expected result: Should be able to access S3.
Actual result: Success, able to access S3.

Test 2: EC2 using IMDSv1, Sys.setenv(USE_IMDS_TOKEN="FALSE") or not set at all.
Expected result: Should be able to access S3, as in the original code.
Actual result: Failure - see comment. Header cannot be NULL.

Test 3: EC2 using IMDSv2, Sys.setenv(USE_IMDS_TOKEN="TRUE")
Expected result: Should be able to access S3 (unlike before).
Actual result: Success, able to access S3.

Test 4: EC2 using IMDSv2, Sys.setenv(USE_IMDS_TOKEN="FALSE") or not set at all.
Expected result: Should not be able to access S3, as in the original code.
Actual result: Not able to access S3, but for wrong reason (header = NULL). Addressing test #2's failure will fix this.

timeout <- get_timeout()
handle <- curl::new_handle(timeout_ms = timeout)

curl::handle_setheaders(handle, 'X-aws-ec2-metadata-token' = token)

Choose a reason for hiding this comment

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

This fails if token = NULL. For example if I use an EC2 using IMDSv1, and I don't set USE_IMDS_TOKEN to TRUE, and I run aws.ec2metadata::metadata$iam_role_names() I get the following error:

Error in curl::handle_setheaders(handle, X-aws-ec2-metadata-token = token): All headers must be strings.

Using a blank string instead seems to work when a token isn't required (IMDSv1).

Copy link
Author

Choose a reason for hiding this comment

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

I've added a check for is.null prior to setting the header. I'm unable to test at the moment, so I would appreciate if you can verify that it solves the issue. Thanks!

Choose a reason for hiding this comment

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

This works! The tests described above all have the expected results now.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Now I guess we just need someone to merge?

@daltschu22
Copy link

Is there a reason this hasnt been merged?

@EwanStephens EwanStephens mentioned this pull request Feb 1, 2024
3 tasks
@tyner
Copy link

tyner commented Feb 20, 2024

Would be great to see it merged

@rokhmanov
Copy link

Please!

@jrandall
Copy link

@jon-mago would you be able to review and merge this, or would you be interested in considering new or additional maintainers for this repo (and possibly aws.signature as well)? If so, I might be interested. Though perhaps @condour or @laurenkahn can comment if they may be interested as well, since they have been the ones actively developing and reviewing the code in this PR.

@laurenkahn
Copy link

I hope we can get this merged as well! Sadly I don't have permissions to merge. I tried to reach out to Jonathan Stott (the current maintainer of the package) via email in March 2024 and the email bounced. I'm not sure how else to get in touch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants