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.

Sign up for GitHub

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 function H5FD__s3comms_load_aws_creds_from_env() #5167

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

Add function H5FD__s3comms_load_aws_creds_from_env() #5167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
87b9237
Add function H5FD__s3comms_load_aws_creds_from_file() to get AWS
lrknox Dec 4, 2024
5cb2c56
Committing clang-format changes
github-actions[bot] Dec 4, 2024
d438442
Typo corrections.
lrknox Dec 4, 2024
b104e7b
Merge branch 'expand_load_aws_credentials' of https://github.com/lrkn…
lrknox Dec 4, 2024
8ebf49d
Committing clang-format changes
github-actions[bot] Dec 4, 2024
d581624
Merge branch 'develop' of https://github.com/HDFGroup/hdf5 into expan…
lrknox Dec 13, 2024
765b3c6
Add working code to use AWS_SESSION_TOKEN/temporary credentials for
lrknox Dec 17, 2024
196ab1d
Merge branch 'expand_load_aws_credentials' of https://github.com/lrkn…
lrknox Dec 17, 2024
5652aa0
Committing clang-format changes
github-actions[bot] Dec 17, 2024
db22586
Merge branch 'develop' of https://github.com/HDFGroup/hdf5 into expan…
lrknox Jan 14, 2025
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
10 changes: 10 additions & 0 deletions src/H5FDros3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,16 @@ H5FD__ros3_cmp(const H5FD_t *_f1, const H5FD_t *_f2)
else if (f2->fa.secret_key[0] != '\0')
HGOTO_DONE(-1);

/* FAPL: SESSION_TOKEN */
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add 'session token' to the comment for this function describing everything used for the comparison

if (f1->fa.session_token[0] != '\0' && f2->fa.session_token[0] != '\0') {
if (strcmp(f1->fa.session_token, f2->fa.session_token))
HGOTO_DONE(-1);
}
else if (f1->fa.session_token[0] != '\0')
HGOTO_DONE(-1);
else if (f2->fa.session_token[0] != '\0')
HGOTO_DONE(-1);

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* H5FD__ros3_cmp() */
Expand Down
1 change: 1 addition & 0 deletions src/H5FDros3.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ typedef struct H5FD_ros3_fapl_t {
char aws_region[H5FD_ROS3_MAX_REGION_LEN + 1];
char secret_id[H5FD_ROS3_MAX_SECRET_ID_LEN + 1];
char secret_key[H5FD_ROS3_MAX_SECRET_KEY_LEN + 1];
char session_token[H5FD_ROS3_MAX_SECRET_TOK_LEN + 1];
} H5FD_ros3_fapl_t;

#ifdef __cplusplus
Expand Down
108 changes: 96 additions & 12 deletions src/H5FDs3comms.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ H5FD_s3comms_s3r_open(const char *url, const char *region, const char *id, const
*************************************/

if ((region != NULL && *region != '\0') || (id != NULL && *id != '\0') || (signing_key != NULL) ||
(token != NULL)) {
(token != NULL && *token != '\0')) {

size_t tmplen;

Expand Down Expand Up @@ -1556,21 +1556,23 @@ H5FD_s3comms_free_purl(parsed_url_t *purl)
*/
static herr_t
H5FD__s3comms_load_aws_creds_from_file(FILE *file, const char *profile_name, char *key_id, char *access_key,
char *aws_region)
char *session_token, char *aws_region)
{
char profile_line[32];
char buffer[128];
char buffer[4096];
const char *setting_names[] = {
"region",
"aws_access_key_id",
"aws_secret_access_key",
"aws_session_token",
};
char *const setting_pointers[] = {
aws_region,
key_id,
access_key,
session_token,
};
unsigned setting_count = 3;
unsigned setting_count = 4;
herr_t ret_value = SUCCEED;
unsigned setting_i = 0;
int found_setting = 0;
Expand All @@ -1585,19 +1587,22 @@ H5FD__s3comms_load_aws_creds_from_file(FILE *file, const char *profile_name, cha

/* Look for start of profile */
do {
memset(buffer, 0, 128);
/* clear buffer */
memset(buffer, 0, 4096);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to replace repeated uses of 4096 with a single buffer_size variable


line_buffer = fgets(line_buffer, 128, file);
if (line_buffer == NULL)
line_buffer = fgets(line_buffer, 4096, file);
if (line_buffer == NULL) /* reached end of file */
goto done;
} while (strncmp(line_buffer, profile_line, strlen(profile_line)));

/* Extract credentials from lines */
do {
memset(buffer, 0, 128);
/* clear buffer and flag */
memset(buffer, 0, 4096);
found_setting = 0;

line_buffer = fgets(line_buffer, 128, file);
/* collect a line from file */
line_buffer = fgets(line_buffer, 4096, file);
if (line_buffer == NULL)
goto done; /* end of file */

Expand Down Expand Up @@ -1648,6 +1653,78 @@ H5FD__s3comms_load_aws_creds_from_file(FILE *file, const char *profile_name, cha
FUNC_LEAVE_NOAPI(ret_value)
} /* end H5FD__s3comms_load_aws_creds_from_file() */

/*-----------------------------------------------------------------------------
*
* Function: H5FD__s3comms_load_aws_creds_from_env()
*
* Purpose:
*
* Get aws credentials from environment variables AWS_ACCESS_KEY_ID,
* AWS_SECRET_ACCESS_KEY, AWS_REGION and AWS_SESSION_TOKEN.
* Values from these environment variables will override any values
* for corresponding variables loaded from credentials and configuration
* files.
*
* Values for AWS_PROFILE and AWS_MAX_ATTEMPTS are not currently obtained.
*
* Return: SUCCEED/FAIL
*
*/
static herr_t
H5FD__s3comms_load_aws_creds_from_env(char *key_id, char *secret_access_key, char *session_token,
char *aws_region)
{
herr_t ret_value = SUCCEED;
char *key_id_env = NULL;
char *secret_access_key_env = NULL;
char *session_token_env = NULL;
char *aws_region_env = NULL;

FUNC_ENTER_PACKAGE

/* AWS_ACCESS_KEY_ID values are typically 16 or 20 characters, with up to 128 allowed.
* Difference in size between the one from the environment and one in cred files
* requires some special handling.
*/
key_id_env = getenv("AWS_ACCESS_KEY_ID");
if (key_id_env != NULL && key_id_env[0] != '\0') {
if (strlen(key_id) == 0 || strncmp(key_id, key_id_env, strlen(key_id)) != 0)
strncpy(key_id, key_id_env, strlen(key_id_env));
key_id[strlen(key_id_env)] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to add a null-terminator to the existing key_id when it's not overwritten by the environment variable?

}

/* AWS_SECRET_ACCESS_KEY values are 40 characters */
secret_access_key_env = getenv("AWS_SECRET_ACCESS_KEY");
if (secret_access_key_env != NULL && secret_access_key_env[0] != '\0') {
if (strlen(secret_access_key) == 0 ||
strncmp(secret_access_key, secret_access_key_env, strlen(secret_access_key)) != 0) {
strncpy(secret_access_key, secret_access_key_env, strlen(secret_access_key_env));
secret_access_key[strlen(secret_access_key_env)] = '\0';
}
}

/* AWS_SESSION_TOKEN values are unbounded, but for now assume < 4096 */
session_token_env = getenv("AWS_SESSION_TOKEN");
if (session_token_env != NULL && session_token_env[0] != '\0') {
if (strlen(session_token) == 0 ||
strncmp(session_token, session_token_env, strlen(session_token_env)) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check for whether to replace the provided session token could produce incorrect behavior if the environment variable is a prefix of the session token from config/credentials files. (e.g. env var = key1, and session token = key11). Both conditions here would be false, and the value would not be overwritten from the environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would also apply to the key and region. I think it's necessary to either take the longer length or just use strcmp() if the buffers are known to be null-terminated.

strncpy(session_token, session_token_env, strlen(session_token_env));
session_token[strlen(session_token_env)] = '\0';
}
}

/* AWS_REGION values are 9 - ~12 characters */
aws_region_env = getenv("AWS_REGION");
if (aws_region_env != NULL && aws_region_env[0] != '\0') {
if (strlen(aws_region) == 0 || strncmp(aws_region, aws_region_env, strlen(aws_region)) != 0) {
strncpy(aws_region, aws_region_env, strlen(aws_region_env));
aws_region[strlen(aws_region_env)] = '\0';
}
}

FUNC_LEAVE_NOAPI(ret_value)
}

/*----------------------------------------------------------------------------
*
* Function: H5FD_s3comms_load_aws_profile()
Expand Down Expand Up @@ -1679,7 +1756,7 @@ H5FD__s3comms_load_aws_creds_from_file(FILE *file, const char *profile_name, cha
*/
herr_t
H5FD_s3comms_load_aws_profile(const char *profile_name, char *key_id_out, char *secret_access_key_out,
char *aws_region_out)
char *session_token_out, char *aws_region_out)
{
herr_t ret_value = SUCCEED;
FILE *credfile = NULL;
Expand All @@ -1703,8 +1780,8 @@ H5FD_s3comms_load_aws_profile(const char *profile_name, char *key_id_out, char *
credfile = fopen(filepath, "r");
if (credfile != NULL) {
if (H5FD__s3comms_load_aws_creds_from_file(credfile, profile_name, key_id_out, secret_access_key_out,
aws_region_out) == FAIL)
HGOTO_ERROR(H5E_VFL, H5E_BADVALUE, FAIL, "unable to load from aws credentials");
session_token_out, aws_region_out) == FAIL)
HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "unable to load from aws credentials");
if (fclose(credfile) == EOF)
HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "unable to close credentials file");
credfile = NULL;
Expand All @@ -1719,13 +1796,20 @@ H5FD_s3comms_load_aws_profile(const char *profile_name, char *key_id_out, char *
if (H5FD__s3comms_load_aws_creds_from_file(
credfile, profile_name, (*key_id_out == 0) ? key_id_out : NULL,
(*secret_access_key_out == 0) ? secret_access_key_out : NULL,
(*session_token_out == 0) ? session_token_out : NULL,
(*aws_region_out == 0) ? aws_region_out : NULL) == FAIL)
HGOTO_ERROR(H5E_VFL, H5E_BADVALUE, FAIL, "unable to load from aws config");
if (fclose(credfile) == EOF)
HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "unable to close config file");
credfile = NULL;
}

/* Check for credentials in environment variables. Environment variables will override
* credentials from credentials/config files and just load them if there were none in
* the files. */
ret_value = H5FD__s3comms_load_aws_creds_from_env(key_id_out, secret_access_key_out, session_token_out,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine not use HGOTO_ERROR here right now since it's at the end of the routine anyway, but it would be good to explicitly check ret_value in case that changes later.

aws_region_out);

/* Fail if not all three settings were loaded */
if (*key_id_out == 0 || *secret_access_key_out == 0 || *aws_region_out == 0)
ret_value = FAIL;
Expand Down
2 changes: 1 addition & 1 deletion src/H5FDs3comms.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ H5_DLL herr_t H5FD_s3comms_aws_canonical_request(char *canonical_request_dest, i
H5_DLL herr_t H5FD_s3comms_free_purl(parsed_url_t *purl);

H5_DLL herr_t H5FD_s3comms_load_aws_profile(const char *name, char *key_id_out, char *secret_access_key_out,
char *aws_region_out);
char *aws_session_token_out, char *aws_region_out);

H5_DLL herr_t H5FD_s3comms_signing_key(unsigned char *md, const char *secret, const char *region,
const char *iso8601now);
Expand Down
19 changes: 18 additions & 1 deletion test/ros3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ static int s3_test_credentials_loaded = 0;
static char s3_test_aws_region[16];
static char s3_test_aws_access_key_id[64];
static char s3_test_aws_secret_access_key[128];
static char s3_test_aws_session_token[4096];

H5FD_ros3_fapl_t restricted_access_fa = {H5FD_CURR_ROS3_FAPL_T_VERSION, /* fapl version */
true, /* authenticate */
Expand Down Expand Up @@ -504,6 +505,8 @@ test_eof_eoa(void)
TEST_ERROR;
if (H5Pset_fapl_ros3(fapl_id, &restricted_access_fa) < 0)
TEST_ERROR;
if (H5Pset_fapl_ros3_token(fapl_id, &restricted_access_fa.session_token) < 0)
TEST_ERROR;

/* Open and verify EOA and EOF in a sample file */
if (NULL == (fd = H5FDopen(url_text_restricted, H5F_ACC_RDONLY, fapl_id, HADDR_UNDEF)))
Expand Down Expand Up @@ -647,6 +650,8 @@ test_vfl_read(void)
TEST_ERROR;
if (H5Pset_fapl_ros3(fapl_id, &restricted_access_fa) < 0)
TEST_ERROR;
if (H5Pset_fapl_ros3_token(fapl_id, &restricted_access_fa.session_token) < 0)
TEST_ERROR;

if (NULL == (fd = H5FDopen(url_text_public, H5F_ACC_RDONLY, fapl_id, HADDR_UNDEF)))
TEST_ERROR;
Expand Down Expand Up @@ -739,6 +744,8 @@ test_vfl_read_without_eoa_set_fails(void)
TEST_ERROR;
if (H5Pset_fapl_ros3(fapl_id, &restricted_access_fa) < 0)
TEST_ERROR;
if (H5Pset_fapl_ros3_token(fapl_id, &restricted_access_fa.session_token) < 0)
TEST_ERROR;

/* Open w/ VFL call */
if (NULL == (fd = H5FDopen(url_text_restricted, H5F_ACC_RDONLY, fapl_id, MAXADDR)))
Expand Down Expand Up @@ -811,6 +818,8 @@ test_noops_and_autofails(void)
TEST_ERROR;
if (H5Pset_fapl_ros3(fapl_id, &anonymous_fa) < 0)
TEST_ERROR;
if (H5Pset_fapl_ros3_token(fapl_id, &anonymous_fa.session_token) < 0)
TEST_ERROR;
if (NULL == (fd = H5FDopen(url_text_public, H5F_ACC_RDONLY, fapl_id, HADDR_UNDEF)))
TEST_ERROR;

Expand Down Expand Up @@ -896,6 +905,8 @@ test_cmp(void)
TEST_ERROR;
if (H5Pset_fapl_ros3(fapl_id, &restricted_access_fa) < 0)
TEST_ERROR;
if (H5Pset_fapl_ros3_token(fapl_id, &restricted_access_fa.session_token) < 0)
TEST_ERROR;

/* Open files */
if (NULL == (fd_raven = H5FDopen(url_text_public, H5F_ACC_RDONLY, fapl_id, HADDR_UNDEF)))
Expand Down Expand Up @@ -974,6 +985,8 @@ test_ros3_access_modes(void)
TEST_ERROR;
if (H5Pset_fapl_ros3(fapl_id, &restricted_access_fa) < 0)
TEST_ERROR;
if (H5Pset_fapl_ros3_token(fapl_id, &restricted_access_fa.session_token) < 0)
TEST_ERROR;

/* Read-Write Open access is not allowed with this file driver */
H5E_BEGIN_TRY
Expand Down Expand Up @@ -1086,17 +1099,21 @@ main(void)
/* Clear profile data strings */
s3_test_aws_access_key_id[0] = '\0';
s3_test_aws_secret_access_key[0] = '\0';
s3_test_aws_session_token[0] = '\0';
s3_test_aws_region[0] = '\0';

/* Attempt to load test credentials - if unable, certain tests will be skipped */
if (SUCCEED == H5FD_s3comms_load_aws_profile(S3_TEST_PROFILE_NAME, s3_test_aws_access_key_id,
s3_test_aws_secret_access_key, s3_test_aws_region)) {
s3_test_aws_secret_access_key, s3_test_aws_session_token,
s3_test_aws_region)) {
s3_test_credentials_loaded = 1;
strncpy(restricted_access_fa.aws_region, (const char *)s3_test_aws_region, H5FD_ROS3_MAX_REGION_LEN);
strncpy(restricted_access_fa.secret_id, (const char *)s3_test_aws_access_key_id,
H5FD_ROS3_MAX_SECRET_ID_LEN);
strncpy(restricted_access_fa.secret_key, (const char *)s3_test_aws_secret_access_key,
H5FD_ROS3_MAX_SECRET_KEY_LEN);
strncpy(restricted_access_fa.session_token, (const char *)s3_test_aws_session_token,
H5FD_ROS3_MAX_SECRET_TOK_LEN);
}

/******************
Expand Down
29 changes: 16 additions & 13 deletions test/s3comms.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@
*/
static int s3_test_credentials_loaded = 0;
static char s3_test_aws_region[16] = "";
static char s3_test_aws_access_key_id[64] = "";
static char s3_test_aws_access_key_id[128] = "";
static char s3_test_aws_secret_access_key[128] = "";
static char s3_test_aws_security_token[1024] = "";
static char s3_test_aws_session_token[4096] = "";
static char s3_test_bucket_url[S3_TEST_MAX_URL_SIZE] = "";
static bool s3_test_bucket_defined = false;

Expand Down Expand Up @@ -1031,7 +1031,7 @@ test_s3r_open(void)
{
handle = H5FD_s3comms_s3r_open(
url_missing, (const char *)s3_test_aws_region, (const char *)s3_test_aws_access_key_id,
(const unsigned char *)signing_key, (const char *)s3_test_aws_security_token);
(const unsigned char *)signing_key, (const char *)s3_test_aws_session_token);
}
H5E_END_TRY
if (handle != NULL)
Expand All @@ -1055,7 +1055,7 @@ test_s3r_open(void)
{
handle = H5FD_s3comms_s3r_open(url_shakespeare, (const char *)s3_test_aws_region, "I_MADE_UP_MY_ID",
(const unsigned char *)signing_key,
(const char *)s3_test_aws_security_token);
(const char *)s3_test_aws_session_token);
}
H5E_END_TRY
if (handle != NULL)
Expand All @@ -1066,7 +1066,7 @@ test_s3r_open(void)
{
handle = H5FD_s3comms_s3r_open(
url_shakespeare, (const char *)s3_test_aws_region, (const char *)s3_test_aws_access_key_id,
(const unsigned char *)EMPTY_SHA256, (const char *)s3_test_aws_security_token);
(const unsigned char *)EMPTY_SHA256, (const char *)s3_test_aws_session_token);
}
H5E_END_TRY
if (handle != NULL)
Expand All @@ -1089,7 +1089,7 @@ test_s3r_open(void)
/* Using authentication on anonymously-accessible file? */
handle = H5FD_s3comms_s3r_open(
url_raven, (const char *)s3_test_aws_region, (const char *)s3_test_aws_access_key_id,
(const unsigned char *)signing_key, (const char *)s3_test_aws_security_token);
(const unsigned char *)signing_key, (const char *)s3_test_aws_session_token);
if (handle == NULL)
TEST_ERROR;
if (6464 != H5FD_s3comms_s3r_get_filesize(handle))
Expand All @@ -1101,7 +1101,7 @@ test_s3r_open(void)
/* Authenticating */
handle = H5FD_s3comms_s3r_open(
url_shakespeare, (const char *)s3_test_aws_region, (const char *)s3_test_aws_access_key_id,
(const unsigned char *)signing_key, (const char *)s3_test_aws_security_token);
(const unsigned char *)signing_key, (const char *)s3_test_aws_session_token);
if (handle == NULL)
TEST_ERROR;
if (5458199 != H5FD_s3comms_s3r_get_filesize(handle))
Expand Down Expand Up @@ -1165,12 +1165,13 @@ test_s3r_read(void)
if (6464 != H5FD_s3comms_s3r_get_filesize(handle))
TEST_ERROR;

/*****************************
* Tests that should succeed *
*****************************/
if (handle != NULL)
/*****************************
* Tests that should succeed *
*****************************/

/* Read from start of file */
memset(buffer, 0, S3COMMS_READ_BUFFER_SIZE);
/* Read from start of file */
memset(buffer, 0, S3COMMS_READ_BUFFER_SIZE);
if (H5FD_s3comms_s3r_read(handle, (haddr_t)0, (size_t)118, buffer) < 0)
TEST_ERROR;
if (strcmp("Once upon a midnight dreary, while I pondered, weak and weary,\n"
Expand Down Expand Up @@ -1286,6 +1287,7 @@ main(void)
/* "clear" profile data strings */
s3_test_aws_access_key_id[0] = '\0';
s3_test_aws_secret_access_key[0] = '\0';
s3_test_aws_session_token[0] = '\0';
s3_test_aws_region[0] = '\0';
s3_test_bucket_url[0] = '\0';

Expand All @@ -1297,7 +1299,8 @@ main(void)
* if unable, certain tests will be skipped
*/
if (SUCCEED == H5FD_s3comms_load_aws_profile(S3_TEST_PROFILE_NAME, s3_test_aws_access_key_id,
s3_test_aws_secret_access_key, s3_test_aws_region)) {
s3_test_aws_secret_access_key, s3_test_aws_session_token,
s3_test_aws_region)) {
s3_test_credentials_loaded = 1;
}

Expand Down
Loading