Add AWS Console browser sign-in for S3 - #18331
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “Amazon S3 (AWS Console Sign-In)” connection profile that authenticates via AWS CLI v2 browser sign-in, exporting short-lived S3 credentials into memory and pinning the AWS identity to the bookmark. It also updates macOS and Windows UI logic so password/keychain-save controls are disabled for protocols that don’t use stored credentials.
Changes:
- Add
s3-loginprotocol + bundled profile, and implementAWSConsoleLoginCredentialsStrategythat runsaws login/aws configure export-credentialsand pins identity viasts get-caller-identity. - Wire the new credentials strategy into
S3Sessionand avoid redundant STS identity calls for the new protocol. - Update macOS/Windows bookmark & connection controllers and
LoginOptionsso keychain-save controls are disabled for credentialless protocols (and add tests forLoginOptions+ the new AWS strategy).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| windows/src/main/csharp/ch/cyberduck/ui/controller/ConnectionController.cs | Moves save-password UI state initialization into Update() so it stays in sync with protocol/options. |
| windows/src/main/csharp/ch/cyberduck/ui/controller/BookmarkController.cs | Sets credentials “saved” flag on protocol change based on updated login options. |
| osx/src/main/java/ch/cyberduck/ui/cocoa/controller/ConnectionController.java | Updates keychain checkbox state via bookmark observer to reflect current options/credentials. |
| osx/src/main/java/ch/cyberduck/ui/cocoa/controller/BookmarkController.java | Sets credentials “saved” flag on protocol change based on updated login options. |
| core/src/main/java/ch/cyberduck/core/LoginOptions.java | Recomputes keychain and save based on protocol capabilities + preference. |
| core/src/test/java/ch/cyberduck/core/LoginOptionsTest.java | Adds coverage for LoginOptions.configure() keychain/save behavior. |
| s3/src/main/java/ch/cyberduck/core/signin/AWSConsoleLoginCredentialsStrategy.java | New AWS CLI browser sign-in credentials strategy with in-memory token cache + identity pinning. |
| s3/src/test/java/ch/cyberduck/core/signin/AWSConsoleLoginCredentialsStrategyTest.java | Tests for export/login behavior, memory caching, error detail hygiene, and identity pinning. |
| s3/src/main/java/ch/cyberduck/core/s3/S3Session.java | Uses the new strategy for s3-login and skips STS identity call already handled by CLI strategy. |
| s3/src/main/java/ch/cyberduck/core/s3/S3LoginProtocol.java | New protocol identifier s3-login and disables CredentialsConfigurator. |
| profiles/default/Amazon S3 (AWS Console Sign-In).cyberduckprofile | Bundled profile definition for the new s3-login protocol and region list. |
| osx/build.xml | Sets JVM process launch mechanism to FORK in the packaged macOS runtime args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
s3/src/main/java/ch/cyberduck/core/signin/AWSConsoleLoginCredentialsStrategy.java:163
parsetreats missing/blankSessionTokenthe same as other parse failures and throws the generic “AWS CLI 2.32 or later is required” message. When the CLI exports long‑lived access keys (no session token), the failure reason is different and the current message is misleading for users who do have a new CLI but used the wrong profile/credential type.
Consider distinguishing the "permanent credentials" case with a specific error detail (and optionally an explicit message for expired credentials) so users know they must use browser sign-in / short‑lived session credentials.
final String accessKey = value.path("AccessKeyId").asText();
final String secretKey = value.path("SecretAccessKey").asText();
final String sessionToken = value.path("SessionToken").asText();
final long expiration = OffsetDateTime.parse(value.path("Expiration").asText()).toInstant().toEpochMilli();
if(StringUtils.isAnyBlank(accessKey, secretKey, sessionToken) || expiration <= System.currentTimeMillis()) {
throw failure();
}
9584466 to
a226a1f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
s3/src/main/java/ch/cyberduck/core/signin/AWSConsoleLoginCredentialsStrategy.java:215
- The expiry calculation subtracts a fixed 5 minute buffer from expiresIn; if the server ever returns expiresIn <= 300s, the computed expiry will be <= now and the tokens will be immediately expired, potentially causing an authorization/refresh loop. Clamp the buffer so the expiry is always in the future (at least by a small amount).
return new TemporaryAccessTokens(accessKey, secretKey, sessionToken,
System.currentTimeMillis() + expires * 1000L - 5L * 60L * 1000L);
oauth/src/main/java/ch/cyberduck/core/oauth/LoopbackOAuth2AuthorizationCodeProvider.java:75
- The state listener registered with OAuth2TokenListenerRegistry is only removed on a successful notify(). If the flow errors before notify() or the user cancels/timeout occurs, the listener remains in the global registry map, causing a memory leak and potential unexpected callbacks later. Consider adding an explicit unregister/remove API on OAuth2TokenListenerRegistry and calling it from a finally block here (and in other prompt implementations) when the flow terminates without a notify().
final CountDownLatch signal = new CountDownLatch(1);
final AtomicReference<String> authenticationCode = new AtomicReference<>();
OAuth2TokenListenerRegistry.get().register(expectedState, code -> {
if(StringUtils.isBlank(code)) {
signal.countDown();
}
else {
authenticationCode.set(code);
}
});
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
s3/src/main/java/ch/cyberduck/core/signin/AWSConsoleLoginCredentialsStrategy.java:288
validateIdentity(host, idToken)decodes the JWT without any sanity checks on thesubformat. If AWS ever returns an unexpected subject (or the token is corrupted), this can pin an invalid identity value into the bookmark and later block logins. Add minimal validation thatsubis a well‑formed AWS ARN and that the extracted account id is 12-digit numeric before accepting it.
static void validateIdentity(final Host host, final String idToken) throws LoginFailureException {
try {
final String arn = JWT.decode(idToken).getSubject();
validateIdentity(host, StringUtils.substringBetween(arn, "::", ":"), arn);
}
oauth/src/main/java/ch/cyberduck/core/oauth/LoopbackOAuth2AuthorizationCodeProvider.java:61
LoopbackOAuth2AuthorizationCodeProvidernow has a new overload (Function-based URL builder), dynamic loopback binding, and a different callback response path (200 "Login successful" vs 302). There are oauth module tests for other providers, but none that exercise this updated loopback flow, making regressions in state validation/redirect handling hard to catch.
public String prompt(final Host bookmark, final LoginCallback prompt,
final Function<String, String> authorizationCodeUrl, final String state) throws BackgroundException {
return this.prompt(bookmark, prompt, authorizationCodeUrl, null, state);
}
|
Hi David, thank you for the detailed review. I now have a better understanding of this repo and its design principles. I agree that removing CLI is the right call that makes feature impls more consistent and self-contained. I have removed the direct AWS CLI invocation and replaced it with native AWS auth flow using PKCE and DPoP, by reusing Cyberduck's existing components like loopback OAuth, S3 credential strategy, connection profile, and secure pw store. I also removed the unrelated UI, keychain, and process launch changes, and now select the auth strategy through a connection profile property rather than concrete protocol class checks. This PR is ready for another review. Thanks again and I would appreciate another look. Yiming |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
oauth/src/main/java/ch/cyberduck/core/oauth/LoopbackOAuth2AuthorizationCodeProvider.java:101
- The loopback callback currently treats a request as accepted as long as the state matches and the listener is notified, even if the
codeparameter is missing/blank. In that case the handler will still return the “Login successful” page (when using the ephemeral 127.0.0.1 redirect), but the application will later treat the login as canceled becauseauthenticationCodeis null. This is inconsistent UX and makes it harder to diagnose callback failures/denials.
Consider distinguishing “listener notified” from “successful authorization code received”, and only show the success page when code is non-blank (still notifying the listener so the waiting thread can continue).
final boolean accepted = StringUtils.equals(expectedState, state) && OAuth2TokenListenerRegistry.get().notify(state, code);
try {
if(!accepted) {
exchange.sendResponseHeaders(400, 0);
}
Summary
Add a bundled Amazon S3 (AWS Console Sign-In) profile with native browser authentication. It requires neither AWS CLI nor long-lived access keys.
Motivation
The existing S3 profile expects access keys, while the IAM Identity Center flow requires organization-side Identity Center configuration. AWS Console Sign-In lets a user authenticate with an existing AWS Console identity without creating persistent access keys, installing AWS CLI, or having IAM Identity Center access.
Implementation notes
127.0.0.1, exact state validation, PKCE S256, and ES256 DPoP.Validation
mvn -pl oauth,s3 -am -DskipTests package: all 8 reactor modules passed on macOS and Windows.git diff --check origin/master...HEADpassed.