Enforce ECSCredentialsProvider IP Rules #234

waahm7 · 2024-04-19T18:24:49Z

Description of changes:

The resolved ip address of ECSCredentials provider must follow certain limitations if it is HTTP. This PR enforces those limitations so that we don't make requests on potential malicious HTTP servers.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2024-04-19T18:43:15Z

Codecov Report

Attention: Patch coverage is 60.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 79.43%. Comparing base (277c8fb) to head (f99dce1).

Files	Patch %	Lines
source/credentials_provider_ecs.c	60.00%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #234      +/-   ##
==========================================
- Coverage   79.54%   79.43%   -0.12%     
==========================================
  Files          33       33              
  Lines        5872     5907      +35     
==========================================
+ Hits         4671     4692      +21     
- Misses       1201     1215      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

source/credentials_provider_ecs.c

graebm · 2024-04-26T00:29:19Z

source/credentials_provider_ecs.c

+        result |= aws_byte_cursor_eq(&address, &ecs_container_host_address);
+        result |= aws_byte_cursor_eq(&address, &eks_container_host_address);
+    } else if (aws_host_utils_is_ipv6(address, false)) {
+        /* Check for both the short form and long form of an IPv6 address to be safe. */


TODO: it would be cool if we had a function to convert these IP strings into their actual byte values

since there are even more ways to write these addresses (e.g. "0:0::1"), which are weird, but still legal

We are getting these IP strings from a system call, and we can convert them back to byte values using an OS dependent system call and memcmp them (at least that's what ChatGPT told me). I can add a function to common, but the fact that we are getting these IPs from system calls, we discussed that it should be good enough to just compare both the short form and the long form and ignore the weird ones.

Testing the OS behavior for n different platforms led to the addition of an actual mock server, but unfortunately, GitHub runners don't support IPv6.

graebm · 2024-04-26T14:23:26Z

source/credentials_provider_ecs.c

+    /* validate ip address if the connection manager is not mocked */
+    if (impl->function_table == g_aws_credentials_provider_http_function_table &&
+        !s_is_valid_remote_host_ip(ecs_user_data, connection)) {


don't have the implementation skip functionality when it detects it's in a test

Do we need to add aws_http_connection_get_remote_endpoint() to the function_table?

graebm · 2024-04-26T14:25:50Z

tests/CMakeLists.txt

@@ -57,6 +57,12 @@ add_test_case(credentials_provider_ecs_no_auth_token_success)
 add_test_case(credentials_provider_ecs_success_multi_part_doc)
 add_test_case(credentials_provider_ecs_real_new_destroy)

+if(ENABLE_AUTH_MOCK_SERVER_TESTS)
+    # TODO: Add IPv6 tests when the Github runner support it. See: https://github.com/actions/runner-images/issues/668


You could add the test, and have it return AWS_OP_SKIP if it fails due to lack of IPv6 support

graebm · 2024-04-26T14:32:31Z

tests/credentials_provider_ecs_tests.c

+    aws_credentials_provider_get_credentials(provider, s_get_credentials_callback, NULL);
+    s_aws_wait_for_credentials_result();
+    ASSERT_NULL(s_tester.credentials);
+    ASSERT_TRUE(s_tester.error_code == AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST);


trivial:

Suggested change

ASSERT_TRUE(s_tester.error_code == AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST);

ASSERT_INT_EQUALS(AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST, s_tester.error_code);

it gives a nice printf about what we got, vs what was expected

graebm · 2024-04-26T14:33:21Z

tests/credentials_provider_ecs_tests.c

+    aws_credentials_provider_get_credentials(provider, s_get_credentials_callback, NULL);
+    s_aws_wait_for_credentials_result();
+    ASSERT_NOT_NULL(s_tester.credentials);
+    ASSERT_TRUE(s_tester.error_code == AWS_OP_SUCCESS);


graebm · 2024-04-26T15:05:06Z

tests/credentials_provider_ecs_tests.c

@@ -536,6 +557,87 @@ static int s_credentials_provider_ecs_basic_success(struct aws_allocator *alloca

 AWS_TEST_CASE(credentials_provider_ecs_basic_success, s_credentials_provider_ecs_basic_success);

+static int s_credentials_provider_ecs_mocked_server_basic_ipv4_invalid(struct aws_allocator *allocator, void *ctx) {


There's already a system for mocking HTTP calls in auth tests. Is there a really good reason for adding another?

graebm · 2024-04-26T15:13:17Z

source/credentials_provider_ecs.c

+    bool result = false;
+
+    const struct aws_byte_cursor address =
+        aws_byte_cursor_from_c_str(aws_http_connection_get_remote_endpoint(connection)->address);


This technique won't work with many other HTTP clients. It's uncommon that you have a chance to inspect the IP before making the actual request.

just asking the host_resolver directly would be more portable

I get that you're trying to avoid additional calls to the resolver. You could do like the Rust SDK and only validate when full-uri is passed in by the user.

Is the fact that this relies on the HTTP client what's forcing you to add a 2nd way of mocking HTTP calls?

Co-authored-by: Michael Graeb <[email protected]>

waahm7 · 2024-05-13T16:52:16Z

Closing in favor of #238

waahm7 added 4 commits April 19, 2024 11:07

add mocked server

db02f8a

fail fast is false

bd4562e

Fix fail-fast

8f0d14e

If not mocked

d2cb25c

waahm7 added 15 commits April 19, 2024 11:57

Github runners do not support ipv6 tests, so only check for ipv4

d2b722e

Just use google.com for invalid address

9bb9a19

Better bootstrap null handling

1df8151

rename functions

a3fe185

fix typo

ea816ae

Use different port from s3 mock server

62b6733

fix typo

1eaf146

update server

b7edcec

different mock config name

e10cc80

remove comments

2e4a212

Add comment

228fb27

Add comment again

89e2d2c

Add comments regarding ip address rules

3c5fa1c

Add a todo for ipv6

ec434c2

Log the remote endpoint ip address

f99dce1

waahm7 changed the title ~~WIP | ECSCredentialsProvider IP Rules Enforce~~ ECSCredentialsProvider IP Rules Enforce Apr 22, 2024

waahm7 changed the title ~~ECSCredentialsProvider IP Rules Enforce~~ Enforce ECSCredentialsProvider IP Rules Apr 22, 2024

waahm7 marked this pull request as ready for review April 22, 2024 20:24

graebm reviewed Apr 26, 2024

View reviewed changes

Update source/credentials_provider_ecs.c

c3cae42

Co-authored-by: Michael Graeb <[email protected]>

waahm7 closed this May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce ECSCredentialsProvider IP Rules #234

Enforce ECSCredentialsProvider IP Rules #234

waahm7 commented Apr 19, 2024 •

edited

Loading

codecov-commenter commented Apr 19, 2024 •

edited

Loading

graebm Apr 26, 2024

waahm7 Apr 26, 2024

graebm Apr 26, 2024

graebm Apr 26, 2024

graebm Apr 26, 2024

graebm Apr 26, 2024

graebm Apr 26, 2024

graebm Apr 26, 2024

waahm7 commented May 13, 2024

	ASSERT_TRUE(s_tester.error_code == AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST);
	ASSERT_INT_EQUALS(AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST, s_tester.error_code);

		@@ -536,6 +557,87 @@ static int s_credentials_provider_ecs_basic_success(struct aws_allocator *alloca

		AWS_TEST_CASE(credentials_provider_ecs_basic_success, s_credentials_provider_ecs_basic_success);

		static int s_credentials_provider_ecs_mocked_server_basic_ipv4_invalid(struct aws_allocator allocator, void ctx) {

Enforce ECSCredentialsProvider IP Rules #234

Enforce ECSCredentialsProvider IP Rules #234

Conversation

waahm7 commented Apr 19, 2024 • edited Loading

codecov-commenter commented Apr 19, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

waahm7 commented May 13, 2024

waahm7 commented Apr 19, 2024 •

edited

Loading

codecov-commenter commented Apr 19, 2024 •

edited

Loading