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

Add flag to AwsLambdaReceiver to enable/disable signature verification #2107

Merged
merged 5 commits into from
May 6, 2024
Merged

Add flag to AwsLambdaReceiver to enable/disable signature verification #2107

merged 5 commits into from
May 6, 2024

Conversation

noah-guillory
Copy link
Contributor

Summary

Resolves #2104

This adds a flag to the AwsLambdaReceiver so that signature verification can be disabled, similar to the HTTPReceiver. This allows callers to disable signature verification if they already have something upstream of their Lambda function using the Bolt SDK performing signature verification.

Requirements (place an x in each [ ])

@filmaj filmaj self-assigned this May 6, 2024
@filmaj filmaj added this to the 3.19.0 milestone May 6, 2024
@filmaj
Copy link
Contributor

filmaj commented May 6, 2024

Really appreciate you sending this PR @noah-guillory !

Had some issues this morning with some tests failing due to some transient dependency versions floating around, but those failures on main should be resolve now. If you rebase or merge the latest main into this branch (I think you just did that!) then we should see the tests passing.

@noah-guillory
Copy link
Contributor Author

Really appreciate you sending this PR @noah-guillory !

Had some issues this morning with some tests failing due to some transient dependency versions floating around, but those failures on main should be resolve now. If you rebase or merge the latest main into this branch (I think you just did that!) then we should see the tests passing.

No problem! And yeah I noticed that my fork was out of date so I did merge in main. Looks like most of the CI is working now too, other than the Node 20 failing on the upload to Codecov step.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Looks great! Left a few comments about property optionality and documenting the interface properties.

Also, don't worry about the node v20 tests failing - seems like that is due to this PR coming from your fork and that somehow not jiving with the codecov coverage upload reporting (i'll try to figure out how to avoid that problem when I have time this week)

@@ -40,9 +40,10 @@ export interface AwsResponse {
export type AwsHandler = (event: AwsEvent, context: any, callback: AwsCallback) => Promise<AwsResponse>;

export interface AwsLambdaReceiverOptions {
signingSecret: string;
signingSecret?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, in the interest of keeping it consistent with the HTTPReceiver, let's not make this property optional. FWIW I agree with your approach - if you turn off signature verification, there's no point in storing the signing secret - but there's a more accurate and exact approach we can use to express this in TypeScript, though it will be a breaking change (which we should wait for bolt v4; I was thinking we could use a union expressing different sets of options when signatureVerification is true vs. false).

Anywho, that's just musings for the future. In the mean time, let's just keep it consistent with HTTPReceiver and keep this property as required. I think this also reduces the chance that someone accidentally forgets to add their signing secret and they end up hitting the "Invalid Signature" error at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, made it required in 3fd0b3d.

I was thinking we could use a union expressing different sets of options when signatureVerification is true vs. false).

Yeah I think that would be a great approach too. As a consumer, I'd only want to be able to represent a single state of how I want signature verification configured. I've been on this kind of kick lately after reading this article on HN the other day: https://geeklaunch.io/blog/make-invalid-states-unrepresentable/

Copy link
Contributor

Choose a reason for hiding this comment

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

Damn that is an awesome post, this part particularly resonated with me:

In order to reduce bugs, we therefore need to minimize this gap, and to do that, we can either:

increase the number of cases handled by the code, or,
decrease the number of representable states.
Increasing the number of cases handled by the code tends to increase its complexity, and complex code also has a high tendency to be buggy if you’re not careful. Therefore, in this post, we’re covering the latter strategy: decreasing the number of representable states.

I love the way this is framed: reducing complexity by decreasing the number of representable states. 🤯

logger?: Logger;
logLevel?: LogLevel;
signatureVerification?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, do you mind adding some JSDoc descriptors for what these properties mean? Would be a nice touch for developers to see that as they edit their code in their IDEs supporting autocomplete.

You don't have to do it for all the properties here, but at least for the ones you are changing, that would be much appreciated ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a stab at adding JSDocs for everything. Let me know if any of them aren't quite correct!

private customPropertiesExtractor: (request: AwsEvent) => StringIndexed;

public constructor({
signingSecret,
signingSecret = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this default as well. This should be explicitly set by the dev, even if the intention is to disable verification. Disabling signature verification is very dangerous, as then outsiders could maliciously spoof Slack events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, sure thing. Removed!

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.03%. Comparing base (06b0f61) to head (dd5bf1d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2107      +/-   ##
==========================================
+ Coverage   81.99%   82.03%   +0.03%     
==========================================
  Files          18       18              
  Lines        1533     1536       +3     
  Branches      440      442       +2     
==========================================
+ Hits         1257     1260       +3     
  Misses        178      178              
  Partials       98       98              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

So good! Thanks a lot 🙇
Will merge as soon as the tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add flag to AwsLambdaReceiver to disable Signature Verification
2 participants