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.

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

Allow connections to share a port by adding restrictions on zero-length connection IDs #2851

Merged
merged 9 commits into from
Sep 13, 2019

Conversation

DavidSchinazi
Copy link
Contributor

In the current spec, zero-length connection IDs can be used when a host wants to route connections on the 4-tuple (remote IP/port, local IP/port). However, doing this prevents your peer from multiplexing connections on a single port. This PR restricts zero-length connection IDs to only be used when a host can route connection on the local 2-tuple (IP/port).

The current spec does not allow clients to share a local port ever, because at the time they start their connection they cannot know if a given server will want to route on the 4-tuple. Additionally, some clients might even be forced to perform this multiplexing because they are going through a proxy. Slightly restricting the use of zero-length connection IDs gives value to client connection IDs.

Fixes #2844.

@mikkelfj
Copy link
Contributor

The current spec does not allow clients to share a local port ever, because at the time they start their connection they cannot know if a given server will want to route on the 4-tuple.

I'm not sure that is true, but if is, it should be fixed.

A P2P server scenario will definitely want to use a single port on both ends, and which one is client, might change at any given moment. In this case you have out of band knowledge about path behaviour.

For client p2p I'd hope that it is also possible to only use a single port when a CID is used.

@DavidSchinazi
Copy link
Contributor Author

@mikkelfj in the P2P case you often have a bootstrapping protocol (ICE, STUN, TURN, etc.) that gives you a single-use ephemeral port on both sides, so you can then happily use zero-length connection IDs because you'll only have one connection on that port. If you expect to share the port for multiple connections, then you can use non-zero-length CIDs, there's nothing in the spec (or this PR) that prevents that.

@mikkelfj
Copy link
Contributor

On client P2P / ICE: These are yesterdays protocols. You can have a new version of these brokers. If you use IPV6 you could skipping the port step. Also a client implementation might skip the part of supporting multiple ports because you a skipping the OS and go with raw sockets, netmap, etc.. It is much simpler to just rely on the CID. Especially if you consider implementations that can also run outside of IP networks.

@mikkelfj
Copy link
Contributor

mikkelfj commented Jun 27, 2019

On server P2P you do not have a boot strapping protocol. Then it is integral to the service logic being implemented. I have a strong use case for this and would be very sad if ports cannot be stable.

@DavidSchinazi
Copy link
Contributor Author

@mikkelfj if your use-case uses non-zero-length connection IDs, then it is not relevant to this PR. This PR does not change how non-zero-length CIDs work.

@mikkelfj
Copy link
Contributor

My objection was to the sentence "The current spec does not allow clients to share a local port ever"
which implies also with CID. Whether you PR fixes this or not is not clear to me, but I find it problematic if is correct and then not fixed.

@DavidSchinazi
Copy link
Contributor Author

Yes it is problematic, and this PR attempts to fix that.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

I like this. It makes sense. I've suggested a rewording of the text you proposed, but I think that this the right insight. Namely, you can only use a zero-length connection ID if other information that is under your control is sufficient to identify a connection.

My only reservation is over the potential for an endpoint to be willing to use trial decryption here. In theory, you could demux using trial decryption. It seems crazy, but we've already got cases for ESNI where that turns out to be a useful concept.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
@DavidSchinazi
Copy link
Contributor Author

Thanks @martinthomson . I think the current text doesn't explicitly ban trial decryption: technically that endpoint is able to identify the connection using only its IP and port so it's good to go. The important bit is that it doesn't require the peer to alter its behavior.

@martinthomson martinthomson added the design An issue that affects the design of the protocol; resolution requires consensus. label Jul 23, 2019
Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

👍 The requirements look good to me. I only have one editorial suggestion.

draft-ietf-quic-transport.md Show resolved Hide resolved
A zero-length connection ID can be used when a connection ID is not needed
to route to the correct endpoint. An endpoint SHOULD NOT use a zero-length
connection ID unless it can use only its IP address and port to identify a
connection. The IP address and port used by a peer cannot be used for routing
Copy link
Member

Choose a reason for hiding this comment

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

The substance of the discussion we had suggests that this "cannot" is really a SHOULD NOT.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we wanted to say that those weren't adequate on their own. I would also change this to active. "An endpoint SHOULD NOT use only the peer's IP address and port ..."

I would actually suggest dropping this sentence and the sentence after this one. You can go straight to the multiplexing sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Co-Authored-By: Mike Bishop <mbishop@evequefou.be>
A zero-length connection ID can be used when a connection ID is not needed
to route to the correct endpoint. An endpoint SHOULD NOT use a zero-length
connection ID unless it can use only its IP address and port to identify a
connection. The IP address and port used by a peer cannot be used for routing
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we wanted to say that those weren't adequate on their own. I would also change this to active. "An endpoint SHOULD NOT use only the peer's IP address and port ..."

I would actually suggest dropping this sentence and the sentence after this one. You can go straight to the multiplexing sentence.

lifetime, and the peer can reuse a given address and port for additional
connections. Similarly, the peer's connection IDs cannot be used for routing
or identification, as they are not transmitted in the short header packets
they send. Note that multiplexing while using zero-length connection IDs and
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested: "Multiplexing connections on the same local IP address and port while using zero-length connection IDs will cause failures in the presence of connection migration, NAT rebinding, and client
port reuse." Drop the rest of this sentence -- its redundant with the "SHOULD NOT" above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped that around to remove the redundancy

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Show resolved Hide resolved
local IP address and port while using zero-length connection IDs will cause
failures in the presence of connection migration, NAT rebinding, and client port
reuse; and therefore MUST NOT be done unless an endpoint is certain that those
protocol features are not in use.
Copy link
Member

Choose a reason for hiding this comment

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

If I were to add commentary here, I would say "While this usage model appears to be substantially similar to common TCP connection idioms, the realities of UDP deployment mean that relying on peer addressing information for identifying connections can be significantly less reliable in UDP than TCP."

Copy link
Member

Choose a reason for hiding this comment

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

(Replying self, bad look, sorry.) Is this (^^) something that the applicability draft might want to say? @mirjak, @britram, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both sound good to me, I agree that this is a good point to make, and the applicability draft sounds like a good home

@martinthomson
Copy link
Member

I rebased this. There was a conflict that might be worth checking: @erickinnear rewrote the disable_[active_]migration bit. Where this removed text forbidding migration with a zero-length connection ID, that text no longer appeared.

@DavidSchinazi
Copy link
Contributor Author

Thanks for the rebase @martinthomson

@erickinnear
Copy link
Contributor

Rebase looks good to me too, thanks @martinthomson!

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
DavidSchinazi and others added 2 commits August 16, 2019 13:22
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
@DavidSchinazi
Copy link
Contributor Author

What next steps should be taken here? Is this ready for a consensus call?

@martinthomson
Copy link
Member

Yeah, the chairs have this on their list (the issue is marked). Patience.

@martinthomson martinthomson merged commit fba5600 into quicwg:master Sep 13, 2019
@martinthomson
Copy link
Member

That came around sooner than expected :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client connection IDs are broken
9 participants