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

Updated examples in Section 5.6 #188

Merged
merged 4 commits into from Dec 28, 2020
Merged

Updated examples in Section 5.6 #188

merged 4 commits into from Dec 28, 2020

Conversation

hannestschofenig
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@ekr ekr left a comment

Choose a reason for hiding this comment

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

@kaduk is this what you had in mind.

Remarks:

- The HelloRetryRequest shown in (1) is an optional message sent by the server.
- The ACK message is sent by a client in response to a NewSessionTicket message from the server (as noted in (3)) or sent by a server in response to receipt of flight (2). In those cases it represent a flight on its own.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to think we should not think of an ACK as part of a flight. It's kind of outside this model.

| (2) | x | | Certificate*, CertificateVerify*, Finished |
| | | x | ACK |
| (3) | | x | NewSessionTicket |
{: #tab-flights title="Flight Handshake Message Combinations."}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't quite see what the Note column is doing.

@ekr ekr merged commit 391b64d into master Dec 28, 2020
Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

Thanks, this addresses the issues I had raised for this section.
(It's perhaps a little janky to have just a box with "Flight" for each one, but I don't have any better ideas, so we should probably roll with it!)

| (1) | | x | HelloRetryRequest |
| | | x | ServerHello, EncryptedExtensions, CertificateRequest*, Certificate*, CertificateVerify*, Finished |
| (2) | x | | Certificate*, CertificateVerify*, Finished |
| | | x | ACK |
Copy link
Contributor

Choose a reason for hiding this comment

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

(A client will send ACK in response to post-handshake messages, just not during the initial handshake. I think this is okay as the table contents, but maybe we should ensure that the prose is clear this is just for the initial handshake.) Though, given ekr's comment below, perhaps it's going away anyway and becomes irrelevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually left ACK in because it was more confusing not to.

Note: The application data sent by the client is not included in the
timeout and retransmission calculation.

{: #dtls-post-handshake-ticket title="Message flights for the NewSessionTicket message"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to mention KeyUpdate/NewConnectionID/RequestConnectionID here? It seems overkill to have a separate figure showing the message and ACK for each of them...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I guess that would work.

@ekr
Copy link
Collaborator

ekr commented Dec 29, 2020

@kaduk I merged this. Any chance you want to send a PR or do you want me to?

@kaduk
Copy link
Contributor

kaduk commented Dec 29, 2020

@kaduk I merged this. Any chance you want to send a PR or do you want me to?

I'm not in a great spot to make a PR at the moment; please go ahead.

@ekr
Copy link
Collaborator

ekr commented Dec 29, 2020

I just fixed this in the main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants