Support Duration for expirationTime in Aws4PresignerParams

[dhobbs]

Expected Behavior

When setting the expirationTime in the Aws4PresignerParams.Builder, it requires a value indicating
how many seconds the URI should be valid for. A clearer type to use to indicate a duration is java.time.Duration.

Current Behavior

Currently it uses java.time.Instant, which represents a single point in time, not a period of time. This makes the API confusing, as to specify eg. 1 hour expiry, the value Instant.ofEpochSecond(60 * 60) must be used. This value actually means a point in time of 1970-01-01T01:00:00Z. Using Instant implies to the caller that a point in time from Instant.now() should be specified, but this is interpreted as a very large expiration time and throws an exception.

Possible Solution

Modify the API to take Duration objects instead of Instant.

Steps to Reproduce (for bugs)

Call .expirationTime(Instant.now()) on Aws4PresignerParams.Builder and try to create a presigned URI.

Context

Your Environment

• AWS Java SDK version used: 2.3.9
• JDK version used: 11.0.2
• Operating System and version: macOS 10.14.2

[brainstorm]

I concur with @dhobbs, I just hit a similar issue right now when presigning URLs:

software.amazon.awssdk.core.exception.SdkClientException: null
        at software.amazon.awssdk.core.exception.SdkClientException$BuilderImpl.build(SdkClientException.java:97) ~[sdk-core-2.5.0.jar:?]
        at org.broad.igv.util.S3Presigner.presignS3DownloadLink(S3Presigner.java:64) ~[main/:?]
        at org.broad.igv.util.AmazonUtils.translateAmazonCloudURL(AmazonUtils.java:261) ~[main/:?]
        at org.broad.igv.util.ResourceLocator.setPath(ResourceLocator.java:330) ~[main/:?]
        at org.broad.igv.util.ResourceLocator.<init>(ResourceLocator.java:129) ~[main/:?]
        at org.broad.igv.aws.S3LoadDialog.lambda$loadButtonActionPerformed$0(S3LoadDialog.java:114) ~[main/:?]
        at org.broad.igv.util.LongRunningTask.call(LongRunningTask.java:72) [main/:?]
        at java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
        at java.lang.Thread.run(Thread.java:834) [?:?]
Caused by: java.time.DateTimeException: Instant exceeds minimum or maximum instant

[goosmurf]

This goes deeper to

aws-sdk-java-v2/core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAws4Signer.java

Line 405 in 985ec92

if (expirationInSeconds > SignerConstant.PRESIGN_URL_MAX_EXPIRATION_SECONDS) {

It's weird (to me at least) that an Instant is used, but it's really treated as a duration. So it feels like the API is the worst of both worlds; giving the impression that expirationTime is an actual point-in-real-world-time due to it being an Instant but in reality it's used as a duration.

[millems]

The bug with Instant has been fixed in the latest version. We'll take this as a feature request to implement support for Duration.

[jeffalder]

@millems So this is just adding an alternate method to the builder and calculating Instant.now().plus(duration)? Sounds like most of the original scope has been addressed and users should already only be using "real" instants (versus "Instant as Duration" as originally reported).

[millems]

@jeffalder Seems like that's about it. We'd probably want to go based on the requestSigningDateTime in the Aws4SignerRequestParams instead of Instant.now(), for consistency with the way the duration is currently calculated. Or we could go the other way: make Duration the source of truth, since we already just use the Instant to calculate a signature duration in the AbstractAws4Signer, and just make Instant a convenience method for Duration.

Either way, we probably should validate that people aren't trying to set both, since that indicates a misunderstanding of the API that could manifest in weird ways if they both mutate the same configuration value.