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

SNS - SES Notification - Invalid signature if special characters are present #2340

Closed
jplippi opened this issue May 28, 2020 · 8 comments
Closed
Assignees
Labels
bug This issue is a bug. closed-for-staleness needs-reproduction This issue needs reproduction. p3 This is a minor priority issue

Comments

@jplippi
Copy link

jplippi commented May 28, 2020

When receiving SES notifications through an HTTP/S endpoint subscribed to a SNS topic, the SDK cannot validate the signature if the string to sign contains special characters, such as á or ç

Describe the bug

Signature verification fails when the string to sign resulting of the notification contains special characters. Since the string signature contains the email subject, the subject is susceptible to contain special characters

Expected Behavior

Signature validation passes

Current Behavior

Signature validation fails

Steps to Reproduce

  • Create a SNS topic and subscribe to a Java web application endpoint
  • Configure SES to have a Configuration Set with an SNS destination and Bounce event type enabled
  • Send an email through SES SDK with the pre-configured Configuration Set, and with special characters in the Subject field
  • Use SNS SDK to handle the message:
    SnsMessageManager().handleMessage(inputStream, SnsMessageHandler())
  • Verify that SnsMessageHandler will throw an exception SdkClientException("Signature in SNS message was invalid")

Context

When trying to verify the signature of the SNS message to handle email bounces and complaints, signature validation would fail only for emails that contained special characters on the subject field

Your Environment

  • AWS Java SDK version used: 1.11.791
  • JDK version used: 1.8
  • Operating System and version: Windows 10 Pro 64-bit (10.0, Build 18362)
@jplippi jplippi added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 28, 2020
@debora-ito
Copy link
Member

@jplippi what version of Apache httpclient are you using?

We had issues with httpclient changing its behavior in version 4.5.7 and affecting special characters (see #1919), just want to eliminate this possibility before doing a deep dive in the problem.

@debora-ito debora-ito added response-requested Waiting on additional info or feedback. Will move to "closing-soon" in 5 days. and removed needs-triage This issue or PR still needs to be triaged. labels May 29, 2020
@jplippi
Copy link
Author

jplippi commented May 29, 2020

@debora-ito httpclient version 4.5. I just did some testing with 4.5.12 as well, and had the same problem

@github-actions github-actions bot removed the response-requested Waiting on additional info or feedback. Will move to "closing-soon" in 5 days. label May 29, 2020
@debora-ito
Copy link
Member

Can you provide a code sample that I can use to reproduce?

@debora-ito debora-ito added the response-requested Waiting on additional info or feedback. Will move to "closing-soon" in 5 days. label May 29, 2020
@jplippi
Copy link
Author

jplippi commented May 29, 2020

@debora-ito I've made a simple Tomcat Web Application, since this is the usecase that I currently have:
https://github.com/jplippi/aws-sns-bug

I hope this helps!

@github-actions github-actions bot removed the response-requested Waiting on additional info or feedback. Will move to "closing-soon" in 5 days. label May 29, 2020
@jplippi
Copy link
Author

jplippi commented May 29, 2020

Ok, I made a simple unit test method that didn't reproduce the issue. The validation was successful through a unit test, but not with an HTTP request to the web application.

I put a breakpoint inside SignatureChecker.verifySignature() and even though the String content for the parameter message is the same, the method message.getBytes() returns two different sized byte arrays in each case.

When using the unit test, I got a byte[1570], but when using the API endpoint, got a byte[1560].

I noticed that if I specify a charset whtn using message.getBytes() I can reproduce the behavior:

message.getBytes(Charsets.ISO_8859_1) returns a byte[1560]
message.getBytes(Charsets.UTF_8) returns a byte[1570]

So I believe an explicit Charset has to be provided for the signature checker.

I don't know if it falls within the scope of the SDK or if I should handle the encoding myself before sending the InputStream

@jplippi
Copy link
Author

jplippi commented May 29, 2020

Ok, I verified it to be the case:
When running an unit test, Charset.defaultCharset() returns "UTF-8", but when running the application from Tomcat, Charset.defaultCharset() returns "windows-1252".

Since a specific encoding is required for the signature verifiction, I believe it has to be explicitly set within the SDK

@debora-ito debora-ito added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Jun 1, 2020
@debora-ito
Copy link
Member

Quick update: I'm still working on the repro case set up, will work on the issue this week.

@debora-ito debora-ito self-assigned this Jun 19, 2020
@debora-ito debora-ito added needs-reproduction This issue needs reproduction. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Oct 12, 2020
@debora-ito
Copy link
Member

@jplippi apologies for losing track of this.

Since you identified the issue was in the encoding of the signature checker, at this point I'll recommend you add the encoding to your application. If we add the encoding to the SDK, there's a risk we'll be double-encoding the content for customers who already added the encoding to their code, and this will be a breaking change.

@debora-ito debora-ito added p3 This is a minor priority issue closing-soon This issue will close in 2 days unless further comments are made. labels Mar 25, 2023
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will close in 2 days unless further comments are made. labels Mar 27, 2023
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. closed-for-staleness needs-reproduction This issue needs reproduction. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

2 participants