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

sigv4 for Lattice unsigned-payload not supported for http requests #5103

Open
shulin-sq opened this issue Apr 12, 2024 · 1 comment
Open
Labels
feature-request A feature should be added or improved. service-api This issue is due to a problem in a service API, not the SDK implementation.

Comments

@shulin-sq
Copy link

Describe the bug

When making http (not https) requests to lattice, the payload will always be signed because the signing logic will always sign the body when the protocol http and a streaming body is present. This isn't compatible with VPC Lattice which allows for GRPC over HTTP.

Expected Behavior

requests to be signed with 'x-amz-content-sha256: UNSIGNED-PAYLOAD'

Current Behavior

-H 'x-amz-content-sha256: STREAMING-AWS4-ECDSA-P256-SHA256-PAYLOAD'

Reproduction Steps

      SdkHttpFullRequest request = SdkHttpFullRequest.builder()
          .protocol("HTTP")
          .host("test.com:80")
          .method(SdkHttpMethod.POST)
          .encodedPath("/_status")
          .contentStreamProvider(() - > new ByteArrayInputStream("{\"status\":\"ok\"}".getBytes()))
          .build();
  
      ExecutionAttributes ea = new ExecutionAttributes();
      ea.putAttribute(AwsSignerExecutionAttribute.AWS_CREDENTIALS, EnvironmentVariableCredentialsProvider.create().resolveCredentials());
      ea.putAttribute(AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME, "vpc-lattice-svcs");
      ea.putAttribute(S3SignerExecutionAttribute.ENABLE_PAYLOAD_SIGNING, false);
  
      AwsCrtS3V4aSigner signer = AwsCrtS3V4aSigner.builder()
          .defaultRegionScope(RegionScope.GLOBAL)
          .build();
      
      var signedRequest = signer.sign(request, ea);
      for (var header: signedRequest.headers().entrySet()) {
        for (var value: header.getValue()) {
          System.out.print(" -H '" + header.getKey() + ": " + value + "'\n");
        }
      }

will print out some headers, notably

 -H 'x-amz-content-sha256: STREAMING-AWS4-ECDSA-P256-SHA256-PAYLOAD'

Possible Solution

In DefaultAwsCrtS3V4aSigner (and other signers) change logic to something like

    private boolean shouldSignPayload(SdkHttpFullRequest request, ExecutionAttributes executionAttributes) {
        Boolean payloadSigning =
            executionAttributes.getAttribute(S3SignerExecutionAttribute.ENABLE_PAYLOAD_SIGNING);
        if (payloadSigning != null && !booleanValue(payloadSigning)) {
            return false;
        }

        if (!request.protocol().equals("https") && request.contentStreamProvider().isPresent()) {
            return true;
        }

to prioritize user choice

or look at AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME == vpc-lattice-svcs to decide if the request body should be signed.

Additional Information/Context

No response

AWS Java SDK version used

2.21.20

JDK version used

21.0.2

Operating System and version

macOS 14.4.1

@shulin-sq shulin-sq added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 12, 2024
@debora-ito
Copy link
Member

Hi @shulin-sq

We don't have current plans to support unsigned payloads over HTTP requests - HTTP requests with payloads are signed for security reasons.

If this is something that the VPC Lattice service allows, we need to expose some kind of control mechanism in the SDK signer that the service can use, and it must be something that works across all language SDKs, not only Java. Adding a check in the code to see if the SERVICE_SIGNING_NAME is "vpc-lattice-svcs" is not easy to maintain and is not scalable.

We are in contact with the VPC Lattice team internally, we'll discuss the next steps and I'll post updates here later on.

@debora-ito debora-ito added feature-request A feature should be added or improved. service-api This issue is due to a problem in a service API, not the SDK implementation. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. service-api This issue is due to a problem in a service API, not the SDK implementation.
Projects
None yet
Development

No branches or pull requests

2 participants