From 9dbbdcc5cf76b103374195250dda8059d4226b54 Mon Sep 17 00:00:00 2001 From: Andrew Baptist Date: Mon, 23 Sep 2024 17:33:52 +0000 Subject: [PATCH 1/2] util: don't panic on IPv6 entries in cidr mapping Previously the code would encounter an index out of bounds if the cidr mapping file had a cidr length greater than 32 bits. This could only happen with IPv6 addresses. Note that if there any invalid mappings the code will display the error in the logs but won't process any of the file. The code already handled mapping lookups for IPv6, but these code changes also make that more explicit. Epic: none Informs: #130814 Release note: None --- pkg/util/cidr/cidr.go | 9 ++++++++- pkg/util/cidr/cidr_test.go | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/util/cidr/cidr.go b/pkg/util/cidr/cidr.go index cc454e9ca230..19ad54d5f85c 100644 --- a/pkg/util/cidr/cidr.go +++ b/pkg/util/cidr/cidr.go @@ -283,7 +283,7 @@ func (c *Lookup) setDestinations(ctx context.Context, contents []byte) error { if err := json.Unmarshal(contents, &destinations); err != nil { return err } - // TODO(baptist): This only handles IPv4. We could change to 128 if we want + // TODO(#130814): This only handles IPv4. We could change to 128 if we want // to handle IPv6. byLength := make([]map[string]string, 33) for i := range 33 { @@ -295,6 +295,9 @@ func (c *Lookup) setDestinations(ctx context.Context, contents []byte) error { return err } lenBits, _ := cidr.Mask.Size() + if lenBits > 32 { + return fmt.Errorf("invalid mask size: %d", lenBits) + } mask := net.CIDRMask(lenBits, 32) val := hexString(cidr.IP.Mask(mask)) byLength[lenBits][val] = d.Name @@ -334,6 +337,10 @@ func (c *Lookup) onChange(ctx context.Context) { func (c *Lookup) LookupIP(ip net.IP) string { byLength := *c.byLength.Load() ip = ip.To4() + // Don't map IPv6 addresses. + if ip == nil { + return "" + } for i := len(byLength) - 1; i >= 0; i-- { m := (byLength)[i] if len(m) == 0 { diff --git a/pkg/util/cidr/cidr_test.go b/pkg/util/cidr/cidr_test.go index b472e812af34..862b0070dad1 100644 --- a/pkg/util/cidr/cidr_test.go +++ b/pkg/util/cidr/cidr_test.go @@ -54,6 +54,7 @@ func TestCIDRLookup(t *testing.T) { {"10.0.0.2", "CIDR3"}, {"10.0.0.1", "CIDR4"}, {"172.16.0.1", ""}, + {"2001:0db8:0a0b:12f0:0000:0000:0000:0001", ""}, } for _, tc := range testCases { t.Run(tc.ip, func(t *testing.T) { @@ -92,6 +93,7 @@ func TestInvalidCIDR(t *testing.T) { {"int name ", `[ { "Name": 1, "Ipnet": "192.168.0.0/24" } ]`}, {"missing cidr", `[ { Name: "CIDR1" } ]`}, {"malformed cidr", `[ { "Name": "CIDR1", "Ipnet": "192.168.0.0.1/24" } ]`}, + {"ipv6", `[ { "Name": "CIDR1", "Ipnet": "2001:db8::/40" } ]`}, } c := Lookup{} for _, tc := range testCases { From 611bce859d35302d55d2aa12081c548a84f2a311 Mon Sep 17 00:00:00 2001 From: Andrew Baptist Date: Mon, 23 Sep 2024 17:41:10 +0000 Subject: [PATCH 2/2] util: remove unused cidr method We don't use the WrapTLS method and it is better to remove it and if we need it in the future bring it back. Epic: none Release note: None --- pkg/util/cidr/cidr.go | 37 ------------------------------------- pkg/util/cidr/cidr_test.go | 31 ------------------------------- 2 files changed, 68 deletions(-) diff --git a/pkg/util/cidr/cidr.go b/pkg/util/cidr/cidr.go index 19ad54d5f85c..bdd50b6cb651 100644 --- a/pkg/util/cidr/cidr.go +++ b/pkg/util/cidr/cidr.go @@ -12,7 +12,6 @@ package cidr import ( "context" - "crypto/tls" "encoding/json" "fmt" io "io" @@ -407,42 +406,6 @@ func (m *NetMetrics) Wrap(dial DialContext, labels ...string) DialContext { } } -// WrapTLS is like Wrap, but can be used if the underlying library doesn't -// expose a way to plug in a dialer for TLS connections. This is unfortunately -// pretty ugly... Copied from tls.Dial and kgo.DialTLS because they don't expose -// a dial call with a DialContext. Ideally you don't have to use this if the -// third party API does a sensible thing and exposes the ability to replace the -// "DialContext" directly. -func (m *NetMetrics) WrapTLS(dial DialContext, tlsCfg *tls.Config, labels ...string) DialContext { - return func(ctx context.Context, network, host string) (net.Conn, error) { - c := tlsCfg.Clone() - if c.ServerName == "" { - server, _, err := net.SplitHostPort(host) - if err != nil { - return nil, fmt.Errorf("unable to split host:port for dialing: %w", err) - } - c.ServerName = server - } - - rawConn, err := dial(ctx, network, host) - if err != nil { - return nil, err - } - scopedConn := rawConn - // m can be nil in tests. - if m != nil { - scopedConn = m.track(rawConn, labels...) - } - - conn := tls.Client(scopedConn, c) - if err := conn.HandshakeContext(ctx); err != nil { - scopedConn.Close() - return nil, err - } - return conn, nil - } -} - type Dialer interface { Dial(network, addr string) (c net.Conn, err error) DialContext(ctx context.Context, network, addr string) (c net.Conn, err error) diff --git a/pkg/util/cidr/cidr_test.go b/pkg/util/cidr/cidr_test.go index 862b0070dad1..10e62ad0b157 100644 --- a/pkg/util/cidr/cidr_test.go +++ b/pkg/util/cidr/cidr_test.go @@ -12,7 +12,6 @@ package cidr import ( "context" - "crypto/tls" "errors" "fmt" "net" @@ -220,36 +219,6 @@ func TestWrapHTTP(t *testing.T) { require.Greater(t, m.mu.childMetrics["foo/test"].ReadBytes.Value(), int64(1)) } -// TestWrapHTTP validates the wrapping function for HTTP connections. -func TestWrapHTTPS(t *testing.T) { - s := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer s.Close() - // Create a mapping for this server's IP. - mapping := fmt.Sprintf(`[ { "Name": "test", "Ipnet": "%s/32" } ]`, s.Listener.Addr().(*net.TCPAddr).IP.String()) - c := Lookup{} - require.NoError(t, c.setDestinations(context.Background(), []byte(mapping))) - - // This is the standard way to wrap the transport. - m := c.MakeNetMetrics(writeBytes, readBytes, "label") - transport := http.DefaultTransport.(*http.Transport).Clone() - transport.DialTLSContext = m.WrapTLS(transport.DialContext, &tls.Config{InsecureSkipVerify: true}, "foo") - - // Create a simple get request. - client := &http.Client{Transport: transport} - _, err := client.Get(s.URL) - require.NoError(t, err) - - // Ideally we could check the actual value, but the header includes the date - // and could be flaky. - require.Greater(t, m.WriteBytes.Count(), int64(1)) - require.Greater(t, m.ReadBytes.Count(), int64(1)) - // Also check the child metrics by looking up in the map directly. - m.mu.Lock() - defer m.mu.Unlock() - require.Greater(t, m.mu.childMetrics["foo/test"].WriteBytes.Value(), int64(1)) - require.Greater(t, m.mu.childMetrics["foo/test"].ReadBytes.Value(), int64(1)) -} - func TestWrapDialer(t *testing.T) { s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) defer s.Close()