Merge commit from fork

* feat: limit payload size Signed-off-by: pashakostohrys <[email protected]> * cover with tests and provide ability to define variable from CM Signed-off-by: pashakostohrys <[email protected]> * cover with tests and provide ability to define variable from CM Signed-off-by: pashakostohrys <[email protected]> * improve error message and add documentation Signed-off-by: pashakostohrys <[email protected]> * fix tests Signed-off-by: pashakostohrys <[email protected]> * fix comments Signed-off-by: pashakostohrys <[email protected]> * fix linter Signed-off-by: pashakostohrys <[email protected]> --------- Signed-off-by: pashakostohrys <[email protected]>
argoproj · Jul 22, 2024 · c9c7989 · c9c7989
1 parent 5d21725
commit c9c7989
Show file tree

Hide file tree

Showing 6 changed files with 70 additions and 4 deletions.
diff --git a/docs/operator-manual/argocd-cm.yaml b/docs/operator-manual/argocd-cm.yaml
@@ -420,3 +420,5 @@ 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,8 @@ 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
@@ -1073,7 +1073,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.ArgoCDServerOpts.WebhookParallelism, a.AppClientset, a.settings, a.settingsMgr, a.RepoServerCache, a.Cache, argoDB)
+	acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.ArgoCDServerOpts.WebhookParallelism, 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
@@ -434,6 +434,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
@@ -517,6 +519,11 @@ const (
 	RespectRBACValueNormal = "normal"
 )
 
+const (
+	// default max webhook payload size is 1GB
+	defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024
+)
+
 var sourceTypeToEnableGenerationKey = map[v1alpha1.ApplicationSourceType]string{
 	v1alpha1.ApplicationSourceTypeKustomize: "kustomize.enable",
 	v1alpha1.ApplicationSourceTypeHelm:      "helm.enable",
@@ -2257,3 +2264,22 @@ func (mgr *SettingsManager) GetExcludeEventLabelKeys() []string {
 	}
 	return labelKeys
 }
+
+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
@@ -66,9 +66,10 @@ type ArgoCDWebhookHandler struct {
 	gogs                   *gogs.Webhook
 	settingsSrc            settingsSource
 	queue                  chan interface{}
+	maxWebhookPayloadSizeB int64
 }
 
-func NewHandler(namespace string, applicationNamespaces []string, webhookParallelism int, appClientset appclientset.Interface, set *settings.ArgoCDSettings, settingsSrc settingsSource, repoCache *cache.Cache, serverCache *servercache.Cache, argoDB db.ArgoDB) *ArgoCDWebhookHandler {
+func NewHandler(namespace string, applicationNamespaces []string, webhookParallelism int, 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")
@@ -119,6 +120,7 @@ func NewHandler(namespace string, applicationNamespaces []string, webhookParalle
 		serverCache:            serverCache,
 		db:                     argoDB,
 		queue:                  make(chan interface{}, payloadQueueSize),
+		maxWebhookPayloadSizeB: maxWebhookPayloadSizeB,
 	}
 
 	acdWebhook.startWorkerPool(webhookParallelism)
@@ -417,6 +419,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 {
@@ -459,6 +463,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
@@ -56,6 +56,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 +77,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) {
@@ -419,7 +424,7 @@ func TestInvalidEvent(t *testing.T) {
 	close(h.queue)
 	h.Wait()
 	assert.Equal(t, http.StatusBadRequest, w.Code)
-	expectedLogResult := "Webhook processing failed: error parsing payload"
+	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()
@@ -632,3 +637,22 @@ 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)
+	h.Wait()
+	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()
+}