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

PONG frame #878

Merged
merged 3 commits into from Oct 17, 2017
Merged

PONG frame #878

merged 3 commits into from Oct 17, 2017

Conversation

martinthomson
Copy link
Member

Split this from the migration patch.

@mikkelfj
Copy link
Contributor

I like the PONG especially because it can elicit an immediate response when app protocol so desires as opposed to delayed ACKs. This can be used liveness checks at various levels in the stack.

I assume the length field is 8 bytes in anticipation of a future var length encoding that has not yet been made to mainstream, otherwise 8 bytes is a way to big number. Allowing it to be large may put unnecessary security constraints on the app protocols because the PING frame can be abusively large on large MTU's forcing PING response caches waiting for app protocol to respond and thus waste memory. There is on overall flow control limit, but no limit like for stream data.

My immediate thought was that a simple single integer would suffice and simplify the idea without giving up much. But, I can see that you might want a serial number + an application subsystem identifier, or more generally, an encoded message in one of many possible binary encodings.

Thus, I think some safety concerns must be considered, possibly limiting the frequence or size, or at least very clearly state the application protocol must be aware of any potential attack vectors.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this out. A couple of points, inline.


Data:

: This variable-length field contains arbitrary data.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Upon loss detection, a retransmitted PING MUST carry the same data."

Copy link
Member 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 that's necessary. On the contrary, it could increase the state commitment for the sender. What is being tested is the need for a PING/PONG. We're not trying to send data.

For reference, ICE sends similar probes and every single one of them contains a different transaction ID (the field that is analogous to the field that this PR adds), especially when the probe is being sent multiple times over a long period of time. QUIC doesn't have the same identification problem - it can use ACKs to correlate packets - but it does have to verify that a particular was correctly received, and it can be more certain of that if the PING contains fresh entropy every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm -- yeah, I forgot that there are ACKs for PINGs.

An endpoint that receives an unsolicited PONG frame - that is, a PONG frame
containing a payload that is either empty or not identical to the content of a
PING frame that the endpoint previously sent - MUST generate a connection error
of type UNSOLICITED_PONG. Note that the Data field of the frame is the only
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to account for retransmission/duplication of PONG packets. If an endpoint transmits a PONG, has a timeout and retransmits the PONG, then the receiver of the first PONG will discard state associated with the PING, and will close the connection on receiving the second PONG.

You could ask the PING sender to remember PINGs sent over the past X RTTs, or simply discard PONGs with unknown data. I'd recommend (and implement) the latter, since its easier, has very little state, and there's no way to enforce that an endpoint terminate a connection on receiving a bad PONG.

Copy link
Member Author

Choose a reason for hiding this comment

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

...or you could include a MAC in the payload and avoid all state, thereby avoiding the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. My suggestion here was to remove the requirement that a PONG MUST match an open PING. This seems unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you be OK with MAY? I want to ensure that bad actors can't guess PONG values without consequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bad actors can simply send random PONGs and cause the connection to be torn down, that's quite a bad consequence, isn't it? I was trying to prevent the connection from getting torn down because of legitimate operational issues, such as a duplicate PONG reaching the PING sender late. Basically, you're currently requiring that the sender of a PING hold some state without stating exactly what state, and if it sees a PONG that doesn't match the unspecified state, that it kill the connection.

I am fine with saying MAY, but I would qualify the sentence with something like "If the receiver of a PONG can be certain that the received Data was not sent by it .... ". WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can work with that; it's what I was thinking.

FWIW, I didn't have a design that required state in mind. But you are right to point out that a MUST narrows the range of reasonable designs considerably.

Not sure about the "bad actors" point you make here. The point is to have the connection torn down if someone starts generating random PONGs. After all, if you want to close the connection and you have the keys, you have other more direct methods available to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can totally see that you can build this without state, but we have to prepare for a naive implementer who doesn't recognize that a simple implementation that remembers exactly one PING is likely to break in a variety of situations.

About the "bad actors", agreed.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

Thanks for working through this!

@martinthomson martinthomson merged commit dd717ab into master Oct 17, 2017
@martinthomson martinthomson deleted the pong branch October 17, 2017 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants