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

Final salt and retry keys #4431

Merged
merged 4 commits into from
Dec 13, 2020
Merged

Final salt and retry keys #4431

merged 4 commits into from
Dec 13, 2020

Conversation

martinthomson
Copy link
Member

Also, examples use final version numbers.

IMPORTANT: the TLS extension codepoint needs to be updated, both in the
text and in the examples (which will result in changes to authentication
tags and the affected ciphertext).

Also, examples use final version numbers.

IMPORTANT: the TLS extension codepoint needs to be updated, both in the
text and in the examples (which will result in changes to authentication
tags and the affected ciphertext).
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -tls labels Dec 8, 2020
ghedo added a commit to cloudflare/quiche that referenced this pull request Dec 9, 2020
@ghedo
Copy link
Member

ghedo commented Dec 9, 2020

I plugged the test vectors into the quiche test suite and it checks out https://github.com/cloudflare/quiche/tree/its-the-final-saltdown

@@ -947,7 +947,7 @@ from the Destination Connection ID field from the client's first Initial
packet.

This secret is determined by using HKDF-Extract (see Section 2.2 of
{{!HKDF=RFC5869}}) with a salt of 0xafbfec289993d24c9e9786f19c6111e04390a899
{{!HKDF=RFC5869}}) with a salt of 0x38762cf7f55934b34d179ae6a4c80cadccbb7f0a
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, how were these new salts derived? Are we going to learn after publication that this was a hash of @martinthomson 's social security number?

Copy link
Member

Choose a reason for hiding this comment

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

My guess is that he rolled some D&D dice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Revealing how it was done would ruin the mystery.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I was sort of hoping for us to use a provable process here à la NomCom selection (pick a future date and hash some unpredictable values like weather in San Francisco and the AAPL stock price on that day) to ensure that @martinthomson doesn't accept money from AnOnyMoUs so they don't hack all our initial packets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hacking initial packets doesn't need a lot of mystery-solving -- the salt is published here :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@janaiyengar yes that was a joke :-) but more seriously I was truly expecting us to come up with a convoluted way to establish this salt for the final version to ensure it is impartial. Not that I'll lie down on the tracks over this, as a wise person one said...

@@ -2105,67 +2105,67 @@ The unprotected header includes the connection ID and a 4-byte packet number
encoding for a packet number of 2:

~~~
c3ff000020088394c8f03e5157080000449e00000002
c300000001088394c8f03e5157080000449e00000002
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 implying that implementors of draft-33 should use the 00000001 version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see #4430. Though I would (strongly) encourage people not to deploy that version until we get a little further through the process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify what "a little further through the process" means? Having folks deploy 00000001 before the RFC is published in its immutable glory could prevent us from making last-minute fixes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I'm just a little scared of what the IESG might uncover. Certain IESG members have a reputation for being quite thorough. They probably won't find anything substantive, but I would like to keep the option just a tiny bit available until their reviews are in.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I'm not personally worried about IESG review; I'm more concerned about what we'll find when we actually deploy this protocol in production (no one has launched 0-RTT or migration yet as far as I know).

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that everyone is retaining h3-29, I would suggest that we continue with that until at least after IESG review. There's nothing left in the process at this point that is expected to wait until we have more deployment experience, given that IETF LC is officially complete.

I understand and agree with your concern, @DavidSchinazi, but given that we don't have a specific point in time where we can expect to see migration or 0-RTT deployed, I don't see that waiting is a useful strategy. I would argue that we can leverage v2 for this if we need to fix something.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want final values in the document, because they will need to be tested. I've already found one problem as a result of implementing this change. I don't want any surprises. That said, yes, keep up with the -29 deployments for now. It's just as good.

And you just reminded me that I need to tweak the ALPN values in the sample. That's going to suck, but best that I caught it.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I just worked out that the example uses an ALPN of "alpn", so there is no need to update. Of course, someone could call me on that being a potentially valid value rather than one reserved for examples, but then I wouldn't care.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in agreement with you that we need to test a few things before publication, it's just the matter of when we switch to the official codepoints. But I think you did hit the nail on the head that at the end of the day, it's going to be h3-29 for now, then v1 when available - nothing we do between the two will actually matter

Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

New values seem to work in my testing.

@martinthomson martinthomson marked this pull request as ready for review December 11, 2020 09:00
@martinthomson martinthomson merged commit ace1ec1 into master Dec 13, 2020
@martinthomson martinthomson deleted the final-salts branch December 13, 2020 22:17
@ghedo ghedo mentioned this pull request Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header protection mask for server Initial packet example from -tls A.3 seems incorrect
6 participants