Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

webtransport: Add helpers to transform webtransport multiaddrs in AddrsFactory #2227

Closed
wants to merge 5 commits into from
Closed

webtransport: Add helpers to transform webtransport multiaddrs in AddrsFactory #2227

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4039cf5
Add helpers to transform webtransport multiaddrs in AddrsFactory
MarcoPolo Mar 27, 2023
3e0b6c7
Remove unused line
MarcoPolo Mar 27, 2023
dd7cc53
PR nits
MarcoPolo Mar 27, 2023
fa0e2e6
Add wrapper around AddrsFactory to fill in missing certhash
MarcoPolo Mar 29, 2023
22c403d
PR nits
MarcoPolo Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions libp2p_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
tls "github.com/libp2p/go-libp2p/p2p/security/tls"
quic "github.com/libp2p/go-libp2p/p2p/transport/quic"
"github.com/libp2p/go-libp2p/p2p/transport/tcp"
webtransport "github.com/libp2p/go-libp2p/p2p/transport/webtransport"

ma "github.com/multiformats/go-multiaddr"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -289,3 +290,48 @@ func TestSecurityConstructor(t *testing.T) {
require.Contains(t, err.Error(), "failed to negotiate security protocol")
require.NoError(t, h2.Connect(context.Background(), ai))
}

func TestTransportConstructorWebTransport(t *testing.T) {
h, err := New(
Transport(webtransport.New),
DisableRelay(),
)
require.NoError(t, err)
defer h.Close()
require.NoError(t, h.Network().Listen(ma.StringCast("/ip4/127.0.0.1/udp/0/quic-v1/webtransport")))
err = h.Network().Listen(ma.StringCast("/ip4/127.0.0.1/udp/0/quic-v1/"))
require.Error(t, err)
require.Contains(t, err.Error(), swarm.ErrNoTransport.Error())
}

func TestTransportCustomAddressWebTransport(t *testing.T) {
customAddr, err := ma.NewMultiaddr("/ip4/1.2.3.4/udp/1234/quic-v1/webtransport")
if err != nil {
t.Fatal(err)
}
h, err := New(
Transport(webtransport.New),
DisableRelay(),
AddrsFactory(func(multiaddrs []ma.Multiaddr) []ma.Multiaddr {
customAddrWithCertHash := customAddr
for _, addr := range multiaddrs {
if webtransport.IsWebtransportMultiaddrWithCerthash(addr) {
customAddrWithCertHash = webtransport.CopyCerthashes(addr, customAddrWithCertHash)
break
}
}
return []ma.Multiaddr{customAddrWithCertHash}
}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind surfacing this requirement to any libp2p applications who require something like webtransport + port forwarding rather than just handling it internally (e.g. applying this automatically in the host constructor in config.go)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we could do the right thing in 100% of cases. For example, what if the server is listening on two webtransport addresses, which one are you forwarding to (the cerhashes need to line up)? This is something the application using go-libp2p has insight, but not go-libp2p itself. Another example, maybe you also want to return both multiaddrs instead of replacing it.

At a high level though, I think this option AddrsFactory is hard to use, and I'd like a better api that covers 90% of use cases in a simpler way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cerhashes need to line up

Aren't the certhashes always exactly the same on every webtransport interface you're listening on, so it doesn't matter what interface's certhashes you choose they should all be the same.

go-libp2p/p2p/transport/webtransport/cert_manager.go

Line 63 in 8c466ab

func newCertManager(hostKey ic.PrivKey, clock clock.Clock) (*certManager, error) {
only takes the private key and time and there's one private key per host and one time globally 😄.

An alternative approach to copy certhashes here could've been to just recalculate the deterministic certhashes and use them, but those functions are unexported in the webtransport package (as they probably should be since they're definitely internal implementation details). The way you've implemented this (and me in #2224) happens to be slightly less abstraction breaking.

At a high level though, I think this option AddrsFactory is hard to use, and I'd like a better api that covers 90% of use cases in a simpler way.

Better APIs sound😄. However, I don't want to rely on future refactors to fix existing issues especially if they could take a while to materialize.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't the certhashes always exactly the same on every webtransport interface you're listening on, so it doesn't matter what interface's certhashes you choose they should all be the same.

Good point. But this may not always be true. Maybe this is an argument for trying to do this ourselves?

You could have an address that doesn't have certhashes (I don't think we support this right now, but we want to) because it has a valid TLS cert. In that case you would want to forward something without a certhash. Automatically appending a certhash would be wrong.

#2224 was helpful, I took the tests from it. But I didn't like the change where config.go now has special knowledge about WebTransport addresses. I want to keep AddrsFactory as simple as possible.

However, I don't want to rely on future refactors to fix existing issues especially if they could take a while to materialize.

This change should unblock kubo, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to come up with a better change that keeps AddrsFactory simple and makes things better for the user if you have any suggestions!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. But this may not always be true. Maybe this is an argument for trying to do this ourselves?

Yeah, my hope is to ask fewer people to worry about the complexity here assuming it's not much worse in the central place we're dealing with it (e.g. in go-libp2p).

This change should unblock kubo, no?

Yeah, it will. Technically no changes were needed to unblock kubo as none of the changes made here access private fields/functions, these functions do make things easier though 😄. However, needing to add those extra lines of code seems like to surprise people.

If we went the currently proposed route I'd probably try to save other people, at least in the IPFS ecosystem, the hassle and also add it in a boxo helper so other IPFS implementations don't need to worry too much about it (e.g. ipfs/boxo#230). Maybe that'll help enough 🤷.

You could have an address that doesn't have certhashes (I don't think we support this right now, but we want to) because it has a valid TLS cert.

Yeah that's a good point. We could certainly do this and that would complicate things, although IIUC it would only cause a problem in a case where:

  1. You have an externally provided TLS cert for that address
  2. The TLS cert lives outside of go-libp2p (where the constructor couldn't access it) and that part of the QUIC connection establishment happens outside of go-libp2p

Maybe this a reasonable thing. I have no idea how common such a QUIC proxy setup is in general or would be for go-libp2p users. I don't know if people would complain too much if they had to pass extra options to deal with externally established connections.

My preference is to try and eat the complexity so the users don't have to. Given the cases not handled by automatic fixing in the constructor seem to not exist quite yet it seems like we can protect go-libp2p users from thinking about this for a little while longer which is 👍.

Happy to come up with a better change that keeps AddrsFactory simple and makes things better for the user if you have any suggestions!

Unfortunately, nothing better than what I've suggested is coming to mind at the moment. I can see the advantages to both views here, although I have my preference 😄. I'll take whatever the maintainers decide and incorporate with it in the downstreams I help maintain though.

Copy link
Collaborator Author

@MarcoPolo MarcoPolo Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The TLS cert lives outside of go-libp2p (where the constructor couldn't access it) and that part of the QUIC connection establishment happens outside of go-libp2p

No, we could pass a static tls config to the constructor and that would be fine. This isn't like websockets.

Okay. I think eating this complexity/messiness for the user is the nice thing to do. I'll update this PR.

Concretely:

  • If the user returns a webtransport multiaddr that doesn't have a certhash, and all our known addrs have a certhash, copy the certhash over.
    • If we only have a webtransport multiaddr without a certhash (client passed a static tls config), don't copy.
    • If we have both, log a warning and don't copy the certhash.

Does that seem right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that seems right to me. I suspect if we reach a scenario where someone is trying to instantiate two or more webtransport libp2p transports (e.g. 1+ with CA certs and 1 with the autogenerated ones) and they're having trouble specifying to the AddrsFactory how to signal "please give me autogenerated certs" that we can deal with it then.

I couldn't find an issue for enabling being a server with CA certs for webtransport (since they're not currently supported

go-libp2p/p2p/transport/webtransport/transport.go

Line 316 in 8c466ab

return nil, errors.New("static TLS config not supported on WebTransport")
). If not we should probably start one and then flag this edge case there since this is the kind of thing that could regress or surprise people if the feature isn't implemented in the near future (when any of us still remember this issue 😅).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2230 is the issue.

I've updated the PR. Thanks for your input @aschmahmann ☺️

)
require.NoError(t, err)
defer h.Close()
require.NoError(t, h.Network().Listen(ma.StringCast("/ip4/127.0.0.1/udp/0/quic-v1/webtransport")))
addrs := h.Addrs()
require.Len(t, addrs, 1)
require.NotEqual(t, addrs[0], customAddr)
restOfAddr, lastComp := ma.SplitLast(addrs[0])
restOfAddr, secondToLastComp := ma.SplitLast(restOfAddr)
require.Equal(t, lastComp.Protocol().Code, ma.P_CERTHASH)
require.Equal(t, secondToLastComp.Protocol().Code, ma.P_CERTHASH)
require.True(t, restOfAddr.Equal(customAddr))
}
59 changes: 56 additions & 3 deletions p2p/transport/webtransport/multiaddr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,13 @@ import (
"strconv"

ma "github.com/multiformats/go-multiaddr"
mafmt "github.com/multiformats/go-multiaddr-fmt"
manet "github.com/multiformats/go-multiaddr/net"
"github.com/multiformats/go-multibase"
"github.com/multiformats/go-multihash"
)

var webtransportMA = ma.StringCast("/quic-v1/webtransport")

var webtransportMatcher = mafmt.And(mafmt.IP, mafmt.Base(ma.P_UDP), mafmt.Base(ma.P_QUIC_V1), mafmt.Base(ma.P_WEBTRANSPORT))

func toWebtransportMultiaddr(na net.Addr) (ma.Multiaddr, error) {
addr, err := manet.FromNetAddr(na)
if err != nil {
Expand Down Expand Up @@ -78,3 +75,59 @@ func addrComponentForCert(hash []byte) (ma.Multiaddr, error) {
}
return ma.NewComponent(ma.ProtocolWithCode(ma.P_CERTHASH).Name, certStr)
}

// IsWebtransportMultiaddrWithCerthash returns true if the given multiaddr is a
// well formed webtransport multiaddr. Returns the number of certhashes found.
func IsWebtransportMultiaddr(multiaddr ma.Multiaddr) (bool, int) {
const (
init = iota
foundUDP
foundQuicV1
foundWebTransport
)
state := init
certhashCount := 0

ma.ForEach(multiaddr, func(c ma.Component) bool {
if c.Protocol().Code == ma.P_QUIC_V1 && state == init {
state = foundUDP
}
if c.Protocol().Code == ma.P_QUIC_V1 && state == foundUDP {
state = foundQuicV1
}
if c.Protocol().Code == ma.P_WEBTRANSPORT && state == foundQuicV1 {
state = foundWebTransport
}
Comment on lines +91 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use an address matcher? We shouldn't be rewriting basic multiaddr functions here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address matcher doesn't do exactly what I want which is:

  1. I don't care if it's ipv4/dns4/ipv6/dns6
  2. I don't care if it has certhashes or not, but if it does tell me how many.

I know about address matcher, it just didn't do what I needed. See that hack that I removed in CanDial. This is pretty straightforward, but it would be good to expand the address matcher to handle these use cases. Doing that in this PR is out of scope though.

if c.Protocol().Code == ma.P_CERTHASH && state == foundWebTransport {
certhashCount++
}
return true
})
return state == foundWebTransport, certhashCount
}

// IsWebtransportMultiaddrWithCerthash returns true if the given multiaddr is a
// well formed webtransport multiaddr with a certificate hash.
func IsWebtransportMultiaddrWithCerthash(multiaddr ma.Multiaddr) bool {
ok, n := IsWebtransportMultiaddr(multiaddr)
return ok && n > 0
}

// CopyCerthashes copies the certificate hashes from the first multiaddr to the
// second. If there are no multiaddrs in the first, the second is returned
// unchanged. Otherwise a new multiaddr is returned with certhashes appeneded
// to it
func CopyCerthashes(existingMultiaddrWithCerthashes, multiaddrWithoutCerthashes ma.Multiaddr) ma.Multiaddr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to just have a AddCerthashes(ma.Multiaddr) ma.Multiaddr that adds current set of certhashes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would for the other thing, but not for this. It would need a reference to the
transport which would be annoying. This is a pure function that can be used in
the AddrsFactory with no other context.

certhashComponents := make([]ma.Component, 0, 2) // 2 is the common case
ma.ForEach(existingMultiaddrWithCerthashes, func(c ma.Component) bool {
if c.Protocol().Code == ma.P_CERTHASH {
certhashComponents = append(certhashComponents, c)
}
return true
})
out := multiaddrWithoutCerthashes
for _, certComponent := range certhashComponents {
out = out.Encapsulate(&certComponent)
}
return out
}
48 changes: 48 additions & 0 deletions p2p/transport/webtransport/multiaddr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,51 @@ func TestWebtransportResolve(t *testing.T) {
require.Error(t, err)
})
}

func TestIsWebtransportMultiaddrWithCerthash(t *testing.T) {
fooHash := encodeCertHash(t, []byte("foo"), multihash.SHA2_256, multibase.Base58BTC)
barHash := encodeCertHash(t, []byte("bar"), multihash.SHA2_256, multibase.Base58BTC)

testCases := []struct {
addr string
want bool
certhashCount int
}{
{addr: "/ip4/1.2.3.4/udp/60042/quic-v1/webtransport", want: true},
{addr: "/ip4/1.2.3.4/udp/60042/quic-v1/webtransport/certhash/" + fooHash, want: true, certhashCount: 1},
{addr: "/ip4/1.2.3.4/udp/60042/quic-v1/webtransport/certhash/" + fooHash + "/certhash/" + barHash, want: true, certhashCount: 2},
{addr: "/dns4/example.com/udp/60042/quic-v1/webtransport/certhash/" + fooHash, want: true, certhashCount: 1},
{addr: "/dns4/example.com/udp/60042/webrtc/certhash/" + fooHash, want: false},
}

for _, tc := range testCases {
t.Run(tc.addr, func(t *testing.T) {
got, n := IsWebtransportMultiaddr(ma.StringCast(tc.addr))
require.Equal(t, tc.want, got)
require.Equal(t, tc.certhashCount, n)
require.Equal(t, tc.want && n > 0, IsWebtransportMultiaddrWithCerthash(ma.StringCast(tc.addr)))
})
}
}

func TestCopyCerthashes(t *testing.T) {
fooHash := ma.StringCast("/certhash/" + encodeCertHash(t, []byte("foo"), multihash.SHA2_256, multibase.Base58BTC)).String()
barHash := ma.StringCast("/certhash/" + encodeCertHash(t, []byte("bar"), multihash.SHA2_256, multibase.Base58BTC)).String()

testCases := []struct {
addr string
want string
}{
{addr: "/ip4/1.2.3.4/udp/60042/quic-v1/webtransport", want: "/ip4/4.3.2.1/udp/24006/quic-v1/webtransport"},
{addr: "/ip4/1.2.3.4/udp/60042/quic-v1/webtransport" + fooHash, want: "/ip4/4.3.2.1/udp/24006/quic-v1/webtransport" + fooHash},
{addr: "/ip4/1.2.3.4/udp/60042/quic-v1/webtransport" + fooHash + barHash, want: "/ip4/4.3.2.1/udp/24006/quic-v1/webtransport" + fooHash + barHash},
}

for _, tc := range testCases {
t.Run(tc.addr, func(t *testing.T) {
got := CopyCerthashes(ma.StringCast(tc.addr), ma.StringCast("/ip4/4.3.2.1/udp/24006/quic-v1/webtransport"))
require.Equal(t, tc.want, got.String())
})
}

}
18 changes: 4 additions & 14 deletions p2p/transport/webtransport/transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,23 +286,13 @@ func decodeCertHashesFromProtobuf(b [][]byte) ([]multihash.DecodedMultihash, err
}

func (t *transport) CanDial(addr ma.Multiaddr) bool {
var numHashes int
ma.ForEach(addr, func(c ma.Component) bool {
if c.Protocol().Code == ma.P_CERTHASH {
numHashes++
}
return true
})
// Remove the /certhash components from the multiaddr.
// If the multiaddr doesn't contain any certhashes, the node might have a CA-signed certificate.
for i := 0; i < numHashes; i++ {
addr, _ = ma.SplitLast(addr)
}
return webtransportMatcher.Matches(addr)
ok, _ := IsWebtransportMultiaddr(addr)
return ok
}

func (t *transport) Listen(laddr ma.Multiaddr) (tpt.Listener, error) {
if !webtransportMatcher.Matches(laddr) {
isWebTransport, _ := IsWebtransportMultiaddr(laddr)
if !isWebTransport {
return nil, fmt.Errorf("cannot listen on non-WebTransport addr: %s", laddr)
}
if t.staticTLSConf == nil {
Expand Down