aws: Implementation of IAM Roles anywhere support in aws request signing - part 2 by nbaws · Pull Request #39025 · envoyproxy/envoy

nbaws · 2025-04-05T22:54:59Z

Commit Message: aws: Implementation of IAM Roles anywhere support in aws request signing - part 2
Additional Description:
Part 2 of 3 patches to implement IAM Roles anywhere capability. Adds IAM Roles Anywhere credential provider, x509 credential provider and the IAM Roles Anywhere Signer.

Design brief: #37440
Previous PR: #37193
Part 1 of this patch: #38786

(https://docs.aws.amazon.com/rolesanywhere/latest/userguide/introduction.html)

Risk Level: Negligible - code currently unlinked
Testing: Unit
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws · 2025-05-13T11:48:48Z

@adisuissa have addressed all feedback previously provided.

nbaws · 2025-05-13T12:24:57Z

/retest

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws · 2025-05-14T12:28:45Z

/retest

adisuissa · 2025-05-14T14:20:22Z

test/extensions/common/aws/credential_providers/iam_roles_anywhere_credentials_provider_test.cc

+namespace Aws {
+
+// Example certificates generated for test cases
+std::string server_root_cert_rsa_pem = R"EOF(


I'm not an expert, but just want to make sure these certificates won't expire in the future, and some tests will fail.
If that's possible, I suggest decoupling the certificates from the test (please see other places where certificates testing is being done).

Please also consider future maintenance of this - say one wants to change the certificate, how can that be done in a safe way so it doesn't break the current tests?

These tests are just validating signature generation. Whilst the certificates have to be valid enough to pass through our certificate loading algorithms, the expiration of the certificate is only taken into account during the credential refresh process and not during signature generation.
We should not need to rotate these certificates at all, as the only output that we care about are the raw signature bytes.

Certificate expiration is validated server side, however we don't currently validate that in this code apart from using it to determine when to refresh the certificates. I will add some extra code to reject already expired certificates on envoy side - thank you for pointing this out.

Regarding the certificates expiring in the future, because the tests are dependent on the system time to generate a repeatable signature, we hardwire the systemtime in each test case. The certificates will not expire in our test scenarios, because the system time is always set to a fixed (earlier) value.

test/extensions/common/aws/credential_providers/iam_roles_anywhere_credentials_provider_test.cc

adisuissa · 2025-05-14T14:26:05Z

test/extensions/common/aws/credential_providers/iam_roles_anywhere_credentials_provider_test.cc

+                         << TestUtility::addLeftAndRightPadding("") // line full of padding
+                         << "\n";
+      }
+    }


Just to validate: no need to compare trailers in this case, correct?

correct - this is just checking headers and body for the correct iam roles anywhere payload

adisuissa · 2025-05-14T14:39:50Z

test/extensions/common/aws/credential_providers/iam_roles_anywhere_credentials_provider_test.cc

+  Http::RequestMessageImpl message(std::move(headers));
+  message.headers().setCopy(
+      Http::LowerCaseString("authorization"),
+      "AWS4-X509-RSA-SHA256 "


IMHO this somewhat fragile, and it may cause an issue if one wants to change the certificates in the future.
I suggest at least an explanation of how these were created so one can change it in the future.

This is actually by design - it's validating that given a known input that our output signatures are accurate.
I generated the test signatures with a tool based on https://github.com/awslabs/iam-roles-anywhere-session

I have included this tool as test/extensions/common/aws/credential_providers/iam_roles_anywhere_test_generator.py now and added a comment to refer to using this tool.

Can you add the comments at the beginning of this file?
IMHO there should be a way to map which of the things generated by the python file need to be copied to each of these fields.
If this was a core Envoy code I would probably suggest to make this more maintainable-friendly (just for the sake of the future person that will need to adjust these tests), but as this is an AWS only extension, I'll let you decide how to maintain that.

...e/extensions/common/aws/credential_providers/iam_roles_anywhere_x509_credentials_provider.cc

adisuissa · 2025-05-14T15:10:08Z

source/common/tls/utility.cc

-  }();
-  return *e;
+const ASN1_TIME& Utility::epochASN1Time() {
+  static thread_local CSmartPtr<ASN1_TIME, ASN1_TIME_free> epoch([]() -> ASN1_TIME* {


As far as I understand it, originally this was a static object that was initialized once and used by all threads until the entire process goes out of context. The idea behind this internal function is probably to follow the CONSTRUCT_ON_FIRST_USE pattern.
I guess it can "leak" only if the program (and the main thread) exists, but the question is whether duplicating it to all worker threads makes sense or not. If there's a case for per-thread duplication, I think we may need to address it in many places in Envoy (or maybe introduce CONSTRUCT_THREAD_LOCAL_ON_FIRST_USE. cc @agrawroh to see if this should be followed up.

Taking a step back, I don't think this function should be exposed as part of the API. I've added a comment in the place of usage, describing an alternative.

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws · 2025-05-14T23:10:12Z

@adisuissa please note I changed iam_roles_anywhere_provider to iam_roles_anywhere_credential_provider in the api definition which has invalidated your previous review. No other changes were made.

...e/extensions/common/aws/credential_providers/iam_roles_anywhere_x509_credentials_provider.cc

api/envoy/extensions/common/aws/v3/credential_provider.proto

adisuissa · 2025-05-15T13:04:45Z

test/extensions/common/aws/credential_providers/iam_roles_anywhere_credentials_provider_test.cc

+  Http::RequestMessageImpl message(std::move(headers));
+  message.headers().setCopy(
+      Http::LowerCaseString("authorization"),
+      "AWS4-X509-RSA-SHA256 "


Can you add the comments at the beginning of this file?
IMHO there should be a way to map which of the things generated by the python file need to be copied to each of these fields.
If this was a core Envoy code I would probably suggest to make this more maintainable-friendly (just for the sake of the future person that will need to adjust these tests), but as this is an AWS only extension, I'll let you decide how to maintain that.

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws · 2025-05-15T23:52:19Z

test/extensions/common/aws/credential_providers/iam_roles_anywhere_test_generator.py

+boto3.set_stream_logger('', logging.DEBUG)
+
+if sys.argv[1] == "1":
+    # used for StandardRSASigning test


Comments added here to indicate which test case the generator is for

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

adisuissa

LGTM thanks!
/lgtm api

adisuissa · 2025-05-16T17:33:57Z

extension-only code changes by code-owner, merging.

…ing - part 2 (envoyproxy#39025) Signed-off-by: Nigel Brittain <nbaws@amazon.com> Signed-off-by: Peter Leng <yleng@pinterest.com>

nbaws · 2025-05-16T21:48:24Z

@niax @agrawroh @adisuissa thank you for all the time and attention to this PR, it is greatly appreciated!

…ing - part 2 (envoyproxy#39025) Signed-off-by: Nigel Brittain <nbaws@amazon.com> Signed-off-by: Ting Pan <panting@google.com>

…ing - part 3 (#39521) Commit Message: aws: Implementation of IAM Roles anywhere support in aws request signing - part 3 Additional Description: Part 3 of 3 patches to implement IAM Roles anywhere capability. Integrates the previously added components with the credential chain and unhides the IAM Roles Anywhere credential provider definition in the API. Also reverts the coverage limit created in #39138 back to default coverage requirements. Design brief: #37440 Part 2 of this patch: #39025 (https://docs.aws.amazon.com/rolesanywhere/latest/userguide/introduction.html) Risk Level: Low - extension only changes Testing: Unit Docs Changes: N/A Release Notes: N/A Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Nigel Brittain <nbaws@amazon.com>

…ing - part 3 (#39521) Commit Message: aws: Implementation of IAM Roles anywhere support in aws request signing - part 3 Additional Description: Part 3 of 3 patches to implement IAM Roles anywhere capability. Integrates the previously added components with the credential chain and unhides the IAM Roles Anywhere credential provider definition in the API. Also reverts the coverage limit created in envoyproxy/envoy#39138 back to default coverage requirements. Design brief: envoyproxy/envoy#37440 Part 2 of this patch: envoyproxy/envoy#39025 (https://docs.aws.amazon.com/rolesanywhere/latest/userguide/introduction.html) Risk Level: Low - extension only changes Testing: Unit Docs Changes: N/A Release Notes: N/A Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Nigel Brittain <nbaws@amazon.com> Mirrored from https://github.com/envoyproxy/envoy @ 1ed3da6fe3c72d15008d8abb110573db3b3e5091

nbaws added 30 commits March 1, 2025 00:49

more refactoring

94bf911

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

add dwyu

8aa90cf

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

refactor

f039dcc

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

finished refactor

9397db0

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

remove commented

896f17b

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update includes

0c8fc0e

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update test cases

ec51df0

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update test cases

3a2c047

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

dwyu commit

56eceec

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update credential_provider_chains

0f23f99

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update deps

ba7a1e9

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update tests

d18330a

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

remove extraneous comments

6c82f54

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

fix logging

1f28f19

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

more log statements

98a6a6d

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

coverage

c88732c

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

remove working file

ced9f29

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update tests

0772ec0

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

test leak

decf4a1

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

test case coverage

f0cabb8

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

remove untestable code path

1a44c4d

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

fix test cases

307ffc5

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

format

48e2ca1

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

format

e9e5645

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

remove comments

bee28f3

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

dedupe friend class

4e2ac1e

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

revert key derivation change

19a9031

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

stub key derivation

794478f

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

stub tests

2568d6c

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

fix stubbed test

b537f5d

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws added 4 commits May 13, 2025 10:11

update for optref

af18d47

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

revert build

c995896

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update build

df531e0

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

format

22c6038

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

api typo

063a02d

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

repokitteh-read-only bot added the api label May 14, 2025

adisuissa reviewed May 14, 2025

View reviewed changes

update feedback

54a8c94

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws commented May 14, 2025

View reviewed changes

...e/extensions/common/aws/credential_providers/iam_roles_anywhere_x509_credentials_provider.cc Show resolved Hide resolved

adisuissa reviewed May 15, 2025

View reviewed changes

nbaws added 2 commits May 15, 2025 23:26

address feedback

4877589

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

format

030ef74

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

nbaws commented May 15, 2025

View reviewed changes

nbaws added 3 commits May 16, 2025 00:50

doc format

eb0828b

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

update commit

318f468

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

add hide

bd94eed

Signed-off-by: Nigel Brittain <nbaws@amazon.com>

adisuissa approved these changes May 16, 2025

View reviewed changes

repokitteh-read-only bot removed the api label May 16, 2025

adisuissa merged commit 77a9069 into envoyproxy:main May 16, 2025
25 checks passed

nbaws mentioned this pull request May 17, 2025

aws: Implementation of IAM Roles anywhere support in aws request signing - part 3 #39521

Merged

Conversation

nbaws commented Apr 5, 2025

Uh oh!

nbaws commented May 13, 2025

Uh oh!

nbaws commented May 13, 2025

Uh oh!

nbaws commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbaws commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa commented May 16, 2025

Uh oh!

Uh oh!

nbaws commented May 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants