add timeouts to prevent slowloris attacks #22739
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
theme/cli
sanikachavan5
added
backport/all
sanikachavan5
force-pushed
the
fix-slowloris-http-endpoints
branch
from
15b7ae5 to
50dfa2e
Compare
boruszak
previously approved these changes
Sep 22, 2025
|
|
||
| Refer to the [formatting specification](https://golang.org/pkg/time/#ParseDuration) for additional information. | ||
|
|
||
| #### Duration validation |
There was a problem hiding this comment.
Suggested change
| #### Duration validation | |
| #### Duration formatting validation |
|
|
||
| #### Duration validation | ||
|
|
||
| When configuring timeout and duration values, Consul validates the format and enforces minimum values where applicable. Invalid duration strings will cause the agent to fail to start with a descriptive error message. |
There was a problem hiding this comment.
Suggested change
| When configuring timeout and duration values, Consul validates the format and enforces minimum values where applicable. Invalid duration strings will cause the agent to fail to start with a descriptive error message. | |
| When you configure timeout and duration values, Consul validates the formatting and enforces minimum values where applicable. When you use invalid duration strings, the agent will fail to start and the output will include a descriptive error message. |
Comment on lines
94
to
98
| **Valid examples:** | ||
| - `"10s"` - 10 seconds | ||
| - `"1.5m"` - 1.5 minutes (90 seconds) | ||
| - `"2h30m45s"` - Complex duration with hours, minutes, and seconds | ||
| - `"0s"` - Zero duration (where allowed) |
There was a problem hiding this comment.
Suggested change
| **Valid examples:** | |
| - `"10s"` - 10 seconds | |
| - `"1.5m"` - 1.5 minutes (90 seconds) | |
| - `"2h30m45s"` - Complex duration with hours, minutes, and seconds | |
| - `"0s"` - Zero duration (where allowed) | |
| **Examples of valid duration formatting**: | |
| - `"10s"` - 10 seconds | |
| - `"1.5m"` - 1.5 minutes, equivalent to 90 seconds (`90s`) | |
| - `"2h30m45s"` - Complex duration with hours, minutes, and seconds | |
| - `"0s"` - Zero duration, although not all fields support this value. |
Comment on lines
100
to
105
| **Invalid examples that will cause errors:** | ||
| - `"10"` - Missing unit specification | ||
| - `"10x"` - Invalid unit (`x` is not a recognized unit) | ||
| - `""` - Empty string | ||
| - `"abc"` - Non-numeric string | ||
| - `"-5s"` - Negative durations (not accepted for timeout configurations) |
There was a problem hiding this comment.
Suggested change
| **Invalid examples that will cause errors:** | |
| - `"10"` - Missing unit specification | |
| - `"10x"` - Invalid unit (`x` is not a recognized unit) | |
| - `""` - Empty string | |
| - `"abc"` - Non-numeric string | |
| - `"-5s"` - Negative durations (not accepted for timeout configurations) | |
| **Examples of invalid formatting that produce errors**: | |
| - `"10"` - Missing unit specification | |
| - `"10x"` - Invalid unit, as `x` is not a recognized duration | |
| - `""` - Empty string | |
| - `"abc"` - Non-numeric string | |
| - `"-5s"` - Negative durations are not accepted for timeout configurations |
| - `"abc"` - Non-numeric string | ||
| - `"-5s"` - Negative durations (not accepted for timeout configurations) | ||
|
|
||
| **Minimum value enforcement:** Some timeout configurations enforce minimum values to prevent misconfigurations that could cause connection issues or security vulnerabilities. For example, handshake timeouts typically require a minimum of 1 second. When a configured value is below the minimum, Consul will report a validation error during startup. |
There was a problem hiding this comment.
Suggested change
| **Minimum value enforcement:** Some timeout configurations enforce minimum values to prevent misconfigurations that could cause connection issues or security vulnerabilities. For example, handshake timeouts typically require a minimum of 1 second. When a configured value is below the minimum, Consul will report a validation error during startup. | |
| Some timeout configurations enforce minimum values to prevent connection issues or security vulnerabilities. For example, handshake timeouts typically require a minimum of 1 second. When a configured value is below the minimum, Consul reports a validation error during startup. |
boruszak
added
backport/1.21
…22741) * - Added support for IPv6 virtual IP offset calculation and validation. - Upgraded `github.com/miekg/dns` dependency to v1.1.68. - Included `BindAddr` in agent endpoint and enabled binding logic in IP offset calculations. - Updated `go.mod` and `go.sum` to reflect dependency changes. * Refactored `addIPOffset` to utilize `BindAddr` and introduced `getAgentBindAddr` for improved binding logic and IP offset calculation. Added utility function isDualStack * Executed go mod tidy * Refactored network utilities into `netutil` package and updated `catalog.go` to use new functions for improved modularity and clarity. Removed duplicate implementations of `GetAgentConfig` and `isDualStack`. * Added comprehensive unit tests for `netutil` package covering `GetAgentConfig`, `GetAgentBindAddr`, and `IsDualStack`. Refactored functions to support mocking for improved testability. * only retaining utility function changes. * added changelog
* "Get started" section redirects * deploy & secure * Consul operations complete * register & discover * Finish service networking * Enterprise, Runtimes, and Plugins sections * Reference docs
* Adding private key JWT support for OIDC
) * update: default proxy BindAddress to :: for IPv6 agent bind addr * add: changelog * fixing conflicts --------- Co-authored-by: Mukul Anand <[email protected]>
…22788) fix: suppress lacks token permission while checking dual stack
redhat image version revert to 9.6
* fix path cleaning of proxied urls * add changelog * added more tests * add tests and address review comments * address review changes
use hardcoded names for preventing attacks
1. Support for registering a service with multiple named ports 2. The named ports can be used in DNS queries to discover ports based on usecase
|
@boruszak I have made the required changes, can you take a look? |
sanikachavan5
removed request for
a team,
LeahMarieBush and
ethel-hashicorp
boruszak
approved these changes
Sep 23, 2025
dduzgun-security
requested changes
Sep 23, 2025
| } | ||
|
|
||
| tempDir := testutil.TempDir(t, "consul") | ||
| tempDir, err := os.MkdirTemp("", "consul_sock_test_") |
There was a problem hiding this comment.
Any specific reason why we moved away from the testutil.TempDir function?
| // Use minimum timeout values for security while respecting user configuration | ||
| readTimeout := c.pprofTimeout | ||
| if readTimeout < 1*time.Second { | ||
| readTimeout = 1 * time.Second |
There was a problem hiding this comment.
Is a default of 1 second enough for the pprof timeout?
| require.Contains(r, err.Error(), "EOF") | ||
| // After adding timeouts to prevent slowloris attacks, the connection | ||
| // times out with "i/o timeout" instead of "EOF" | ||
| if !strings.Contains(err.Error(), "EOF") && !strings.Contains(err.Error(), "i/o timeout") { |
There was a problem hiding this comment.
Not sure about this condition. Can we check to see the returned value from err.Error() and adjust the tests accordingly to use require.Contains like before?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces security enhancements and configurability improvements for HTTP server timeouts in the agent and connect proxy, aiming to mitigate Slowloris and related denial-of-service attacks. It adds new configuration options for HTTP timeouts, ensures they are correctly parsed and validated, and updates both the agent and proxy to use these values. Comprehensive tests are added to verify correct behavior and edge cases.
Description
Security and HTTP Timeout Enhancements:
read_timeout,read_header_timeout,write_timeout,idle_timeout) toHTTPConfigandRuntimeConfig, with sensible defaults and minimums, to protect against Slowloris and similar attacks (agent/config/config.go,agent/config/runtime.go,agent/config/builder.go,agent/agent.go). [1] [2] [3] [4]agent/agent.go).--pprof-timeout) and set minimums for security (command/connect/proxy/proxy.go). [1] [2] [3]Testing & Reproduction steps
Links
PR Checklist
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.