fix(webhook): align standalone webhook HTTP/2 hardening#1588
fix(webhook): align standalone webhook HTTP/2 hardening#1588doup2001 wants to merge 2 commits intoargoproj-labs:masterfrom
Conversation
doup2001
requested review from
chengfang,
dkarpele,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThreads a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (--enable-http2)
participant Cmd as WebhookCmd
participant Setup as SetupWebhookServer
participant TLSConf as TLSConfig
participant Server as HTTPServer
CLI->>Cmd: parse flags (enable-http2)
Cmd->>Setup: call SetupWebhookServer(webhookCfg{EnableHTTP2})
Setup->>TLSConf: construct TLSConfig (EnableHTTP2 set)
TLSConf->>TLSConf: buildTLSConfig() — if EnableHTTP2==false set NextProtos=["http/1.1"]
Setup->>Server: start HTTP server with tls.Config (NextProtos applied)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1588 +/- ##
==========================================
+ Coverage 71.48% 73.55% +2.07%
==========================================
Files 50 63 +13
Lines 4667 6591 +1924
==========================================
+ Hits 3336 4848 +1512
- Misses 1133 1425 +292
- Partials 198 318 +120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
doup2001
force-pushed
the
fix-1559-webhook-enable-http2
branch
from
82c8577 to
5b1a51b
Compare
doup2001
force-pushed
the
fix-1559-webhook-enable-http2
branch
2 times, most recently
from
2295e38 to
4f2962a
Compare
|
This is a rebase, so please ignore the force-push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/webhook/server.go`:
- Around line 217-219: The server currently only sets tlsCfg.NextProtos when
t.EnableHTTP2 is false, which doesn't stop Go's net/http from enabling HTTP/2;
update the server setup to set the http.Server's TLSNextProto to an empty map
when t.EnableHTTP2 is false (e.g., server.TLSNextProto =
make(map[string]func(*http.Server, *tls.Conn, http.Handler))) so HTTP/2
negotiation is fully disabled, and add an end-to-end regression test that starts
the TLS server with EnableHTTP2: false, attempts ALPN HTTP/2 negotiation, and
asserts the negotiated protocol is http/1.1 (or that HTTP/2 is rejected).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f4d9ec7-a4d7-4c6c-81ba-776d90101c2e
📒 Files selected for processing (8)
cmd/common.gocmd/common_test.gocmd/run.gocmd/webhook.gocmd/webhook_test.godocs/install/cmd/webhook.mdpkg/webhook/server.gopkg/webhook/server_test.go
✅ Files skipped from review due to trivial changes (3)
- cmd/webhook_test.go
- docs/install/cmd/webhook.md
- cmd/common_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/webhook.go
- cmd/run.go
- pkg/webhook/server_test.go
- cmd/common.go
| if !t.EnableHTTP2 { | ||
| tlsCfg.NextProtos = []string{"http/1.1"} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the webhook TLS path disables net/http automatic HTTP/2, not just tls.Config ALPN.
# Expected after the fix: webhook server setup contains both the http/1.1 NextProtos override
# and a non-nil empty Server.TLSNextProto assignment when EnableHTTP2 is false.
rg -n -C3 '\b(EnableHTTP2|NextProtos|TLSNextProto)\b' --type goRepository: argoproj-labs/argocd-image-updater
Length of output: 5794
🏁 Script executed:
# Find where Server.ListenAndServeTLS is called and Server is initialized
rg -n -B5 -A5 'ListenAndServeTLS|\.Server\s*=' --type go pkg/webhook/Repository: argoproj-labs/argocd-image-updater
Length of output: 2106
🏁 Script executed:
# Check the complete start() method implementation
sed -n '1,350p' pkg/webhook/server.go | tail -100Repository: argoproj-labs/argocd-image-updater
Length of output: 3488
🏁 Script executed:
# Search for any direct Server.TLSNextProto assignments in the entire repo
rg -n 'TLSNextProto' --type goRepository: argoproj-labs/argocd-image-updater
Length of output: 60
🌐 Web query:
Go net/http ListenAndServeTLS TLSNextProto automatic HTTP/2 registration behavior
💡 Result:
In Go's net/http package, http.ListenAndServeTLS(addr, certFile, keyFile, handler) automatically enables HTTP/2 support over TLS ("h2" via ALPN) by default, starting from Go 1.6. This is transparent and requires no explicit configuration. The mechanism works as follows: - ListenAndServeTLS internally creates an *http.Server with the provided handler (or DefaultServeMux if nil). - When the server starts serving TLS connections (via ServeTLS or similar), it checks if HTTP/2 should be configured using shouldConfigureHTTP2ForServe, which returns true if TLSConfig is nil or includes "h2" in NextProtos. - If TLSNextProto is nil (the default), the server calls an internal function (onceSetNextProtoDefaults) which invokes http2ConfigureServer (from the bundled h2_bundle.go, embedding golang.org/x/net/http2). - http2ConfigureServer sets srv.TLSNextProto["h2"] to a handler function that takes over "h2" connections, delegating to the http2.Server implementation for multiplexing, etc. - During TLS handshake, if the client negotiates "h2" via ALPN, the TLSNextProto["h2"] handler is invoked with the *tls.Conn and http.Handler, enabling HTTP/2 automatically. To disable automatic HTTP/2: - Set Server.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn, http.Handler)) (non-nil empty map) before serving. - Or use GODEBUG=http2server=0. If TLSNextProto is set by the user (non-nil), automatic registration does not occur to avoid overriding user handlers, but if "h2" is in TLSConfig.NextProtos, clients may still attempt HTTP/2 (falling back to HTTP/1.1 if no handler). This behavior is consistent across recent Go versions (1.21+ to tip as of 2026), with h2_bundle.go bundling the http2 implementation directly in std. Recent changes (e.g., issue #69649) introduce linkname-based registration for future extensions like h2c, but do not alter the core TLS automatic behavior. Example enabling HTTP/2 (default): log.Fatal(http.ListenAndServeTLS(":443", "cert.pem", "key.pem", nil)) Example disabling: srv := &http.Server{Addr: ":443", TLSNextProto: make(map[string]func(*http.Server, *tls.Conn, http.Handler))} log.Fatal(srv.ListenAndServeTLS("cert.pem", "key.pem", nil))
Citations:
- 1: https://pkg.go.dev/net/http@go1.25.1
- 2: https://pkg.go.dev/net/http
- 3: https://go.dev/src/net/http/h2_bundle.go
- 4: net/http: Cannot set default 'h2' and other NextProto handler concurrently golang/go#16588
- 5: https://github.com/golang/net/blob/master/http2/server.go
- 6: https://tip.golang.org/src/net/http/server.go
- 7: https://github.com/golang/go/blob/master/src/net/http/server.go
- 8: https://github.com/golang/go/blob/go1.20/src/net/http/server.go
🌐 Web query:
Go http.Server TLSNextProto field purpose disable automatic HTTP/2
💡 Result:
The TLSNextProto field in Go's http.Server is a map from ALPN protocol names (e.g., "h2" for HTTP/2) to handler functions that process connections using those protocols over TLS. By default, Go's net/http package registers a handler for "h2" to enable automatic HTTP/2 support on HTTPS servers (since Go 1.6). To disable automatic HTTP/2, set Server.TLSNextProto to a non-nil, empty map. This prevents registration of the HTTP/2 handler, forcing fallback to HTTP/1.1. Example: srv := &http.Server{ Addr: ":443", Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "Hello!") }), TLSNextProto: make(map[string]func(*http.Server, *tls.Conn, http.Handler)), // Disables HTTP/2 } log.Fatal(srv.ListenAndServeTLS("cert.pem", "key.pem")) In Go 1.24+, Server.Protocols offers finer control (e.g., Protocols{}.SetHTTP2(false)), but TLSNextProto remains the standard way to disable HTTP/2 as per official docs. Alternatives: GODEBUG=http2server=0 (env var, not guaranteed stable) or //go:debug http2server=0 (Go 1.21+ source comment).
Citations:
- 1: https://stackoverflow.com/questions/58088956/how-to-disable-http-2-using-server-tlsnextproto
- 2: https://pkg.go.dev/net/http@go1.25.1
- 3: https://pkg.go.dev/net/http
🏁 Script executed:
# Check existing tests for HTTP/2 behavior or protocol negotiation
rg -n 'http2|HTTP/2|protocol.*negotiat|ALPN' --type go pkg/webhook/server_test.goRepository: argoproj-labs/argocd-image-updater
Length of output: 144
🏁 Script executed:
# Verify the complete buildTLSConfig function to ensure nothing else affects HTTP/2
sed -n '190,225p' pkg/webhook/server.goRepository: argoproj-labs/argocd-image-updater
Length of output: 1145
Disable HTTP/2 on the http.Server when EnableHTTP2 is false.
Go's net/http automatically registers an HTTP/2 handler unless Server.TLSNextProto is explicitly set to a non-nil value. Setting only tls.Config.NextProtos prevents ALPN advertisement but does not prevent the server from accepting HTTP/2 connections. Set Server.TLSNextProto to an empty map to fully disable HTTP/2 negotiation.
Proposed fix
if !t.EnableHTTP2 {
tlsCfg.NextProtos = []string{"http/1.1"}
}
return tlsCfg, nil
}
// Start starts the webhook server
func (s *WebhookServer) Start(ctx context.Context) error {
log := log.LoggerFromContext(ctx)
// Create server and register routes
mux := http.NewServeMux()
mux.HandleFunc("/webhook", s.handleWebhook)
mux.HandleFunc("/healthz", s.handleHealth)
addr := fmt.Sprintf(":%d", s.Port)
s.Server = &http.Server{
Addr: addr,
Handler: mux,
}
// errCh captures startup errors from the server goroutine so we can
// fail fast instead of blocking on ctx.Done() with a dead listener.
errCh := make(chan error, 1)
if s.DisableTLS {
log.Warnf("Starting webhook server in insecure mode (plain HTTP) on port %d", s.Port)
go func() {
if err := s.Server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
errCh <- err
}
}()
} else {
// Default TLS settings if not explicitly configured
if s.TLS == nil {
s.TLS = &TLSConfig{
CertFile: DefaultTLSCertPath,
KeyFile: DefaultTLSKeyPath,
MinVersion: DefaultTLSMinVersion,
MaxVersion: DefaultTLSMaxVersion,
}
}
// Build TLS config from settings
tlsCfg, err := s.TLS.buildTLSConfig()
if err != nil {
return fmt.Errorf("failed to configure TLS: %w", err)
}
+ if !s.TLS.EnableHTTP2 {
+ // Prevent net/http from automatically enabling HTTP/2 for TLS servers.
+ s.Server.TLSNextProto = map[string]func(*http.Server, *tls.Conn, http.Handler){}
+ }
// Determine whether to load certs from files or generate self-signedAlso add an end-to-end regression test that starts the TLS server with EnableHTTP2: false, attempts to negotiate HTTP/2 via ALPN, and verifies the negotiated protocol is HTTP/1.1 (or that HTTP/2 is rejected).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/webhook/server.go` around lines 217 - 219, The server currently only sets
tlsCfg.NextProtos when t.EnableHTTP2 is false, which doesn't stop Go's net/http
from enabling HTTP/2; update the server setup to set the http.Server's
TLSNextProto to an empty map when t.EnableHTTP2 is false (e.g.,
server.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn,
http.Handler))) so HTTP/2 negotiation is fully disabled, and add an end-to-end
regression test that starts the TLS server with EnableHTTP2: false, attempts
ALPN HTTP/2 negotiation, and asserts the negotiated protocol is http/1.1 (or
that HTTP/2 is rejected).
Signed-off-by: eedo_y <doup2001@naver.com>
Signed-off-by: eedo_y <doup2001@naver.com>
doup2001
force-pushed
the
fix-1559-webhook-enable-http2
branch
from
4f2962a to
0bdb345
Compare
2 participants
Summary
This change threads the
enable-http2setting through the webhook configuration so the standalone webhook server follows the same HTTP/2 hardening policy as the controller-managed path.When HTTP/2 is not explicitly enabled, the webhook TLS server now restricts ALPN negotiation to
http/1.1. A standalonewebhookcommand flag was also added so users can opt in to HTTP/2 intentionally.Closes #1559
What Changed
EnableHTTP2toWebhookConfig--enable-http2to the standalonewebhookcommandenableHTTP2setting into webhook server setupNextProtos = []string{"http/1.1"}User Impact
--enable-http2Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests