Skip to content

Commit

Permalink
When sourcing credentials from an external process, ignore stderr (#250)
Browse files Browse the repository at this point in the history 
**Issue:**
If the external process logged to `stderr` during a normal successful run, the credentials would fail to parse.

This was happening because the credentials-provider would always combine `stderr` and `stdout` by appending `2>&1` to the external command. Then JSON parsing would fail, due to random lines of logging on `stderr` mixing with valid JSON on `stdout`.

**Description of changes:**
Instead of redirecting `stderr` to `stdout`, redirect it to `/dev/null`.

It would be better to capture `stderr` separately, and display it if the external process fails. But `aws_process_run()` doesn't currently capture `stderr`, and would require a rework to do so.
  • Loading branch information
@graebm
graebm authored Sep 6, 2024
1 parent 3281f86 commit e930f1a
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 48 deletions.
19 changes: 17 additions & 2 deletions source/credentials_provider_process.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,22 @@ static void s_check_or_get_with_profile_config(
}
}

static struct aws_byte_cursor s_stderr_redirect_to_stdout = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL(" 2>&1");
/* Redirect stderr to /dev/null
* As of Sep 2024, aws_process_run() can only capture stdout, and the
* process's stderr goes into the stderr of the application that launched it.
* Some credentials-processes log to stderr during normal operation.
* To prevent this from polluting the application's stderr,
* we redirect the credential-process's stderr into oblivion.
*
* It would be better to fix aws_process_run() so it captures stderr as well,
* and logging it if the process fails. This is recommended by the SEP:
* > SDKs SHOULD make this error message accessible to the customer. */
#ifdef _WIN32
static struct aws_byte_cursor s_stderr_redirect_to_devnull = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL(" 2> nul");
#else
static struct aws_byte_cursor s_stderr_redirect_to_devnull = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL(" 2> /dev/null");
#endif

static struct aws_string *s_get_command(
struct aws_allocator *allocator,
const struct aws_credentials_provider_process_options *options) {
Expand Down Expand Up @@ -190,7 +205,7 @@ static struct aws_string *s_get_command(
goto on_finish;
}

if (aws_byte_buf_append_dynamic(&command_buf, &s_stderr_redirect_to_stdout)) {
if (aws_byte_buf_append_dynamic(&command_buf, &s_stderr_redirect_to_devnull)) {
goto on_finish;
}

Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ add_test_case(credentials_provider_process_new_failed)
add_test_case(credentials_provider_process_bad_command)
add_test_case(credentials_provider_process_incorrect_command_output)
add_test_case(credentials_provider_process_basic_success)
add_test_case(credentials_provider_process_success_ignores_stderr)
add_test_case(credentials_provider_process_basic_success_from_profile_provider)
add_test_case(credentials_provider_process_basic_success_cached)

Expand Down
93 changes: 47 additions & 46 deletions tests/credentials_provider_process_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,28 @@ AWS_STATIC_STRING_FROM_LITERAL(
"\"Expiration\":\"2020-02-25T06:03:31Z\"}'");
#endif

#ifdef _WIN32
AWS_STATIC_STRING_FROM_LITERAL(
s_test_command_with_logging_on_stderr,
"("
"echo Logging on stderr >&2"
" && "
"echo {\"Version\": 1, \"AccessKeyId\": \"AccessKey123\", "
"\"SecretAccessKey\": \"SecretAccessKey321\", \"SessionToken\":\"TokenSuccess\", "
"\"Expiration\":\"2020-02-25T06:03:31Z\"}"
")");
#else
AWS_STATIC_STRING_FROM_LITERAL(
s_test_command_with_logging_on_stderr,
"("
"echo 'Logging on stderr' >&2"
" && "
"echo '{\"Version\": 1, \"AccessKeyId\": \"AccessKey123\", "
"\"SecretAccessKey\": \"SecretAccessKey321\", \"SessionToken\":\"TokenSuccess\", "
"\"Expiration\":\"2020-02-25T06:03:31Z\"}'"
")");
#endif

AWS_STATIC_STRING_FROM_LITERAL(s_bad_test_command, "/i/dont/know/what/is/this/command");
AWS_STATIC_STRING_FROM_LITERAL(s_bad_command_output, "echo \"Hello, World!\"");

Expand Down Expand Up @@ -262,15 +284,14 @@ static int s_credentials_provider_process_new_failed(struct aws_allocator *alloc
}
AWS_TEST_CASE(credentials_provider_process_new_failed, s_credentials_provider_process_new_failed);

static int s_credentials_provider_process_bad_command(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
static int s_test_command_expect_failure(struct aws_allocator *allocator, const struct aws_string *command) {

s_aws_process_tester_init(allocator);

struct aws_byte_buf content_buf;
struct aws_byte_buf existing_content = aws_byte_buf_from_c_str(aws_string_c_str(s_process_config_file_contents));
aws_byte_buf_init_copy(&content_buf, allocator, &existing_content);
struct aws_byte_cursor cursor = aws_byte_cursor_from_string(s_bad_test_command);
struct aws_byte_cursor cursor = aws_byte_cursor_from_string(command);
ASSERT_TRUE(aws_byte_buf_append_dynamic(&content_buf, &cursor) == AWS_OP_SUCCESS);
cursor = aws_byte_cursor_from_c_str("\n");
ASSERT_TRUE(aws_byte_buf_append_dynamic(&content_buf, &cursor) == AWS_OP_SUCCESS);
Expand Down Expand Up @@ -304,49 +325,16 @@ static int s_credentials_provider_process_bad_command(struct aws_allocator *allo
s_aws_process_tester_cleanup();
return 0;
}

static int s_credentials_provider_process_bad_command(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
return s_test_command_expect_failure(allocator, s_bad_test_command);
}
AWS_TEST_CASE(credentials_provider_process_bad_command, s_credentials_provider_process_bad_command);

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

s_aws_process_tester_init(allocator);

struct aws_byte_buf content_buf;
struct aws_byte_buf existing_content = aws_byte_buf_from_c_str(aws_string_c_str(s_process_config_file_contents));
aws_byte_buf_init_copy(&content_buf, allocator, &existing_content);
struct aws_byte_cursor cursor = aws_byte_cursor_from_string(s_bad_command_output);
ASSERT_TRUE(aws_byte_buf_append_dynamic(&content_buf, &cursor) == AWS_OP_SUCCESS);
cursor = aws_byte_cursor_from_c_str("\n");
ASSERT_TRUE(aws_byte_buf_append_dynamic(&content_buf, &cursor) == AWS_OP_SUCCESS);

struct aws_string *config_file_contents = aws_string_new_from_array(allocator, content_buf.buffer, content_buf.len);
ASSERT_TRUE(config_file_contents != NULL);
aws_byte_buf_clean_up(&content_buf);

s_aws_process_test_init_config_profile(allocator, config_file_contents);
aws_string_destroy(config_file_contents);

struct aws_credentials_provider_process_options options = {
.shutdown_options =
{
.shutdown_callback = s_on_shutdown_complete,
.shutdown_user_data = NULL,
},
.profile_to_use = aws_byte_cursor_from_string(s_credentials_process_profile),
};
struct aws_credentials_provider *provider = aws_credentials_provider_new_process(allocator, &options);
ASSERT_NOT_NULL(provider);
aws_credentials_provider_get_credentials(provider, s_get_credentials_callback, NULL);

s_aws_wait_for_credentials_result();

ASSERT_TRUE(s_tester.has_received_credentials_callback == true);
ASSERT_TRUE(s_tester.credentials == NULL);

aws_credentials_provider_release(provider);
s_aws_wait_for_provider_shutdown_callback();
s_aws_process_tester_cleanup();
return 0;
return s_test_command_expect_failure(allocator, s_bad_command_output);
}
AWS_TEST_CASE(
credentials_provider_process_incorrect_command_output,
Expand All @@ -367,15 +355,13 @@ static int s_verify_credentials(struct aws_credentials *credentials) {
return AWS_OP_SUCCESS;
}

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

static int s_test_command_expect_success(struct aws_allocator *allocator, const struct aws_string *command) {
s_aws_process_tester_init(allocator);

struct aws_byte_buf content_buf;
struct aws_byte_buf existing_content = aws_byte_buf_from_c_str(aws_string_c_str(s_process_config_file_contents));
aws_byte_buf_init_copy(&content_buf, allocator, &existing_content);
struct aws_byte_cursor cursor = aws_byte_cursor_from_string(s_test_command);
struct aws_byte_cursor cursor = aws_byte_cursor_from_string(command);
ASSERT_TRUE(aws_byte_buf_append_dynamic(&content_buf, &cursor) == AWS_OP_SUCCESS);
cursor = aws_byte_cursor_from_c_str("\n");
ASSERT_TRUE(aws_byte_buf_append_dynamic(&content_buf, &cursor) == AWS_OP_SUCCESS);
Expand Down Expand Up @@ -409,8 +395,23 @@ static int s_credentials_provider_process_basic_success(struct aws_allocator *al
s_aws_process_tester_cleanup();
return 0;
}

static int s_credentials_provider_process_basic_success(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
return s_test_command_expect_success(allocator, s_test_command);
}
AWS_TEST_CASE(credentials_provider_process_basic_success, s_credentials_provider_process_basic_success);

/* Test that stderr is ignored, if the process otherwise succeeds with exit code 0 and valid JSON to stdout.
* Once upon a time stderr and stdout were merged, and mundane logging to stderr would break things. */
static int s_credentials_provider_process_success_ignores_stderr(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
return s_test_command_expect_success(allocator, s_test_command_with_logging_on_stderr);
}
AWS_TEST_CASE(
credentials_provider_process_success_ignores_stderr,
s_credentials_provider_process_success_ignores_stderr);

static int s_credentials_provider_process_basic_success_from_profile_provider(
struct aws_allocator *allocator,
void *ctx) {
Expand Down

0 comments on commit e930f1a

Please sign in to comment.