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