Harden the webhook handler and stop billing polling on shutdown

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
 			}

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)

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

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)
+}

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=<hex digest>", 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