diff --git a/config/server-config.yaml.example b/config/server-config.yaml.example index ba06fd9469..f5882b1c66 100644 --- a/config/server-config.yaml.example +++ b/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 diff --git a/docs/docs/run_minder_server/run_the_server.md b/docs/docs/run_minder_server/run_the_server.md index 120009ab85..624e30adc2 100644 --- a/docs/docs/run_minder_server/run_the_server.md +++ b/docs/docs/run_minder_server/run_the_server.md @@ -249,6 +249,13 @@ 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 @@ -256,7 +263,7 @@ 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`. \ No newline at end of file +The application will be available on `http://localhost:8080` and gRPC on `localhost:8090`. diff --git a/internal/config/server/webhook.go b/internal/config/server/webhook.go index d691b0ab93..d80d2f5e06 100644 --- a/internal/config/server/webhook.go +++ b/internal/config/server/webhook.go @@ -15,6 +15,12 @@ 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 @@ -22,6 +28,22 @@ type WebhookConfig struct { // 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 } diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go index 0456e716d6..839170d979 100644 --- a/internal/controlplane/handlers_githubwebhooks.go +++ b/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} diff --git a/internal/controlplane/handlers_githubwebhooks_test.go b/internal/controlplane/handlers_githubwebhooks_test.go index 458b5822f2..d1d9a94482 100644 --- a/internal/controlplane/handlers_githubwebhooks_test.go +++ b/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