diff options
| -rw-r--r-- | mod/mod-ci-github-gh.cxx | 117 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.hxx | 68 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 6 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 3 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 116 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 4 |
6 files changed, 262 insertions, 52 deletions
diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx index a25e52c..9a3f295 100644 --- a/mod/mod-ci-github-gh.cxx +++ b/mod/mod-ci-github-gh.cxx @@ -123,7 +123,52 @@ namespace brep { p.next_expect (event::begin_object); - bool ni (false), hb (false), hs (false), cc (false), co (false); + bool ni (false), hb (false), hs (false); + + // Skip unknown/uninteresting members. + // + while (p.next_expect (event::name, event::end_object)) + { + auto c = [&p] (bool& v, const char* s) + { + return p.name () == s ? (v = true) : false; + }; + + if (c (ni, "node_id")) node_id = p.next_expect_string (); + else if (c (hb, "head_branch")) + { + string* v (p.next_expect_string_null ()); + if (v != nullptr) + head_branch = *v; + } + else if (c (hs, "head_sha")) head_sha = p.next_expect_string (); + else p.next_expect_value_skip (); + } + + if (!ni) missing_member (p, "gh_check_suite", "node_id"); + if (!hb) missing_member (p, "gh_check_suite", "head_branch"); + if (!hs) missing_member (p, "gh_check_suite", "head_sha"); + } + + ostream& + operator<< (ostream& os, const gh_check_suite& cs) + { + os << "node_id: " << cs.node_id + << ", head_branch: " << (cs.head_branch ? *cs.head_branch : "null") + << ", head_sha: " << cs.head_sha; + + return os; + } + + // gh_check_suite_ex + // + gh_check_suite_ex:: + gh_check_suite_ex (json::parser& p) + { + p.next_expect (event::begin_object); + + bool ni (false), hb (false), hs (false), cc (false), co (false), + ap (false); // Skip unknown/uninteresting members. // @@ -150,24 +195,42 @@ namespace brep if (v != nullptr) conclusion = *v; } + else if (c (ap, "app")) + { + p.next_expect (event::begin_object); + + bool ai (false); + + // Skip unknown/uninteresting members. + // + while (p.next_expect (event::name, event::end_object)) + { + if (c (ai, "id")) app_id = p.next_expect_number<uint64_t> (); + else p.next_expect_value_skip (); + } + + if (!ai) missing_member (p, "gh_check_suite.app", "id"); + } else p.next_expect_value_skip (); } - if (!ni) missing_member (p, "gh_check_suite", "node_id"); - if (!hb) missing_member (p, "gh_check_suite", "head_branch"); - if (!hs) missing_member (p, "gh_check_suite", "head_sha"); - if (!cc) missing_member (p, "gh_check_suite", "latest_check_runs_count"); - if (!co) missing_member (p, "gh_check_suite", "conclusion"); + if (!ni) missing_member (p, "gh_check_suite_ex", "node_id"); + if (!hb) missing_member (p, "gh_check_suite_ex", "head_branch"); + if (!hs) missing_member (p, "gh_check_suite_ex", "head_sha"); + if (!cc) missing_member (p, "gh_check_suite_ex", "latest_check_runs_count"); + if (!co) missing_member (p, "gh_check_suite_ex", "conclusion"); + if (!ap) missing_member (p, "gh_check_suite_ex", "app"); } ostream& - operator<< (ostream& os, const gh_check_suite& cs) + operator<< (ostream& os, const gh_check_suite_ex& cs) { os << "node_id: " << cs.node_id << ", head_branch: " << (cs.head_branch ? *cs.head_branch : "null") << ", head_sha: " << cs.head_sha << ", latest_check_runs_count: " << cs.check_runs_count - << ", conclusion: " << (cs.conclusion ? *cs.conclusion : "null"); + << ", conclusion: " << (cs.conclusion ? *cs.conclusion : "null") + << ", app_id: " << (cs.app_id ? to_string (*cs.app_id) : "null"); return os; } @@ -208,7 +271,8 @@ namespace brep { p.next_expect (event::begin_object); - bool ni (false), nm (false), st (false), du (false), cs (false); + bool ni (false), nm (false), st (false), du (false), cs (false), + ap (false); // Skip unknown/uninteresting members. // @@ -224,14 +288,31 @@ namespace brep else if (c (st, "status")) status = p.next_expect_string (); else if (c (du, "details_url")) details_url = p.next_expect_string (); else if (c (cs, "check_suite")) check_suite = gh_check_suite (p); + else if (c (ap, "app")) + { + p.next_expect (event::begin_object); + + bool ai (false); + + // Skip unknown/uninteresting members. + // + while (p.next_expect (event::name, event::end_object)) + { + if (c (ai, "id")) app_id = p.next_expect_number<uint64_t> (); + else p.next_expect_value_skip (); + } + + if (!ai) missing_member (p, "gh_check_run_ex.app", "id"); + } else p.next_expect_value_skip (); } - if (!ni) missing_member (p, "gh_check_run", "node_id"); - if (!nm) missing_member (p, "gh_check_run", "name"); - if (!st) missing_member (p, "gh_check_run", "status"); - if (!du) missing_member (p, "gh_check_run", "details_url"); - if (!cs) missing_member (p, "gh_check_run", "check_suite"); + if (!ni) missing_member (p, "gh_check_run_ex", "node_id"); + if (!nm) missing_member (p, "gh_check_run_ex", "name"); + if (!st) missing_member (p, "gh_check_run_ex", "status"); + if (!du) missing_member (p, "gh_check_run_ex", "details_url"); + if (!cs) missing_member (p, "gh_check_run_ex", "check_suite"); + if (!ap) missing_member (p, "gh_check_run_ex", "app"); } @@ -250,7 +331,8 @@ namespace brep { os << static_cast<const gh_check_run&> (cr) << ", details_url: " << cr.details_url - << ", check_suite: { " << cr.check_suite << " }"; + << ", check_suite: { " << cr.check_suite << " }" + << ", app_id: " << cr.app_id; return os; } @@ -356,7 +438,8 @@ namespace brep << "path: " << pr.head_path << ", ref: " << pr.head_ref << ", sha: " << pr.head_sha - << " }"; + << " }" + << ", app_id: " << pr.app_id; return os; } @@ -452,7 +535,7 @@ namespace brep }; if (c (ac, "action")) action = p.next_expect_string (); - else if (c (cs, "check_suite")) check_suite = gh_check_suite (p); + else if (c (cs, "check_suite")) check_suite = gh_check_suite_ex (p); else if (c (rp, "repository")) repository = gh_repository (p); else if (c (in, "installation")) installation = gh_installation (p); else p.next_expect_value_skip (); diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx index 05c289e..0028129 100644 --- a/mod/mod-ci-github-gh.hxx +++ b/mod/mod-ci-github-gh.hxx @@ -43,7 +43,11 @@ namespace brep // namespace json = butl::json; - // The "check_suite" object within a check_suite webhook event request. + // The check_suite member of a check_run webhook event (gh_check_run_event). + // + // @@ TMP In the check_run context only the head_sha is used so perhaps it + // would be better to move the other members directly into the _ex + // version? // struct gh_check_suite { @@ -51,15 +55,34 @@ namespace brep optional<string> head_branch; string head_sha; + explicit + gh_check_suite (json::parser&); + + gh_check_suite () = default; + }; + + // The check_suite member of a check_suite webhook event + // (gh_check_suite_event). + // + struct gh_check_suite_ex: gh_check_suite + { size_t check_runs_count; optional<string> conclusion; + // Note: unlike the check_run webhook's app_id this can be null. + // + optional<uint64_t> app_id; + explicit - gh_check_suite (json::parser&); + gh_check_suite_ex (json::parser&); - gh_check_suite () = default; + gh_check_suite_ex () = default; }; + // The check_run object returned in response to GraphQL requests + // (e.g. create or update check run). Note that we specifiy the set of + // members to return in the GraphQL request. + // struct gh_check_run { string node_id; @@ -72,17 +95,24 @@ namespace brep gh_check_run () = default; }; + // The check_run member of a check_run webhook event (gh_check_run_event). + // struct gh_check_run_ex: gh_check_run { string details_url; gh_check_suite check_suite; + uint64_t app_id; + explicit gh_check_run_ex (json::parser&); gh_check_run_ex () = default; }; + // The pull_request member of a pull_request webhook event + // (gh_pull_request_event). + // struct gh_pull_request { string node_id; @@ -96,12 +126,26 @@ namespace brep string head_ref; string head_sha; + // Note: not received from GitHub but set from the app-id webhook query + // parameter instead. + // + // For some reason, unlike the check_suite and check_run webhooks, the + // pull_request webhook does not contain the app id. For the sake of + // simplicity we emulate check_suite and check_run by storing the app-id + // webhook query parameter here. + // + // @@ TODO Explain multiple apps in INSTALL-GITHUB-DEV. + // + uint64_t app_id; + explicit gh_pull_request (json::parser&); gh_pull_request () = default; }; + // @@ TODO Move these functions below the remaining structs? + // Return the GitHub check run status corresponding to a build_state. // string @@ -128,6 +172,8 @@ namespace brep string gh_check_run_name (const build&, const build_queued_hints* = nullptr); + // The repository member of various webhook events. + // struct gh_repository { string node_id; @@ -140,6 +186,8 @@ namespace brep gh_repository () = default; }; + // The installation member of various webhook events. + // struct gh_installation { uint64_t id; // Note: used for installation access token (REST API). @@ -150,12 +198,12 @@ namespace brep gh_installation () = default; }; - // The check_suite webhook event request. + // The check_suite webhook event. // struct gh_check_suite_event { string action; - gh_check_suite check_suite; + gh_check_suite_ex check_suite; gh_repository repository; gh_installation installation; @@ -165,6 +213,8 @@ namespace brep gh_check_suite_event () = default; }; + // The check_run webhook event. + // struct gh_check_run_event { string action; @@ -178,6 +228,8 @@ namespace brep gh_check_run_event () = default; }; + // The pull_request webhook event. + // struct gh_pull_request_event { string action; @@ -198,6 +250,9 @@ namespace brep gh_pull_request_event () = default; }; + // Installation access token (IAT) returned when we authenticate as a GitHub + // app installation. + // struct gh_installation_access_token { string token; @@ -227,6 +282,9 @@ namespace brep operator<< (ostream&, const gh_check_suite&); ostream& + operator<< (ostream&, const gh_check_suite_ex&); + + ostream& operator<< (ostream&, const gh_check_run&); ostream& diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index 31a556d..4c6c9fe 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -54,6 +54,7 @@ namespace brep p.next_expect_name ("installation_access"); installation_access = gh_installation_access_token (p); + app_id = p.next_expect_member_number<uint64_t> ("app_id"); installation_id = p.next_expect_member_number<uint64_t> ("installation_id"); @@ -135,6 +136,7 @@ namespace brep service_data (bool ws, string iat_tok, timestamp iat_ea, + uint64_t aid, uint64_t iid, string rid, string rcu, @@ -146,6 +148,7 @@ namespace brep : kind (k), pre_check (pc), re_request (rr), warning_success (ws), installation_access (move (iat_tok), iat_ea), + app_id (aid), installation_id (iid), repository_node_id (move (rid)), repository_clone_url (move (rcu)), @@ -161,6 +164,7 @@ namespace brep service_data (bool ws, string iat_tok, timestamp iat_ea, + uint64_t aid, uint64_t iid, string rid, string rcu, @@ -174,6 +178,7 @@ namespace brep : kind (k), pre_check (pc), re_request (rr), warning_success (ws), installation_access (move (iat_tok), iat_ea), + app_id (aid), installation_id (iid), repository_node_id (move (rid)), repository_clone_url (move (rcu)), @@ -233,6 +238,7 @@ namespace brep s.end_object (); + s.member ("app_id", app_id); s.member ("installation_id", installation_id); s.member ("repository_node_id", repository_node_id); s.member ("repository_clone_url", repository_clone_url); diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 0f4c760..fc97cfa 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -89,6 +89,7 @@ namespace brep // gh_installation_access_token installation_access; + uint64_t app_id; uint64_t installation_id; string repository_node_id; // GitHub-internal opaque repository id. @@ -151,6 +152,7 @@ namespace brep service_data (bool warning_success, string iat_token, timestamp iat_expires_at, + uint64_t app_id, uint64_t installation_id, string repository_node_id, string repository_clone_url, @@ -165,6 +167,7 @@ namespace brep service_data (bool warning_success, string iat_token, timestamp iat_expires_at, + uint64_t app_id, uint64_t installation_id, string repository_node_id, string repository_clone_url, diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index ba2dece..737d93d 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -61,6 +61,8 @@ namespace brep void ci_github:: init (scanner& s) { + HANDLER_DIAG; + { shared_ptr<tenant_service_base> ts ( dynamic_pointer_cast<tenant_service_base> (shared_from_this ())); @@ -78,6 +80,9 @@ namespace brep if (options_->build_config_specified () && options_->ci_github_app_webhook_secret_specified ()) { + if (!options_->ci_github_app_id_private_key_specified ()) + fail << "no app id/private key mappings configured"; + ci_start::init (make_shared<options::ci_start> (*options_)); database_module::init (*options_, options_->build_db_retry ()); @@ -221,35 +226,57 @@ namespace brep fail << "unable to compute request HMAC: " << e; } - // Process the `warning` webhook request query parameter. - // - // @@ Add app-id parameter (required). + // Process the `app-id` and `warning` webhook request query parameters. // + uint64_t app_id; bool warning_success; { const name_values& rps (rq.parameters (1024, true /* url_only */)); - auto i (find_if (rps.begin (), rps.end (), - [] (auto&& rp) {return rp.name == "warning";})); + bool ai (false), wa (false); - if (i == rps.end ()) - throw invalid_request (400, - "missing 'warning' webhook query parameter"); + auto badreq = [] (const string& m) + { + throw invalid_request (400, m); + }; + + for (const name_value& rp: rps) + { + if (rp.name == "app-id") + { + if (!rp.value) + badreq ("missing 'app-id' webhook query parameter value"); - if (!i->value) - throw invalid_request ( - 400, "missing 'warning' webhook query parameter value"); + ai = true; - const string& v (*i->value); + // Parse the value. + // + char* e (nullptr); + app_id = strtoull (rp.value->c_str (), &e, 10); + if (*e != '\0' || app_id == 0 || app_id == ULLONG_MAX) + { + badreq ("invalid 'app-id' webhook query parameter value: '" + + *rp.value + '\''); + } + } + else if (rp.name == "warning") + { + if (!rp.value) + badreq ("missing 'warning' webhook query parameter value"); - if (v == "success") warning_success = true; - else if (v == "failure") warning_success = false; - else - { - throw invalid_request ( - 400, - "invalid 'warning' webhook query parameter value: '" + v + '\''); + wa = true; + const string& v (*rp.value); + + if (v == "success") warning_success = true; + else if (v == "failure") warning_success = false; + else + badreq ("invalid 'warning' webhook query parameter value: '" + v + + '\''); + } } + + if (!ai) badreq ("missing 'app-id' webhook query parameter"); + if (!wa) badreq ("missing 'warning' webhook query parameter"); } // There is a webhook event (specified in the x-github-event header) and @@ -281,6 +308,18 @@ namespace brep throw invalid_request (400, move (m)); } + // It's unclear under what circumstances the app_id can be null but it + // shouldn't happen unless something is broken. + // + if (!cs.check_suite.app_id) + throw invalid_request (400, "absent app.id in check_suite webhook"); + + if (*cs.check_suite.app_id != app_id) + { + fail << "webhook check_suite app.id " << *cs.check_suite.app_id + << " does not match app-id query parameter " << app_id; + } + if (cs.action == "requested") { return handle_check_suite_request (move (cs), warning_success); @@ -329,6 +368,12 @@ namespace brep throw invalid_request (400, move (m)); } + if (cr.check_run.app_id != app_id) + { + fail << "webhook check_run app.id " << cr.check_run.app_id + << " does not match app-id query parameter " << app_id; + } + if (cr.action == "rerequested") { // Someone manually requested to re-run a specific check run. @@ -374,6 +419,14 @@ namespace brep throw invalid_request (400, move (m)); } + // Store the app-id webhook query parameter in the gh_pull_request + // object (see gh_pull_request for an explanation). + // + // When we receive the other webhooks we do check that the app ids in + // the payload and query match but here we have to assume it is valid. + // + pr.pull_request.app_id = app_id; + if (pr.action == "opened" || pr.action == "synchronize") { @@ -521,7 +574,7 @@ namespace brep // let's obtain it to flush out any permission issues early. Also, it is // valid for an hour so we will most likely make use of it. // - optional<string> jwt (generate_jwt (trace, error)); + optional<string> jwt (generate_jwt (*cs.check_suite.app_id, trace, error)); if (!jwt) throw server_error (); @@ -605,6 +658,7 @@ namespace brep service_data sd (warning_success, iat->token, iat->expires_at, + *cs.check_suite.app_id, cs.installation.id, move (cs.repository.node_id), move (cs.repository.clone_url), @@ -853,7 +907,7 @@ namespace brep auto get_iat = [this, &trace, &error, &cr] () -> optional<gh_installation_access_token> { - optional<string> jwt (generate_jwt (trace, error)); + optional<string> jwt (generate_jwt (cr.check_run.app_id, trace, error)); if (!jwt) return nullopt; @@ -1300,7 +1354,7 @@ namespace brep // let's obtain it to flush out any permission issues early. Also, it is // valid for an hour so we will most likely make use of it. // - optional<string> jwt (generate_jwt (trace, error)); + optional<string> jwt (generate_jwt (pr.pull_request.app_id, trace, error)); if (!jwt) throw server_error (); @@ -1381,6 +1435,7 @@ namespace brep service_data sd (warning_success, move (iat->token), iat->expires_at, + pr.pull_request.app_id, pr.installation.id, move (pr.repository.node_id), move (pr.repository.clone_url), @@ -1684,7 +1739,7 @@ namespace brep if (system_clock::now () > sd.installation_access.expires_at) { - if (optional<string> jwt = generate_jwt (trace, error)) + if (optional<string> jwt = generate_jwt (sd.app_id, trace, error)) { new_iat = obtain_installation_access_token (sd.installation_id, move (*jwt), @@ -2087,7 +2142,7 @@ namespace brep if (system_clock::now () > sd.installation_access.expires_at) { - if (optional<string> jwt = generate_jwt (trace, error)) + if (optional<string> jwt = generate_jwt (sd.app_id, trace, error)) { new_iat = obtain_installation_access_token (sd.installation_id, move (*jwt), @@ -2252,7 +2307,7 @@ namespace brep if (system_clock::now () > sd.installation_access.expires_at) { - if (optional<string> jwt = generate_jwt (trace, error)) + if (optional<string> jwt = generate_jwt (sd.app_id, trace, error)) { new_iat = obtain_installation_access_token (sd.installation_id, move (*jwt), @@ -2458,7 +2513,7 @@ namespace brep if (system_clock::now () > sd.installation_access.expires_at) { - if (optional<string> jwt = generate_jwt (trace, error)) + if (optional<string> jwt = generate_jwt (sd.app_id, trace, error)) { new_iat = obtain_installation_access_token (sd.installation_id, move (*jwt), @@ -2883,19 +2938,22 @@ namespace brep } optional<string> ci_github:: - generate_jwt (const basic_mark& trace, + generate_jwt (uint64_t app_id, + const basic_mark& trace, const basic_mark& error) const { string jwt; try { + string ai (to_string (app_id)); + // Set token's "issued at" time 60 seconds in the past to combat clock // drift (as recommended by GitHub). // jwt = brep::generate_jwt ( *options_, - options_->ci_github_app_private_key (), - to_string (options_->ci_github_app_id ()), + options_->ci_github_app_id_private_key ()[ai], + ai, chrono::seconds (options_->ci_github_jwt_validity_period ()), chrono::seconds (60)); diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index f97bf05..a9617fa 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -120,7 +120,9 @@ namespace brep details_url (const build&) const; optional<string> - generate_jwt (const basic_mark& trace, const basic_mark& error) const; + generate_jwt (uint64_t app_id, + const basic_mark& trace, + const basic_mark& error) const; // Authenticate to GitHub as an app installation. Return the installation // access token (IAT). Issue diagnostics and return nullopt if something |