port/cpl_aws: add credential process #13239
Conversation
get or refresh port/cpl_aws: support process_credentials
|
@Kontinuation Can you take a look? |
| CPLError(CE_Failure, CPLE_AppDefined, | ||
| "Failed to execute credential_process: %s", | ||
| osCredentialProcess.c_str()); |
There was a problem hiding this comment.
Should we include VSIStrerror(errno) in the error message to show the reason why popen failed?
| CPLStringList aosOptions; | ||
| aosOptions.SetNameValue("TIMEOUT", "30"); | ||
|
|
||
| FILE *fp = popen(osCredentialProcess.c_str(), "r"); |
There was a problem hiding this comment.
Should we use CPL-family functions in cpl_spawn.h such as CPLPipeRead for multi-platform compatibility instead of directly calling POSIX API here?
There was a problem hiding this comment.
yes please use CPLSpawn(). You'll need to tokenize the command line using CSLTokenizeString2(osCredetialsProcess.c_str(), " ", CSLT_HONOURSTRINGS)
| if (osSessionToken.empty()) | ||
| { | ||
| CPLError(CE_Failure, CPLE_AppDefined, | ||
| "credential_process did not return required SessionToken"); | ||
| return false; | ||
| } | ||
|
|
||
| if (osExpiration.empty()) | ||
| { | ||
| CPLError(CE_Failure, CPLE_AppDefined, | ||
| "credential_process did not return required Expiration"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Are these required fields? At least Expiration is not required according to https://docs.aws.amazon.com/sdkref/latest/guide/feature-process-credentials.html#feature-process-credentials-output
There was a problem hiding this comment.
Good point. I'll handle the optional case.
| } | ||
|
|
||
| // Release mutex before calling external process | ||
| oHolder.~CPLMutexHolder(); |
There was a problem hiding this comment.
A common practice is to use CPLMutexHolderD and wrap everything in critical section using a proper lexical scope.
| CPLStringList aosOptions; | ||
| aosOptions.SetNameValue("TIMEOUT", "30"); | ||
|
|
||
| FILE *fp = popen(osCredentialProcess.c_str(), "r"); |
There was a problem hiding this comment.
yes please use CPLSpawn(). You'll need to tokenize the command line using CSLTokenizeString2(osCredetialsProcess.c_str(), " ", CSLT_HONOURSTRINGS)
| return true; | ||
| } | ||
|
|
||
| if (!osCredentialProcess.empty()) |
There was a problem hiding this comment.
this block should likely be moved in the above scope before the return true at line 2157. And it should call GetCredentialsFromProcess() insead of GetOrRefreshTemporaryCredentialsForProcess(), and it should return false if GetCredentialsFromProcess() returns false
| // First, check if we have a cached credential_process command | ||
| std::string osCredentialProcess; | ||
| { | ||
| CPLMutexHolder oHolder(&ghMutex); | ||
| osCredentialProcess = gosCredentialProcessCommand; | ||
| } | ||
|
|
||
| // If no cached command, re-read config files to get the credential_process setting | ||
| if (osCredentialProcess.empty()) | ||
| { | ||
| std::string osSecretAccessKey, osAccessKeyId, osSessionToken, | ||
| osRegion; | ||
| std::string osCredentials, osRoleArn, osSourceProfile, osExternalId; | ||
| std::string osMFASerial, osRoleSessionName, osWebIdentityTokenFile; | ||
| std::string osSSOStartURL, osSSOAccountID, osSSORoleName, | ||
| osSSOSession; | ||
|
|
||
| if (GetConfigurationFromAWSConfigFiles( | ||
| osPathForOption, nullptr, osSecretAccessKey, osAccessKeyId, | ||
| osSessionToken, osRegion, osCredentials, osRoleArn, | ||
| osSourceProfile, osExternalId, osMFASerial, | ||
| osRoleSessionName, osWebIdentityTokenFile, osSSOStartURL, | ||
| osSSOAccountID, osSSORoleName, osSSOSession, | ||
| osCredentialProcess)) | ||
| { | ||
| // Cache the command for future use | ||
| CPLMutexHolder oHolder(&ghMutex); | ||
| gosCredentialProcessCommand = osCredentialProcess; | ||
| } | ||
| } | ||
|
|
||
| // Now use the cached refresh mechanism | ||
| if (!osCredentialProcess.empty()) |
There was a problem hiding this comment.
I believe all of his should be deleted, so as to be "symetric" with the other credential soures
| echo '{"AccessKeyId":"AWS_ACCESS_KEY_ID","SecretAccessKey":"AWS_SECRET_ACCESS_KEY","SessionToken":"session_token","Expiration":"2025-01-01T12:00:00Z"}' | ||
| """ | ||
|
|
||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) as f: |
There was a problem hiding this comment.
instead of a shell script, you could use a python script to be platform independent, and use something like "credential_process = {sys.executable} {script_path}" in the aws_config file
4 participants
What does this PR do?
Adds support for https://docs.aws.amazon.com/sdkref/latest/guide/feature-process-credentials.html.
Please note I am not a C++ developer and solely used copilot. Happy to close this in favor of a better implementation or receive guidance on how to get this in if this is a feature GDAL would like to incorporate 🙏.
What are related issues/pull requests?
Tasklist
Environment
Provide environment details, if relevant: