Add function H5FD__s3comms_load_aws_creds_from_env() #5167
Conversation
credentials from environment variables. These will override any corresponding variables loaded from files.
…ox/hdf5 into expand_load_aws_credentials
lrknox
changed the title
derobins
added
Component - C Library
…d_load_aws_credentials
…ox/hdf5 into expand_load_aws_credentials
|
s3comms and ros3 tests pass now with valid temporary AWS credentials (region, key_id, secret and session_token) from credentials/config files or from AWS* environment variables. Values from AWS* environment variables will override values from credentials/config files. |
lrknox
requested review from
derobins,
byrnHDF,
fortnern,
jhendersonHDF,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
| @@ -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 */ | |||
There was a problem hiding this comment.
Should add 'session token' to the comment for this function describing everything used for the comparison
| @@ -1775,21 +1777,21 @@ H5FD__s3comms_load_aws_creds_from_file(FILE *file, const char *profile_name, cha | |||
| /* look for start of profile */ | |||
| do { | |||
| /* clear buffer */ | |||
| memset(buffer, 0, 128); | |||
| memset(buffer, 0, 4096); | |||
There was a problem hiding this comment.
It would be nice to replace repeated uses of 4096 with a single buffer_size variable
| 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'; |
There was a problem hiding this comment.
Is there a reason to add a null-terminator to the existing key_id when it's not overwritten by the environment variable?
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| /* 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, |
There was a problem hiding this comment.
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.
to get AWS credentials from environment variables. These will override any corresponding variables loaded from files.
Tested so far with s3comms and ros3 tests, where credentials from environment variables override invalid credentials loaded from the .aws/credentials file, don't fail with identical credentials loaded from the .aws/credentials file, and provide necessary credentials when none are loaded from the credentials/config files.
AWS_SESSION_TOKEN will be added next.