-
Notifications
You must be signed in to change notification settings - Fork 30
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
Enforce ECSCredentialsProvider IP Rules #234
Changes from all commits
db02f8a
bd4562e
8f0d14e
d2cb25c
d2b722e
9bb9a19
1df8151
a3fe185
ea816ae
62b6733
1eaf146
b7edcec
e10cc80
2e4a212
228fb27
89e2d2c
3c5fa1c
ec434c2
f99dce1
c3cae42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
""" | ||
Setup local mock server for tests | ||
""" | ||
|
||
import Builder | ||
|
||
import os | ||
import sys | ||
import subprocess | ||
import atexit | ||
|
||
|
||
class MockServerSetup(Builder.Action): | ||
""" | ||
Set up this machine for running the mock server test | ||
|
||
This action should be run in the 'pre_build_steps' or 'build_steps' stage. | ||
""" | ||
|
||
def run(self, env): | ||
if not env.project.needs_tests(env): | ||
print("Skipping mock server setup because tests disabled for project") | ||
return | ||
|
||
self.env = env | ||
python_path = sys.executable | ||
|
||
# set cmake flag so mock server tests are enabled | ||
env.project.config['cmake_args'].extend( | ||
['-DENABLE_AUTH_MOCK_SERVER_TESTS=ON', '-DASSERT_LOCK_HELD=ON']) | ||
|
||
base_dir = os.path.dirname(os.path.realpath(__file__)) | ||
dir = os.path.join(base_dir, "..", "..", "tests", "mock_server") | ||
process = subprocess.Popen([python_path, "mock_auth_server.py"], cwd=dir) | ||
|
||
@atexit.register | ||
def close_mock_server(): | ||
process.terminate() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,5 +20,9 @@ | |
"test_steps": [ | ||
"auth-ci-prep", | ||
"test" | ||
], | ||
"pre_build_steps": [ | ||
"mock-server-setup" | ||
] | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
#include <aws/common/clock.h> | ||
#include <aws/common/date_time.h> | ||
#include <aws/common/environment.h> | ||
#include <aws/common/host_utils.h> | ||
#include <aws/common/string.h> | ||
#include <aws/http/connection.h> | ||
#include <aws/http/connection_manager.h> | ||
|
@@ -42,6 +43,7 @@ struct aws_credentials_provider_ecs_impl { | |
struct aws_string *path_and_query; | ||
struct aws_string *auth_token_file_path; | ||
struct aws_string *auth_token; | ||
bool is_https; | ||
}; | ||
|
||
/* | ||
|
@@ -430,9 +432,59 @@ static void s_ecs_query_task_role_credentials(struct aws_credentials_provider_ec | |
s_ecs_finalize_get_credentials_query(ecs_user_data); | ||
} | ||
} | ||
/* | ||
* The host must use either HTTPS or the resolved IP address must satisfy one of the following: | ||
* 1. within the loopback CIDR (IPv4 127.0.0.0/8, IPv6 ::1/128) | ||
* 2. corresponds to the ECS container host 169.254.170.2 | ||
* 3. corresponds to the EKS container host IPs (IPv4 169.254.170.23, IPv6 fd00:ec2::23) | ||
*/ | ||
static bool s_is_valid_remote_host_ip( | ||
struct aws_credentials_provider_ecs_user_data *ecs_user_data, | ||
struct aws_http_connection *connection) { | ||
struct aws_credentials_provider_ecs_impl *impl = ecs_user_data->ecs_provider->impl; | ||
|
||
if (impl->is_https) { | ||
return true; | ||
} | ||
|
||
bool result = false; | ||
|
||
const struct aws_byte_cursor address = | ||
aws_byte_cursor_from_c_str(aws_http_connection_get_remote_endpoint(connection)->address); | ||
AWS_LOGF_INFO( | ||
AWS_LS_AUTH_CREDENTIALS_PROVIDER, | ||
"id=%p: the ip address of connected remove endpoint is " PRInSTR "", | ||
(void *)ecs_user_data->ecs_provider, | ||
AWS_BYTE_CURSOR_PRI(address)); | ||
if (aws_host_utils_is_ipv4(address)) { | ||
const struct aws_byte_cursor ipv4_loopback_address_prefix = aws_byte_cursor_from_c_str("127."); | ||
const struct aws_byte_cursor ecs_container_host_address = aws_byte_cursor_from_c_str("169.254.170.2"); | ||
const struct aws_byte_cursor eks_container_host_address = aws_byte_cursor_from_c_str("169.254.170.23"); | ||
|
||
result |= aws_byte_cursor_starts_with(&address, &ipv4_loopback_address_prefix); | ||
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)) { | ||
waahm7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/* Check for both the short form and long form of an IPv6 address to be safe. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
const struct aws_byte_cursor ipv6_loopback_address = aws_byte_cursor_from_c_str("::1"); | ||
const struct aws_byte_cursor ipv6_loopback_address_verbose = aws_byte_cursor_from_c_str("0:0:0:0:0:0:0:1"); | ||
const struct aws_byte_cursor eks_container_host_ipv6_address = aws_byte_cursor_from_c_str("fd00:ec2::23"); | ||
const struct aws_byte_cursor eks_container_host_ipv6_address_verbose = | ||
aws_byte_cursor_from_c_str("fd00:ec2:0:0:0:0:0:23"); | ||
|
||
result |= aws_byte_cursor_eq(&address, &ipv6_loopback_address); | ||
result |= aws_byte_cursor_eq(&address, &ipv6_loopback_address_verbose); | ||
result |= aws_byte_cursor_eq(&address, &eks_container_host_ipv6_address); | ||
result |= aws_byte_cursor_eq(&address, &eks_container_host_ipv6_address_verbose); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
static void s_ecs_on_acquire_connection(struct aws_http_connection *connection, int error_code, void *user_data) { | ||
struct aws_credentials_provider_ecs_user_data *ecs_user_data = user_data; | ||
struct aws_credentials_provider_ecs_impl *impl = ecs_user_data->ecs_provider->impl; | ||
|
||
if (connection == NULL) { | ||
AWS_LOGF_WARN( | ||
|
@@ -446,9 +498,23 @@ static void s_ecs_on_acquire_connection(struct aws_http_connection *connection, | |
s_ecs_finalize_get_credentials_query(ecs_user_data); | ||
return; | ||
} | ||
|
||
ecs_user_data->connection = connection; | ||
|
||
/* 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)) { | ||
Comment on lines
+503
to
+505
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
AWS_LOGF_ERROR( | ||
AWS_LS_AUTH_CREDENTIALS_PROVIDER, | ||
"id=%p: ECS provider failed to establish connection to a valid remote with error %d(%s)", | ||
(void *)ecs_user_data->ecs_provider, | ||
AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST, | ||
aws_error_str(AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST)); | ||
|
||
ecs_user_data->error_code = AWS_AUTH_CREDENTIALS_PROVIDER_ECS_INVALID_HOST; | ||
s_ecs_finalize_get_credentials_query(ecs_user_data); | ||
return; | ||
} | ||
|
||
s_ecs_query_task_role_credentials(ecs_user_data); | ||
} | ||
|
||
|
@@ -522,6 +588,12 @@ struct aws_credentials_provider *aws_credentials_provider_new_ecs( | |
struct aws_allocator *allocator, | ||
const struct aws_credentials_provider_ecs_options *options) { | ||
|
||
if (!options->bootstrap) { | ||
AWS_LOGF_ERROR(AWS_LS_AUTH_CREDENTIALS_PROVIDER, "a bootstrap instance must be provided"); | ||
aws_raise_error(AWS_ERROR_INVALID_ARGUMENT); | ||
return NULL; | ||
} | ||
|
||
struct aws_credentials_provider *provider = NULL; | ||
struct aws_credentials_provider_ecs_impl *impl = NULL; | ||
|
||
|
@@ -555,6 +627,7 @@ struct aws_credentials_provider *aws_credentials_provider_new_ecs( | |
aws_error_debug_str(aws_last_error())); | ||
goto on_error; | ||
} | ||
impl->is_https = true; | ||
} | ||
|
||
struct aws_socket_options socket_options; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could add the test, and have it return AWS_OP_SKIP if it fails due to lack of IPv6 support |
||
add_test_case(credentials_provider_ecs_mocked_server_basic_ipv4_success) | ||
add_test_case(credentials_provider_ecs_mocked_server_basic_ipv4_invalid) | ||
endif() | ||
|
||
if(AWS_BUILDING_ON_ECS) | ||
add_test_case(credentials_provider_ecs_real_success) | ||
endif() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?