proto: provide reason when refusing connections

[djc]

No description provided.

[Ralith]

Connection::close takes Bytes. Should we do the same here?

[djc]

Why? ApplicationClose::reason is (publicly) Bytes but TransportError::reason is (publicly) String -- I feel like String is probably more reasonable in terms of the public API, and it doesn't feel like there's a bunch of performance to be had from refcounting error reasons instead of allocating?

[Ralith]

It gets baked down into ConnectionClose::reason, which exactly mirrors ApplicationClose::reason as Bytes, and is not strictly required to be UTF-8. The involvement of TransportError is an artifact of the private initial_close API, which we can easily revisit. Most important, though, is just that it would be more consistent with close, which is morally the same operation, just at a different time.

[djc]

RFC 9000, section 19.19:

• Reason Phrase: Additional diagnostic information for the closure. This can be zero length if the sender chooses not to give details beyond the Error Code value. This SHOULD be a UTF-8 encoded string [RFC3629], though the frame does not carry information, such as language tags, that would aid comprehension by any entity other than the one that created the text.

Which makes me feel that String is a better API than Bytes.

[Ralith]

I agree that there's a case for preferring String here, though I don't think it's totally clear cut -- it's nice to leave SHOULD stuff to the application when feasible, especially when the costs of doing it wrong are limited.

However, we already have a Bytes API for this elsewhere. IMO consistency trumps this concern entirely.

[djc]

@gretchenfrage you mentioned you want to review this -- friendly ping?

[gretchenfrage]

Sorry, I hadn't meant to block this as long as I have! I'll try not to much longer.

[gretchenfrage]

Is this compliant with RFC 9000? 10.2.3-3 states:

Sending a CONNECTION_CLOSE of type 0x1d in an Initial or Handshake packet could expose application state or be used to alter application state. A CONNECTION_CLOSE of type 0x1d MUST be replaced by a CONNECTION_CLOSE of type 0x1c when sending the frame in Initial or Handshake packets. Otherwise, information about the application state might be revealed. Endpoints MUST clear the value of the Reason Phrase field and SHOULD use the APPLICATION_ERROR code when converting to a CONNECTION_CLOSE of type 0x1c.

From what I can tell:

• Endpoint::initial_close always constructs frame::Close from TransportError, thus constructing a frame::Close::Connection(ConnectionClose) and encoding a CONNECTION_CLOSE of type 0x1c.
• frame::ConnectionClose::encode doesn't actually perform any dynamic checks to clear the reason phrase if the phase is initial or handshake, so I'm supposing that we must be relying on the fact that we simply never attempt to encode and a frame::Close with a reason phrase if the handshake isn't finished.
• This PR seems to change that, and this is a "MUST" clause in RFC 9000.

[Ralith]

My reading of that text is that the reason phrase and code must be cleared in cases where we would normally send a 0x1d frame. In this case we're explicitly sending 0x1c from the start, so it doesn't apply. For comparison, consider the various error codes that mostly come up during handshakes, e.g. INVALID_TOKEN.

That said, providing an avenue to expose application state in a CONNECTION_CLOSE reason might be against the spirit of this text.

[gretchenfrage]

My reading of that text is that the reason phrase and code must be cleared in cases where we would normally send a 0x1d frame. In this case we're explicitly sending 0x1c from the start, so it doesn't apply. For comparison, consider the various error codes that mostly come up during handshakes, e.g. INVALID_TOKEN.

That said, providing an avenue to expose application state in a CONNECTION_CLOSE reason might be against the spirit of this text.

Based on this, possibly a PR to add the hard-coded reason messages would be less controversial than adding refuse_reason and deprecating refuse?

I'll wait to see what djc says.

Unrelated: Despite the commit and PR message, this change extends beyond proto. This may cause mistakes to be made when writing release change logs.

[djc]

That said, providing an avenue to expose application state in a CONNECTION_CLOSE reason might be against the spirit of this text.

I agree. Going to close this.

@gretchenfrage thanks for digging into this and finding the relevant spec reference!