Support Github rebuild via webooks. by tmcgilchrist · Pull Request #283 · ocurrent/ocurrent

tmcgilchrist · 2021-07-23T05:46:51Z

Passing job_id through to set_status, which then gets sent back via webhooks and can be looked up as a job to rebuild. The core piece is CheckRunStatus now includes the job_id

  type t = {
      state: state;
      description: string option;
      url: Uri.t option;
      job_id: string option;
    }

talex5

This looks good. However, if we're performing actions in response to webhooks (rather than just triggering another query to GitHub) then we really should be validating incoming webhooks:

https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks

We should also think a bit about access control. Who does GitHub allow to click on the rebuild button? Possibly it should be tied in with the existing has_role access control system. At least, it needs to be documented.

examples/github_app.ml

plugins/github/api.ml

plugins/github/current_github.mli

tmcgilchrist · 2021-07-26T04:31:29Z

We should also think a bit about access control. Who does GitHub allow to click on the rebuild button? Possibly it should be tied in with the existing has_role access control system. At least, it needs to be documented.

Happily we get the GH User (as sender on the Webhook) that requested the rebuild so we can filter with has_role. We don't seem to have an option to not show the Re-run button for different users.
But we can provide our own set of actions as buttons on the CheckRun.

talex5

I think the secret is needed for all web-hooks, not just for apps.

lib_web/current_web.mli

talex5 · 2021-08-11T14:31:52Z

plugins/github/current_github.ml

-let webhook = object
+let validate_webhook_payload webhook_secret body headers =
+  match webhook_secret with
+  | None -> true (* No webhook secret supplied skip the validation. *)


I think we should just require the secret in all cases.

Renamed this.

doc/examples/github_app.ml

lib_web/current_web.mli

talex5 · 2021-08-12T10:08:06Z

plugins/github/current_github.ml

+    begin match event, validate_webhook_payload webhook_secret body headers with
+      | Some "installation_repositories", true -> Installation.input_installation_repositories_webhook ()
+      | Some "installation", true -> App.input_installation_webhook ()
+      | Some ("pull_request" | "push" | "create"), true -> Api.input_webhook json_body
+      | Some "check_run", true -> Api.rebuild_webhook ~engine ~has_role json_body 
+      | Some x, b -> Log.warn (fun f -> f "Unknown GitHub event type %S or invalid X-Hub-Signature-256 %b" x b)
+      | None, _ -> Log.warn (fun f -> f "Missing GitHub event type in webhook!")
    end;


Checking for true in each case seems unnecessary. Also, the error message is rather unclear ("or invalid X-Hub-Signature-256 false").

Suggested change

begin match event, validate_webhook_payload webhook_secret body headers with

| Some "installation_repositories", true -> Installation.input_installation_repositories_webhook ()

| Some "installation", true -> App.input_installation_webhook ()

| Some ("pull_request" | "push" | "create"), true -> Api.input_webhook json_body

| Some "check_run", true -> Api.rebuild_webhook ~engine ~has_role json_body

| Some x, b -> Log.warn (fun f -> f "Unknown GitHub event type %S or invalid X-Hub-Signature-256 %b" x b)

| None, _ -> Log.warn (fun f -> f "Missing GitHub event type in webhook!")

end;

if validate_webhook_payload webhook_secret body headers then (

match event with

| Some "installation_repositories" -> Installation.input_installation_repositories_webhook ()

| Some "installation" -> App.input_installation_webhook ()

| Some ("pull_request" | "push" | "create") -> Api.input_webhook json_body

| Some "check_run" -> Api.rebuild_webhook ~engine ~has_role json_body

| Some x -> Log.warn (fun f -> f "Unknown GitHub event type %S" x)

| None, _ -> Log.warn (fun f -> f "Missing GitHub event type in webhook!")

) else (

Log.warn (fun f -> f "Invalid X-Hub-Signature-256" x)

)

talex5 · 2021-08-12T10:09:56Z

doc/examples/github.ml

    ~docv:"REPO"
    []

+let webhook_secret =


Perhaps this should be moved into the plugin? Everyone using OAuth is going to need this.

This was only for the demo, the webhook secret from a file is here.
https://github.com/ocurrent/ocurrent/pull/283/files/4e0305335ff9d4feafae708c8926a1880766bdd6#diff-cdd21afd7356cc62801e8a3d1e29ed9791707d3f6639c817603b72cd21817ae3R210-R216

That link is broken now. But we want to make sure people read the secret from a file for OAuth pipelines too.

Changed the examples to read this from a file and restructured the types in Current_github.App and Api to include the web hook_secret. The examples now work the same as the other pipelines like ocaml-ci

plugins/github/current_github.ml

Passing job_id through to set_status, which then gets sent back via webhooks and can be looked up as a job to rebuild.

talex5

Looks good to me!

talex5 · 2021-08-25T07:51:46Z

plugins/github/current_github.mli

-    (** Construct a CheckRunStatus.t 
-        A maximum of three actions are accepted by GitHub.
+    val v : ?text:string -> ?summary:string -> ?url:Uri.t -> ?actions:action list -> ?identifier:string -> state -> t
+    (** [v ?text ?summary ?url ?actions ?identifier] creates a CheckRunStatus with [?text] description, a link to


Suggested change

(** [v ?text ?summary ?url ?actions ?identifier] creates a CheckRunStatus with [?text] description, a link to

(** [v ?text ?summary ?url ?actions ?identifier state] creates a CheckRunStatus with [?text] description, a link to

@ewanmellor

…, current_github, current_git, current_examples, current_docker and current (0.6) CHANGES: Core: - Implement labelling of clusters on the Graphviz diagram (@ewanmellor ocurrent/ocurrent#255) - Abort propagation on constant/equal changes (@art-w ocurrent/ocurrent#318) API: - GitHub: Record build status using CheckRun (@tmcgilchrist ocurrent/ocurrent#279) - GitHub: Add details_url to check_run. (@tmcgilchrist ocurrent/ocurrent#282) - GitHub: Add Current_github.Api.cmdliner_opt to allow writing pipelines which can optionally be run as GitHub apps. (@talex5 ocurrent/ocurrent#281) - GitHub: Provide markdown details for CheckRun. (@tmcgilchrist ocurrent/ocurrent#288) - GitHub: Fix wrong name used for repository (@tmcgilchrist ocurrent/ocurrent#289 ocurrent/ocurrent#290) - GitHub: Support Github rebuild via webooks. (@tmcgilchrist ocurrent/ocurrent#283) - GitHub: monitor GraphQL queries (@art-w ocurrent/ocurrent#298) - GitHub: Limit CheckRunStatus summary and text fields to 65535. (@tmcgilchrist ocurrent/ocurrent#300) - GitHub: Log extra context for Webhook validation failure. (@tmcgilchrist ocurrent/ocurrent#302) - GitLab: Initial GitLab plugin work. (@tmcgilchrist ocurrent/ocurrent#299) - Git: Make git reset less verbose (@kit-ty-kate ocurrent/ocurrent#293) Web UI: - Use Lwt.pause instead of Lwt_unix.yield (@MisterDA ocurrent/ocurrent#297) - Use `ansi` instead of `current_ansi` (@samoht ocurrent/ocurrent#321) - Show line numbers and allow jumping to specific lines in job logs (@punchagan ocurrent/ocurrent#309) Docker: - Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocurrent#304) - Stop using `Dockerfile.t` completely and use strings instead. (@MisterDA ocurrent/ocurrent#301 ocurrent/ocurrent#316) Other: - Update to cohttp 4.0.0 (ocurrent/ocurrent#274, @talex5) - Move `Current_incr` to its own repository (ocurrent/ocurrent#284, @talex5)

talex5 reviewed Jul 23, 2021

View reviewed changes

examples/github_app.ml Outdated Show resolved Hide resolved

plugins/github/api.ml Outdated Show resolved Hide resolved

plugins/github/current_github.mli Outdated Show resolved Hide resolved

tmcgilchrist force-pushed the rebuild branch 2 times, most recently from e910470 to 6006baa Compare July 30, 2021 06:51

tmcgilchrist force-pushed the rebuild branch from 6006baa to 3a885e4 Compare August 10, 2021 03:18

talex5 reviewed Aug 11, 2021

View reviewed changes

tmcgilchrist commented Aug 12, 2021

View reviewed changes

doc/examples/github_app.ml Show resolved Hide resolved

tmcgilchrist force-pushed the rebuild branch from 79667d0 to 4e03053 Compare August 12, 2021 06:14

talex5 reviewed Aug 12, 2021

View reviewed changes

SquidDev reviewed Aug 12, 2021

View reviewed changes

plugins/github/current_github.ml Outdated Show resolved Hide resolved

tmcgilchrist force-pushed the rebuild branch 2 times, most recently from 7a7f62c to fe804df Compare August 17, 2021 03:09

Support Github rebuild via webooks.

3bcf730

Passing job_id through to set_status, which then gets sent back via webhooks and can be looked up as a job to rebuild.

tmcgilchrist force-pushed the rebuild branch 4 times, most recently from 6f5a2be to de8aa3f Compare August 25, 2021 01:16

talex5 approved these changes Aug 25, 2021

View reviewed changes

Secure GitHub webhooks.

37e5e29

tmcgilchrist force-pushed the rebuild branch from de8aa3f to 37e5e29 Compare August 25, 2021 07:58

tmcgilchrist merged commit 3c1979d into ocurrent:master Aug 25, 2021

tmcgilchrist deleted the rebuild branch August 25, 2021 07:59

tmcgilchrist mentioned this pull request Apr 7, 2022

[new release] current_web, current_slack, current_rpc, current_gitlab, current_github, current_git, current_examples, current_docker and current (0.6) ocaml/opam-repository#21133

Merged

-    begin match event, validate_webhook_payload webhook_secret body headers with
-      | Some "installation_repositories", true -> Installation.input_installation_repositories_webhook ()
-      | Some "installation", true -> App.input_installation_webhook ()
-      | Some ("pull_request" | "push" | "create"), true -> Api.input_webhook json_body
-      | Some "check_run", true -> Api.rebuild_webhook ~engine ~has_role json_body
-      | Some x, b -> Log.warn (fun f -> f "Unknown GitHub event type %S or invalid X-Hub-Signature-256 %b" x b)
-      | None, _ -> Log.warn (fun f -> f "Missing GitHub event type in webhook!")
-    end;
+    if validate_webhook_payload webhook_secret body headers then (
+      match event with
+      | Some "installation_repositories" -> Installation.input_installation_repositories_webhook ()
+      | Some "installation" -> App.input_installation_webhook ()
+      | Some ("pull_request" | "push" | "create") -> Api.input_webhook json_body
+      | Some "check_run" -> Api.rebuild_webhook ~engine ~has_role json_body
+      | Some x -> Log.warn (fun f -> f "Unknown GitHub event type %S" x)
+      | None, _ -> Log.warn (fun f -> f "Missing GitHub event type in webhook!")
+    ) else (
+      Log.warn (fun f -> f "Invalid X-Hub-Signature-256" x)
+    )

	(** [v ?text ?summary ?url ?actions ?identifier] creates a CheckRunStatus with [?text] description, a link to
	(** [v ?text ?summary ?url ?actions ?identifier state] creates a CheckRunStatus with [?text] description, a link to

Conversation

tmcgilchrist commented Jul 23, 2021

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmcgilchrist commented Jul 26, 2021

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants