From ac573e7470b0d016750c9115b46ad101f311d896 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 17 Sep 2024 13:25:42 +0300 Subject: [PATCH] Reapply "Add webhook secret bootstrap and verification for Gitlab" (#4506) * Reapply "Add webhook secret bootstrap and verification for Gitlab (#4492)" (#4504) This reverts commit a06e930e807ab5b1b8b714ad30352608e39cbf13. * Handle embedded structurs in our config parsing our config parsing sets the defaults and registers options in viper. It did not handle the case of embedded structures (or squashed tags). This adds that functionality. Signed-off-by: Juan Antonio Osorio --------- Signed-off-by: Juan Antonio Osorio --- cmd/dev/app/rule_type/rttst.go | 2 +- internal/config/server/gitlab.go | 6 + internal/config/server/webhook.go | 14 ++- internal/config/utils.go | 13 +- internal/controlplane/fuzz_test.go | 6 +- internal/providers/gitlab/gitlab.go | 20 ++- internal/providers/gitlab/manager/manager.go | 36 +++++- internal/providers/gitlab/manager/webhook.go | 59 ++++++++- internal/providers/gitlab/registration.go | 27 +++-- .../gitlab/webhooksecret/whsecret.go | 57 +++++++++ .../gitlab/webhooksecret/whsecret_test.go | 114 ++++++++++++++++++ 11 files changed, 327 insertions(+), 27 deletions(-) create mode 100644 internal/providers/gitlab/webhooksecret/whsecret.go create mode 100644 internal/providers/gitlab/webhooksecret/whsecret_test.go diff --git a/cmd/dev/app/rule_type/rttst.go b/cmd/dev/app/rule_type/rttst.go index eaf904a3eb..51a857b7de 100644 --- a/cmd/dev/app/rule_type/rttst.go +++ b/cmd/dev/app/rule_type/rttst.go @@ -438,7 +438,7 @@ func getProvider(pstr string, token string, providerConfigFile string) (provifv1 } // We may pass a "fake" webhook URL here as it is not used in the test - client, err := gitlab.New(credentials.NewGitLabTokenCredential(token), cfg, "fake") + client, err := gitlab.New(credentials.NewGitLabTokenCredential(token), cfg, "fake", "fake") if err != nil { return nil, fmt.Errorf("error instantiating gitlab provider: %w", err) } diff --git a/internal/config/server/gitlab.go b/internal/config/server/gitlab.go index 5f89f8c4d3..58a94c3c6b 100644 --- a/internal/config/server/gitlab.go +++ b/internal/config/server/gitlab.go @@ -19,6 +19,12 @@ package server type GitLabConfig struct { OAuthClientConfig `mapstructure:",squash"` + // WebhookSecrets is the configuration for the GitLab webhook secrets + // setup and verification. This is used to verify incoming webhook requests + // from GitLab, as well as to generate the webhook URL for GitLab to send + // events to. + WebhookSecrets `mapstructure:",squash"` + // Scopes is the list of scopes to request from the GitLab OAuth provider Scopes []string `mapstructure:"scopes"` } diff --git a/internal/config/server/webhook.go b/internal/config/server/webhook.go index 32079b8026..55fe9838ce 100644 --- a/internal/config/server/webhook.go +++ b/internal/config/server/webhook.go @@ -23,10 +23,20 @@ import ( // WebhookConfig is the configuration for our webhook capabilities type WebhookConfig struct { + // WebhookSecrets is the configuration for the webhook secrets. + // This is embedded in the WebhookConfig so that the secrets can be + // used in the WebhookConfig, as the GitHub provider needs for now. + WebhookSecrets `mapstructure:",squash"` // 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"` +} + +// WebhookSecrets is the configuration for the webhook secrets. this is useful +// to import in whatever provider configuration that needs to use some webhook +// secrets. +type WebhookSecrets struct { // WebhookSecret is the secret that we will use to sign our webhook WebhookSecret string `mapstructure:"webhook_secret"` // WebhookSecretFile is the location of the file containing the webhook secret @@ -39,7 +49,7 @@ type WebhookConfig struct { // 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) { +func (wc *WebhookSecrets) 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) @@ -51,6 +61,6 @@ func (wc *WebhookConfig) GetPreviousWebhookSecrets() ([]string, error) { } // GetWebhookSecret returns the GitHub App's webhook secret -func (wc *WebhookConfig) GetWebhookSecret() (string, error) { +func (wc *WebhookSecrets) GetWebhookSecret() (string, error) { return fileOrArg(wc.WebhookSecretFile, wc.WebhookSecret, "webhook secret") } diff --git a/internal/config/utils.go b/internal/config/utils.go index a14a1c1ff0..62e48bd741 100644 --- a/internal/config/utils.go +++ b/internal/config/utils.go @@ -207,7 +207,18 @@ func SetViperStructDefaults(v *viper.Viper, prefix string, s any) { // Error, need a tag panic(fmt.Sprintf("Untagged config struct field %q", field.Name)) } - valueName := strings.ToLower(prefix + field.Tag.Get("mapstructure")) + + var valueName string + // Check if the tag is "squash" and if so, don't add the field name to the prefix + if field.Tag.Get("mapstructure") == ",squash" { + if strings.HasSuffix(prefix, ".") { + valueName = strings.ToLower(prefix[:len(prefix)-1]) + } else { + valueName = strings.ToLower(prefix) + } + } else { + valueName = strings.ToLower(prefix + field.Tag.Get("mapstructure")) + } fieldType := field.Type // Extract a default value the `default` struct tag diff --git a/internal/controlplane/fuzz_test.go b/internal/controlplane/fuzz_test.go index 0a60a9bee9..8c99bbc687 100644 --- a/internal/controlplane/fuzz_test.go +++ b/internal/controlplane/fuzz_test.go @@ -118,7 +118,11 @@ func FuzzGitHubEventParsers(f *testing.F) { } defer os.Remove(whSecretFile.Name()) - whConfig := &server.WebhookConfig{WebhookSecretFile: whSecretFile.Name()} + whConfig := &server.WebhookConfig{ + WebhookSecrets: server.WebhookSecrets{ + WebhookSecretFile: whSecretFile.Name(), + }, + } s := &Server{} ctx := context.Background() diff --git a/internal/providers/gitlab/gitlab.go b/internal/providers/gitlab/gitlab.go index c206a19f29..a2947e7892 100644 --- a/internal/providers/gitlab/gitlab.go +++ b/internal/providers/gitlab/gitlab.go @@ -57,11 +57,20 @@ type gitlabClient struct { glcfg *minderv1.GitLabProviderConfig webhookURL string gitConfig config.GitConfig + + // secret for the webhook. This is stored in the + // structure to allow efficient fetching. + currentWebhookSecret string } // New creates a new GitLab provider // Note that the webhook URL should already contain the provider class in the path -func New(cred provifv1.GitLabCredential, cfg *minderv1.GitLabProviderConfig, webhookURL string) (*gitlabClient, error) { +func New( + cred provifv1.GitLabCredential, + cfg *minderv1.GitLabProviderConfig, + webhookURL string, + currentWebhookSecret string, +) (*gitlabClient, error) { // TODO: We need a context here. cli := oauth2.NewClient(context.Background(), cred.GetAsOAuth2TokenSource()) @@ -74,10 +83,11 @@ func New(cred provifv1.GitLabCredential, cfg *minderv1.GitLabProviderConfig, web } return &gitlabClient{ - cred: cred, - cli: cli, - glcfg: cfg, - webhookURL: webhookURL, + cred: cred, + cli: cli, + glcfg: cfg, + webhookURL: webhookURL, + currentWebhookSecret: currentWebhookSecret, // TODO: Add git config }, nil } diff --git a/internal/providers/gitlab/manager/manager.go b/internal/providers/gitlab/manager/manager.go index 66b8917954..5f83d199f7 100644 --- a/internal/providers/gitlab/manager/manager.go +++ b/internal/providers/gitlab/manager/manager.go @@ -24,6 +24,8 @@ import ( "net/url" "slices" + "github.com/rs/zerolog" + "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/crypto" "github.com/stacklok/minder/internal/db" @@ -39,6 +41,12 @@ type providerClassManager struct { glpcfg *server.GitLabConfig webhookURL string parentContext context.Context + + // secrets for the webhook. These are stored in the + // structure to allow efficient fetching. Rotation + // requires a process restart. + currentWebhookSecret string + previousWebhookSecrets []string } // NewGitLabProviderClassManager creates a new provider class manager for the dockerhub provider @@ -50,17 +58,33 @@ func NewGitLabProviderClassManager( return nil, errors.New("webhook URL is required") } + if cfg == nil { + return nil, errors.New("gitlab config is required") + } + webhookURL, err := url.JoinPath(webhookURLBase, url.PathEscape(string(db.ProviderClassGitlab))) if err != nil { return nil, fmt.Errorf("error joining webhook URL: %w", err) } + whSecret, err := cfg.GetWebhookSecret() + if err != nil { + return nil, fmt.Errorf("error getting webhook secret: %w", err) + } + + previousSecrets, err := cfg.GetPreviousWebhookSecrets() + if err != nil { + zerolog.Ctx(ctx).Error().Err(err).Msg("previous secrets not loaded") + } + return &providerClassManager{ - store: store, - crypteng: crypteng, - glpcfg: cfg, - webhookURL: webhookURL, - parentContext: ctx, + store: store, + crypteng: crypteng, + glpcfg: cfg, + webhookURL: webhookURL, + parentContext: ctx, + currentWebhookSecret: whSecret, + previousWebhookSecrets: previousSecrets, }, nil } @@ -91,7 +115,7 @@ func (g *providerClassManager) Build(ctx context.Context, config *db.Provider) ( return nil, fmt.Errorf("error parsing gitlab config: %w", err) } - cli, err := gitlab.New(creds, cfg, g.webhookURL) + cli, err := gitlab.New(creds, cfg, g.webhookURL, g.currentWebhookSecret) if err != nil { return nil, fmt.Errorf("error creating gitlab client: %w", err) } diff --git a/internal/providers/gitlab/manager/webhook.go b/internal/providers/gitlab/manager/webhook.go index dd5e332b1d..62358fdfe9 100644 --- a/internal/providers/gitlab/manager/webhook.go +++ b/internal/providers/gitlab/manager/webhook.go @@ -15,16 +15,22 @@ package manager import ( + "errors" + "fmt" "net/http" + "strings" + "github.com/google/uuid" "github.com/rs/zerolog" + + "github.com/stacklok/minder/internal/providers/gitlab/webhooksecret" ) // GetWebhookHandler implements the ProviderManager interface // Note that this is where the whole webhook handler is defined and // will live. func (m *providerClassManager) GetWebhookHandler() http.Handler { - return http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { l := zerolog.Ctx(m.parentContext).With(). Str("webhook", "gitlab"). Str("method", r.Method). @@ -34,8 +40,57 @@ func (m *providerClassManager) GetWebhookHandler() http.Handler { Str("content-type", r.Header.Get("Content-Type")). Logger() - // TODO: Implement webhook handler + // Validate the webhook secret + if err := m.validateRequest(r); err != nil { + l.Error().Err(err).Msg("invalid webhook request") + http.Error(w, "invalid webhook request", http.StatusUnauthorized) + return + } l.Debug().Msg("received webhook") }) } + +func (m *providerClassManager) validateRequest(r *http.Request) error { + // Validate the webhook secret + gltok := r.Header.Get("X-Gitlab-Token") + if gltok == "" { + return errors.New("missing X-Gitlab-Token header") + } + + if err := m.validateToken(gltok, r); err != nil { + return fmt.Errorf("invalid X-Gitlab-Token header: %w", err) + } + + return nil +} + +// validateToken validates the incoming GitLab webhook token +// Validation takes the secret from the GitLab webhook configuration +// appens the last element of the path to the URL (which is unique per entity) +func (m *providerClassManager) validateToken(token string, req *http.Request) error { + // Extract the unique ID from the URL path + path := req.URL.Path + uniq := path[strings.LastIndex(path, "/")+1:] + + // uniq must be a valid UUID + _, err := uuid.Parse(uniq) + if err != nil { + return errors.New("invalid unique ID") + } + + // Generate the expected secret + if valid := webhooksecret.Verify(m.currentWebhookSecret, uniq, token); valid { + // If the secret is valid, we can return + return nil + } + + // Check the previous secrets + for _, prev := range m.previousWebhookSecrets { + if valid := webhooksecret.Verify(prev, uniq, token); valid { + return nil + } + } + + return errors.New("invalid webhook token") +} diff --git a/internal/providers/gitlab/registration.go b/internal/providers/gitlab/registration.go index 8318161046..de340e3602 100644 --- a/internal/providers/gitlab/registration.go +++ b/internal/providers/gitlab/registration.go @@ -29,6 +29,7 @@ import ( "github.com/xanzy/go-gitlab" "github.com/stacklok/minder/internal/entities/properties" + "github.com/stacklok/minder/internal/providers/gitlab/webhooksecret" "github.com/stacklok/minder/internal/util/ptr" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) @@ -57,8 +58,11 @@ func (c *gitlabClient) RegisterEntity( whprops, err := c.createWebhook(ctx, upstreamID) if err != nil { - // There is already enough context in the error message - return nil, err + zerolog.Ctx(ctx).Error(). + Str("upstreamID", upstreamID). + Str("provider-class", Class). + Err(err).Msg("failed to create webhook") + return nil, errors.New("failed to create webhook") } return props.Merge(whprops), nil @@ -102,15 +106,20 @@ func (c *gitlabClient) createWebhook(ctx context.Context, upstreamID string) (*p return nil, fmt.Errorf("failed to join URL path for webhook: %w", err) } + sec, err := webhooksecret.New(c.currentWebhookSecret, hookUUID.String()) + if err != nil { + return nil, fmt.Errorf("failed to create webhook secret: %w", err) + } + trve := ptr.Ptr(true) hreq := &gitlab.AddProjectHookOptions{ - URL: &webhookUniqueURL, - // TODO: Add secret - PushEvents: trve, - TagPushEvents: trve, - MergeRequestsEvents: trve, - ReleasesEvents: trve, - // TODO: Enable SSL verification + URL: &webhookUniqueURL, + Token: &sec, + PushEvents: trve, + TagPushEvents: trve, + MergeRequestsEvents: trve, + ReleasesEvents: trve, + EnableSSLVerification: trve, } hook, err := c.doCreateWebhook(ctx, createHookPath, hreq) diff --git a/internal/providers/gitlab/webhooksecret/whsecret.go b/internal/providers/gitlab/webhooksecret/whsecret.go new file mode 100644 index 0000000000..cfb89fc74a --- /dev/null +++ b/internal/providers/gitlab/webhooksecret/whsecret.go @@ -0,0 +1,57 @@ +// +// Copyright 2024 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 webhooksecret provides a way to generate and verify secrets for GitLab webhooks. +package webhooksecret + +import ( + sum "crypto/sha512" + "encoding/hex" + "errors" + "fmt" +) + +var ( + // ErrEmptyBaseOrUniq is returned when the base or uniq strings are empty. + ErrEmptyBaseOrUniq = errors.New("base or uniq strings are empty") +) + +// New creates a new secret for usage in the gitlab webhook. +// The secret is generated by combining the base and uniq strings +// and then hashing the result. +func New(base string, uniq string) (string, error) { + if base == "" || uniq == "" { + return "", ErrEmptyBaseOrUniq + } + + hash := sum.New() + _, err := hash.Write([]byte(base + uniq)) + if err != nil { + return "", fmt.Errorf("failed to write secret: %w", err) + } + + return hex.EncodeToString(hash.Sum(nil)), nil +} + +// Verify checks if the given secret is valid for the given base and uniq strings. +func Verify(base string, uniq string, secret string) bool { + s, err := New(base, uniq) + if err != nil { + // If we can't generate the secret, we can't verify it. + return false + } + + return s == secret +} diff --git a/internal/providers/gitlab/webhooksecret/whsecret_test.go b/internal/providers/gitlab/webhooksecret/whsecret_test.go new file mode 100644 index 0000000000..1900c849dc --- /dev/null +++ b/internal/providers/gitlab/webhooksecret/whsecret_test.go @@ -0,0 +1,114 @@ +// +// Copyright 2024 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 webhooksecret + +import ( + sum "crypto/sha512" + "encoding/hex" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNew(t *testing.T) { + t.Parallel() + + base := "baseString" + uniq := "uniqueString" + expectedHash := sum.New() + expectedHash.Write([]byte(base + uniq)) + expectedSecret := hex.EncodeToString(expectedHash.Sum(nil)) + + secret, err := New(base, uniq) + assert.NoError(t, err) + assert.Equal(t, expectedSecret, secret, "they should be equal") +} + +func TestNew_EmptyStrings(t *testing.T) { + t.Parallel() + + base := "" + uniq := "" + secret, err := New(base, uniq) + assert.Error(t, err) + assert.Equal(t, ErrEmptyBaseOrUniq, err) + assert.Empty(t, secret) +} + +func TestNew_SpecialCharacters(t *testing.T) { + t.Parallel() + + base := "base@String!" + uniq := "unique#String$" + expectedHash := sum.New() + expectedHash.Write([]byte(base + uniq)) + expectedSecret := hex.EncodeToString(expectedHash.Sum(nil)) + + secret, err := New(base, uniq) + assert.NoError(t, err) + assert.Equal(t, expectedSecret, secret, "they should be equal") +} + +func TestVerify(t *testing.T) { + t.Parallel() + + base := "baseString" + uniq := "uniqueString" + secret, err := New(base, uniq) + assert.NoError(t, err) + + assert.True(t, Verify(base, uniq, secret), "the secret should be valid") + assert.False(t, Verify(base, uniq, "invalidSecret"), "the secret should be invalid") +} + +func TestVerify_EmptyStrings(t *testing.T) { + t.Parallel() + + base := "" + uniq := "" + secret, err := New(base, uniq) + assert.Error(t, err) + assert.False(t, Verify(base, uniq, secret), "the secret should be invalid") +} + +func TestVerify_SpecialCharacters(t *testing.T) { + t.Parallel() + + base := "base@String!" + uniq := "unique#String$" + secret, err := New(base, uniq) + assert.NoError(t, err) + + assert.True(t, Verify(base, uniq, secret), "the secret should be valid") + assert.False(t, Verify(base, uniq, "invalidSecret"), "the secret should be invalid") +} + +func TestVerify_DifferentBaseUniq(t *testing.T) { + t.Parallel() + + base1 := "baseString1" + uniq1 := "uniqueString1" + secret1, err := New(base1, uniq1) + assert.NoError(t, err) + + base2 := "baseString2" + uniq2 := "uniqueString2" + secret2, err := New(base2, uniq2) + assert.NoError(t, err) + + assert.False(t, Verify(base1, uniq1, secret2), "the secret should be invalid") + assert.False(t, Verify(base2, uniq2, secret1), "the secret should be invalid") +}