Merge commit from fork

* feat: limit payload size Signed-off-by: pashakostohrys <[email protected]> * git cherry-pick a6841386468365e458a61896cc48ff18163f25c0 Signed-off-by: pashakostohrys <[email protected]> * git cherry-pick a08356a8eb13ced5850ec886f4a3b823de606f65 Signed-off-by: pashakostohrys <[email protected]> * git cherry-pick f0a1f1efe721d486d6fe6cb4f645b269c6781794 Signed-off-by: pashakostohrys <[email protected]> * git cherry-pick 3dd77292073c692f0deb7b4296976a60489bc442 Signed-off-by: pashakostohrys <[email protected]> * git cherry-pick 3bf68adf84c09c44f2c42548b8421df127d61587 Signed-off-by: pashakostohrys <[email protected]> * fix cherry-pick issues Signed-off-by: pashakostohrys <[email protected]> * fix cherry-pick issues Signed-off-by: pashakostohrys <[email protected]> --------- Signed-off-by: pashakostohrys <[email protected]>
argoproj · Jul 22, 2024 · 540e3a5 · 540e3a5
1 parent b980386
commit 540e3a5
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 5 deletions.
diff --git a/docs/operator-manual/argocd-cm.yaml b/docs/operator-manual/argocd-cm.yaml
@@ -410,3 +410,6 @@ data:
               cluster:
                 name: some-cluster
                 server: https://some-cluster
+          
+          # The maximum size of the payload that can be sent to the webhook server.
+          webhook.maxPayloadSizeMB: 1024
diff --git a/docs/operator-manual/webhook.md b/docs/operator-manual/webhook.md
@@ -19,6 +19,7 @@ URL configured in the Git provider should use the `/api/webhook` endpoint of you
 (e.g. `https://argocd.example.com/api/webhook`). If you wish to use a shared secret, input an
 arbitrary value in the secret. This value will be used when configuring the webhook in the next step.
 
+To prevent DDoS attacks with unauthenticated webhook events (the `/api/webhook` endpoint currently lacks rate limiting protection), it is recommended to limit the payload size. You can achieve this by configuring the `argocd-cm` ConfigMap with the `webhook.maxPayloadSizeMB` attribute. The default value is 1GB.
 ## Github
 
 ![Add Webhook](../assets/webhook-config.png "Add Webhook")

diff --git a/server/server.go b/server/server.go
@@ -1048,7 +1048,7 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl
 
 	// Webhook handler for git events (Note: cache timeouts are hardcoded because API server does not write to cache and not really using them)
 	argoDB := db.NewDB(a.Namespace, a.settingsMgr, a.KubeClientset)
-	acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB)
+	acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB, a.settingsMgr.GetMaxWebhookPayloadSize())
 
 	mux.HandleFunc("/api/webhook", acdWebhookHandler.Handler)
 

diff --git a/util/settings/settings.go b/util/settings/settings.go
@@ -431,6 +431,8 @@ const (
 	settingsWebhookAzureDevOpsUsernameKey = "webhook.azuredevops.username"
 	// settingsWebhookAzureDevOpsPasswordKey is the key for Azure DevOps webhook password
 	settingsWebhookAzureDevOpsPasswordKey = "webhook.azuredevops.password"
+	// settingsWebhookMaxPayloadSize is the key for the maximum payload size for webhooks in MB
+	settingsWebhookMaxPayloadSizeMB = "webhook.maxPayloadSizeMB"
 	// settingsApplicationInstanceLabelKey is the key to configure injected app instance label key
 	settingsApplicationInstanceLabelKey = "application.instanceLabelKey"
 	// settingsResourceTrackingMethodKey is the key to configure tracking method for application resources
@@ -518,6 +520,11 @@ var (
 	}
 )
 
+const (
+	// default max webhook payload size is 1GB
+	defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024
+)
+
 // SettingsManager holds config info for a new manager with which to access Kubernetes ConfigMaps.
 type SettingsManager struct {
 	ctx             context.Context
@@ -2221,3 +2228,22 @@ func (mgr *SettingsManager) GetResourceCustomLabels() ([]string, error) {
 	}
 	return []string{}, nil
 }
+
+func (mgr *SettingsManager) GetMaxWebhookPayloadSize() int64 {
+	argoCDCM, err := mgr.getConfigMap()
+	if err != nil {
+		return defaultMaxWebhookPayloadSize
+	}
+
+	if argoCDCM.Data[settingsWebhookMaxPayloadSizeMB] == "" {
+		return defaultMaxWebhookPayloadSize
+	}
+
+	maxPayloadSizeMB, err := strconv.ParseInt(argoCDCM.Data[settingsWebhookMaxPayloadSizeMB], 10, 64)
+	if err != nil {
+		log.Warnf("Failed to parse '%s' key: %v", settingsWebhookMaxPayloadSizeMB, err)
+		return defaultMaxWebhookPayloadSize
+	}
+
+	return maxPayloadSizeMB * 1024 * 1024
+}
diff --git a/util/webhook/webhook.go b/util/webhook/webhook.go
@@ -41,6 +41,8 @@ type settingsSource interface {
 // https://github.com/shadow-maint/shadow/blob/master/libmisc/chkname.c#L36
 const usernameRegex = `[a-zA-Z0-9_\.][a-zA-Z0-9_\.-]{0,30}[a-zA-Z0-9_\.\$-]?`
 
+const payloadQueueSize = 50000
+
 var (
 	_                              settingsSource = &settings.SettingsManager{}
 	errBasicAuthVerificationFailed                = errors.New("basic auth verification failed")
@@ -61,9 +63,11 @@ type ArgoCDWebhookHandler struct {
 	azuredevopsAuthHandler func(r *http.Request) error
 	gogs                   *gogs.Webhook
 	settingsSrc            settingsSource
+	queue                  chan interface{}
+	maxWebhookPayloadSizeB int64
 }
 
-func NewHandler(namespace string, applicationNamespaces []string, appClientset appclientset.Interface, set *settings.ArgoCDSettings, settingsSrc settingsSource, repoCache *cache.Cache, serverCache *servercache.Cache, argoDB db.ArgoDB) *ArgoCDWebhookHandler {
+func NewHandler(namespace string, applicationNamespaces []string, appClientset appclientset.Interface, set *settings.ArgoCDSettings, settingsSrc settingsSource, repoCache *cache.Cache, serverCache *servercache.Cache, argoDB db.ArgoDB, maxWebhookPayloadSizeB int64) *ArgoCDWebhookHandler {
 	githubWebhook, err := github.New(github.Options.Secret(set.WebhookGitHubSecret))
 	if err != nil {
 		log.Warnf("Unable to init the GitHub webhook")
@@ -113,6 +117,8 @@ func NewHandler(namespace string, applicationNamespaces []string, appClientset a
 		repoCache:              repoCache,
 		serverCache:            serverCache,
 		db:                     argoDB,
+		queue:                  make(chan interface{}, payloadQueueSize),
+		maxWebhookPayloadSizeB: maxWebhookPayloadSizeB,
 	}
 
 	return &acdWebhook
@@ -388,6 +394,8 @@ func (a *ArgoCDWebhookHandler) Handler(w http.ResponseWriter, r *http.Request) {
 	var payload interface{}
 	var err error
 
+	r.Body = http.MaxBytesReader(w, r.Body, a.maxWebhookPayloadSizeB)
+
 	switch {
 	case r.Header.Get("X-Vss-Activityid") != "":
 		if err = a.azuredevopsAuthHandler(r); err != nil {
@@ -430,6 +438,14 @@ func (a *ArgoCDWebhookHandler) Handler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if err != nil {
+		// If the error is due to a large payload, return a more user-friendly error message
+		if err.Error() == "error parsing payload" {
+			msg := fmt.Sprintf("Webhook processing failed: The payload is either too large or corrupted. Please check the payload size (must be under %v MB) and ensure it is valid JSON", a.maxWebhookPayloadSizeB/1024/1024)
+			log.WithField(common.SecurityField, common.SecurityHigh).Warn(msg)
+			http.Error(w, msg, http.StatusBadRequest)
+			return
+		}
+
 		log.Infof("Webhook processing failed: %s", err)
 		status := http.StatusBadRequest
 		if r.Method != http.MethodPost {

diff --git a/util/webhook/webhook_test.go b/util/webhook/webhook_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"github.com/stretchr/testify/require"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -56,6 +57,11 @@ type reactorDef struct {
 }
 
 func NewMockHandler(reactor *reactorDef, applicationNamespaces []string, objects ...runtime.Object) *ArgoCDWebhookHandler {
+	defaultMaxPayloadSize := int64(1) * 1024 * 1024 * 1024
+	return NewMockHandlerWithPayloadLimit(reactor, applicationNamespaces, defaultMaxPayloadSize, objects...)
+}
+
+func NewMockHandlerWithPayloadLimit(reactor *reactorDef, applicationNamespaces []string, maxPayloadSize int64, objects ...runtime.Object) *ArgoCDWebhookHandler {
 	appClientset := appclientset.NewSimpleClientset(objects...)
 	if reactor != nil {
 		defaultReactor := appClientset.ReactionChain[0]
@@ -72,7 +78,7 @@ func NewMockHandler(reactor *reactorDef, applicationNamespaces []string, objects
 		1*time.Minute,
 		1*time.Minute,
 		10*time.Second,
-	), servercache.NewCache(appstate.NewCache(cacheClient, time.Minute), time.Minute, time.Minute, time.Minute), &mocks.ArgoDB{})
+	), servercache.NewCache(appstate.NewCache(cacheClient, time.Minute), time.Minute, time.Minute, time.Minute), &mocks.ArgoDB{}, maxPayloadSize)
 }
 
 func TestGitHubCommitEvent(t *testing.T) {
@@ -392,8 +398,9 @@ func TestInvalidEvent(t *testing.T) {
 	req.Header.Set("X-GitHub-Event", "push")
 	w := httptest.NewRecorder()
 	h.Handler(w, req)
-	assert.Equal(t, w.Code, http.StatusBadRequest)
-	expectedLogResult := "Webhook processing failed: error parsing payload"
+	close(h.queue)
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	expectedLogResult := "Webhook processing failed: The payload is either too large or corrupted. Please check the payload size (must be under 1024 MB) and ensure it is valid JSON"
 	assert.Equal(t, expectedLogResult, hook.LastEntry().Message)
 	assert.Equal(t, expectedLogResult+"\n", w.Body.String())
 	hook.Reset()
@@ -604,3 +611,21 @@ func Test_getWebUrlRegex(t *testing.T) {
 		})
 	}
 }
+
+func TestGitHubCommitEventMaxPayloadSize(t *testing.T) {
+	hook := test.NewGlobal()
+	maxPayloadSize := int64(100)
+	h := NewMockHandlerWithPayloadLimit(nil, []string{}, maxPayloadSize)
+	req := httptest.NewRequest(http.MethodPost, "/api/webhook", nil)
+	req.Header.Set("X-GitHub-Event", "push")
+	eventJSON, err := os.ReadFile("testdata/github-commit-event.json")
+	require.NoError(t, err)
+	req.Body = io.NopCloser(bytes.NewReader(eventJSON))
+	w := httptest.NewRecorder()
+	h.Handler(w, req)
+	close(h.queue)
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	expectedLogResult := "Webhook processing failed: The payload is either too large or corrupted. Please check the payload size (must be under 0 MB) and ensure it is valid JSON"
+	assert.Equal(t, expectedLogResult, hook.LastEntry().Message)
+	hook.Reset()
+}