probot / adapter-aws-lambda-serverless

An extension for running Probot on Lambda
ISC License
93 stars 36 forks source link

README improvments proposal #61

Closed axel3rd closed 3 years ago

axel3rd commented 3 years ago

Following #58, README content is nice for a Probot v11 from scratch.

Here a draft (I will do a PR after acceptance/remarks) for (potentials) README improvments on 3 themes:

  1. Synthesis of server-less adapter changes for Probot prior v11 migration (it could be quite easy to forget an item when you are migrating from Probot <=v10)
  2. Lambda endpoint URL
  3. Smoke test with Probot v11 (Pain point for to propose a PR)

//1. This new part between 'Examples' and 'LICENSE'

Probot v11 migration key points

For Probot v11 support, this adapter introduces significant changes. Here the key points to update (in addition of Probot v11 breaking changes):

Key point / Probot <= v10 >= v11
NPM package name @probot/serverless-lambda @probot/adapter-aws-lambda-serverless
handler.js content See Usage v1.x See Usage
AWS Lambda Runtime handler.probot handler.webhooks
AWS Lambda Handler Node.js 12.x (preferred) Node.js 12.x (required)

//2. Lambda endpoint URL

Make sure to configure your GitHub App registration's webhook URL to <your lambda's URL>/api/github/webhooks

The /api/github/webhooks is quite disturbing ... from Probot v10 usage it is like we need to add this URL suffix (but no).

I propose to remove it and explain that the lambda's URL is the invoke URL for POST verb, which can be found in the Stages configuration from the Amazon API Gateway console.


//3. Smoke test with Probot v11

This adapter is amazing, the best way to deploy stateless Probot 😗.

But with the matrix AwsNodeRuntime + Probot version + Serverless adapter version, it is sometime hard to troubleshoot a problem, and end2end configuration with a GitHub is not the simplest.

Having the way todo a smoke test could be nice. With Probot v10, this one worked fine:

$ curl --request POST --header "X-Github-Event: test" --data "{\"action\": \"test\"}"  https://x.execute-api.y.amazonaws.com/github-probots/sample-probot
{"message":"Received test.test"}

With Probot v11, we should deal with X-Hub-Signature header 😢 (Thanks to Securing your webhooks ^^).

I would like provide a snippet like that, but I haven't succeeded at this time (is it achievable with openssl dgst ?) ... if anybody has some advices, many thanks !

$ LAMBDA_URL=https://x.execute-api.y.amazonaws.com/github-probots/sample-probot
$ TMP_DATA_FILE=/tmp/smoke.data
$
$ echo "{\"action\": \"test\"}" > $TMP_DATA_FILE
$ SIGN=$(openssl dgst -sha1 -hmac $HERE_THE_WEBHOOK_SECRET_??_OR_NOT $TMP_DATA_FILE | cut -d" " -f2)
$ curl --request POST --header "X-Hub-Signature: sha1=$SIGN" --header "X-Github-Event: test" --data-binary "@$TMP_DATA_FILE" $LAMBDA_URL
{"message":"Received test.test"}
gr2m commented 3 years ago

Thank you Alix, I greatly appreciate help with improving the README. I'm not very familiar with AWS Lambda or Serverless myself, I'm sure there is lots to improve.

The /api/github/webhooks is quite disturbing ... from Probot v10 usage it is like we need to add this URL suffix (but no).

At some point in future, I want to change Probot's webhook URL from POST / to POST /api/github/webhooks for two reasons

  1. Making it easier to have a GET / route for a custom UI. In many cases I just want to show a static index.html at the / root path. It can be quite cumbersome to make the / root path work for both, showing static assets (GET /) and receiving webhooks (POST /)
  2. Align with @octokit (https://github.com/octokit/app.js/#middlewares)

Maybe there is something we can do to help with the transition as part of this adapter, but ultimately I want to move away from the POST / route to receive webhooks

Having the way todo a smoke test could be nice. With Probot v10, this one worked fine:

$ curl --request POST --header "X-Github-Event: test" --data "{\"action\": \"test\"}"  https://x.execute-api.y.amazonaws.com/github-probots/sample-probot
{"message":"Received test.test"}

I'm not familiar with how this worked before. Was POST /github-probots/sample-probot a custom route? If so, there should be no need for signature verification, it does not go through the webhook handler?

If we need to sign a payload, we could export a binary which sends the request? We can use probot.webhooks.sign(payload). That should be much simpler and would work across all operating systems?

What would the smoke test do, exactly? How did it work before?

GitHub
octokit/app.js
GitHub Apps toolset for Node.js. Contribute to octokit/app.js development by creating an account on GitHub.
axel3rd commented 3 years ago

@gr2m : Thanks for your reply.

At some point in future, I want to change Probot's webhook URL from POST / to POST /api/github/webhooks for two reasons

I definitely understand the given reasons.

But having more than one / route for Lambda endpoint complexify the Swagger to produce for AWS API Gateway (currently the most simple way to expose URL for Lambda, specially when Lambda is on private VPC-subnet to communicate with GitHub GHE). I agree this is not part of this adapter and linked to network exposition, but could be hard for users not familiar with AWS API Gateway 🤣 (AWS Lambda deployment is quite simple).


is it achievable with openssl dgst ?

Yes, because this Javascript code:

const { sign } = require('@octokit/webhooks')
const fs = require('fs')

const data = fs.readFileSync('/tmp/smoke.data', 'utf8')
console.log(sign('some-secret', data))

And

openssl dgst -sha1 -hmac some-secret /tmp/smoke.data

produce the same output.


I'm not familiar with how this worked before. Was POST /github-probots/sample-probot a custom route? If so, there should be no need for signature verification, it does not go through the webhook handler?

This /github-probots/sample-probot route is the sample of my Lambda endpoint, and this Probot (migrated to v11) is working fine.

With Probot v11 using this serverless adapter v2 the signature seems required for every payload (error http 500 otherwise). It is not a nightmare to use it even in a smoke test, but I stumble on something perhaps trivial about the secret 🤔.

What would the smoke test do, exactly? How did it work before?

For an existing Probot v10 + this serverless adapter 1.x which is working fine:

$ curl -X POST --header "X-Github-Event: test" --data "{\"action\": \"test\"}" https://xxxxxx.execute-api.eu-west-3.amazonaws.com/github-probot/probot-xxxxx

{"message":"Received test.test"}

=> Even if "functionnal" behavior is not tested (to simple payload), this call validates the Probot is correctly deployed.

If we need to sign a payload, we could export a binary which sends the request?

Fully agree with that, but using a Probot v11 which is working fine

  1. Reproduce in any REST tools a Webhook call is working fine (data retrieved from GitHub App config "Deliveries" webhook), proof more simple with a picture:

image

  1. Signing the same payload with the Probot secret doesn't produce same SHA1 ; I know my Probot app secret otherwise it can't work ... except if I become crazy 😆.

So I'm not sur the "secret" to use (for sign) it the WEBHOOK_SECRET 🤔.

I'm going to let the weekend go by and resume it with a rested head after going outside for a bit, it should be better.

gr2m commented 3 years ago
1. Signing the same payload with the Probot secret doesn't produce same SHA1

Make sure you sign the payload of the same JSON string (with the same spaces/new lines) as in the request body.

{"action": "test"}

is not the same as

{
  "action": "test"
}
axel3rd commented 3 years ago

Make sure you sign the payload of the same JSON string (with the same spaces/new lines) as in the request body.

Good catch, thanks you. Debugging probot.webhooks.verifyAndReceive and writing event.payload in a file, there are no new line and no spaces, GitHub UI delivery representation is nice but quite confusing in this case 😁.

=> Correct smoke test is:

$ LAMBDA_URL=https://x.execute-api.y.amazonaws.com/github-probots/sample-probot
$ SECRET=the_webhook_secret
$ TMP_DATA_FILE=/tmp/smoke.data

$ echo -n "{\"action\":\"test\"}" > $TMP_DATA_FILE
$ SIGN=$(openssl dgst -sha1 -hmac $SECRET $TMP_DATA_FILE | cut -d" " -f2)
$ curl --request POST --header "X-Hub-Signature: sha1=$SIGN" --header "X-Github-Event: test" --data-binary "@$TMP_DATA_FILE" $LAMBDA_URL
{"ok":true}        <-- Concent for Probot v10: {"message":"Received test.test"}

At some point in future, I want to change Probot's webhook URL from POST / to POST /api/github/webhooks for two reasons

Is it possible to share the produced swagger.yml of example-aws-lambda-serverless ? From AWS API Gateway console > Stages > Export as Swagger + API Gateway Extensions > YAML.

I'm quite curious of x-amazon-apigateway-integration definition to support GET / and POST /api/github/webhooks. It could be nice to provide (in my PR) a swagger.yml template/sample compatible with AWS API Gateway without Serverless Framwork, it is not the only way to deploy something on AWS (could be done via ansible, etc).

gr2m commented 3 years ago

Is it possible to share the produced swagger.yml of example-aws-lambda-serverless ?

I hope I did this right:

openapi: "3.0.1"
info:
  title: "dev-example-aws-lambda-serverless"
  version: "2021-02-22 19:48:33UTC"
servers:
- url: "https://g3pd272da7.execute-api.us-east-1.amazonaws.com/{basePath}"
  variables:
    basePath:
      default: ""
paths:
  /api/github/webhooks:
    post:
      responses:
        default:
          description: "Default response for POST /api/github/webhooks"
      x-amazon-apigateway-integration:
        payloadFormatVersion: "2.0"
        type: "aws_proxy"
        httpMethod: "POST"
        uri: "arn:aws:apigateway:us-east-1:lambda:path/2015-03-31/functions/arn:aws:lambda:us-east-1:152606098741:function:example-aws-lambda-serverless-dev-webhooks/invocations"
        connectionType: "INTERNET"
        timeoutInMillis: 6500
x-amazon-apigateway-importexport-version: "1.0"

It could be nice to provide (in my PR) a swagger.yml template/sample compatible with AWS API Gateway without Serverless Framwork, it is not the only way to deploy something on AWS (could be done via ansible, etc).

I'd be happy to create another example using ansible. I just never used it myself.

I'm quite curious of x-amazon-apigateway-integration definition to support GET / and POST /api/github/webhooks.

The current example app does not respond to GET / at all: https://g3pd272da7.execute-api.us-east-1.amazonaws.com/api/github/webhooks

axel3rd commented 3 years ago

I'd be happy to create another example using ansible. I just never used it myself.

I have developed (in my company) an Ansible role to deploy stateless Probot (from Git repo endpoint) on AWS Lambda. It is company oriented (proxy usage, naming conventions ...) so requires some work to be open-sourced, but could perhaps be a good idea (middle term roadmap, need time todo that if relevant).


The current example app does not respond to GET / at all:

  1. Align with @octokit (https://github.com/octokit/app.js/#middlewares)

To have "multiple" responses from Lambda (Probot doc from GET / and webhook from POST /api/github/webhooks), using /{proxy+} seems required to catch all (http) resources. I will try to understand it.


I hope I did this right: [swagger/openapi]

I have the ~same (I was not sure how is using the /api/github/webhooks in your sample) => I will provide a PR.

GitHub
octokit/app.js
GitHub Apps toolset for Node.js. Contribute to octokit/app.js development by creating an account on GitHub.
axel3rd commented 3 years ago

Fixed by #63