Add support for AWS credential_process configuration, allowing external processes to provide temporary AWS credentials.#17864
Add support for AWS credential_process configuration, allowing external processes to provide temporary AWS credentials.#17864
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for AWS credential_process in Cyberduck’s S3 credential auto-configuration so profiles can source temporary credentials from an external command (issue #11664).
Changes:
- Execute
credential_processcommands for process-based AWS profiles and parse returned JSON intoTemporaryAccessTokens. - Update AWS SDK BOM version (likely to pick up
BasicProfilesupport for process-based profiles).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| s3/src/main/java/ch/cyberduck/core/s3/S3CredentialsConfigurator.java | Adds credential_process handling by launching an external process and mapping JSON output to temporary tokens. |
| pom.xml | Bumps aws-java-sdk-bom from 1.12.778 to 1.12.797. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
s3/src/main/java/ch/cyberduck/core/s3/S3CredentialsConfigurator.java
Outdated
Show resolved
Hide resolved
| catch(IOException e) { | ||
| log.warn("Failure \"{}\" parsing cached credentials from {}", e.getMessage(), command); | ||
| return credentials; |
There was a problem hiding this comment.
The warning message says "parsing cached credentials" but this path is parsing credential_process output, not a cache file, and it drops useful failure details like exit code and stderr. Updating the log message (and including stderr/exit status) will make troubleshooting misconfigured profiles much easier.
| if(profile.isProcessBasedProfile()) { | ||
| // Uses external process to retrieve temporary credentials | ||
| final String command = profile.getCredentialProcess(); | ||
| final ObjectMapper mapper = JsonMapper.builder() |
There was a problem hiding this comment.
New credential_process support isn’t covered by tests, while this class already has dedicated unit tests and fixtures for other profile types. Add a test profile in src/test/resources/.../.aws plus a test that runs a small helper command (or a mocked process runner) returning the expected JSON, and assert the parsed tokens/expiry are applied.
| switch(Factory.Platform.getDefault()) { | ||
| case windows: | ||
| cmd.add("cmd"); | ||
| cmd.add("/c"); | ||
| break; | ||
| default: | ||
| cmd.add("sh"); | ||
| cmd.add("-c"); | ||
| break; | ||
| } | ||
| cmd.add(command); |
There was a problem hiding this comment.
Using cmd /c or sh -c runs the credential_process through a shell, which changes quoting/escaping semantics and expands shell metacharacters. AWS credential_process is typically executed as a direct process invocation; consider parsing the command into an argv list and calling ProcessBuilder without a shell to better match expected behavior and reduce unintended command interpretation.
| final ProcessBuilder builder = new ProcessBuilder(cmd); | ||
| try { | ||
| final Process process = builder.start(); | ||
| try(InputStream reader = process.getInputStream()) { | ||
| final CachedCredential cached = mapper.readValue(reader, CachedCredential.class); | ||
| return credentials.setTokens(new TemporaryAccessTokens( | ||
| cached.accessKey, cached.secretKey, cached.sessionToken, Instant.parse(cached.expiration).toEpochMilli())); | ||
| } |
There was a problem hiding this comment.
credential_process execution reads only stdout and never checks exit status or drains stderr; a process that writes to stderr (or never exits) can deadlock or block login. Consider redirecting stderr (or consuming it), waiting for the process to finish (with a timeout), and logging stderr/exit code on failure before falling back to existing credentials.
2 participants
Fix #11664.