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.

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

Allow connection profiles to use custom STS and OIDC endpoints #14972

Merged
merged 84 commits into from Aug 28, 2023

Conversation

dkocher
Copy link
Contributor

@dkocher dkocher commented Aug 7, 2023

Resolves #13804.

@dkocher dkocher requested a review from a team as a code owner August 7, 2023 14:29
@dkocher dkocher marked this pull request as draft August 7, 2023 14:30
@dkocher dkocher added this to the 8.7.0 milestone Aug 7, 2023
@dkocher dkocher force-pushed the feature/GH-13804-oidc branch 2 times, most recently from 6fb5dee to 450138b Compare August 8, 2023 20:50
defaults/src/main/resources/default.properties Outdated Show resolved Hide resolved
.find(host.getProtocol().getOAuthAuthorizationUrl()), this, prompt).build(), host)
.withRedirectUri(host.getProtocol().getOAuthRedirectUrl())
.withFlowType(OAuth2AuthorizationService.FlowType.valueOf(host.getProtocol().getAuthorization())));
configuration.addInterceptorLast(sts = new STSAssumeRoleCredentialsRequestInterceptor(oauth, this, trust, key));
Copy link
Contributor

Choose a reason for hiding this comment

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

As sts comes last, does this mean we renew oauth even if STS token is still valid? Is this necessary? Comment on this. Do we have tests for all possible cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OAuth should be refreshed first using the standard OAuth2ErrorResponseInterceptor although it must be verified this actually works as it only handles 401. We are missing tests cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

OAuth should be refreshed first

This seems not self-evident to me - can you reason why?

If OAuth is refreshed, will this force to renew STS as well even if it is still valid? If yes, is this intended? Which cases should be tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation currently is intended as follows

  1. Refresh of OAuth tokens when expired preemptively in OAuth2RequestInterceptor 1
  2. Refresh STS tokens using cached OAuth token preemptively in STSAssumeRoleCredentialsRequestInterceptor 2
  3. On error response handle cases in STSAssumeRoleTokenExpiredResponseInterceptor as follows
  • On 401, 403 refresh OIDC Id token
  • On 400 reuse cached OIDC Id token
  • For both cases above refresh STS tokens

Footnotes

  1. https://github.com/iterate-ch/cyberduck/pull/14972/files#diff-7ec37d50b17b31a07e5438bce7f0f3f4eba8a930b1c100e12e3ad84dda666ff8R106

  2. https://github.com/iterate-ch/cyberduck/pull/14972/files#diff-c51064e4ed9ed159481473d881ec27aae4bd319978568b45480976f8f989b043R89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated handling of authentication failures in S3AuthenticationResponseInterceptor as of 8ea744a.

  • Handle 403 and 400
  • Refresh STS token with cached OIDC Id token when not expired
  • Otherwise refresh OIDC Id token first
  • On failure refresh STS token with refreshed OIDC Id token
Google OpenID Provider AWS API HTTP Status Code Error Code (in XML response body)
OpenID Id Token Expired S3 ListObjects 1
OpenID Id Token Expired STS AssumeRoleWithWebIdentity 400 ExpiredTokenException 2
STS Tokens Expired S3 ListObjects 400 ExpiredToken 3
STS Tokens Invalid S3 ListObjects 400 InvalidToken [^4]
HTTP/1.1 400 Bad Request
x-amzn-RequestId: b6baa352-afcb-4763-a32c-a67d7eb5020b
Content-Type: text/xml
Content-Length: 346
Date: Sun, 13 Aug 2023 10:53:10 GMT
Connection: close

<ErrorResponse xmlns="https://sts.amazonaws.com/doc/2011-06-15/">
  <Error>
    <Type>Sender</Type>
    <Code>ExpiredTokenException</Code>
    <Message>Token expired: current date/time 1691923690 must be before the expiration date/time1691878565</Message>
  </Error>
  <RequestId>b6baa352-afcb-4763-a32c-a67d7eb5020b</RequestId>
</ErrorResponse>

Footnotes

  1. Token expired: current date/time 1691790439 must be before the expiration date/time1691788167

  2. The provided token has expired.

@dkocher dkocher force-pushed the feature/GH-13804-oidc branch 6 times, most recently from adff9aa to de8f6e1 Compare August 9, 2023 21:23
.find(host.getProtocol().getOAuthAuthorizationUrl()), this, prompt).build(), host)
.withRedirectUri(host.getProtocol().getOAuthRedirectUrl())
.withFlowType(OAuth2AuthorizationService.FlowType.valueOf(host.getProtocol().getAuthorization())));
configuration.addInterceptorLast(sts = new STSAssumeRoleCredentialsRequestInterceptor(oauth, this, trust, key));
Copy link
Contributor

Choose a reason for hiding this comment

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

OAuth should be refreshed first

This seems not self-evident to me - can you reason why?

If OAuth is refreshed, will this force to renew STS as well even if it is still valid? If yes, is this intended? Which cases should be tested?

s3/src/main/java/ch/cyberduck/core/s3/S3Session.java Outdated Show resolved Hide resolved
s3/src/main/java/ch/cyberduck/core/s3/S3Session.java Outdated Show resolved Hide resolved
s3/src/main/java/ch/cyberduck/core/s3/S3Session.java Outdated Show resolved Hide resolved
s3/src/test/resources/S3-OIDC-Testing.cyberduckprofile Outdated Show resolved Hide resolved
s3/src/test/resources/testcontainer/docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@ylangisc ylangisc left a comment

Choose a reason for hiding this comment

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

@dkocher dkocher requested a review from ylangisc August 28, 2023 10:09
@dkocher dkocher merged commit f5df84d into master Aug 28, 2023
4 checks passed
@dkocher dkocher deleted the feature/GH-13804-oidc branch August 28, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants