From b68ca59eec2fd3c67fa3556f7d88d44d8c66b3cc Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 22 Mar 2024 08:49:40 +0100 Subject: [PATCH] Add a CLI for rotating webhook secrets (#2735) * Split out the isMinderHook helper into the GH provider code * Add EditHook() to the GH provider interface * Add CLI to update webhooks --- cmd/server/app/webhook.go | 35 +++ cmd/server/app/webhook_update.go | 202 ++++++++++++++++++ docs/docs/run_minder_server/run_the_server.md | 10 + .../handlers_repositories_test.go | 4 + internal/providers/github/common.go | 23 ++ internal/providers/github/mock/github.go | 15 ++ .../repositories/github/webhooks/manager.go | 18 +- pkg/providers/v1/providers.go | 1 + 8 files changed, 291 insertions(+), 17 deletions(-) create mode 100644 cmd/server/app/webhook.go create mode 100644 cmd/server/app/webhook_update.go diff --git a/cmd/server/app/webhook.go b/cmd/server/app/webhook.go new file mode 100644 index 0000000000..f8aa563ec0 --- /dev/null +++ b/cmd/server/app/webhook.go @@ -0,0 +1,35 @@ +// +// Copyright 2023 Stacklok, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package app + +import ( + "github.com/spf13/cobra" +) + +// cmdWebhook is the root command for the webhook subcommands +func cmdWebhook() *cobra.Command { + var whCmd = &cobra.Command{ + Use: "webhook", + Short: "Webhook management tool", + } + + whCmd.AddCommand(cmdWebhookUpdate()) + return whCmd +} + +func init() { + RootCmd.AddCommand(cmdWebhook()) +} diff --git a/cmd/server/app/webhook_update.go b/cmd/server/app/webhook_update.go new file mode 100644 index 0000000000..35e7bbb5ed --- /dev/null +++ b/cmd/server/app/webhook_update.go @@ -0,0 +1,202 @@ +// +// Copyright 2023 Stacklok, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package app + +import ( + "context" + "database/sql" + "errors" + "fmt" + "net/url" + "os" + "strings" + + "github.com/rs/zerolog" + "github.com/spf13/cobra" + "github.com/spf13/viper" + + "github.com/stacklok/minder/internal/config" + serverconfig "github.com/stacklok/minder/internal/config/server" + "github.com/stacklok/minder/internal/crypto" + "github.com/stacklok/minder/internal/db" + "github.com/stacklok/minder/internal/logger" + "github.com/stacklok/minder/internal/providers" + ghprovider "github.com/stacklok/minder/internal/providers/github" + provifv1 "github.com/stacklok/minder/pkg/providers/v1" +) + +func cmdWebhookUpdate() *cobra.Command { + var updateCmd = &cobra.Command{ + Use: "update", + Short: "update the webhook configuration", + Long: `Command to upgrade webhook configuration`, + RunE: runCmdWebhookUpdate, + } + + updateCmd.Flags().StringP("provider", + "p", "github", + "what provider interface must the provider implement to be updated") + viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + + if err := updateCmd.MarkFlagRequired("provider"); err != nil { + fmt.Fprintf(os.Stderr, "Error marking flag as required: %s\n", err) + os.Exit(1) + } + + return updateCmd +} + +func runCmdWebhookUpdate(cmd *cobra.Command, _ []string) error { + cfg, err := config.ReadConfigFromViper[serverconfig.Config](viper.GetViper()) + if err != nil { + return fmt.Errorf("unable to read config: %w", err) + } + + ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + + providerName := cmd.Flag("provider").Value.String() + + cryptoEng, err := crypto.EngineFromAuthConfig(&cfg.Auth) + if err != nil { + return fmt.Errorf("failed to create crypto engine: %w", err) + } + + dbConn, _, err := cfg.Database.GetDBConnection(ctx) + if err != nil { + return fmt.Errorf("unable to connect to database: %w", err) + } + defer func(dbConn *sql.DB) { + err := dbConn.Close() + if err != nil { + zerolog.Ctx(ctx).Error().Err(err).Msg("error closing database connection") + } + }(dbConn) + + store := db.NewStore(dbConn) + allProviders, err := store.GlobalListProviders(ctx) + if err != nil { + return fmt.Errorf("unable to list providers: %w", err) + } + + webhookUrl, err := url.Parse(cfg.WebhookConfig.ExternalWebhookURL) + if err != nil { + return fmt.Errorf("unable to parse webhook url: %w", err) + } + + for _, provider := range allProviders { + zerolog.Ctx(ctx).Info().Str("name", provider.Name).Str("uuid", provider.ID.String()).Msg("provider") + pb, err := providers.GetProviderBuilder(ctx, provider, store, cryptoEng, &cfg.Provider) + if err != nil { + return fmt.Errorf("unable to get provider builder: %w", err) + } + + if !pb.Implements(db.ProviderType(providerName)) { + zerolog.Ctx(ctx).Info(). + Str("name", provider.Name). + Str("uuid", provider.ID.String()). + Msg("provider does not implement the requested provider interface") + continue + } + + var updateErr error + + if db.ProviderType(providerName) == db.ProviderTypeGithub { + ghCli, err := pb.GetGitHub() + if err != nil { + zerolog.Ctx(ctx).Err(err).Msg("cannot get github client") + } + updateErr = updateGithubWebhooks(ctx, ghCli, store, provider, webhookUrl.Host, cfg.WebhookConfig.WebhookSecret) + } else { + updateErr = fmt.Errorf("provider type %s not supported", providerName) + } + + if updateErr != nil { + zerolog.Ctx(ctx).Err(updateErr).Msg("unable to update webhooks") + } + } + + return nil +} + +func updateGithubWebhooks( + ctx context.Context, + ghCli provifv1.GitHub, + store db.Store, + provider db.Provider, + webhookHost string, + secret string, +) error { + repos, err := store.ListRegisteredRepositoriesByProjectIDAndProvider(ctx, + db.ListRegisteredRepositoriesByProjectIDAndProviderParams{ + Provider: provider.Name, + ProjectID: provider.ProjectID, + }) + if err != nil { + return fmt.Errorf("unable to list registered repositories: %w", err) + } + + for _, repo := range repos { + err := updateGithubRepoHooks(ctx, ghCli, repo, webhookHost, secret) + if err != nil { + zerolog.Ctx(ctx).Err(err).Msg("unable to update repo hooks") + continue + } + } + + return nil +} + +func updateGithubRepoHooks( + ctx context.Context, + ghCli provifv1.GitHub, + repo db.Repository, + webhookHost string, + secret string, +) error { + repoLogger := zerolog.Ctx(ctx).With().Str("repo", repo.RepoName).Str("uuid", repo.ID.String()).Logger() + repoLogger.Info().Msg("updating repo hooks") + + hooks, err := ghCli.ListHooks(ctx, repo.RepoOwner, repo.RepoName) + if errors.Is(err, ghprovider.ErrNotFound) { + repoLogger.Debug().Msg("no hooks found") + return nil + } else if err != nil { + return fmt.Errorf("unable to list hooks: %w", err) + } + + for _, hook := range hooks { + hookLogger := repoLogger.With().Int64("hook_id", hook.GetID()).Str("url", hook.GetURL()).Logger() + isMinder, err := ghprovider.IsMinderHook(hook, webhookHost) + if err != nil { + hookLogger.Err(err).Msg("unable to determine if hook is a minder hook") + continue + } + if !isMinder { + hookLogger.Info().Msg("hook is not a minder hook") + continue + } + + hook.Config["secret"] = secret + _, err = ghCli.EditHook(ctx, repo.RepoOwner, repo.RepoName, hook.GetID(), hook) + if err != nil { + hookLogger.Err(err).Msg("unable to update hook") + continue + } + hookLogger.Info().Msg("hook updated") + } + + return nil +} diff --git a/docs/docs/run_minder_server/run_the_server.md b/docs/docs/run_minder_server/run_the_server.md index a044f2380f..120009ab85 100644 --- a/docs/docs/run_minder_server/run_the_server.md +++ b/docs/docs/run_minder_server/run_the_server.md @@ -249,6 +249,16 @@ webhook-config: After these steps, your Minder server should be ready to receive webhook events from GitHub, and add webhooks to repositories. +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 +to match the provider's secret. + ## Run the application ```bash diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go index 2f6d57cdcc..75b98deb24 100644 --- a/internal/controlplane/handlers_repositories_test.go +++ b/internal/controlplane/handlers_repositories_test.go @@ -395,6 +395,10 @@ func (_ *StubGitHub) CreateHook(_ context.Context, _ string, _ string, _ *github panic("unimplemented") } +func (*StubGitHub) EditHook(context.Context, string, string, int64, *github.Hook) (*github.Hook, error) { + panic("unimplemented") +} + // CreatePullRequest implements v1.GitHub. func (*StubGitHub) CreatePullRequest(context.Context, string, string, string, string, string, string) (*github.PullRequest, error) { panic("unimplemented") diff --git a/internal/providers/github/common.go b/internal/providers/github/common.go index c816f7cdcf..effd452f40 100644 --- a/internal/providers/github/common.go +++ b/internal/providers/github/common.go @@ -493,6 +493,12 @@ func (c *GitHub) CreateHook(ctx context.Context, owner, repo string, hook *githu return h, err } +// EditHook edits an existing Hook. +func (c *GitHub) EditHook(ctx context.Context, owner, repo string, id int64, hook *github.Hook) (*github.Hook, error) { + h, _, err := c.client.Repositories.EditHook(ctx, owner, repo, id, hook) + return h, err +} + // CreateSecurityAdvisory creates a new security advisory func (c *GitHub) CreateSecurityAdvisory(ctx context.Context, owner, repo, severity, summary, description string, v []*github.AdvisoryVulnerability) (string, error) { @@ -826,3 +832,20 @@ func convertRepository(repo *github.Repository) *minderv1.Repository { IsFork: *repo.Fork, } } + +// IsMinderHook checks if a GitHub hook is a Minder hook +func IsMinderHook(hook *github.Hook, hostURL string) (bool, error) { + configURL, ok := hook.Config["url"].(string) + if !ok || configURL == "" { + return false, fmt.Errorf("unexpected hook config structure: %v", hook.Config) + } + parsedURL, err := url.Parse(configURL) + if err != nil { + return false, err + } + if parsedURL.Host == hostURL { + return true, nil + } + + return false, nil +} diff --git a/internal/providers/github/mock/github.go b/internal/providers/github/mock/github.go index 583819557a..5fb869ee01 100644 --- a/internal/providers/github/mock/github.go +++ b/internal/providers/github/mock/github.go @@ -388,6 +388,21 @@ func (mr *MockGitHubMockRecorder) Do(ctx, req any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Do", reflect.TypeOf((*MockGitHub)(nil).Do), ctx, req) } +// EditHook mocks base method. +func (m *MockGitHub) EditHook(ctx context.Context, owner, repo string, id int64, hook *github.Hook) (*github.Hook, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "EditHook", ctx, owner, repo, id, hook) + ret0, _ := ret[0].(*github.Hook) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// EditHook indicates an expected call of EditHook. +func (mr *MockGitHubMockRecorder) EditHook(ctx, owner, repo, id, hook any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EditHook", reflect.TypeOf((*MockGitHub)(nil).EditHook), ctx, owner, repo, id, hook) +} + // GetBaseURL mocks base method. func (m *MockGitHub) GetBaseURL() string { m.ctrl.T.Helper() diff --git a/internal/repositories/github/webhooks/manager.go b/internal/repositories/github/webhooks/manager.go index 520e832f88..d13f2dcd96 100644 --- a/internal/repositories/github/webhooks/manager.go +++ b/internal/repositories/github/webhooks/manager.go @@ -148,7 +148,7 @@ func (w *webhookManager) cleanupStaleHooks( for _, hook := range hooks { // it is our hook, we can remove it - shouldDelete, err := isMinderHook(hook, webhookHost) + shouldDelete, err := ghprovider.IsMinderHook(hook, webhookHost) // If err != nil, shouldDelete == false - use one error check for both calls if shouldDelete { err = w.DeleteWebhook(ctx, client, repoOwner, repoName, hook.GetID()) @@ -160,19 +160,3 @@ func (w *webhookManager) cleanupStaleHooks( return nil } - -func isMinderHook(hook *github.Hook, hostURL string) (bool, error) { - configURL, ok := hook.Config["url"].(string) - if !ok || configURL == "" { - return false, fmt.Errorf("unexpected hook config structure: %v", hook.Config) - } - parsedURL, err := url.Parse(configURL) - if err != nil { - return false, err - } - if parsedURL.Host == hostURL { - return true, nil - } - - return false, nil -} diff --git a/pkg/providers/v1/providers.go b/pkg/providers/v1/providers.go index cd4bcf5994..1b7c094af5 100644 --- a/pkg/providers/v1/providers.go +++ b/pkg/providers/v1/providers.go @@ -99,6 +99,7 @@ type GitHub interface { GetOwner() string ListHooks(ctx context.Context, owner, repo string) ([]*github.Hook, error) DeleteHook(ctx context.Context, owner, repo string, id int64) (*github.Response, error) + EditHook(ctx context.Context, owner, repo string, id int64, hook *github.Hook) (*github.Hook, error) CreateHook(ctx context.Context, owner, repo string, hook *github.Hook) (*github.Hook, error) CreateSecurityAdvisory(ctx context.Context, owner, repo, severity, summary, description string, v []*github.AdvisoryVulnerability) (string, error)