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

Change text that describes version negotiation in terms of crypto keys. #141

Merged
merged 3 commits into from Jan 12, 2017

Conversation

janaiyengar
Copy link
Contributor

No description provided.

@janaiyengar janaiyengar added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Jan 12, 2017
Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

I support the change Martin was trying to make, but I think it could use a bit more clarification, as the existing text is a bit confusing, because it doesn't explain why the version must be specified, even though version negotiation has technically completed.

As such, I'd support a merge of the functional change Martin made with some combined text?

@@ -411,8 +411,7 @@ have the VERSION bit set. This bit is always set on packets that are sent prior
to connection establishment. When receiving a packet that is not associated
with an existing connection, packets without a VERSION bit MUST be discarded.

While there might be similarities between different versions of this protocol,
implementations have to assume that a version that it does not support uses a
Implementations have to assume that a version that it does not support uses a
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: How about "Implementations must assume an unsupported version may use a different packet format."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, with a minor edit.

protected packets can include a version. A client MUST NOT change the version
it uses unless it is in response to a version negotiation packet from the
server.
The client MUST set the VERSION flag on all packets until version negotiation
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section just a rollback?

@janaiyengar
Copy link
Contributor Author

My goal is to separate this functional change into a separate smaller PR, separate from the additions to the draft that have happened in the other PR. Technically, if we aren't requiring TLS in the QUIC document, talking about 0-RTT and 1-RTT keys pre-supposes a specific mechanism, which, if universal, ought to be documented in the "Crypto Handshake Requirements", or should be removed. Either ways, this should happen separately, so I'm changing the text back to what it described, modulo Martin's nice editorializing.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

Thanks Jana, one minor tweak, but otherwise it LGTM.

The client MUST set the VERSION flag on all packets until version negotiation
concludes. Version negotiation successfully concludes when the client receives a
packet from the server with the VERSION flag unset. All subsequent packets sent
by the client MUST have the VERSION flag unset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I've discussed this more, how about just changing this to SHOULD, since the MUST is not enforcable anyway.

Copy link
Member

Choose a reason for hiding this comment

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

It is absolutely enforceable. Unless your point is that the presence of absence of the version field isn't authenticated, in which case I guess I'd have to concede that you don't want an attacker ADDING a version to your packet so that you would kill the connection.

If we are going with a SHOULD here, we need to explain why (and the consequences if the SHOULD is not respected, I'll open a small PR).

@janaiyengar janaiyengar merged commit c989c65 into master Jan 12, 2017
@martinthomson martinthomson deleted the undoversion branch January 13, 2017 00:33
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants