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

CloudFrontUtilities clobbers port number if present when signing URLs #5042

Open
sagebind opened this issue Mar 25, 2024 · 1 comment
Open
Labels
bug This issue is a bug. p3 This is a minor priority issue

Comments

@sagebind
Copy link

Describe the bug

When signing a resource URL with CloudFrontUtilities::getSignedUrlWithCannedPolicy, if the given URL contains a port number in the authority, the signature will be generated based on the original string with the port, but the port will be stripped from the actual string that is returned. The signature will be invalid.

Expected Behavior

CloudFrontUtilities::getSignedUrlWithCannedPolicy should not modify the URL authority that it is given.

Current Behavior

Current behavior is that CloudFrontUtilities::getSignedUrlWithCannedPolicy always strips the port number from the authority given, after it has already generated a signature.

Reproduction Steps

CloudFrontUtilities cloudFrontUtilities = CloudFrontUtilities.create();

String urlToSign = "https://abc.cloudfront.net:443/foo/bar.html";
String signedUrl = cloudFrontUtilities.getSignedUrlWithCannedPolicy(signer -> signer
    .resourceUrl(uri.toString())
    .keyPairId(keyName)
    .privateKey(privateKey)
    .expirationDate(normalizedExpires))
    .url();

// signedUrl will be "https://abc.cloudfront.net/foo/bar.html", but the signature was
// signed for "https://abc.cloudfront.net:443/foo/bar.html".
assert signedUrl.startsWith(urlToSign);

Possible Solution

When reconstructing the signed URL, use the original URL's authority string unmodified, or add conditional logic to append the port to the reconstructed URL if given.

Possible implementation (untested):

             URI uri = URI.create(resourceUrl);
             String protocol = uri.getScheme();
             String domain = uri.getHost();
+            String authority = uri.getAuthority();
             String encodedPath = uri.getRawPath()
                                  + (uri.getQuery() != null ? "?" + uri.getRawQuery() + "&" : "?")
                                  + "Expires=" + request.expirationDate().getEpochSecond()
                                  + "&Signature=" + urlSafeSignature
                                  + "&Key-Pair-Id=" + request.keyPairId();
             return DefaultSignedUrl.builder().protocol(protocol).domain(domain).encodedPath(encodedPath)
-                                   .url(protocol + "://" + domain + encodedPath).build();
+                                   .url(protocol + "://" + authority + encodedPath).build();

Additional Information/Context

We sign "dummy URLs" in our unit and local application tests. These dummy URLs sometimes point to local servers and not real CloudFront hosts. This behavior broke such code when we upgraded from the AWS SDK for Java v1, since these local servers don't run on port 80 or 443.

In general, modifying the URL authority seems unexpected and unnecessary anyway.

AWS Java SDK version used

2.24.13

JDK version used

1.8.0_332

Operating System and version

macOS 13.6.5

@sagebind sagebind added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 25, 2024
@debora-ito debora-ito added needs-review This issue or PR needs review from the team. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 23, 2024
@debora-ito
Copy link
Member

@sagebind thank you for the reaching out, marking as a bug.

@debora-ito debora-ito added p3 This is a minor priority issue and removed needs-review This issue or PR needs review from the team. labels Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

2 participants