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

MarcoPolo · 2023-03-27T22:16:13Z

I think the AddrsFactory is a bit hard to use. This PR doesn't address that.

This PR adds some helper functions to allow users to do the correct thing inside AddrsFactory. Fixes #2223

Users can add their own webtransport mappings by copying certhashes from an existing multiaddr. For example,

AddrsFactory(func(multiaddrs []ma.Multiaddr) []ma.Multiaddr {
	outAddrs := make([]ma.Multiaddr, 0, len(multiaddrs)+1)
	for _, addr := range multiaddrs {
		// Keep the original addrs.
		outAddrs = append(outAddrs, addr)
		if webtransport.IsWebtransportMultiaddrWithCerthash(addr) {
			// Add our custom addr with certhashes from an existing addr.
			outAddrs = append(outAddrs, webtransport.CopyCerthashes(addr, customAddr))
		}
	}
	return outAddrs
})

This also allows users to do this only for specifics addresses in case they are listening on multiple addresses.

marten-seemann

It's unfortunate that we need this. It's a lot of additional API for something that "should just work". AddrsFactory really is a terrbile API, and we should work on cleaning that up soon.

p2p/transport/webtransport/multiaddr.go

marten-seemann · 2023-03-27T22:27:14Z

p2p/transport/webtransport/multiaddr.go

+	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
+		}


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

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

I don't care if it's ipv4/dns4/ipv6/dns6

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.

p2p/transport/webtransport/multiaddr.go

p2p/host/basic/basic_host.go

aschmahmann · 2023-03-28T17:40:55Z

libp2p_test.go

+		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}
+		}),


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)?

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.

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.

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?

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

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:

You have an externally provided TLS cert for that address

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.

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?

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 😅).

#2230 is the issue.

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

aschmahmann · 2023-03-29T09:59:44Z

options.go

+		if factory == nil {
+			return fmt.Errorf("cannot specify a nil address factory")


Putting this in this function is likely fine for everyone, but why not in config.go? Technically users can also instantiate directly with config.NewNode()

go-libp2p/config/config.go

Line 288 in 8c466ab

func (cfg *Config) NewNode() (host.Host, error) {

although I couldn't find any examples of anyone who did.

Is the idea to give people a way out of this functionality if there ends up being a problem/the user wants a little more control?

The idea is that this is just middleware. It just wraps the function and the rest of the code doesn't care that this changed.

Another reason to not put it in config.go is that function is already a bit unwieldy

Technically users can also instantiate directly with config.NewNode()

go-libp2p/config/config.go

Line 288 in 8c466ab

func (cfg *Config) NewNode() (host.Host, error) {

although I couldn't find any examples of anyone who did.

Oh no, please don't do that, @aschmahmann! config will be removed soon-ish.

Lol no problem will keep not using it 😜. Was just flagging the code path here as exposed/not having receiving this fix. If you just want to leave it alone your call though.

marten-seemann · 2023-04-05T02:42:43Z

options.go

+		if factory == nil {
+			return fmt.Errorf("cannot specify a nil address factory")


Technically users can also instantiate directly with config.NewNode()

go-libp2p/config/config.go

Line 288 in 8c466ab

func (cfg *Config) NewNode() (host.Host, error) {

although I couldn't find any examples of anyone who did.

Oh no, please don't do that, @aschmahmann! config will be removed soon-ish.

options.go

marten-seemann · 2023-04-05T02:49:32Z

p2p/transport/webtransport/multiaddr.go

+// 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 {


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

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.

marten-seemann

LGTM, modulo one suggestion for the comment.

marten-seemann · 2023-04-05T04:48:43Z

options.go

+								// This will give us the certhash. All certhashes across all listening addresses.
+								multiaddrWithCerthash = inMultiaddr
+							} else if ok && n == 0 {
+								// This is possible if we are listening with a valid static TLS cert.


Suggested change

// This is possible if we are listening with a valid static TLS cert.

// This shouldn't be possible, since we don't support static TLS certs

Add helpers to transform webtransport multiaddrs in AddrsFactory

4039cf5

marten-seemann reviewed Mar 27, 2023

View reviewed changes

MarcoPolo added 2 commits March 27, 2023 15:47

Remove unused line

3e0b6c7

PR nits

dd7cc53

aschmahmann reviewed Mar 28, 2023

View reviewed changes

MarcoPolo mentioned this pull request Mar 29, 2023

feat: Enable static tls config in Webtransport #2230

Open

Add wrapper around AddrsFactory to fill in missing certhash

fa0e2e6

aschmahmann mentioned this pull request Mar 31, 2023

Cannot advertise WebTransport with public IP addresses discovered by identify (e.g. port forwarding behind a NAT with a dynamic public IP) #2233

Closed

This was referenced Apr 3, 2023

fix: fix externally defined webtransport addresses #2224

Closed

v0.26.5 #2236

Closed

aschmahmann reviewed Apr 4, 2023

View reviewed changes

marten-seemann mentioned this pull request Apr 5, 2023

go-libp2p v0.27 #2123

Closed

21 tasks

marten-seemann reviewed Apr 5, 2023

View reviewed changes

PR nits

22c403d

marten-seemann approved these changes Apr 5, 2023

View reviewed changes

MarcoPolo mentioned this pull request Apr 5, 2023

correctly handle WebTransport addresses without certhashes #2239

Merged

marten-seemann closed this in #2239 Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MarcoPolo commented Mar 27, 2023

marten-seemann left a comment

marten-seemann Mar 27, 2023

MarcoPolo Mar 27, 2023

aschmahmann Mar 28, 2023

MarcoPolo Mar 28, 2023

aschmahmann Mar 28, 2023

MarcoPolo Mar 28, 2023

MarcoPolo Mar 28, 2023

aschmahmann Mar 28, 2023

MarcoPolo Mar 29, 2023 •

edited

Loading

aschmahmann Mar 29, 2023

MarcoPolo Mar 29, 2023

aschmahmann Mar 29, 2023

MarcoPolo Apr 5, 2023

marten-seemann Apr 5, 2023

aschmahmann Apr 5, 2023

marten-seemann Apr 5, 2023

marten-seemann Apr 5, 2023

MarcoPolo Apr 5, 2023

marten-seemann left a comment

marten-seemann Apr 5, 2023

		if factory == nil {
		return fmt.Errorf("cannot specify a nil address factory")

	// This is possible if we are listening with a valid static TLS cert.
	// This shouldn't be possible, since we don't support static TLS certs

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

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

Conversation

MarcoPolo commented Mar 27, 2023

marten-seemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo Mar 29, 2023 •

edited

Loading