reactiverse / aws-sdk

Using vertx-client for AWS SDK v2
Apache License 2.0
49 stars 14 forks source link

S3 list request signature verification failure due to missing query parameters in request #51

Closed ajthomp2 closed 4 years ago

ajthomp2 commented 4 years ago

I was receiving a signature verification exception when trying to make an S3 list request. Making the request with both this Vert.x async client and the built-in client that AWS provides, I noticed that the request using Vert.x was missing the query parameters

Vert.x

GET / HTTP/1.1
amz-sdk-invocation-id: 8cd60990-8554-bb8c-3683-cd004c6c0c00
....

Built-in async client

GET /?prefix=some-prefix%2F HTTP/1.1
amz-sdk-invocation-id: 78f057b3-2e8a-e93b-213b-fb5ee1d31071
....

This fix to the VertxNioAsyncHttpClient worked:

    private static RequestOptions getRequestOptions(SdkHttpRequest request) {
        return new RequestOptions()
                .setHost(request.host())
                .setPort(request.port())
                .setURI(createRelativeUri(request.getUri()))
                .setSsl("https".equals(request.protocol()));
    }

    private static String createRelativeUri(URI uri) {
        return (StringUtils.isEmpty(uri.getPath()) ? "/" : uri.getPath()) +
                // AWS requires query parameters to be encoded as defined by RFC 3986.
                // see https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
                // uri.toASCIIString() returns the URI encoded in this manner
                (StringUtils.isEmpty(uri.getQuery()) ? "" : "?" + Strings.split(uri.toASCIIString(), "?")[1]);
    }
aesteve commented 4 years ago

Thanks for reporting and providing a fix!

I'll see when I have some time available to test it.

Do you have a way to reproduce this locally? S3-Localstack integration tests didn't catch it unfortunately.

ajthomp2 commented 4 years ago

I haven't been able to reproduce with Localstack. I my project, I reproduced by calling ListObjectsV2 with maxKeys set. maxKeys becomes a query parameter on the GET request and was causing the signature verification to fail. This test should fail, but unfortunately doesn't.

    @Test
    @Order(4)
    void listObjectsV2(Vertx vertx, VertxTestContext ctx) throws Exception {
      final Context originalContext = vertx.getOrCreateContext();
      final S3AsyncClient s3 = s3(originalContext);
      single(s3.putObject(b -> putObjectReq(b, "obj1"), AsyncRequestBody.fromString("hello")))
        .flatMap(putObjectResponse1 -> single(s3.putObject(b -> putObjectReq(b, "obj2"), AsyncRequestBody.fromString("hi"))))
        .flatMap(putObjectResponse2 -> single(s3.listObjectsV2(VertxS3ClientSpec::listObjectsV2Req))
          .flatMap(listObjectsV2Response1 -> single(s3.listObjectsV2(b -> listObjectsV2ReqWithContToken(b, listObjectsV2Response1.nextContinuationToken())))
            .map(listObjectsV2Response2 -> {
              List<S3Object> allObjects = new ArrayList<>(listObjectsV2Response1.contents());
              allObjects.addAll(listObjectsV2Response2.contents());
              return allObjects;
            })
        ))
        .subscribe(allObjects -> {
          ctx.verify(() -> {
            assertEquals(3, allObjects.size());
            ctx.completeNow();
          });
        }, ctx::failNow);
    }

    private static ListObjectsV2Request.Builder listObjectsV2Req(ListObjectsV2Request.Builder lovr) {
      return  lovr.maxKeys(2).bucket(BUCKET_NAME);
    }

    private static ListObjectsV2Request.Builder listObjectsV2ReqWithContToken(ListObjectsV2Request.Builder lovr, String token) {
      return lovr.maxKeys(2).bucket(BUCKET_NAME).continuationToken(token);
    }

    private static PutObjectRequest.Builder putObjectReq(PutObjectRequest.Builder por, String key) {
      return por.bucket(BUCKET_NAME).key(key);
    }

Also, made a small edit to my comment above to URI encode the query parameter as per AWS signature docs.

aesteve commented 4 years ago

Thanks a LOT for trying. I’ll give a try over the weekend. Maybe there is another service using query parameters or maybe upgrading localstack would help.

Thanks for your help

Le ven. 6 nov. 2020 à 19:07, Alex Thompson notifications@github.com a écrit :

I haven't been able to reproduce with Localstack. I my project, I reproduced by calling ListObjectsV2 with maxKeys set. maxKeys becomes a query parameter on the GET request and was causing the signature verification to fail. This test should fail, but unfortunately doesn't.

@Test
@Order(4)
void listObjectsV2(Vertx vertx, VertxTestContext ctx) throws Exception {
  final Context originalContext = vertx.getOrCreateContext();
  final S3AsyncClient s3 = s3(originalContext);
  single(s3.putObject(b -> putObjectReq(b, "obj1"), AsyncRequestBody.fromString("hello")))
    .flatMap(putObjectResponse1 -> single(s3.putObject(b -> putObjectReq(b, "obj2"), AsyncRequestBody.fromString("hi"))))
    .flatMap(putObjectResponse2 -> single(s3.listObjectsV2(VertxS3ClientSpec::listObjectsV2Req))
      .flatMap(listObjectsV2Response1 -> single(s3.listObjectsV2(b -> listObjectsV2ReqWithContToken(b, listObjectsV2Response1.nextContinuationToken())))
        .map(listObjectsV2Response2 -> {
          List<S3Object> allObjects = new ArrayList<>(listObjectsV2Response1.contents());
          allObjects.addAll(listObjectsV2Response2.contents());
          return allObjects;
        })
    ))
    .subscribe(allObjects -> {
      ctx.verify(() -> {
        assertEquals(3, allObjects.size());
        ctx.completeNow();
      });
    }, ctx::failNow);
}

Also, made a small edit to my comment above to URI encode the query parameter as per AWS signature docs.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/reactiverse/aws-sdk/issues/51#issuecomment-723222537, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALDRPJX62AKM6T6BKBGTV3SOQ3O5ANCNFSM4TLSKW4Q .

aesteve commented 4 years ago

I was able to reproduce your issue after upgrading:

Thanks a ton for your reproducer, it was indeed failing (not with the signature issue, but the maxkeys parameter was indeed ignored!!). Using the fix in your first post indeed fixed the test and made it pass.

Again, a HUGE thank you for your help. Perfect issue report.