From 7240fcfa95e1772f752425d8edfa048affe62d12 Mon Sep 17 00:00:00 2001 From: Jorropo Date: Fri, 18 Aug 2023 15:51:07 +0200 Subject: [PATCH 1/2] routing/http: do not leak a bytes.Buffer in drjson --- routing/http/internal/drjson/json.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/routing/http/internal/drjson/json.go b/routing/http/internal/drjson/json.go index 3bc3ab942..91b91332d 100644 --- a/routing/http/internal/drjson/json.go +++ b/routing/http/internal/drjson/json.go @@ -5,22 +5,22 @@ import ( "encoding/json" ) -func marshalJSON(val any) (*bytes.Buffer, error) { +func marshalJSON(val any) ([]byte, error) { buf := &bytes.Buffer{} enc := json.NewEncoder(buf) enc.SetEscapeHTML(false) err := enc.Encode(val) - return buf, err + return buf.Bytes(), err } // MarshalJSONBytes is needed to avoid changes // on the original bytes due to HTML escapes. func MarshalJSONBytes(val any) ([]byte, error) { - buf, err := marshalJSON(val) + bytes, err := marshalJSON(val) if err != nil { return nil, err } // remove last \n added by Encode - return buf.Bytes()[:buf.Len()-1], nil + return bytes[:len(bytes)-1], nil } From a97f44fdc51fe6344d51230f58b22bcb74cc166c Mon Sep 17 00:00:00 2001 From: Jorropo Date: Mon, 21 Aug 2023 09:59:30 +0200 Subject: [PATCH 2/2] routing/http: add more type and WithDynamicProviderInfo --- routing/http/client/client.go | 49 +++++++++++-- routing/http/client/client_test.go | 72 ++++++++++++++++++- routing/http/contentrouter/contentrouter.go | 6 +- .../http/contentrouter/contentrouter_test.go | 5 +- 4 files changed, 119 insertions(+), 13 deletions(-) diff --git a/routing/http/client/client.go b/routing/http/client/client.go index b3a74150c..f4aadcee7 100644 --- a/routing/http/client/client.go +++ b/routing/http/client/client.go @@ -8,6 +8,7 @@ import ( "fmt" "mime" "net/http" + "net/url" "strings" "time" @@ -54,7 +55,7 @@ type client struct { accepts string peerID peer.ID - addrs []types.Multiaddr + addrs func() ([]types.Multiaddr, error) identity crypto.PrivKey // called immeidately after signing a provide req @@ -104,10 +105,37 @@ func WithUserAgent(ua string) Option { } func WithProviderInfo(peerID peer.ID, addrs []multiaddr.Multiaddr) Option { + taddrs := make([]types.Multiaddr, len(addrs)) + for i, v := range addrs { + taddrs[i] = types.Multiaddr{Multiaddr: v} + } + return func(c *client) { c.peerID = peerID - for _, a := range addrs { - c.addrs = append(c.addrs, types.Multiaddr{Multiaddr: a}) + c.addrs = func() ([]types.Multiaddr, error) { + return taddrs, nil + } + } +} + +// WithDynamicProviderInfo is like [WithProviderInfo] but the addresses will be queried on each publish operation. +// This is usefull for nodes with changing addresses, like P2P daemons behind NATs. +// Note: due to API limitations can't trivially batch update previous records with new addresses, so you are still relient +// on an consumers using a PeerRouter able to follow your new addresses, for example the IPFS DHT. +func WithDynamicProviderInfo(peerID peer.ID, addrs func() ([]multiaddr.Multiaddr, error)) Option { + return func(c *client) { + c.peerID = peerID + c.addrs = func() ([]types.Multiaddr, error) { + addrs, err := addrs() + if err != nil { + return nil, err + } + + taddrs := make([]types.Multiaddr, len(addrs)) + for i, v := range addrs { + taddrs[i] = types.Multiaddr{Multiaddr: v} + } + return taddrs, nil } } } @@ -120,6 +148,7 @@ func WithStreamResultsRequired() Option { // New creates a content routing API client. // The Provider and identity parameters are option. If they are nil, the `Provide` method will not function. +// Consider using the more type-safe option [NewURL]. func New(baseURL string, opts ...Option) (*client, error) { client := &client{ baseURL: baseURL, @@ -140,6 +169,11 @@ func New(baseURL string, opts ...Option) (*client, error) { return client, nil } +// NewURL is a more type-safe version of [New], it takes in an [url.URL]. +func NewURL(baseURL url.URL, opts ...Option) (*client, error) { + return New(baseURL.String(), opts...) +} + // measuringIter measures the length of the iter and then publishes metrics about the whole req once the iter is closed. // Of course, if the caller forgets to close the iter, this won't publish anything. type measuringIter[T any] struct { @@ -251,6 +285,11 @@ func (c *client) ProvideBitswap(ctx context.Context, keys []cid.Cid, ttl time.Du now := c.clock.Now() + addrs, err := c.addrs() + if err != nil { + return 0, fmt.Errorf("failed to query our addresses: %w", err) + } + req := types.WriteBitswapProviderRecord{ Protocol: "transport-bitswap", Schema: types.SchemaBitswap, @@ -259,10 +298,10 @@ func (c *client) ProvideBitswap(ctx context.Context, keys []cid.Cid, ttl time.Du AdvisoryTTL: &types.Duration{Duration: ttl}, Timestamp: &types.Time{Time: now}, ID: &c.peerID, - Addrs: c.addrs, + Addrs: addrs, }, } - err := req.Sign(c.peerID, c.identity) + err = req.Sign(c.peerID, c.identity) if err != nil { return 0, err } diff --git a/routing/http/client/client_test.go b/routing/http/client/client_test.go index 06ad1d6b4..1a1fb4811 100644 --- a/routing/http/client/client_test.go +++ b/routing/http/client/client_test.go @@ -144,7 +144,7 @@ func drAddrsToAddrs(drmas []types.Multiaddr) (addrs []multiaddr.Multiaddr) { return } -func makeBSReadProviderResp() types.ReadBitswapProviderRecord { +func makeBSReadProviderResp(t *testing.T) types.ReadBitswapProviderRecord { peerID, addrs, _ := makeProviderAndIdentity() return types.ReadBitswapProviderRecord{ Protocol: "transport-bitswap", @@ -194,7 +194,7 @@ func (e *osErrContains) errContains(t *testing.T, err error) { } func TestClient_FindProviders(t *testing.T) { - bsReadProvResp := makeBSReadProviderResp() + bsReadProvResp := makeBSReadProviderResp(t) bitswapProvs := []iter.Result[types.ProviderResponse]{ {Val: &bsReadProvResp}, } @@ -412,11 +412,18 @@ func TestClient_Provide(t *testing.T) { } } + var addrs []types.Multiaddr + if f := client.addrs; f != nil { + var err error + addrs, err = client.addrs() + require.NoError(t, err) + } + expectedProvReq := &server.BitswapWriteProvideRequest{ Keys: c.cids, Timestamp: clock.Now().Truncate(time.Millisecond), AdvisoryTTL: c.ttl, - Addrs: drAddrsToAddrs(client.addrs), + Addrs: drAddrsToAddrs(addrs), ID: client.peerID, } @@ -442,3 +449,62 @@ func TestClient_Provide(t *testing.T) { }) } } + +func TestWithDynamicClient(t *testing.T) { + t.Parallel() + + const ttl = time.Hour + + const testUserAgent = "testUserAgent" + peerID, addrs, identity := makeProviderAndIdentity() + router := &mockContentRouter{} + recordingHandler := &recordingHandler{ + Handler: server.Handler(router), + f: []func(*http.Request){ + func(r *http.Request) { + assert.Equal(t, testUserAgent, r.Header.Get("User-Agent")) + }, + }, + } + srv := httptest.NewServer(recordingHandler) + t.Cleanup(srv.Close) + serverAddr := "http://" + srv.Listener.Addr().String() + recordingHTTPClient := &recordingHTTPClient{httpClient: defaultHTTPClient} + var rAddrs []multiaddr.Multiaddr + client, err := New(serverAddr, + WithDynamicProviderInfo(peerID, func() ([]multiaddr.Multiaddr, error) { return rAddrs, nil }), + WithIdentity(identity), + WithUserAgent(testUserAgent), + WithHTTPClient(recordingHTTPClient), + ) + require.NoError(t, err) + + c := makeCID() + rAddrs = addrs[:1] + + clock := clock.NewMock() + clock.Set(time.Now()) + client.clock = clock + + expectedProvReq := &server.BitswapWriteProvideRequest{ + Keys: []cid.Cid{c}, + Timestamp: clock.Now().Truncate(time.Millisecond), + AdvisoryTTL: ttl, + Addrs: rAddrs, + ID: peerID, + } + router.On("ProvideBitswap", mock.Anything, expectedProvReq).Return(ttl, nil) + + ctx := context.Background() + _, err = client.ProvideBitswap(ctx, []cid.Cid{c}, ttl) + require.NoError(t, err) + + c = makeCID() + rAddrs = addrs[1:] + + expectedProvReq.Keys[0] = c + expectedProvReq.Addrs = rAddrs + + _, err = client.ProvideBitswap(ctx, []cid.Cid{c}, ttl) + require.NoError(t, err) +} diff --git a/routing/http/contentrouter/contentrouter.go b/routing/http/contentrouter/contentrouter.go index 8318a3163..070b5dc56 100644 --- a/routing/http/contentrouter/contentrouter.go +++ b/routing/http/contentrouter/contentrouter.go @@ -124,9 +124,9 @@ func readProviderResponses(iter iter.ResultIter[types.ProviderResponse], ch chan continue } - var addrs []multiaddr.Multiaddr - for _, a := range result.Addrs { - addrs = append(addrs, a.Multiaddr) + addrs := make([]multiaddr.Multiaddr, len(result.Addrs)) + for i, a := range result.Addrs { + addrs[i] = a.Multiaddr } ch <- peer.AddrInfo{ diff --git a/routing/http/contentrouter/contentrouter_test.go b/routing/http/contentrouter/contentrouter_test.go index 3830482e2..d223e1c2e 100644 --- a/routing/http/contentrouter/contentrouter_test.go +++ b/routing/http/contentrouter/contentrouter_test.go @@ -10,6 +10,7 @@ import ( "github.com/ipfs/boxo/routing/http/types/iter" "github.com/ipfs/go-cid" "github.com/libp2p/go-libp2p/core/peer" + "github.com/multiformats/go-multiaddr" "github.com/multiformats/go-multihash" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -135,8 +136,8 @@ func TestFindProvidersAsync(t *testing.T) { } expected := []peer.AddrInfo{ - {ID: p1}, - {ID: p2}, + {ID: p1, Addrs: []multiaddr.Multiaddr{}}, + {ID: p2, Addrs: []multiaddr.Multiaddr{}}, } require.Equal(t, expected, actualAIs)