Provide an array of fallback secrets to allow secret migration (#2734)

In order to provide for a clean webhook secret migration, we need to
provide a window of using the old secret. For this, we add an array of
PreivousWebhookSecrets to the WebhookConfig structure. If validating the
payload with the WebhookConfig.WebhookSecret fails, we fall back to
validating using the previous secrets.

Because validating the request reads the request body, we need to read
the body first and save in a byte slice in case we do have the fallbacks
configured.

Related: #2722

config/server-config.yaml.example

@@ -65,6 +65,7 @@ webhook-config:
   external_webhook_url: "https://example.com/api/v1/webhook/github"
   external_ping_url: "https://example.com/api/v1/health"
   webhook_secret: "your-password"
+# previous_webhook_secret_file: ./previous_secrets
 
 # OAuth2 Configuration (used during enrollment)
 # These values are to be set within the GitHub OAuth2 App page

docs/docs/run_minder_server/run_the_server.md

@@ -249,14 +249,21 @@ webhook-config:
 
 After these steps, your Minder server should be ready to receive webhook events from GitHub, and add webhooks to repositories.
 
+In case you need to update the webhook secret, you can do so by putting the
+new secret in `webhook-config.webhook_secret` and for the duration of the
+migration, the old secret(s) in a file referenced by
+`webhook-config.previous_webhook_secret_file`. The old webhook secrets will
+then only be used to verify incoming webhooks messages, not for creating or
+updating webhooks and can be removed after the migration is complete.
+
 In order to rotate webhook secrets, you can use the `minder-server` CLI tool to update the webhook secret.
 
 ```bash
 minder-server webhook update -p github
 ```
 
 Note that the command simply replaces the webhook secret on the provider
-side. You will need to update the webhook secret in the server configuration
+side. You will still need to update the webhook secret in the server configuration
 to match the provider's secret.
 
 ## Run the application
@@ -283,4 +290,4 @@ After configuring `server-config.yaml`, you can run the application using `docke
 docker compose up -d minder
 ```
 
-The application will be available on `http://localhost:8080` and gRPC on `localhost:8090`.
+The application will be available on `http://localhost:8080` and gRPC on `localhost:8090`.

internal/config/server/webhook.go

@@ -15,13 +15,35 @@
 
 package server
 
+import (
+	"fmt"
+	"os"
+	"strings"
+)
+
 // WebhookConfig is the configuration for our webhook capabilities
 type WebhookConfig struct {
 	// ExternalWebhookURL is the URL that we will send our webhook to
 	ExternalWebhookURL string `mapstructure:"external_webhook_url"`
 	// ExternalPingURL is the URL that we will send our ping to
 	ExternalPingURL string `mapstructure:"external_ping_url"`
 	// WebhookSecret is the secret that we will use to sign our webhook
-	// TODO: Check if this is actually used and needed
 	WebhookSecret string `mapstructure:"webhook_secret"`
+	// PreviousWebhookSecretFile is a reference to a file that contains previous webhook secrets. This is used
+	// in case we are rotating secrets and the external service is still using the old secret. These will not
+	// be used when creating new webhooks.
+	PreviousWebhookSecretFile string `mapstructure:"previous_webhook_secret_file"`
+}
+
+// GetPreviousWebhookSecrets retrieves the previous webhook secrets from a file specified in the WebhookConfig.
+// It reads the contents of the file, splits the data by whitespace, and returns it as a slice of strings.
+func (wc *WebhookConfig) GetPreviousWebhookSecrets() ([]string, error) {
+	data, err := os.ReadFile(wc.PreviousWebhookSecretFile)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read previous webhook secrets from file: %w", err)
+	}
+
+	// Split the data by whitespace and return it as a slice of strings
+	secrets := strings.Fields(string(data))
+	return secrets, nil
 }

internal/controlplane/handlers_githubwebhooks.go

@@ -17,11 +17,14 @@
 package controlplane
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
+	"mime"
 	"net/http"
 	"sort"
 	"strconv"
@@ -32,10 +35,10 @@ import (
 	"github.com/google/uuid"
 	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/log"
-	"github.com/spf13/viper"
 	"golang.org/x/exp/slices"
 	"google.golang.org/protobuf/types/known/timestamppb"
 
+	"github.com/stacklok/minder/internal/config/server"
 	"github.com/stacklok/minder/internal/controlplane/metrics"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine/entities"
@@ -132,7 +135,7 @@ func (s *Server) HandleGitHubWebHook() http.HandlerFunc {
 		segments := strings.Split(r.URL.Path, "/")
 		_ = segments[len(segments)-1]
 
-		rawWBPayload, err := github.ValidatePayload(r, []byte(viper.GetString("webhook-config.webhook_secret")))
+		rawWBPayload, err := validatePayloadSignature(r, &s.cfg.WebhookConfig)
 		if err != nil {
 			log.Printf("Error validating webhook payload: %v", err)
 			w.WriteHeader(http.StatusBadRequest)
@@ -186,6 +189,72 @@ func (s *Server) HandleGitHubWebHook() http.HandlerFunc {
 	}
 }
 
+func validatePayloadSignature(r *http.Request, wc *server.WebhookConfig) (payload []byte, err error) {
+	var br *bytes.Reader
+	br, err = readerFromRequest(r)
+	if err != nil {
+		return
+	}
+
+	signature := r.Header.Get(github.SHA256SignatureHeader)
+	if signature == "" {
+		signature = r.Header.Get(github.SHA1SignatureHeader)
+	}
+	contentType, _, err := mime.ParseMediaType(r.Header.Get("Content-Type"))
+	if err != nil {
+		return
+	}
+
+	payload, err = github.ValidatePayloadFromBody(contentType, br, signature, []byte(wc.WebhookSecret))
+	if err == nil {
+		return
+	}
+
+	payload, err = validatePreviousSecrets(r.Context(), signature, contentType, br, wc)
+	return
+}
+
+func readerFromRequest(r *http.Request) (*bytes.Reader, error) {
+	b, err := io.ReadAll(r.Body)
+	if err != nil {
+		return nil, err
+	}
+	err = r.Body.Close()
+	if err != nil {
+		return nil, err
+	}
+	return bytes.NewReader(b), nil
+}
+
+func validatePreviousSecrets(
+	ctx context.Context,
+	signature, contentType string,
+	br *bytes.Reader,
+	wc *server.WebhookConfig,
+) (payload []byte, err error) {
+	previousSecrets := []string{}
+	if wc.PreviousWebhookSecretFile != "" {
+		previousSecrets, err = wc.GetPreviousWebhookSecrets()
+		if err != nil {
+			return
+		}
+	}
+
+	for _, prevSecret := range previousSecrets {
+		_, err = br.Seek(0, io.SeekStart)
+		if err != nil {
+			return
+		}
+		payload, err = github.ValidatePayloadFromBody(contentType, br, signature, []byte(prevSecret))
+		if err == nil {
+			zerolog.Ctx(ctx).Warn().Msg("used previous secret to validate payload")
+			return
+		}
+	}
+
+	return
+}
+
 func handleParseError(typ string, parseErr error) *metrics.WebhookEventState {
 	state := &metrics.WebhookEventState{Typ: typ, Accepted: false, Error: true}
 

internal/controlplane/handlers_githubwebhooks_test.go

@@ -23,6 +23,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"os"
 	"testing"
 	"time"
 
@@ -90,6 +91,7 @@ func (s *UnitTestSuite) TestHandleWebHookPing() {
 
 	mockStore := mockdb.NewMockStore(ctrl)
 	srv, evt := newDefaultServer(t, mockStore)
+	srv.cfg.WebhookConfig.WebhookSecret = "test"
 	defer evt.Close()
 
 	pq := testqueue.NewPassthroughQueue(t)
@@ -126,6 +128,8 @@ func (s *UnitTestSuite) TestHandleWebHookPing() {
 
 	req.Header.Add("X-GitHub-Event", "ping")
 	req.Header.Add("X-GitHub-Delivery", "12345")
+	// the ping event has an empty body ({}), the value below is a SHA256 hmac of the empty body with the shared key "test"
+	req.Header.Add("X-Hub-Signature-256", "sha256=5f5863b9805ad4e66e954a260f9cab3f2e95718798dec0bb48a655195893d10e")
 	req.Header.Add("Content-Type", "application/json")
 	resp, err := httpDoWithRetry(client, req)
 	require.NoError(t, err, "failed to make request")
@@ -207,8 +211,16 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
+	prevCredsFile, err := os.CreateTemp("", "prevcreds*")
+	require.NoError(t, err, "failed to create temporary file")
+	_, err = prevCredsFile.WriteString("also-not-our-secret\ntest")
+	require.NoError(t, err, "failed to write to temporary file")
+	defer os.Remove(prevCredsFile.Name())
+
 	mockStore := mockdb.NewMockStore(ctrl)
 	srv, evt := newDefaultServer(t, mockStore)
+	srv.cfg.WebhookConfig.WebhookSecret = "not-our-secret"
+	srv.cfg.WebhookConfig.PreviousWebhookSecretFile = prevCredsFile.Name()
 	defer evt.Close()
 
 	pq := testqueue.NewPassthroughQueue(t)
@@ -307,6 +319,7 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
 	req.Header.Add("X-GitHub-Event", "meta")
 	req.Header.Add("X-GitHub-Delivery", "12345")
 	req.Header.Add("Content-Type", "application/json")
+	req.Header.Add("X-Hub-Signature-256", "sha256=ab22bd9a3712e444e110c8088011fd827143ed63ba8655f07e76ed1a0f05edd1")
 	resp, err := httpDoWithRetry(client, req)
 	require.NoError(t, err, "failed to make request")
 	// We expect OK since we don't want to leak information about registered repositories