From 06fac16a1e049f0f28c9ce2e26faaecee95d5c85 Mon Sep 17 00:00:00 2001
From: akshya96 <87045294+akshya96@users.noreply.github.com>
Date: Fri, 30 Aug 2024 11:00:12 -0700
Subject: [PATCH] Add maximum request duration (timeouts) for all requests
 except actual monitor and events requests (#28230)

* fix paths for sys/monitor and sys/events

* add changelog

* add changelog

* Update http/handler.go

Co-authored-by: Violet Hynes <violet.hynes@hashicorp.com>

* added some modifications to check for timeout

---------

Co-authored-by: Violet Hynes <violet.hynes@hashicorp.com>
---
 changelog/28230.txt                                      | 3 +++
 .../cache/static_secret_cache_updater_test.go            | 9 +++++++++
 http/handler.go                                          | 4 +++-
 http/sys_monitor_test.go                                 | 5 +++--
 4 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100644 changelog/28230.txt

diff --git a/changelog/28230.txt b/changelog/28230.txt
new file mode 100644
index 0000000000..13934ace8c
--- /dev/null
+++ b/changelog/28230.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+core: Fixed an issue where maximum request duration timeout was not being added to all requests containing strings sys/monitor and sys/events. With this change, timeout is now added to all requests except monitor and events endpoint. 
+```
diff --git a/command/agentproxyshared/cache/static_secret_cache_updater_test.go b/command/agentproxyshared/cache/static_secret_cache_updater_test.go
index 81fc76508e..a115fe608b 100644
--- a/command/agentproxyshared/cache/static_secret_cache_updater_test.go
+++ b/command/agentproxyshared/cache/static_secret_cache_updater_test.go
@@ -6,6 +6,7 @@ package cache
 import (
 	"context"
 	"fmt"
+	"os"
 	"sync"
 	syncatomic "sync/atomic"
 	"testing"
@@ -281,6 +282,8 @@ func TestOpenWebSocketConnection_AutoAuthSelfHeal(t *testing.T) {
 // works as expected with the default KVV1 mount, and then the connection can be used to receive an event.
 // This acts as more of an event system sanity check than a test of the updater
 // logic. It's still important coverage, though.
+// It also adds a client timeout of 1 second and checks that the connection does not timeout as this is a
+// streaming request.
 func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) {
 	if !constants.IsEnterprise {
 		t.Skip("test can only run on enterprise due to requiring the event notification system")
@@ -290,6 +293,11 @@ func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) {
 	cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{
 		HandlerFunc: vaulthttp.Handler,
 	})
+
+	oldClientTimeout := os.Getenv("VAULT_CLIENT_TIMEOUT")
+	os.Setenv("VAULT_CLIENT_TIMEOUT", "1")
+	defer os.Setenv("VAULT_CLIENT_TIMEOUT", oldClientTimeout)
+
 	client := cluster.Cores[0].Client
 
 	updater := testNewStaticSecretCacheUpdater(t, client)
@@ -318,6 +326,7 @@ func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) {
 
 		// This method blocks until it gets a secret, so this test
 		// will only pass if we're receiving events correctly.
+		// It will fail here if the connection times out.
 		_, _, err = conn.Read(context.Background())
 		require.NoError(t, err)
 	}
diff --git a/http/handler.go b/http/handler.go
index 7de46649f4..ceaaf34b45 100644
--- a/http/handler.go
+++ b/http/handler.go
@@ -386,7 +386,9 @@ func wrapGenericHandler(core *vault.Core, h http.Handler, props *vault.HandlerPr
 		ctx := r.Context()
 		var cancelFunc context.CancelFunc
 		// Add our timeout, but not for the monitor or events endpoints, as they are streaming
-		if strings.HasSuffix(r.URL.Path, "sys/monitor") || strings.Contains(r.URL.Path, "sys/events") {
+		// Request URL path for sys/monitor looks like /v1/sys/monitor
+		// Request URL paths for event subscriptions look like /v1/sys/events/subscribe/{eventType}. Example: /v1/sys/events/subscribe/kv*
+		if r.URL.Path == "/v1/sys/monitor" || strings.HasPrefix(r.URL.Path, "/v1/sys/events/subscribe") {
 			ctx, cancelFunc = context.WithCancel(ctx)
 		} else {
 			ctx, cancelFunc = context.WithTimeout(ctx, maxRequestDuration)
diff --git a/http/sys_monitor_test.go b/http/sys_monitor_test.go
index 5e6b49d0d1..9eea00f867 100644
--- a/http/sys_monitor_test.go
+++ b/http/sys_monitor_test.go
@@ -97,7 +97,8 @@ func TestSysMonitorStreamingLogs(t *testing.T) {
 			}
 			jsonLog := &jsonlog{}
 
-			timeCh := time.After(5 * time.Second)
+			// default timeout is 90 seconds
+			timeCh := time.After(120 * time.Second)
 
 			for {
 				select {
@@ -119,7 +120,7 @@ func TestSysMonitorStreamingLogs(t *testing.T) {
 						return
 					}
 				case <-timeCh:
-					t.Fatal("Failed to get a DEBUG message after 5 seconds")
+					t.Fatal("Failed to get a DEBUG message after 120 seconds")
 				}
 			}
 		})