Verify sns message signature #198
Conversation
There was a problem hiding this comment.
@WtfJoke thanks for contribution!
I have an issue that we should address to have PR ready for Merge
| @@ -46,8 +51,15 @@ | |||
|
|
|||
| private final MessageConverter payloadConverter; | |||
|
|
|||
| private final SnsMessageManager snsMessageManager; | |||
There was a problem hiding this comment.
This will cause an issue if .aws/config has different region than properties for sns specific.
this.regionProvider = properties.getRegion() == null ? regionProvider.getIfAvailable() : new StaticRegionProvider(properties.getRegion());
Is used while configurating SNSclient, (meaning region in properties takes precedence over config file). Which means if we have in a file eu-central-1 and in properties if we define cloud.aws.sns.region: us-east-1 verification will always fail. (Client will use us-east-1 and SnsMessageManager will use eu-central-1)
My recommendation would be create SnsMessageManager and pass it through constructor (will require refactoring in some other classes as well since it will have to be passed through few constructors).
Or second option pass region and use properties region to configure SnsMessageManager.
There was a problem hiding this comment.
@maciejwalkowiak First of nice find.
I know its been a while, but I dont understand the recommended approach fully 😅. SnsMessageManager has the default constructor (which uses the wrong region) and the one where I pass the region. So I should use that one.
How and where can I inject the correct region? I suppose I shouldnt duplicate the code in the SnsAutoConfiguration, right? Or should I simply inject SNSProperties and use this region (but which region provider do I use if the property is null, the default one)?
There was a problem hiding this comment.
Sorry, will try to explain the approach in more detail.
Personally, I would create SnsMessageManager bean in SnsAutoConfiguration and pass it to NotificationRequestConverter.
For a region use regionProvider located in SnsAutoConfiguration.
@maciejwalkowiak @eddumelendez WDYT? Could you assist further?
There was a problem hiding this comment.
Your explaination helped, thank you. I pushed the changes.
I always struggle a bit with formatting issues 😅 I've only managed to do this on my linux machine (and only when using the fully qualified plugin name eg /mvnw io.spring.javaformat:spring-javaformat-maven-plugin:0.0.31:apply
Havent yet tested the changes, but I think you still can take a look at it.
Just noticed the test failures, will take a look. Sorry 😅
There was a problem hiding this comment.
Strangely enough, all tests are green on my machine :S
EDIT: Found the cause, its the default region provided in my .aws/config
|
Just wanted to let you know that I will soon take care of the requested changes. |
19e6448 to
7386791
Compare
- Create snsMessageManager with region provided by the region provider (spring properties) - Pass snsMessageManager down to NotificationRequestConverter where it's used to verify the sns message signature
7386791 to
9a5d8bb
Compare
There was a problem hiding this comment.
I've had another look and I think we approached in from a wrong side.
Signature verification should happen for messages sent to an HTTP endpoint, not ones sent to SQS queue. This means, we do not need to verify signature in QueueMessageHandler but rather somewhere in NotificationMessageHandlerMethodArgumentResolver. SQS related code should ideally not need to change.
I know it's been a while, so is this something @WtfJoke you would be still interested in doing?
|
@maciejwalkowiak no worries, I know the struggle of the life of a maintainer :D However currently im quite occupied, so I dont want to block anybody which is interested in doing that. Feel free to close the PR. Thank you both for taking a look to the PR in the past. |
Fixes gh-176
📢 Type of change
📜 Description
Verify the signature of a SNS Message by utilizing SnsMessageManager#parseMessage which also validates the Signature using SignatureVerifier. The downside of it, it does more than just validate the Signature, but I think it doesnt hurt.
As an alternative we could also validate the Signature using SignatureChecker (aws sdk v1), but the class is deprecated and they suggest using SnsMessageManager.
In aws sdk 2 there would be a more optimal approach using SignatureChecker (aws sdk v2), but we still have aws sdk 1 here.
💡 Motivation and Context
#176
💚 How did you test it?
I'll added two junit tests, which tests two cases (invalid/valid signature).
For that I needed to add an additional constructor in order to be able to mock SnsMessageManager. I hope thats ok (appreciate comments for better ideas :)).
I also verified it, by publishing some messages in my sns topic through the aws management console.
📝 Checklist
🔮 Next steps