From 29aef59dae9cbaaa47bee00795663c307f118706 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:52:10 +1100 Subject: [PATCH] fix: enforce HttpPutResponseHopLimit on the IMDSv2 token response (#1201) --- go.mod | 2 +- spinifex/handlers/imds/hop_limit.go | 140 +++++++++ spinifex/handlers/imds/hop_limit_test.go | 279 ++++++++++++++++++ spinifex/handlers/imds/imdsv1_test.go | 1 + spinifex/handlers/imds/instance_lookup.go | 1 + .../handlers/imds/instance_lookup_test.go | 3 +- spinifex/handlers/imds/metadata.go | 25 +- spinifex/handlers/imds/metadata_test.go | 14 +- spinifex/handlers/imds/resolver.go | 1 + spinifex/handlers/imds/tap_responder.go | 4 +- 10 files changed, 460 insertions(+), 10 deletions(-) create mode 100644 spinifex/handlers/imds/hop_limit.go create mode 100644 spinifex/handlers/imds/hop_limit_test.go diff --git a/go.mod b/go.mod index 1b2c41a21..9d5f000f2 100644 --- a/go.mod +++ b/go.mod @@ -46,7 +46,6 @@ require ( go.uber.org/goleak v1.3.1-0.20260602155401-5f791e3ef83b go.yaml.in/yaml/v3 v3.0.5 golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 golang.org/x/term v0.47.0 @@ -197,6 +196,7 @@ require ( go.uber.org/nilaway v0.0.0-20260721205819-4a0f7653488a // indirect golang.org/x/exp/typeparams v0.0.0-20260611194520-c48552f49976 // indirect golang.org/x/mod v0.41.0 // indirect + golang.org/x/net v0.61.0 // indirect golang.org/x/telemetry v0.0.0-20260924152758-ed294f943157 // indirect golang.org/x/text v0.43.0 // indirect golang.org/x/vuln v1.6.0 // indirect diff --git a/spinifex/handlers/imds/hop_limit.go b/spinifex/handlers/imds/hop_limit.go new file mode 100644 index 000000000..0ecdc7fe7 --- /dev/null +++ b/spinifex/handlers/imds/hop_limit.go @@ -0,0 +1,140 @@ +package handlers_imds + +import ( + "context" + "errors" + "log/slog" + "net" + "sync" + "syscall" + + "golang.org/x/sys/unix" +) + +// defaultPutHopLimit is the HttpPutResponseHopLimit the token response falls +// back to whenever the instance's own value cannot be established. +const defaultPutHopLimit = 1 + +// maxPutHopLimit is the AWS-valid upper bound for HttpPutResponseHopLimit. +const maxPutHopLimit = 64 + +// connTTLSetter sets the IP TTL of the connection a request arrived on. +type connTTLSetter func(ttl int) error + +// connTTLFromConn returns a setter for c's socket, or nil if c exposes no socket. +func connTTLFromConn(c net.Conn) connTTLSetter { + sc, ok := c.(syscall.Conn) + if !ok { + return nil + } + return func(ttl int) error { + raw, err := sc.SyscallConn() + if err != nil { + return err + } + var sockErr error + if err := raw.Control(func(fd uintptr) { + sockErr = unix.SetsockoptInt(int(fd), unix.IPPROTO_IP, unix.IP_TTL, ttl) + }); err != nil { + return err + } + return sockErr + } +} + +// withConnTTL is the http.Server.ConnContext that lets handleToken reach the +// accepted socket. A conn without one gets no setter, so the PUT fails closed. +func withConnTTL(ctx context.Context, c net.Conn) context.Context { + if set := connTTLFromConn(c); set != nil { + return context.WithValue(ctx, ctxKeyConnTTL, set) + } + return ctx +} + +var errNoConnTTL = errors.New("no TTL setter for request connection") + +// setRequestConnTTL sets the IP TTL of the connection r's context came from. +func setRequestConnTTL(ctx context.Context, ttl int) error { + set, _ := ctx.Value(ctxKeyConnTTL).(connTTLSetter) + if set == nil { + return errNoConnTTL + } + return set(ttl) +} + +// putResponseHopLimit resolves the instance's current HttpPutResponseHopLimit +// on every call, so ModifyInstanceMetadataOptions applies on the next PUT. A +// lookup failure, missing instance, or unset or out-of-range value yields 1. +func (s *IMDSServiceImpl) putResponseHopLimit(ctx context.Context, eni *eniFacts) int { + inst, err := s.resolver.resolveInstance(ctx, eni) + if err != nil { + slog.WarnContext(ctx, "IMDS: hop limit lookup failed, using default", + "instance_id", eni.instanceID, "hop_limit", defaultPutHopLimit, "err", err) + return defaultPutHopLimit + } + if inst == nil || inst.putHopLimit < 1 || inst.putHopLimit > maxPutHopLimit { + return defaultPutHopLimit + } + return int(inst.putHopLimit) +} + +// limitListener caps concurrently open conns like netutil.LimitListener, but +// its conns still expose the accepted socket so the token PUT can set IP_TTL. +type limitListener struct { + net.Listener + + sem chan struct{} + closeOnce sync.Once + done chan struct{} +} + +func newLimitListener(l net.Listener, n int) *limitListener { + return &limitListener{Listener: l, sem: make(chan struct{}, n), done: make(chan struct{})} +} + +// Accept blocks while n conns are open. It returns net.ErrClosed once the +// listener is closed, even while waiting for a slot. +func (l *limitListener) Accept() (net.Conn, error) { + select { + case l.sem <- struct{}{}: + case <-l.done: + return nil, net.ErrClosed + } + c, err := l.Listener.Accept() + if err != nil { + <-l.sem + return nil, err + } + return &limitConn{Conn: c, release: func() { <-l.sem }}, nil +} + +func (l *limitListener) Close() error { + err := l.Listener.Close() + l.closeOnce.Do(func() { close(l.done) }) + return err +} + +// limitConn releases its listener slot on the first Close. +type limitConn struct { + net.Conn + + releaseOnce sync.Once + release func() +} + +var _ syscall.Conn = (*limitConn)(nil) + +func (c *limitConn) Close() error { + err := c.Conn.Close() + c.releaseOnce.Do(c.release) + return err +} + +// SyscallConn exposes the wrapped conn's socket. +func (c *limitConn) SyscallConn() (syscall.RawConn, error) { + sc, ok := c.Conn.(syscall.Conn) + if !ok { + return nil, errors.New("wrapped conn exposes no socket") + } + return sc.SyscallConn() +} diff --git a/spinifex/handlers/imds/hop_limit_test.go b/spinifex/handlers/imds/hop_limit_test.go new file mode 100644 index 000000000..e1a3520bf --- /dev/null +++ b/spinifex/handlers/imds/hop_limit_test.go @@ -0,0 +1,279 @@ +package handlers_imds + +import ( + "context" + "errors" + "io" + "net" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +// hopLimitResolver serves a fakeResolver whose instance hop limit can change +// between requests, standing in for ModifyInstanceMetadataOptions. +type hopLimitResolver struct { + fakeResolver + + hopLimit atomic.Int64 +} + +func (h *hopLimitResolver) resolveInstance(_ context.Context, _ *eniFacts) (*instanceFacts, error) { + return &instanceFacts{putHopLimit: h.hopLimit.Load()}, nil +} + +// putToken sends PUT /latest/api/token through h, recording the TTL the handler +// asked the connection for (0 if it asked for none). +func putToken(t *testing.T, res eniResolver) (*httptest.ResponseRecorder, int) { + t.Helper() + svc, _ := newTestService(res, &fakeIAM{}, &fakeAssumer{}) + var ttl int + h := withTapConn(svc.httpHandler(), testENI(), func(v int) error { ttl = v; return nil }) + req := httptest.NewRequest(http.MethodPut, "http://"+MetaDataServerIP+pathToken, nil) + req.Header.Set(hdrTokenTTL, "60") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec, ttl +} + +func TestHTTP_TokenPUTSetsHopLimitTTL(t *testing.T) { + cases := []struct { + name string + res *fakeResolver + want int + }{ + {"default", &fakeResolver{eni: testENI(), inst: &instanceFacts{putHopLimit: 1}}, 1}, + {"raised", &fakeResolver{eni: testENI(), inst: &instanceFacts{putHopLimit: 2}}, 2}, + {"maximum", &fakeResolver{eni: testENI(), inst: &instanceFacts{putHopLimit: 64}}, 64}, + {"unset fails closed", &fakeResolver{eni: testENI(), inst: &instanceFacts{}}, 1}, + {"out of range fails closed", &fakeResolver{eni: testENI(), inst: &instanceFacts{putHopLimit: 65}}, 1}, + {"instance missing fails closed", &fakeResolver{eni: testENI()}, 1}, + {"lookup error fails closed", &fakeResolver{eni: testENI(), instErr: errors.New("gather timeout")}, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rec, ttl := putToken(t, tc.res) + require.Equal(t, http.StatusOK, rec.Code) + assert.NotEmpty(t, rec.Body.String()) + assert.Equal(t, tc.want, ttl) + assert.Equal(t, "close", rec.Header().Get("Connection"), + "a keep-alive connection must not outlive the restricted TTL") + }) + } +} + +// ModifyInstanceMetadataOptions on a running instance applies to the next PUT. +func TestHTTP_TokenPUTFollowsModifiedHopLimit(t *testing.T) { + res := &hopLimitResolver{fakeResolver: fakeResolver{eni: testENI()}} + res.hopLimit.Store(1) + _, ttl := putToken(t, res) + assert.Equal(t, 1, ttl) + + res.hopLimit.Store(2) + _, ttl = putToken(t, res) + assert.Equal(t, 2, ttl) +} + +// Without a TTL in place, the token must not be sent at all. +func TestHTTP_TokenPUTFailsClosedWithoutTTL(t *testing.T) { + cases := []struct { + name string + setTTL connTTLSetter + }{ + {"no setter", nil}, + {"setter fails", func(int) error { return errors.New("setsockopt failed") }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + svc, _ := newTestService(&fakeResolver{eni: testENI()}, &fakeIAM{}, &fakeAssumer{}) + h := withTapConn(svc.httpHandler(), testENI(), tc.setTTL) + req := httptest.NewRequest(http.MethodPut, "http://"+MetaDataServerIP+pathToken, nil) + req.Header.Set(hdrTokenTTL, "60") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Empty(t, rec.Body.String()) + }) + } +} + +// ttlProbeListener records each accepted socket's IP_TTL as the server closes +// it, so the TTL the response actually left with is observable after the fact. +type ttlProbeListener struct { + net.Listener + + closed chan int +} + +func (l *ttlProbeListener) Accept() (net.Conn, error) { + c, err := l.Listener.Accept() + if err != nil { + return nil, err + } + tcp, ok := c.(*net.TCPConn) + if !ok { + _ = c.Close() + return nil, errors.New("not a TCP conn") + } + return &ttlProbeConn{TCPConn: tcp, closed: l.closed}, nil +} + +type ttlProbeConn struct { + *net.TCPConn + + once sync.Once + closed chan int +} + +func (c *ttlProbeConn) Close() error { + c.once.Do(func() { c.closed <- socketTTL(c.TCPConn) }) + return c.TCPConn.Close() +} + +func socketTTL(sc syscall.Conn) int { + raw, err := sc.SyscallConn() + if err != nil { + return -1 + } + ttl := -1 + _ = raw.Control(func(fd uintptr) { + if v, err := unix.GetsockoptInt(int(fd), unix.IPPROTO_IP, unix.IP_TTL); err == nil { + ttl = v + } + }) + return ttl +} + +func awaitTTL(t *testing.T, closed <-chan int) int { + t.Helper() + select { + case ttl := <-closed: + return ttl + case <-time.After(5 * time.Second): + t.Fatal("server never closed the connection") + return 0 + } +} + +// The token connection's server-side IP_TTL follows HttpPutResponseHopLimit +// through the production responder; metadata GETs keep the kernel default. +func TestTapResponder_TokenResponseTTLIsHopLimit(t *testing.T) { + const ( + eniID = "eni-aaa" + endpoint = "ime-aaa" + ) + res := &hopLimitResolver{fakeResolver: fakeResolver{eni: testENI()}} + res.hopLimit.Store(1) + svc, _ := newTestService(res, &fakeIAM{}, &fakeAssumer{}) + + closed := make(chan int, 4) + var addr string + listen := func(_ context.Context, _ string) (net.Listener, error) { + ln, err := net.Listen("tcp4", "127.0.0.1:0") + if err != nil { + return nil, err + } + addr = ln.Addr().String() + return &ttlProbeListener{Listener: ln, closed: closed}, nil + } + m := newTapResponderManager(svc.httpHandler(), staticResolve(map[string]*eniFacts{eniID: testENI()}), listen) + require.NoError(t, m.start(context.Background(), eniID, endpoint)) + t.Cleanup(func() { m.stop(eniID) }) + + token := putTapToken(t, addr) + assert.Equal(t, 1, awaitTTL(t, closed), "default hop limit must send the token with TTL 1") + + res.hopLimit.Store(2) + putTapToken(t, addr) + assert.Equal(t, 2, awaitTTL(t, closed), "a modified hop limit must apply to the next PUT") + + ref, err := net.ListenPacket("udp4", "127.0.0.1:0") + require.NoError(t, err) + defaultTTL := socketTTL(ref.(*net.UDPConn)) + _ = ref.Close() + + client := &http.Client{Transport: &http.Transport{DisableKeepAlives: true}} + req, err := http.NewRequest(http.MethodGet, "http://"+addr+prefixMetaData+"instance-id", nil) + require.NoError(t, err) + req.Header.Set(hdrToken, token) + resp, err := client.Do(req) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, defaultTTL, awaitTTL(t, closed), "only the token response is hop-limited") +} + +func TestLimitListener_CapsReleasesAndExposesSocket(t *testing.T) { + inner, err := net.Listen("tcp4", "127.0.0.1:0") + require.NoError(t, err) + l := newLimitListener(inner, 1) + defer func() { _ = l.Close() }() + + dial := func() net.Conn { + c, err := net.Dial("tcp4", inner.Addr().String()) + require.NoError(t, err) + t.Cleanup(func() { _ = c.Close() }) + return c + } + dial() + dial() + + first, err := l.Accept() + require.NoError(t, err) + _, ok := first.(syscall.Conn) + require.True(t, ok, "accepted conns must expose the socket for IP_TTL") + require.NotNil(t, connTTLFromConn(first)) + require.NoError(t, connTTLFromConn(first)(7)) + assert.Equal(t, 7, socketTTL(first.(syscall.Conn))) + + accepted := make(chan net.Conn, 1) + go func() { + c, err := l.Accept() + if err == nil { + accepted <- c + } + }() + select { + case <-accepted: + t.Fatal("Accept must block while the cap is reached") + case <-time.After(100 * time.Millisecond): + } + + require.NoError(t, first.Close()) + _ = first.Close() // a second Close must not release a second slot + select { + case c := <-accepted: + _ = c.Close() + case <-time.After(5 * time.Second): + t.Fatal("closing a conn must release its slot") + } +} + +func TestLimitListener_CloseUnblocksAccept(t *testing.T) { + inner, err := net.Listen("tcp4", "127.0.0.1:0") + require.NoError(t, err) + l := newLimitListener(inner, 1) + l.sem <- struct{}{} // cap reached + + done := make(chan error, 1) + go func() { + _, err := l.Accept() + done <- err + }() + require.NoError(t, l.Close()) + select { + case err := <-done: + assert.ErrorIs(t, err, net.ErrClosed) + case <-time.After(5 * time.Second): + t.Fatal("Close must unblock an Accept waiting for a slot") + } +} diff --git a/spinifex/handlers/imds/imdsv1_test.go b/spinifex/handlers/imds/imdsv1_test.go index 8e3c2f59e..6630f0467 100644 --- a/spinifex/handlers/imds/imdsv1_test.go +++ b/spinifex/handlers/imds/imdsv1_test.go @@ -72,6 +72,7 @@ func TestIMDSv1_TokenedRequestSkipsLookup(t *testing.T) { h := withTapENI(svc.httpHandler(), testENI()) token := issueToken(t, h) + res.calls = 0 // the PUT resolves the hop limit; only the GET is under test rec := get(t, h, prefixMetaData+"local-ipv4", token) require.Equal(t, http.StatusOK, rec.Code) assert.Zero(t, res.calls, "a valid token must not trigger an http-tokens lookup") diff --git a/spinifex/handlers/imds/instance_lookup.go b/spinifex/handlers/imds/instance_lookup.go index fa56834c8..2ccef9640 100644 --- a/spinifex/handlers/imds/instance_lookup.go +++ b/spinifex/handlers/imds/instance_lookup.go @@ -143,6 +143,7 @@ func instanceFactsFromVM(v *vm.VM) *instanceFacts { } if inst.MetadataOptions != nil { facts.httpTokens = aws.StringValue(inst.MetadataOptions.HttpTokens) + facts.putHopLimit = aws.Int64Value(inst.MetadataOptions.HttpPutResponseHopLimit) } return facts } diff --git a/spinifex/handlers/imds/instance_lookup_test.go b/spinifex/handlers/imds/instance_lookup_test.go index 0610b93e8..a7e41fcd4 100644 --- a/spinifex/handlers/imds/instance_lookup_test.go +++ b/spinifex/handlers/imds/instance_lookup_test.go @@ -67,7 +67,7 @@ func testVM(id string) *vm.VM { Architecture: aws.String("x86_64"), AmiLaunchIndex: aws.Int64(0), LaunchTime: aws.Time(launch), - MetadataOptions: &ec2.InstanceMetadataOptionsResponse{HttpTokens: aws.String("optional")}, + MetadataOptions: &ec2.InstanceMetadataOptionsResponse{HttpTokens: aws.String("optional"), HttpPutResponseHopLimit: aws.Int64(3)}, BlockDeviceMappings: []*ec2.InstanceBlockDeviceMapping{ {DeviceName: aws.String("/dev/vda")}, }, @@ -91,6 +91,7 @@ func TestDescribe_LocalHit_ResolvesWithoutRecordSpace(t *testing.T) { assert.Equal(t, "arn:aws:iam::111111111111:instance-profile/demo", facts.iamInstanceProfileArn) assert.Equal(t, "spot", facts.lifecycleType) assert.Equal(t, "optional", facts.httpTokens) + assert.Equal(t, int64(3), facts.putHopLimit) assert.Equal(t, int64(0), facts.amiLaunchIndex) assert.Equal(t, "r-abc123", facts.reservationID) assert.Equal(t, []byte("hello"), facts.userData) diff --git a/spinifex/handlers/imds/metadata.go b/spinifex/handlers/imds/metadata.go index e49c0af63..cb725c466 100644 --- a/spinifex/handlers/imds/metadata.go +++ b/spinifex/handlers/imds/metadata.go @@ -99,6 +99,18 @@ func (s *IMDSServiceImpl) handleToken(w http.ResponseWriter, r *http.Request) { return } + // AWS sends the token with IP TTL equal to HttpPutResponseHopLimit, so a + // workload more routed hops away than that never receives it. No token is + // issued unless the TTL is in place. + hopLimit := s.putResponseHopLimit(r.Context(), eni) + if err := setRequestConnTTL(r.Context(), hopLimit); err != nil { + slog.ErrorContext(r.Context(), "IMDS: setting token response TTL failed", "eni_id", eni.eniID, "hop_limit", hopLimit, "err", err) + w.WriteHeader(http.StatusInternalServerError) + return + } + // Closing the connection keeps the restricted TTL scoped to this response. + w.Header().Set("Connection", "close") + token, err := s.tokens.issue(eni.eniID, ttl, s.now()) if err != nil { slog.ErrorContext(r.Context(), "IMDS: token issuance failed", "eni_id", eni.eniID, "err", err) @@ -803,9 +815,14 @@ func writeJSON(w http.ResponseWriter, v any) { } // ctxKey is the unexported context-key type used to thread the per-tap ENI -// identity into each request. +// identity and the per-connection TTL setter into each request. type ctxKey int -// ctxKeyENI carries a *eniFacts resolved once per per-tap responder — the -// authoritative caller identity for every request it serves. -const ctxKeyENI ctxKey = iota +const ( + // ctxKeyENI carries a *eniFacts resolved once per per-tap responder — the + // authoritative caller identity for every request it serves. + ctxKeyENI ctxKey = iota + // ctxKeyConnTTL carries the connTTLSetter for the request's accepted socket, + // threaded per connection by the per-tap responder's ConnContext. + ctxKeyConnTTL +) diff --git a/spinifex/handlers/imds/metadata_test.go b/spinifex/handlers/imds/metadata_test.go index c7c2d35f3..9c49f5dc7 100644 --- a/spinifex/handlers/imds/metadata_test.go +++ b/spinifex/handlers/imds/metadata_test.go @@ -120,11 +120,21 @@ func testENI() *eniFacts { // request, the way the per-tap responder's BaseContext does in production. A nil // eni leaves the context empty so resolveCaller misses and the request 404s. func withTapENI(h http.Handler, eni *eniFacts) http.Handler { + return withTapConn(h, eni, func(int) error { return nil }) +} + +// withTapConn is withTapENI with an explicit connection TTL setter, standing in +// for the responder's ConnContext. A nil setter leaves it out of the context. +func withTapConn(h http.Handler, eni *eniFacts, setTTL connTTLSetter) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() if eni != nil { - r = r.WithContext(context.WithValue(r.Context(), ctxKeyENI, eni)) + ctx = context.WithValue(ctx, ctxKeyENI, eni) } - h.ServeHTTP(w, r) + if setTTL != nil { + ctx = context.WithValue(ctx, ctxKeyConnTTL, setTTL) + } + h.ServeHTTP(w, r.WithContext(ctx)) }) } diff --git a/spinifex/handlers/imds/resolver.go b/spinifex/handlers/imds/resolver.go index 6e983d2c6..15f56dcd1 100644 --- a/spinifex/handlers/imds/resolver.go +++ b/spinifex/handlers/imds/resolver.go @@ -54,6 +54,7 @@ type instanceFacts struct { reservationID string lifecycleType string // "spot" for spot-launched instances, else empty (on-demand) httpTokens string // "optional" permits IMDSv1; empty or "required" does not + putHopLimit int64 // HttpPutResponseHopLimit; 0 when unset amiLaunchIndex int64 pendingTime time.Time userData []byte diff --git a/spinifex/handlers/imds/tap_responder.go b/spinifex/handlers/imds/tap_responder.go index 32b70592a..7de21cf3f 100644 --- a/spinifex/handlers/imds/tap_responder.go +++ b/spinifex/handlers/imds/tap_responder.go @@ -11,7 +11,6 @@ import ( "syscall" "time" - "golang.org/x/net/netutil" "golang.org/x/sys/unix" ) @@ -124,7 +123,7 @@ func (m *tapResponderManager) start(ctx context.Context, eniID, endpoint string) return fmt.Errorf("listen on endpoint %s: %w", endpoint, err) } // Cap per-tap connections so one guest cannot exhaust vpcd's shared descriptors. - listener = netutil.LimitListener(listener, 32) + listener = newLimitListener(listener, 32) ifindex, err := m.ifindex(endpoint) if err != nil { slog.DebugContext(ctx, "IMDS: endpoint ifindex unavailable; recreated-endpoint detection degraded", "endpoint", endpoint, "err", err) @@ -140,6 +139,7 @@ func (m *tapResponderManager) start(ctx context.Context, eniID, endpoint string) BaseContext: func(net.Listener) context.Context { return context.WithValue(ctx, ctxKeyENI, eni) }, + ConnContext: withConnTTL, } m.mu.Lock()