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

Discarding connection state at server on unvalidated client #2656

Closed
janaiyengar opened this issue Apr 28, 2019 · 13 comments
Closed

Discarding connection state at server on unvalidated client #2656

janaiyengar opened this issue Apr 28, 2019 · 13 comments
Assignees
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@janaiyengar
Copy link
Contributor

The recovery draft says:

Until the server has validated the client's address on the path, the amount of	
data it can send is limited, as specified in Section 8.1 of {{QUIC-TRANSPORT}}.	
If not all unacknowledged CRYPTO data can be sent, then all unacknowledged	
CRYPTO data sent in Initial packets should be retransmitted.  If no data can be	
sent, then no alarm should be armed until data has been received from the	
client.

@mikkelfj notes in #2655 that the sender should discard connection state after some time.

This should probably happen after the idle timeout. In which case, the idle timeout timer ought to be armed.

@janaiyengar janaiyengar added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Apr 28, 2019
@mikkelfj
Copy link
Contributor

I think idle timeout might be too long depending on have cheaply a rogue client can initiate new connection attempts, possibly from spoofed source addresses, but that needs more analysis.

@ianswett
Copy link
Contributor

I agree there should be a handshake timeout, which I believe is what you're discussing. But if so, that seems like a transport issue?

@marten-seemann
Copy link
Contributor

I agree with @ianswett that this is a transport issue, and doesn't belong in the recovery document. There we are only talking about the timers needed for loss recovery, and I would find it confusing to talk about closing connections there.
I'm not sure if we need to have any text about a handshake timeout. An idle timeout should be sufficient to garbage-collect connections that aren't active any more. Having a (shorter) handshake timeout is an optimization that implementations might or might not want to have, depending on the threat model.

@mikkelfj
Copy link
Contributor

More generally on idle timeout - would it be OK to do a periodic GC scan risking up to 100% later timeout than the advertised idle timeout? Avoiding timers in passive connections is much cheaper.

@martinthomson
Copy link
Member

Yes, idle timeout on a periodic sweep is likely better than assuming that a timer is in play. As @marten-seemann says, anything we say here is in optimization territory. It is likely a very useful optimization, but we don't need to say too much in the spec unless there is an interop or security hazard. Offhand, I can't see anything other than a resource exhaustion problem, but that's clearly something that the periodic sweep should address.

@mikkelfj
Copy link
Contributor

Am I correct in assuming that the PTO timer is not generally armed when data is transmitted actively on quality connections, or a while after data has been delivered?

Combined with idle timeout scans this would mean that most long running connections would likely not have an active timer.

This is a highly desirable property since a precise timer is O(log N). As far as I can tell theoretically O(1) timer wheels are no more effective than a good O(log N) implementation, including foregoing ordering below a certain resolution. There is a big difference between 1,000 entries and 100,000 timer entries.

@martinthomson martinthomson added -transport and removed -recovery design An issue that affects the design of the protocol; resolution requires consensus. labels Apr 30, 2019
@ianswett
Copy link
Contributor

This seems somewhat related to Issue #2602

@janaiyengar
Copy link
Contributor Author

Fair point, @ianswett (and others) -- this should be against transport. It's probably worth clarifying that last sentence so it's not confusing to implementers.

@mnot mnot added the design An issue that affects the design of the protocol; resolution requires consensus. label May 10, 2019
@mnot mnot moved this from Triage to Design Issues in Late Stage Processing May 10, 2019
@larseggert
Copy link
Member

Discussed in Montreal, decision is to flip to editorial

@mnot mnot added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Aug 16, 2019
@mnot mnot moved this from Design Issues to Consensus Emerging in Late Stage Processing Aug 16, 2019
@mnot mnot moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Aug 16, 2019
@mnot mnot added editorial An issue that does not affect the design of the protocol; does not require consensus. and removed design An issue that affects the design of the protocol; resolution requires consensus. proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. labels Aug 26, 2019
@mnot mnot moved this from Consensus Call issued to Editorial Issues in Late Stage Processing Aug 26, 2019
@martinthomson
Copy link
Member

After reviewing this, I'm inclined to do nothing. The idle timeout covers this already. Servers are always permitted to drop connections sooner, but we don't need to specify anything. My primitive server doesn't have a problem with this as connections eventually drop based on the idle timeout.

@ianswett
Copy link
Contributor

The originally quoted section has been substantially rewritten and now specifies the PTO alarm, so there's no confusion about which alarm.

So I support @martinthomson suggestion to close this with no further action/OBE.

@martinthomson
Copy link
Member

@janaiyengar, if you agree with the previous suggestions, we could have one fewer open issue with very little effort.

@janaiyengar
Copy link
Contributor Author

My experience with this suggests that things work fine with just the idle timeout, so closing this.

Late Stage Processing automation moved this from Editorial Issues to Issue Handled Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

7 participants