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

Add an Option to Disable v2 to v1 Fallback #218

Merged
merged 10 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/aws/auth/aws_imds_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ struct aws_imds_client_options {
*/
enum aws_imds_protocol_version imds_version;

/*
* If true, fallback from v2 to v1 will be disabled for all cases
*/
bool ec2_metadata_v1_disabled;

/*
* Table holding all cross-system functional dependencies for an imds client.
*
Expand Down
5 changes: 5 additions & 0 deletions include/aws/auth/credentials.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ struct aws_credentials_provider_imds_options {
*/
enum aws_imds_protocol_version imds_version;

/*
* If true, fallback from v2 to v1 will be disabled for all cases
*/
bool ec2_metadata_v1_disabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

ec2_metadata_v1_disabled, but you can still set the imds_version to V1 manually.

And, it's already part of IMDS options, I think we can skip the ec2_metadata part

Maybe v1_fallback_disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ec2_metadata_v1_disabled will be used cross SDKs along with the relevant env variable.


/* For mocking the http layer in tests, leave NULL otherwise */
struct aws_auth_http_system_vtable *function_table;
};
Expand Down
12 changes: 10 additions & 2 deletions source/aws_imds_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ struct aws_imds_client {
struct aws_linked_list pending_queries;
struct aws_mutex token_lock;
struct aws_condition_variable token_signal;
bool ec2_metadata_v1_disabled;

struct aws_atomic_var ref_count;
};
Expand Down Expand Up @@ -146,6 +147,7 @@ struct aws_imds_client *aws_imds_client_new(
client->function_table =
options->function_table ? options->function_table : g_aws_credentials_provider_http_function_table;
client->token_required = options->imds_version == IMDS_PROTOCOL_V1 ? false : true;
client->ec2_metadata_v1_disabled = options->ec2_metadata_v1_disabled;
client->shutdown_options = options->shutdown_options;

struct aws_socket_options socket_options;
Expand Down Expand Up @@ -220,6 +222,7 @@ struct imds_user_data {
/* Indicate the request is a fallback from a failure call. */
bool is_fallback_request;
bool is_imds_token_request;
bool ec2_metadata_v1_disabled;
int status_code;
int error_code;

Expand Down Expand Up @@ -281,6 +284,7 @@ static struct imds_user_data *s_user_data_new(
}

wrapped_user_data->imds_token_required = client->token_required;
wrapped_user_data->ec2_metadata_v1_disabled = client->ec2_metadata_v1_disabled;
aws_atomic_store_int(&wrapped_user_data->ref_count, 1);
return wrapped_user_data;

Expand Down Expand Up @@ -509,7 +513,8 @@ static void s_client_on_token_response(struct imds_user_data *user_data) {
* we should fall back to insecure request. Otherwise, we should use
* token in following requests.
*/
waahm7 marked this conversation as resolved.
Show resolved Hide resolved
if (user_data->status_code != AWS_HTTP_STATUS_CODE_200_OK || user_data->current_result.len == 0) {
if (!user_data->ec2_metadata_v1_disabled &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to be more clear about this:

Suggested change
if (!user_data->ec2_metadata_v1_disabled &&
if (user_data->status_code != AWS_HTTP_STATUS_CODE_200_OK || user_data->current_result.len == 0) {
if (user_data->ec2_metadata_v1_disabled) {
//XXX failed to fetch creds and fallback disabled
}
else {
//XXX Fallback to retry
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have refactored the code to the following.

	if(success){
	} else if(fallback disabled){
		// error out
	} else {
		fallback to v1.
	}

(user_data->status_code != AWS_HTTP_STATUS_CODE_200_OK || user_data->current_result.len == 0)) {
AWS_LOGF_DEBUG(
AWS_LS_IMDS_CLIENT,
"(id=%p) IMDS client failed to fetch token for requester %p, fall back to v1 for the same "
Expand All @@ -533,7 +538,10 @@ static void s_client_on_token_response(struct imds_user_data *user_data) {
uint64_t expire_timestamp = aws_add_u64_saturating(
current, aws_timestamp_convert(s_imds_token_ttl_secs, AWS_TIMESTAMP_SECS, AWS_TIMESTAMP_NANOS, NULL));
s_update_token_safely(
user_data->client, cursor.len == 0 ? NULL : &user_data->imds_token, cursor.len != 0, expire_timestamp);
user_data->client,
cursor.len == 0 ? NULL : &user_data->imds_token,
Copy link
Contributor

Choose a reason for hiding this comment

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

we checked that user_data->current_result is not empty and we have a 200, should we just error out if the cursor here is still empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added an assert, as it should not be empty at this point.

cursor.len != 0 || user_data->ec2_metadata_v1_disabled,
expire_timestamp);
}
}

Expand Down
1 change: 1 addition & 0 deletions source/credentials_provider_imds.c
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ struct aws_credentials_provider *aws_credentials_provider_new_imds(
.bootstrap = options->bootstrap,
.function_table = options->function_table,
.imds_version = options->imds_version,
.ec2_metadata_v1_disabled = options->ec2_metadata_v1_disabled,
.shutdown_options =
{
.shutdown_callback = s_on_imds_client_shutdown,
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ add_test_case(imds_client_new_release)
add_test_case(imds_client_connect_failure)
add_test_case(imds_client_token_request_failure)
add_test_case(imds_client_insecure_fallback_request_failure)
add_test_case(imds_client_v1_fallback_disabled_failure)
add_test_case(imds_client_resource_request_failure)
add_test_case(imds_client_resource_request_success)
add_test_case(imds_client_insecure_resource_request_success)
Expand Down
70 changes: 61 additions & 9 deletions tests/aws_imds_client_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,67 @@ static int s_imds_client_insecure_fallback_request_failure(struct aws_allocator

AWS_TEST_CASE(imds_client_insecure_fallback_request_failure, s_imds_client_insecure_fallback_request_failure);

AWS_STATIC_STRING_FROM_LITERAL(
s_good_response,
"{\"AccessKeyId\":\"SuccessfulAccessKey\", \n \"SecretAccessKey\":\"SuccessfulSecret\", \n "
"\"Token\":\"TokenSuccess\", \n \"Expiration\":\"2020-02-25T06:03:31Z\"}");
AWS_STATIC_STRING_FROM_LITERAL(s_access_key, "SuccessfulAccessKey");
AWS_STATIC_STRING_FROM_LITERAL(s_secret_key, "SuccessfulSecret");
AWS_STATIC_STRING_FROM_LITERAL(s_token, "TokenSuccess");
AWS_STATIC_STRING_FROM_LITERAL(s_expiration, "2020-02-25T06:03:31Z");

static int s_imds_client_v1_fallback_disabled_failure(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

s_aws_imds_tester_init(allocator);
/* secure data flow is not supported */
s_tester.response_code[0] = AWS_HTTP_STATUS_CODE_403_FORBIDDEN;
/* v1 would have worked if fallback was not disabled */
struct aws_byte_cursor good_response_cursor = aws_byte_cursor_from_string(s_good_response);
aws_array_list_push_back(&s_tester.response_data_callbacks[1], &good_response_cursor);

struct aws_imds_client_options options = {
.bootstrap = s_tester.bootstrap,
.function_table = &s_mock_function_table,
.ec2_metadata_v1_disabled = true,
.shutdown_options =
{
.shutdown_callback = s_on_shutdown_complete,
.shutdown_user_data = NULL,
},
};

struct aws_imds_client *client = aws_imds_client_new(allocator, &options);

aws_imds_client_get_resource_async(
client, aws_byte_cursor_from_string(s_expected_imds_resource_uri), s_get_resource_callback, NULL);

s_aws_wait_for_resource_result();

ASSERT_TRUE(s_tester.has_received_resource_callback == true);
ASSERT_TRUE(s_tester.resource.len == 0);
/* There was no fallback so we didn't get any resource */
ASSERT_TRUE(s_validate_uri_path_and_resource(1, false /*no resource*/) == 0);
ASSERT_TRUE(s_tester.token_ttl_header_exist[0]);
ASSERT_TRUE(s_tester.token_ttl_header_expected[0]);
ASSERT_FALSE(s_tester.token_header_exist[0]);

ASSERT_UINT_EQUALS(s_tester.error_code, AWS_AUTH_IMDS_CLIENT_SOURCE_FAILURE);

aws_imds_client_release(client);

s_aws_wait_for_imds_client_shutdown_callback();

/* Because we mock the http connection manager, we never get a callback back from it */
aws_mem_release(allocator, client);

ASSERT_SUCCESS(s_aws_imds_tester_cleanup());

return 0;
}

AWS_TEST_CASE(imds_client_v1_fallback_disabled_failure, s_imds_client_v1_fallback_disabled_failure);

static int s_imds_client_resource_request_failure(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

Expand Down Expand Up @@ -655,15 +716,6 @@ static int s_imds_client_resource_request_failure(struct aws_allocator *allocato

AWS_TEST_CASE(imds_client_resource_request_failure, s_imds_client_resource_request_failure);

AWS_STATIC_STRING_FROM_LITERAL(
s_good_response,
"{\"AccessKeyId\":\"SuccessfulAccessKey\", \n \"SecretAccessKey\":\"SuccessfulSecret\", \n "
"\"Token\":\"TokenSuccess\", \n \"Expiration\":\"2020-02-25T06:03:31Z\"}");
AWS_STATIC_STRING_FROM_LITERAL(s_access_key, "SuccessfulAccessKey");
AWS_STATIC_STRING_FROM_LITERAL(s_secret_key, "SuccessfulSecret");
AWS_STATIC_STRING_FROM_LITERAL(s_token, "TokenSuccess");
AWS_STATIC_STRING_FROM_LITERAL(s_expiration, "2020-02-25T06:03:31Z");

static int s_imds_client_resource_request_success(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

Expand Down
Loading