Support IMDSv2 #552

jornfranke · 2022-11-06T16:51:39Z

Support IDMSv2.

Tested with Sagemaker Notebooks with IMDSv2 enforced.

Configuration:
Optionally, set the environment variable "PAWS_EC2_IMDSV2_TOKEN_TIMEOUT_SECONDS" to the number of seconds after which the token should expire. The default (30 seconds) should be sufficient for nearly all use cases no configuration we use the same method as in boto3

Solves: #441

DyfanJones · 2022-11-07T13:25:18Z

Hi @jornfranke thanks for the PR, can you create a unit test for this.

DyfanJones · 2022-11-07T13:25:51Z

paws.common/R/config.R

  metadata_url <- file.path(
    "http://169.254.169.254/latest/meta-data",
    query_path
  )
+  if (!(token=="")) {


Shouldn't we use token != "" ?

will adapt it

DyfanJones · 2022-11-07T13:29:26Z

paws.common/R/config.R

+    "http://169.254.169.254/latest/api/token"
+  )
+ metadata_token_request <-
+    new_http_request("PUT", metadata_token_url, timeout = 1, header=c("X-aws-ec2-metadata-token-ttl-seconds"= tokentimeout))


Doesn't this exceed 80 characters? We haven't set up an lint just yet, however if you could update this to multiple lines that would be perfect :D something like

metadata_token_request <- new_http_request( "PUT", metadata_token_url, timeout = 1, header=c("X-aws-ec2-metadata-token-ttl-seconds"= tokentimeout) )

DyfanJones · 2022-11-07T13:30:57Z

paws.common/R/config.R

+   metadata_token_url <-  file.path(
+    "http://169.254.169.254/latest/api/token"
+  )


small thing, do we need file.path for just assigning a character :)

This was taken over from the previous function - I just kept it the same to avoid inconsistencies as much as possible.

DyfanJones · 2022-11-07T13:32:52Z

paws.common/R/config.R

+      NULL
+    }
+  )
+  if (!((is.null(metadata_token_response) && metadata_token_response$status_code != 200))) {


Another small thing, isn't there 1 too many brackets? wouldn't something like this do?

if (!(is.null(metadata_token_response) && metadata_token_response$status_code != 200))

yes you are right, it is a mistake

DyfanJones · 2022-11-07T13:35:11Z

paws.common/R/net.R

@@ -55,7 +55,7 @@ HttpResponse <- struct(
 # @param close Whether to immediately close the connection, or else reuse connections.
 # @param timeout How long to wait for an initial response.
 # @param dest Control where the response body is written
-new_http_request <- function(method, url, body = NULL, close = FALSE, timeout = NULL, dest = NULL) {
+new_http_request <- function(method, url, body = NULL, close = FALSE, timeout = NULL, dest = NULL, header = list()) {


I believe we are missing the corresponding documentation for header

@param header blah blah

Do you mind updating the documentation for this? I believe you will need to run roxygen2 to create the doc for it.

yes of course - this one slipped through

DyfanJones · 2022-11-07T13:37:40Z

@davidkretch when you have time do you have anything extra to add to this PR?

jornfranke · 2022-11-07T14:07:45Z

thanks a lot for the detailed and fast review. All valid, will adapt it as soon as possible.
About the unit test: It would be merely an integration test (in the end just connecting to IMDSv2). I have to check if there is a previous test for the IMDSv1 and then copy this to create one for IMDSv2.

jornfranke · 2022-11-07T14:13:43Z

Hi @jornfranke thanks for the PR, can you create a unit test for this.

It makes sense, but we do merely integration with the new IMDSv2 API. So we would need to create a mock IMDSv2 service and then write the test cases. I have not found one test for the previous IMDSv1 call (https://github.com/paws-r/paws/blob/main/paws.common/tests/testthat/test_config.R). Can you point me to one?

Otherwise I propose to create a new issue for the integration test of IMDS (for both versions) as this probably is a bigger effort than providing the feature. Nevertheless, maybe you have already integration tests for other HTTP services used in paws?

For now I did only test manually the integration using AWS Sagemaker Notebooks with IMDSv1 enforced. The library without the change does not work there. Only after adding IMDSv2 supprot it works.

Let me know.

DyfanJones · 2022-11-07T14:23:14Z

Hi @jornfranke thanks for the PR, can you create a unit test for this.

It makes sense, but we do merely integration with the new IMDSv2 API. So we would need to create a mock IMDSv2 service and then write the test cases. I have not found one test for the previous IMDSv1 call (https://github.com/paws-r/paws/blob/main/paws.common/tests/testthat/test_config.R). Can you point me to one?

Otherwise I propose to create a new issue for the integration test of IMDS (for both versions) as this probably is a bigger effort than providing the feature. Nevertheless, maybe you have already integration tests for other HTTP services used in paws?

For now I did only test manually the integration using AWS Sagemaker Notebooks with IMDSv1 enforced. The library without the change does not work there. Only after adding IMDSv2 supprot it works.

Let me know.

Have you considered a mock test? so we can stud the api call? I am happy to look into it if you haven't done mock testing before :)

jornfranke · 2022-11-07T15:12:34Z

no problem to create a mocktest. Do you have a preferred framework or so? Then I can use the preferred framework.

I did not want create too many changes in paws as I am new to the paws code.

DyfanJones · 2022-11-07T15:25:34Z

We haven't chosen a mock testing framework. I am accustomed with such frameworks as mockery and mockthat. I reckon one of these should be fine :) Which ever one of these is the easiest to stub the api call should do :D We can always revisit mocking frameworks in the future :D

jornfranke · 2022-11-07T15:47:28Z

Ok, I will try with mockery (for the reason that it seems to be more frequently maintained and have more contributors). I have to think about where I put the mock (probably I will mock the function new_http_request) and integrate mockery in paws. It will take a bit of time, but should not be too much.

DyfanJones · 2022-11-07T16:12:47Z

no worries :) if you need any help please feel free to reach out :)

DyfanJones · 2022-11-07T19:29:56Z

@jornfranke the first thing i should of asked is. Is this done in the other aws sdk? :P I went straight into dev mode without asking (my bad hahaha).

jornfranke · 2022-11-07T19:57:59Z

@jornfranke the first thing i should of asked is. Is this done in the other aws sdk? :P I went straight into dev mode without asking (my bad hahaha).

not sure what you mean by "is this done in the other aws sdk"?

jornfranke · 2022-11-07T19:59:01Z

I incorporated your comments. I also added two tests: one for the original functionality (that I did not implement) for IMDSv1 and one for the additional functionality (IMDSv2) that I implemented. Please let me know your feedback
edit: tests implemented using mockery mocking the "issue" function of net.R

jornfranke · 2022-11-07T20:10:34Z

Btw.: Here you find how they do it in the official AWS Python SDK: https://github.com/boto/botocore/blob/master/botocore/utils.py#L372

I think it is roughly comparable how it is done for what I have implemented

DyfanJones · 2022-11-07T19:22:23Z

paws.common/R/config.R

@@ -182,14 +182,41 @@ get_instance_metadata <- function(query_path = "") {
  if (trimws(tolower(get_env("AWS_EC2_METADATA_DISABLED"))) %in% c("true", "1")) {
    return(NULL)
  }
+  # Get token timeout for IMDSv2 tokens
+  tokentimeout=trimws(tolower(get_env("PAWS_EC2_IMDSV2_TOKEN_TIMEOUT_SECONDS")))


can we change the assigning from = to <- :)

I understand this gives us extra flexibility to the user however it seems botocore is using a private attribute: https://github.com/boto/botocore/blob/master/botocore/utils.py#L376. Should we do the same? :)

yes we can do the same, maybe we should not be more clever than AWS :) I will change it

DyfanJones · 2022-11-08T10:52:15Z

paws.common/R/config.R

+  # Get token timeout for IMDSv2 tokens
+  tokentimeout=trimws(tolower(get_env("PAWS_EC2_IMDSV2_TOKEN_TIMEOUT_SECONDS")))
+  if (is.na(as.numeric(tokentimeout))) {
+        tokentimeout="30"


Shouldn't we try and mimic what is being done in botocore? and use '21600' as our token timeout? https://github.com/boto/botocore/blob/master/botocore/utils.py#L376

yes same as above. I will change it

DyfanJones · 2022-11-08T10:55:19Z

paws.common/R/config.R

-
+  # Get token timeout for IMDSv2 tokens
+  tokentimeout=trimws(tolower(get_env("PAWS_EC2_IMDSV2_TOKEN_TIMEOUT_SECONDS")))
+  if (is.na(as.numeric(tokentimeout))) {


NIT: this can be simplified to !nzchar(tokentimeout)

will be anyway removed with the change above

jornfranke · 2022-11-08T18:36:11Z

thanks for the review so far. I have included the latest proposals

codecov · 2022-11-09T11:48:30Z

Codecov Report

Base: 81.82% // Head: 82.00% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (62fe2c5) compared to base (258daa6).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
+ Coverage   81.82%   82.00%   +0.18%     
==========================================
  Files         122      122              
  Lines        6983     7087     +104     
==========================================
+ Hits         5714     5812      +98     
- Misses       1269     1275       +6

Impacted Files	Coverage Δ
paws/make.paws/R/package.R	`86.11% <0.00%> (-13.89%)`	⬇️
Users/runner/work/paws/paws/make.paws/R/package.R	`86.11% <0.00%> (-13.89%)`	⬇️
paws/make.paws/R/cran_category.R	`80.55% <0.00%> (-7.33%)`	⬇️
.../runner/work/paws/paws/make.paws/R/cran_category.R	`80.55% <0.00%> (-7.33%)`	⬇️
paws/make.paws/R/make_sdk.R	`93.05% <0.00%> (-4.99%)`	⬇️
Users/runner/work/paws/paws/make.paws/R/make_sdk.R	`93.05% <0.00%> (-4.99%)`	⬇️
paws/make.paws/R/cran_collection.R	`93.47% <0.00%> (-3.37%)`	⬇️
...unner/work/paws/paws/make.paws/R/cran_collection.R	`93.47% <0.00%> (-3.37%)`	⬇️
paws/make.paws/R/cache.R	`97.82% <0.00%> (-0.05%)`	⬇️
Users/runner/work/paws/paws/make.paws/R/cache.R	`97.82% <0.00%> (-0.05%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

paws.common/R/config.R

davidkretch · 2022-11-12T20:17:58Z

looks good to me, just had one question about forcing use of v2 only.

DyfanJones · 2022-11-16T11:53:36Z

@davidkretch is there anything else required? I think we are in a good place :)

davidkretch · 2022-11-17T01:39:06Z

@DyfanJones looks good to me :-)

jornfranke · 2022-11-17T18:03:17Z

I saw that after the merge one of the pipelines failed (the two others went through):
https://github.com/paws-r/paws/actions/runs/3486467796/jobs/5832980669

It does not look directly related to the added code to me, but let me know if I can support troubleshooting

DyfanJones · 2022-11-17T22:38:51Z

@jornfranke not to worry, retry of the failed job seemed to fix it. Thank you again for pr. Please feel free to pr in the future. We always are grateful for all prs as they really do help in the maintanence and development of paws :)

jornfranke added 2 commits November 6, 2022 16:36

feat: Add support for IMDSv2

0e53620

feat: Add support for IMDSv2

8698ab1

DyfanJones reviewed Nov 7, 2022

View reviewed changes

fix: pull request comments

68224da

feat: add unit test (mock) for IMDSv2 and IMDSv1

4fb56b3

DyfanJones reviewed Nov 8, 2022

View reviewed changes

feat: make IMDSv2 token timeout comparable to boto3

43d490d

davidkretch reviewed Nov 12, 2022

View reviewed changes

paws.common/R/config.R Outdated Show resolved Hide resolved

docs: remove comment on insecure imdsv1, provide general comment

62fe2c5

DyfanJones merged commit c46c6b1 into paws-r:main Nov 17, 2022

DyfanJones mentioned this pull request Jan 9, 2023

s3$list_buckets() hangs #580

Closed

stuart-storypark mentioned this pull request Apr 27, 2023

ECS container fails to retrieve secrets from AWS Secret Manager due to hop limit #610

Closed

Support IMDSv2 #552

Support IMDSv2 #552

Conversation

jornfranke commented Nov 6, 2022 • edited Loading

DyfanJones commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jornfranke Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DyfanJones commented Nov 7, 2022

jornfranke commented Nov 7, 2022

jornfranke commented Nov 7, 2022

DyfanJones commented Nov 7, 2022

jornfranke commented Nov 7, 2022

DyfanJones commented Nov 7, 2022

jornfranke commented Nov 7, 2022 • edited Loading

DyfanJones commented Nov 7, 2022

DyfanJones commented Nov 7, 2022

jornfranke commented Nov 7, 2022

jornfranke commented Nov 7, 2022 • edited Loading

jornfranke commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jornfranke commented Nov 8, 2022

codecov bot commented Nov 9, 2022 • edited Loading

Codecov Report

davidkretch commented Nov 12, 2022

DyfanJones commented Nov 16, 2022

davidkretch commented Nov 17, 2022

jornfranke commented Nov 17, 2022

DyfanJones commented Nov 17, 2022

jornfranke commented Nov 6, 2022 •

edited

Loading

jornfranke Nov 7, 2022 •

edited

Loading

jornfranke commented Nov 7, 2022 •

edited

Loading

jornfranke commented Nov 7, 2022 •

edited

Loading

codecov bot commented Nov 9, 2022 •

edited

Loading