diff --git a/internal/server/billing_metrics_exporter.go b/internal/server/billing_metrics_exporter.go index bf144a2..4b59b88 100644 --- a/internal/server/billing_metrics_exporter.go +++ b/internal/server/billing_metrics_exporter.go @@ -53,6 +53,7 @@ func (c *BillingMetricsExporter) StartOrgBilling(ctx context.Context) error { case <-ticker.C: c.collectOrgBilling(ctx) case <-ctx.Done(): + ticker.Stop() c.Logger.Info("stopped polling for org billing metrics") return } diff --git a/internal/server/billing_metrics_exporter_test.go b/internal/server/billing_metrics_exporter_test.go index d15b777..b390f53 100644 --- a/internal/server/billing_metrics_exporter_test.go +++ b/internal/server/billing_metrics_exporter_test.go @@ -1,10 +1,13 @@ package server import ( + "bytes" "context" "log/slog" "net/http" "net/http/httptest" + "strings" + "sync" "testing" "time" @@ -130,6 +133,38 @@ func Test_BillingMetricsExporter_collectOrgBilling_DropsStaleHostTypes(t *testin assert.InDelta(t, 5, testutil.ToFloat64(totalMinutesUsedByHostTypeActions.WithLabelValues(org, "", "UBUNTU")), 1e-9) } +func Test_BillingMetricsExporter_StartOrgBilling_StopsOnContextCancel(t *testing.T) { + var logs syncBuffer + exporter := newTestBillingExporter(t, func(http.ResponseWriter, *http.Request) {}) + exporter.Logger = slog.New(slog.NewTextHandler(&logs, nil)) + exporter.Opts = Opts{GitHubOrg: "test-org", GitHubAPIToken: "token", BillingAPIPollSeconds: 3600} + + ctx, cancel := context.WithCancel(context.Background()) + require.NoError(t, exporter.StartOrgBilling(ctx)) + cancel() + + assert.Eventually(t, func() bool { + return strings.Contains(logs.String(), "stopped polling for org billing metrics") + }, 5*time.Second, 10*time.Millisecond) +} + +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + func Test_BillingMetricsExporter_collectOrgBilling_APIErrorKeepsMetrics(t *testing.T) { exporter := newTestBillingExporter(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusForbidden) diff --git a/internal/server/server.go b/internal/server/server.go index 8576d64..8d22a71 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -34,6 +34,7 @@ type Server struct { serverIngress *http.Server workflowMetricsExporter *WorkflowMetricsExporter billingExporter *BillingMetricsExporter + billingCancel context.CancelFunc opts Opts } @@ -45,11 +46,12 @@ func NewServer(logger *slog.Logger, opts Opts) *Server { } billingExporter := NewBillingMetricsExporter(logger, opts) - err := billingExporter.StartOrgBilling(context.TODO()) + billingCtx, billingCancel := context.WithCancel(context.Background()) + err := billingExporter.StartOrgBilling(billingCtx) if err != nil { logger.Info("not exporting org billing", "reason", err) } - err = billingExporter.StartUserBilling(context.TODO()) + err = billingExporter.StartUserBilling(billingCtx) if err != nil { logger.Info("not exporting user billing", "reason", err) } @@ -67,6 +69,7 @@ func NewServer(logger *slog.Logger, opts Opts) *Server { serverIngress: httpServerIngress, workflowMetricsExporter: workflowExporter, billingExporter: billingExporter, + billingCancel: billingCancel, opts: opts, } @@ -104,6 +107,8 @@ func (s *Server) Serve(_ context.Context) error { } func (s *Server) Shutdown(ctx context.Context) error { + s.billingCancel() + err := s.serverMetrics.Shutdown(ctx) if err != nil { return err diff --git a/internal/server/webhook_signature_test.go b/internal/server/webhook_signature_test.go new file mode 100644 index 0000000..97ef61c --- /dev/null +++ b/internal/server/webhook_signature_test.go @@ -0,0 +1,177 @@ +package server_test + +import ( + "bytes" + "crypto/hmac" + "crypto/sha1" // nolint: gosec + "crypto/sha256" + "encoding/hex" + "hash" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/cpanato/github_actions_exporter/internal/server" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const pingPayload = `{"hook_id": 42}` + +func newSignatureTestExporter() server.WorkflowMetricsExporter { + return server.WorkflowMetricsExporter{ + Logger: slog.New(slog.DiscardHandler), + Opts: server.Opts{GitHubToken: webhookSecret}, + } +} + +func hmacHex(newHash func() hash.Hash, payload []byte) string { + h := hmac.New(newHash, []byte(webhookSecret)) + h.Write(payload) + return hex.EncodeToString(h.Sum(nil)) +} + +func sha256Hex(payload []byte) string { + return hmacHex(sha256.New, payload) +} + +func sha1Hex(payload []byte) string { + return hmacHex(sha1.New, payload) +} + +func pingRequest(headers map[string]string, payload string) *http.Request { + req := httptest.NewRequest("POST", "/anything", bytes.NewReader([]byte(payload))) + req.Header.Set("X-GitHub-Event", "ping") + for k, v := range headers { + req.Header.Set(k, v) + } + return req +} + +func Test_HandleGHWebHook_AcceptsValidSHA256Signature(t *testing.T) { + subject := newSignatureTestExporter() + req := pingRequest(map[string]string{ + "X-Hub-Signature-256": "sha256=" + sha256Hex([]byte(pingPayload)), + }, pingPayload) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + assert.Equal(t, http.StatusAccepted, res.Result().StatusCode) +} + +func Test_HandleGHWebHook_SHA256SignatureTakesPrecedenceOverSHA1(t *testing.T) { + subject := newSignatureTestExporter() + // A valid legacy signature must not rescue an invalid SHA-256 one. + req := pingRequest(map[string]string{ + "X-Hub-Signature-256": "sha256=" + sha256Hex([]byte("another payload")), + "X-Hub-Signature": "sha1=" + sha1Hex([]byte(pingPayload)), + }, pingPayload) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + assert.Equal(t, http.StatusForbidden, res.Result().StatusCode) +} + +func Test_HandleGHWebHook_FallsBackToSHA1Signature(t *testing.T) { + subject := newSignatureTestExporter() + req := pingRequest(map[string]string{ + "X-Hub-Signature": "sha1=" + sha1Hex([]byte(pingPayload)), + }, pingPayload) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + assert.Equal(t, http.StatusAccepted, res.Result().StatusCode) +} + +func Test_HandleGHWebHook_RejectsWrongSecret(t *testing.T) { + subject := newSignatureTestExporter() + subject.Opts.GitHubToken = "another-secret" + req := pingRequest(map[string]string{ + "X-Hub-Signature-256": "sha256=" + sha256Hex([]byte(pingPayload)), + }, pingPayload) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + assert.Equal(t, http.StatusForbidden, res.Result().StatusCode) +} + +func Test_HandleGHWebHook_RejectsMalformedSignatures(t *testing.T) { + validSHA256 := sha256Hex([]byte(pingPayload)) + validSHA1 := sha1Hex([]byte(pingPayload)) + + tests := map[string]map[string]string{ + "no signature header": {}, + "sha1 without digest": {"X-Hub-Signature": "sha1"}, + "sha1 empty digest": {"X-Hub-Signature": "sha1="}, + "sha1 digest not hex": {"X-Hub-Signature": "sha1=not-hex"}, + "sha256 algorithm in sha1 header": {"X-Hub-Signature": "sha256=" + validSHA256}, + "sha1 algorithm in sha256 header": {"X-Hub-Signature-256": "sha1=" + validSHA1}, + "sha256 without digest": {"X-Hub-Signature-256": "sha256"}, + "sha256 digest not hex": {"X-Hub-Signature-256": "sha256=zz"}, + "sha256 truncated digest": {"X-Hub-Signature-256": "sha256=" + validSHA256[:10]}, + "digest without algorithm": {"X-Hub-Signature-256": validSHA256}, + "uppercase algorithm not supported": {"X-Hub-Signature-256": "SHA256=" + validSHA256}, + } + + for name, headers := range tests { + t.Run(name, func(t *testing.T) { + subject := newSignatureTestExporter() + req := pingRequest(headers, pingPayload) + + res := httptest.NewRecorder() + require.NotPanics(t, func() { subject.HandleGHWebHook(res, req) }) + + assert.Equal(t, http.StatusForbidden, res.Result().StatusCode) + }) + } +} + +// zeroReader endlessly returns zero bytes. +type zeroReader struct{} + +func (zeroReader) Read(p []byte) (int, error) { + clear(p) + return len(p), nil +} + +func Test_HandleGHWebHook_RejectsTooLargeBody(t *testing.T) { + subject := newSignatureTestExporter() + const limit = 25 << 20 + req := httptest.NewRequest("POST", "/anything", io.LimitReader(zeroReader{}, limit+1)) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + assert.Equal(t, http.StatusRequestEntityTooLarge, res.Result().StatusCode) +} + +func Test_HandleGHWebHook_AcceptsBodyAtTheLimit(t *testing.T) { + subject := newSignatureTestExporter() + const limit = 25 << 20 + req := httptest.NewRequest("POST", "/anything", io.LimitReader(zeroReader{}, limit)) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + // The body is read completely and only fails the signature check. + assert.Equal(t, http.StatusForbidden, res.Result().StatusCode) +} + +func Test_HandleGHWebHook_PingWithUndecodableBody(t *testing.T) { + subject := newSignatureTestExporter() + payload := "not json" + req := pingRequest(map[string]string{ + "X-Hub-Signature-256": "sha256=" + sha256Hex([]byte(payload)), + }, payload) + + res := httptest.NewRecorder() + subject.HandleGHWebHook(res, req) + + assert.Equal(t, http.StatusBadRequest, res.Result().StatusCode) +} diff --git a/internal/server/workflow_metrics_exporter.go b/internal/server/workflow_metrics_exporter.go index f6bf69b..63b3606 100644 --- a/internal/server/workflow_metrics_exporter.go +++ b/internal/server/workflow_metrics_exporter.go @@ -4,9 +4,11 @@ import ( "bytes" "crypto/hmac" "crypto/sha1" // nolint: gosec + "crypto/sha256" "encoding/hex" "errors" "fmt" + "hash" "io" "log/slog" "math" @@ -33,26 +35,28 @@ func NewWorkflowMetricsExporter(logger *slog.Logger, opts Opts) *WorkflowMetrics } } +// maxWebhookBodyBytes is the maximum payload size GitHub delivers for a webhook (25 MB). +const maxWebhookBodyBytes = 25 << 20 + // HandleGHWebHook responds to POST /gh_event, when it receives an event from GitHub. func (c *WorkflowMetricsExporter) HandleGHWebHook(w http.ResponseWriter, r *http.Request) { - buf, err := io.ReadAll(r.Body) + buf, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxWebhookBodyBytes)) if err != nil { + var maxBytesErr *http.MaxBytesError + if errors.As(err, &maxBytesErr) { + c.Logger.Error("webhook body too large", "limit", maxBytesErr.Limit) + w.WriteHeader(http.StatusRequestEntityTooLarge) + return + } c.Logger.Error("error reading body", "err", err) w.WriteHeader(http.StatusInternalServerError) return } defer r.Body.Close() - receivedHash := strings.SplitN(r.Header.Get("X-Hub-Signature"), "=", 2) - if receivedHash[0] != "sha1" { - c.Logger.Error("invalid webhook hash signature: SHA1") - w.WriteHeader(http.StatusForbidden) - return - } - - err = validateSignature(c.Opts.GitHubToken, receivedHash, buf) + err = validateSignature(c.Opts.GitHubToken, r.Header, buf) if err != nil { - c.Logger.Error("invalid token", "err", err) + c.Logger.Error("invalid webhook signature", "err", err) w.WriteHeader(http.StatusForbidden) return } @@ -64,10 +68,11 @@ func (c *WorkflowMetricsExporter) HandleGHWebHook(w http.ResponseWriter, r *http case "ping": pingEvent := model.PingEventFromJSON(io.NopCloser(bytes.NewBuffer(buf))) if pingEvent == nil { - c.Logger.Info("ping event", "hookID", pingEvent.GetHookID()) + c.Logger.Error("unable to decode the ping event") w.WriteHeader(http.StatusBadRequest) return } + c.Logger.Info("ping event", "hookID", pingEvent.GetHookID()) w.WriteHeader(http.StatusAccepted) _, _ = w.Write([]byte(`{"status": "honk"}`)) return @@ -162,18 +167,36 @@ func (c *WorkflowMetricsExporter) CollectWorkflowRunEvent(event *github.Workflow c.PrometheusObserver.CountWorkflowRunStatus(org, repo, branch, status, conclusion, workflowName) } -// validateSignature validate the incoming github event. -func validateSignature(gitHubToken string, receivedHash []string, bodyBuffer []byte) error { - hash := hmac.New(sha1.New, []byte(gitHubToken)) - if _, err := hash.Write(bodyBuffer); err != nil { - msg := fmt.Sprintf("Cannot compute the HMAC for request: %s\n", err) - return errors.New(msg) +// validateSignature validates the HMAC signature of an incoming GitHub event. +// X-Hub-Signature-256 is preferred, the legacy SHA-1 X-Hub-Signature is only used when it is absent. +func validateSignature(gitHubToken string, headers http.Header, body []byte) error { + var ( + algorithm string + newHash func() hash.Hash + header string + ) + if value := headers.Get("X-Hub-Signature-256"); value != "" { + algorithm, newHash, header = "sha256", sha256.New, value + } else if value := headers.Get("X-Hub-Signature"); value != "" { + algorithm, newHash, header = "sha1", sha1.New, value + } else { + return errors.New("missing X-Hub-Signature-256 or X-Hub-Signature header") + } + + receivedAlgorithm, receivedHex, found := strings.Cut(header, "=") + if !found || receivedAlgorithm != algorithm { + return fmt.Errorf("malformed signature header, expected %s=", algorithm) + } + + received, err := hex.DecodeString(receivedHex) + if err != nil { + return fmt.Errorf("signature is not a valid hex digest: %w", err) } - expectedHash := hex.EncodeToString(hash.Sum(nil)) - if receivedHash[1] != expectedHash { - msg := fmt.Sprintf("Expected Hash does not match the received hash: %s\n", expectedHash) - return errors.New(msg) + mac := hmac.New(newHash, []byte(gitHubToken)) + mac.Write(body) + if !hmac.Equal(received, mac.Sum(nil)) { + return errors.New("signature does not match the payload") } return nil