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

Saving unknown transport parameters should be prohibited #4112

Closed
marten-seemann opened this issue Sep 21, 2020 · 8 comments · Fixed by #4165
Closed

Saving unknown transport parameters should be prohibited #4112

marten-seemann opened this issue Sep 21, 2020 · 8 comments · Fixed by #4165

Comments

@marten-seemann
Copy link
Contributor

The spec currently says

A client need not store transport parameters it cannot process.

Assume now that the client does store a transport parameter it doesn't understand. At the time when it actually establishes a 0-RTT connection, the implementation was updated (or new extensions were activated), and now it understands the TP. This would lead to unintended consequences, especially if this was a TP that it was prohibited from saving in the first place.

I think the sentence would be clearer if it said MUST NOT. Not sure if that is an editorial change though.

@larseggert larseggert added this to Triage in Late Stage Processing via automation Sep 21, 2020
@martinthomson
Copy link
Member

There is an unstated assumption here: that the client doesn't change its opinion about transport parameters between two connections. This applies to other transport parameters. For instance, a client might like a stream limit it is given when initially connecting, but might change its mind when trying 0-RTT (maybe the limit is too small to bother with 0-RTT).

A MUST NOT in this case is worse than not answering the question. Let's say that the client initially doesn't support max_udp_payload_size as it always sends 1200 byte datagrams. As it cannot process this when it first connects, it doesn't store the value. Later, when it attempts to connect it has been updated to understand this transport parameter. As it has been prohibited from storing the value, it can't read the value that it might have otherwise saved as an opaque value. So it attempts 0-RTT and assumes that it can send 1400 byte packets safely, when it should know that it can't.

Generally, this doesn't happen because defaults for transport parameters are the least permissive value. New transport parameters generally have defaults that are safe to use in 0-RTT in case the original value was not remembered. However, we don't require that.

(This is a bad example, as we explicitly don't worry about violating the datagram size limit in 0-RTT, but I didn't have a more evocative example.)

@larseggert
Copy link
Member

@marten-seemann with @martinthomson's explanation, does anything need to be done still in your opinion?

@marten-seemann
Copy link
Contributor Author

@martinthomson's example seems to be an argument for requiring clients to store unknown TPs, and checking their status (should have been saved for 0-RTT or shouldn't have been saved) when restoring them. Is that something we should require?

@martinthomson
Copy link
Member

Or it is an argument for you to throw out resumption information if you have added new capabilities that might depend on stuff you previously threw out.

@ianswett
Copy link
Contributor

I think it's an argument for discarding resumption information when adding new capabilities. FWIW, Chrome doesn't persist resumption information to disk for security reasons, but this has the side benefit that this information does not persist across restarts, including binary updates.

One exception I can imagine is a client which gets an experiment config push that doesn't require a restart, enabling or disabling a new extension feature. However, in that case, not saving unknown TPs still seems safest, since if the extension depended upon the unknown TP, then it's just not activated.

I'm worried max_udp_payload_size is a bad example because it's part of the core spec and it's not similar to extensions which add new frames or change the behavior of existing frames.

@janaiyengar
Copy link
Contributor

Either interpretation fixes the issue, but I would agree that resumption information shouldn't be used when the implementation changes behavior. But it could store all unknown TPs, and then reinterpret later. The current text allows both, which seems adequate.

This isn't important enough to warrant a design change. I'm fine with the current text, but I'd be open to an editorial fix if someone sent a PR.

@MikeBishop
Copy link
Contributor

If the client stores the value, then gains the capability to interpret it, it also gains the capability to know it should be ignoring any cached value. I'm not too concerned about that variant. If the client doesn't store the value but the server is still enforcing it, that's handled by making the default the least permissive value. If we don't state that assumption, perhaps we should.

@ianswett
Copy link
Contributor

I'd be happy with an editorial PR here to clarify some of this, but I'm not feeling confident enough to write it.

martinthomson added a commit that referenced this issue Sep 24, 2020
Late Stage Processing automation moved this from Triage to Issue Handled Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

6 participants