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.

Sign up for GitHub

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

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

Merged
merged 10 commits into from
Feb 18, 2025
Merged

Decouple policy logic from resource url for getSignedUrlWithCustomPolicy #5862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ba2fe99
Decouple policy logic from resource url for getSignedUrlWithCustomPolicy
RanVaknin Feb 4, 2025
d6a3f5a
Adding validation for resourceUrl to avoid NPE
RanVaknin Feb 4, 2025
94519ac
Add changelog
RanVaknin Feb 4, 2025
c6823a3
Fix checkstyle
RanVaknin Feb 4, 2025
e67972e
Renaming resource policy, adding documentation, refactoring
RanVaknin Feb 6, 2025
0d37a62
Adding not null validations
RanVaknin Feb 6, 2025
766c3cd
Changing resources name for intg test suite recreation
RanVaknin Feb 6, 2025
95dd934
Renaming policyResource, adding a test
RanVaknin Feb 11, 2025
b10ef40
Adding parameterized test
RanVaknin Feb 14, 2025
a9c5755
Fix checkstyle
RanVaknin Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/next-release/bugfix-AmazonCloudfront-1fa6f75.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "bugfix",
"category": "Amazon Cloudfront",
"contributor": "",
"description": "Decouple policy logic from resource url for getSignedUrlWithCustomPolicy"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be implementation detail.

Suggesting:

Allow users to specify resource URL pattern in `CloudFrontUtilities#getSignedUrlWithCustomPolicy`. See [#5577](https://github.com/aws/aws-sdk-java-v2/issues/5577)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Do you want me to raise a PR to amend the changelog?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that'd be great. It may be easier after release is finished.

}
24 changes: 20 additions & 4 deletions ...udfront/src/main/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import software.amazon.awssdk.services.cloudfront.model.CannedSignerRequest;
import software.amazon.awssdk.services.cloudfront.model.CustomSignerRequest;
import software.amazon.awssdk.services.cloudfront.url.SignedUrl;
import software.amazon.awssdk.utils.StringUtils;

/**
*
Expand Down Expand Up @@ -216,7 +217,13 @@ public SignedUrl getSignedUrlWithCustomPolicy(Consumer<CustomSignerRequest.Build
*
* @param request
* A {@link CustomSignerRequest} configured with the following values:
* resourceUrl, privateKey, keyPairId, expirationDate, activeDate (optional), ipRange (optional)
* resourceUrl,
* privateKey,
* keyPairId,
* expirationDate,
* activeDate (optional),
* ipRange (optional),
* resourceUrlPattern (optional)
* @return A signed URL that will permit access to distribution and S3
* objects as specified in the policy document.
*
Expand All @@ -233,6 +240,7 @@ public SignedUrl getSignedUrlWithCustomPolicy(Consumer<CustomSignerRequest.Build
* Path keyFile = myKeyFile;
* Instant activeDate = Instant.now().plus(Duration.ofDays(2));
* String ipRange = "192.168.0.1/24";
* String resourceUrlPattern = "*"; // If not supplied, defaults to the value of resourceUrl.
*
* CustomSignerRequest customRequest = CustomSignerRequest.builder()
* .resourceUrl(resourceUrl)
Expand All @@ -241,16 +249,24 @@ public SignedUrl getSignedUrlWithCustomPolicy(Consumer<CustomSignerRequest.Build
* .expirationDate(expirationDate)
* .activeDate(activeDate)
* .ipRange(ipRange)
* .resourceUrlPattern(resourceUrlPattern)
* .build();
* SignedUrl signedUrl = utilities.getSignedUrlWithCustomPolicy(customRequest);
* String url = signedUrl.url();
* }
*/
public SignedUrl getSignedUrlWithCustomPolicy(CustomSignerRequest request) {
String resourceUrl = request.resourceUrl();
try {
String resourceUrl = request.resourceUrl();
String policy = SigningUtils.buildCustomPolicyForSignedUrl(request.resourceUrl(), request.activeDate(),
request.expirationDate(), request.ipRange());
String resourceUrlPattern = StringUtils.isEmpty(request.resourceUrlPattern())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we are treating empty as null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because resourceUrlPattern maps to the JSON Policy document's Resource field, if a user provides null or "" we default to the resourceUrl itself - meaning, there is no pattern, just use the base url as the Resource field in the policy.

The server side implication of send a policy with a Resource field set to "" will result in a 403. I can't foresee a situation in which users are actively prohibiting all access to their distribution just by passing "".

For example:

        CustomSignerRequest request = CustomSignerRequest.builder()
                                                         .resourceUrl(resourceUrl)
                                                         .privateKey(keyFilePath)
                                                         .keyPairId(keyPairId)
                                                         .resourceUrlPattern("")
                                                         .activeDate(activeDate)
                                                         .expirationDate(expirationDate)
                                                         .build();

Will result in a 403.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, my question is more around: is this something we want to handle in the SDK? Should we just let it fail on the server side instead?

My concern is that some customers may not realize "" is not supported or have bugs in their code to pass empty string to resourceUrlPattern unintentionally, which will be hidden by this SDK logic.

Copy link
Contributor Author

@RanVaknin RanVaknin Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ack. I can just use a standard null check.

? request.resourceUrl()
: request.resourceUrlPattern();

String policy = SigningUtils.buildCustomPolicyForSignedUrl(resourceUrlPattern,
request.activeDate(),
request.expirationDate(),
request.ipRange());

byte[] signatureBytes = SigningUtils.signWithSha1Rsa(policy.getBytes(UTF_8), request.privateKey());
String urlSafePolicy = SigningUtils.makeStringUrlSafe(policy);
String urlSafeSignature = SigningUtils.makeBytesUrlSafe(signatureBytes);
Expand Down
8 changes: 5 additions & 3 deletions ...src/main/java/software/amazon/awssdk/services/cloudfront/internal/utils/SigningUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import software.amazon.awssdk.services.cloudfront.internal.auth.Rsa;
import software.amazon.awssdk.utils.IoUtils;
import software.amazon.awssdk.utils.StringUtils;
import software.amazon.awssdk.utils.Validate;

@SdkInternalApi
public final class SigningUtils {
Expand Down Expand Up @@ -175,12 +176,13 @@ public static String buildCustomPolicyForSignedUrl(String resourceUrl,
Instant activeDate,
Instant expirationDate,
String limitToIpAddressCidr) {
if (expirationDate == null) {
throw SdkClientException.create("Expiration date must be provided to sign CloudFront URLs");
}

Validate.notNull(expirationDate, "Expiration date must be provided to sign CloudFront URLs");

if (resourceUrl == null) {
resourceUrl = "*";
}

return buildCustomPolicy(resourceUrl, activeDate, expirationDate, limitToIpAddressCidr);
}

Expand Down
32 changes: 29 additions & 3 deletions ...t/src/main/java/software/amazon/awssdk/services/cloudfront/model/CustomSignerRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.annotations.ThreadSafe;
import software.amazon.awssdk.services.cloudfront.internal.utils.SigningUtils;
import software.amazon.awssdk.utils.Validate;
import software.amazon.awssdk.utils.builder.CopyableBuilder;
import software.amazon.awssdk.utils.builder.ToCopyableBuilder;

Expand All @@ -35,21 +36,22 @@
@SdkPublicApi
public final class CustomSignerRequest implements CloudFrontSignerRequest,
ToCopyableBuilder<CustomSignerRequest.Builder, CustomSignerRequest> {

private final String resourceUrl;
private final PrivateKey privateKey;
private final String keyPairId;
private final Instant expirationDate;
private final Instant activeDate;
private final String ipRange;
private final String resourceUrlPattern;

private CustomSignerRequest(DefaultBuilder builder) {
this.resourceUrl = builder.resourceUrl;
this.resourceUrl = Validate.notNull(builder.resourceUrl, "resourceUrl must not be null");
this.privateKey = builder.privateKey;
this.keyPairId = builder.keyPairId;
this.expirationDate = builder.expirationDate;
this.activeDate = builder.activeDate;
this.ipRange = builder.ipRange;
this.resourceUrlPattern = builder.resourceUrlPattern;
}

/**
Expand Down Expand Up @@ -99,6 +101,10 @@ public String ipRange() {
return ipRange;
}

public String resourceUrlPattern() {
return resourceUrlPattern;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -114,7 +120,8 @@ public boolean equals(Object o) {
&& Objects.equals(keyPairId, cookie.keyPairId)
&& Objects.equals(expirationDate, cookie.expirationDate)
&& Objects.equals(activeDate, cookie.activeDate)
&& Objects.equals(ipRange, cookie.ipRange);
&& Objects.equals(ipRange, cookie.ipRange)
&& Objects.equals(resourceUrlPattern, cookie.resourceUrlPattern);
}

@Override
Expand All @@ -125,6 +132,7 @@ public int hashCode() {
result = 31 * result + (expirationDate != null ? expirationDate.hashCode() : 0);
result = 31 * result + (activeDate != null ? activeDate.hashCode() : 0);
result = 31 * result + (ipRange != null ? ipRange.hashCode() : 0);
result = 31 * result + (resourceUrlPattern != null ? resourceUrlPattern.hashCode() : 0);
return result;
}

Expand Down Expand Up @@ -179,6 +187,16 @@ public interface Builder extends CopyableBuilder<CustomSignerRequest.Builder, Cu
* IPv6 format is not supported.
*/
Builder ipRange(String ipRange);

/**
* Configure the resource URL pattern to be used in the policy
* <p>
* For custom policies, this specifies the URL pattern that determines which files
* can be accessed with this signed URL. This can include wildcard characters (*) to
* grant access to multiple files or paths. If not specified, the resourceUrl value
* will be used in the policy.
*/
Builder resourceUrlPattern(String resourceUrlPattern);
}

private static final class DefaultBuilder implements Builder {
Expand All @@ -188,6 +206,7 @@ private static final class DefaultBuilder implements Builder {
private Instant expirationDate;
private Instant activeDate;
private String ipRange;
private String resourceUrlPattern;

private DefaultBuilder() {
}
Expand All @@ -199,6 +218,7 @@ private DefaultBuilder(CustomSignerRequest request) {
this.expirationDate = request.expirationDate;
this.activeDate = request.activeDate;
this.ipRange = request.ipRange;
this.resourceUrlPattern = request.resourceUrlPattern;
}

@Override
Expand Down Expand Up @@ -243,6 +263,12 @@ public Builder ipRange(String ipRange) {
return this;
}

@Override
public Builder resourceUrlPattern(String resourceUrlPattern) {
this.resourceUrlPattern = resourceUrlPattern;
return this;
}

@Override
public CustomSignerRequest build() {
return new CustomSignerRequest(this);
Expand Down
118 changes: 117 additions & 1 deletion ...t/java/software/amazon/awssdk/services/cloudfront/CloudFrontUtilitiesIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
Expand Down Expand Up @@ -74,9 +75,12 @@

public class CloudFrontUtilitiesIntegrationTest extends IntegrationTestBase {
private static final Base64.Encoder ENCODER = Base64.getEncoder();
private static final String RESOURCE_PREFIX = "do-not-delete-cf-test-";
private static final String RESOURCE_PREFIX = "do-not-delete-cf-test-v2";
private static final String CALLER_REFERENCE = UUID.randomUUID().toString();
private static final String S3_OBJECT_KEY = "s3ObjectKey";
private static final String S3_OBJECT_KEY_ON_SUB_PATH = "foo/specific-file";
private static final String S3_OBJECT_KEY_ON_SUB_PATH_OTHER = "foo/other-file";


private static String bucket;
private static String domainName;
Expand Down Expand Up @@ -267,6 +271,114 @@ void getCookiesForCustomPolicy_withFutureActiveDate_shouldReturn403Response() th
assertThat(response.httpResponse().statusCode()).isEqualTo(expectedStatus);
}

@Test
void getSignedUrlWithCustomPolicy_shouldAllowQueryParametersWhenUsingWildcard() throws Exception {
Instant expirationDate = LocalDate.of(2050, 1, 1)
.atStartOfDay()
.toInstant(ZoneOffset.of("Z"));

Instant activeDate = LocalDate.of(2022, 1, 1)
.atStartOfDay()
.toInstant(ZoneOffset.of("Z"));

CustomSignerRequest request = CustomSignerRequest.builder()
.resourceUrl(resourceUrl)
.privateKey(keyFilePath)
.keyPairId(keyPairId)
.resourceUrlPattern(resourceUrl + "*")
.activeDate(activeDate)
.expirationDate(expirationDate)
.build();

SignedUrl signedUrl = cloudFrontUtilities.getSignedUrlWithCustomPolicy(request);

String urlWithDynamicParam = signedUrl.url() + "&foo=bar";
URI modifiedUri = URI.create(urlWithDynamicParam);


SdkHttpClient client = ApacheHttpClient.create();
HttpExecuteResponse response = client.prepareRequest(HttpExecuteRequest.builder()
.request(SdkHttpRequest.builder()
.encodedPath(modifiedUri.getRawPath() + "?" + modifiedUri.getRawQuery())
.host(modifiedUri.getHost())
.method(SdkHttpMethod.GET)
.protocol("https")
.build())
.build()).call();
assertThat(response.httpResponse().statusCode()).isEqualTo(200);
}

@Test
void getSignedUrlWithCustomPolicy_wildCardPath() throws Exception {
String resourceUri = "https://" + domainName;
Instant expirationDate = LocalDate.of(2050, 1, 1)
.atStartOfDay()
.toInstant(ZoneOffset.of("Z"));

Instant activeDate = LocalDate.of(2022, 1, 1)
.atStartOfDay()
.toInstant(ZoneOffset.of("Z"));

CustomSignerRequest request = CustomSignerRequest.builder()
.resourceUrl(resourceUri + "/foo/specific-file")
.privateKey(keyFilePath)
.keyPairId(keyPairId)
.resourceUrlPattern(resourceUri + "/foo/*")
.activeDate(activeDate)
.expirationDate(expirationDate)
.build();

SignedUrl signedUrl = cloudFrontUtilities.getSignedUrlWithCustomPolicy(request);


URI modifiedUri = URI.create(signedUrl.url().replace("/specific-file","/other-file"));
SdkHttpClient client = ApacheHttpClient.create();
HttpExecuteResponse response = client.prepareRequest(HttpExecuteRequest.builder()
.request(SdkHttpRequest.builder()
.encodedPath(modifiedUri.getRawPath() + "?" + modifiedUri.getRawQuery())
.host(modifiedUri.getHost())
.method(SdkHttpMethod.GET)
.protocol("https")
.build())
.build()).call();
assertThat(response.httpResponse().statusCode()).isEqualTo(200);
}

@Test
void getSignedUrlWithCustomPolicy_wildCardPolicyResource_allowsAnyPath() throws Exception {
Instant expirationDate = LocalDate.of(2050, 1, 1)
.atStartOfDay()
.toInstant(ZoneOffset.of("Z"));

Instant activeDate = LocalDate.of(2022, 1, 1)
.atStartOfDay()
.toInstant(ZoneOffset.of("Z"));

CustomSignerRequest request = CustomSignerRequest.builder()
.resourceUrl(resourceUrl)
.privateKey(keyFilePath)
.keyPairId(keyPairId)
.resourceUrlPattern("*")
.activeDate(activeDate)
.expirationDate(expirationDate)
.build();

SignedUrl signedUrl = cloudFrontUtilities.getSignedUrlWithCustomPolicy(request);


URI modifiedUri = URI.create(signedUrl.url().replace("/s3ObjectKey","/foo/other-file"));
SdkHttpClient client = ApacheHttpClient.create();
HttpExecuteResponse response = client.prepareRequest(HttpExecuteRequest.builder()
.request(SdkHttpRequest.builder()
.encodedPath(modifiedUri.getRawPath() + "?" + modifiedUri.getRawQuery())
.host(modifiedUri.getHost())
.method(SdkHttpMethod.GET)
.protocol("https")
.build())
.build()).call();
assertThat(response.httpResponse().statusCode()).isEqualTo(200);
}

private static void initStaticFields() throws Exception {
initializeKeyFileAndPair();
originAccessId = getOrCreateOriginAccessIdentity();
Expand Down Expand Up @@ -409,7 +521,11 @@ private static String getOrCreateBucket() throws IOException {
s3Client.waiter().waitUntilBucketExists(r -> r.bucket(newBucketName));

File content = new RandomTempFile("testFile", 1000L);
File content2 = new RandomTempFile("testFile2", 500L);
s3Client.putObject(PutObjectRequest.builder().bucket(newBucketName).key(S3_OBJECT_KEY).build(), RequestBody.fromFile(content));
s3Client.putObject(PutObjectRequest.builder().bucket(newBucketName).key(S3_OBJECT_KEY_ON_SUB_PATH).build(), RequestBody.fromFile(content2));
s3Client.putObject(PutObjectRequest.builder().bucket(newBucketName).key(S3_OBJECT_KEY_ON_SUB_PATH_OTHER).build(), RequestBody.fromFile(content2));


String bucketPolicy = "{\n"
+ "\"Version\":\"2012-10-17\",\n"
Expand Down
Loading
Loading