[#783] Encrypt retry token #833

kansi · 2020-07-29T13:20:48Z

DISCLAIMER: I am a first time contributor (Rust and QUIC).

I would like some feedback on the direction of this 🙂

Resolves #783 , following changes have been introduced,

master_key has been added to ServerConfig for aiding AEAD key derivation
A new trait AeadKey has been introduced which provided following API,
- from_hkdf : derive a AEAD key using master_key and some random_bytes
- seal: seal a message with the given AEAD key
- open: open a message with a given AEAD key

~~TODO~~

[test] scenario when a garbage retry token is received and the opening operation fails

djc

Hey, thanks for taking a look at this! That's some pretty involved code for your first Rust project. 👍

Can you give more context on what the design is here? Which of the proposals as made by @Ralith in the original issue did you follow, and why?

quinn-proto/src/config.rs

kansi · 2020-07-29T13:54:20Z

Hey, thanks for taking a look at this! That's some pretty involved code for your first Rust project. 👍

Thanks 🙂 but I hope it is going in the right direction!

Can you give more context on what the design is here? Which of the proposals as made by @Ralith in the original issue did you follow, and why?

Sure, as I understand the issue #783 proposes to use AEAD instead of HMAC for token authentication (plus encryption), namely following proposal has been made,

Generate a unique AEAD key for each retry token by using a HKDF from a fixed master key on a fixed-length sequence of random data.

A master_key is stored in the ServerConfig and this is later use with some randomly generated bytes, based on whenever a new Initial is received, to generate a new AEAD key.

Encrypt the token payload using a nonce of 0. This is safe because we never reuse a key.

ZeroNonceSequence has been implemented and provided when creating the AEAD keys

Send the concatenation of the random data, the encrypted payload, and the AEAD tag as the token. An outside party cannot distinguish any of this from random data.

The encode method in RetryToken seals with token with the provided AEAD key and prefixes the random bytes which were used to generate the AEAD key

Recover the key for decryption by re-running the HKDF on the random data prefix.

extract_random_bytes method in RetryToken is used to extract the random bytes from the retry token and then used to derive the AEAD key. Later the AEAD key is used in from_bytes method to open the token and returns RetryToken

I chose to place random_bytes in RetryToken struct as the issues suggest that each RetryToken will have a different AEAD key.

Ralith

Thanks for the PR! This will be a nice additional layer of security and robustness. What you've got so far is definitely on the right track.

quinn-proto/src/crypto/ring.rs

quinn-proto/src/endpoint.rs

quinn-proto/src/crypto.rs

quinn-proto/src/config.rs

quinn-proto/src/crypto.rs

quinn-proto/src/config.rs

quinn-proto/src/crypto.rs

quinn-proto/src/endpoint.rs

quinn-proto/src/token.rs

djc

Thanks for working on this! I tried to answer your question and wanted to point out some other things I noticed while trying to figure out your code. Hope that helps, let me know if you have further questions.

quinn-proto/src/token.rs

quinn-proto/src/crypto/ring.rs

quinn-proto/src/token.rs

djc · 2020-08-12T20:24:00Z

(Also, since the commits so far don't seem to be logically distinct, we'd prefer if you squash them into a single one.)

kansi · 2020-08-22T13:52:56Z

(Also, since the commits so far don't seem to be logically distinct, we'd prefer if you squash them into a single one.)

Sounds good. Once this PR gets to a point where it can be merged I will squash the commits 👍

kansi · 2020-08-27T20:36:03Z

@djc @Ralith CI build fail with the following error

error: reached the type-length limit while instantiating `<std::vec::IntoIter<Peer> as std...rs:216:58: 220:6 keylog:&bool]]>`
##[error]     |
     = note: consider adding a `#![type_length_limit="1848396"]` attribute to your crate

error: aborting due to previous error
##[error]could not compile `interop`.

Seems like it cannot compile inerop but I cannot replicate this issue locally 🤔 , any pointers here?

djc · 2020-08-27T20:42:51Z

This is the issue tracked in #839. You should probably just increase the type length limit for the interop crate for now.

Ralith · 2020-09-19T01:49:28Z

A work-around for the build issue was merged in #844, so this should be unblocked with a rebase!

DemiMarie · 2020-09-20T20:34:24Z

I suggest using something like XChaCha20-Poly1305 with a random nonce instead of the current ad-hoc construction. XChaCha20-Poly1305 has a 192-bit nonce, so the likelihood of reuse is negligible. The nonce should be randomly generated for the first use, and then incremented for all subsequent encryptions.

Ralith · 2020-09-21T00:13:04Z

If we could easily do that, the usual 96 bit nonce would be plenty. The problem is there's no way to recover the nonce used by such a scheme when validating a client-supplied retry token without defeating the goal of producing a retry token indistinguishable from random data.

I also don't think the risk of a custom construction here is high. In the unlikely event that a third party manages to decrypt a retry token, they don't learn anything useful. The main goal here is just future-proofing: nobody can bake knowledge of the token's structure into a middlebox if we don't expose any structure.

DemiMarie · 2020-09-21T01:10:41Z

@Ralith a 96-bit nonce would NOT be plenty, as the birthday bound for it is too low. However, if you use a nonce that is chosen randomly for every encryption, then the nonce itself is indistinguishable from random bits and can be made part of the retry token itself.

Ralith · 2020-09-21T04:24:21Z

Randomly generating each use has different dynamics than incrementing each use as you suggested in your prior post, yes. I still don't see the need to rewrite this PR and use a cipher unsupported by ring when the current approach works fine and there's little at risk, in any case.

Ralith

Just a few final tweaks!

quinn-proto/src/lib.rs

quinn-proto/src/token.rs

quinn-proto/src/crypto.rs

Ralith

Thanks! Two more cosmetic issues, then please squash.

quinn-proto/src/token.rs

quinn-proto/src/crypto.rs

Ralith

Thanks for your work!

djc

Thanks for the work! One hopefully minor concern and, if you work on that, a tiny nit to go along with it.

djc · 2020-10-05T18:42:01Z

quinn-proto/src/token.rs

@@ -43,46 +50,59 @@ impl RetryToken {
                .unwrap_or(0),
        );

-        let signature_pos = buf.len();
+        let mut additional_data = [0u8; Self::MAX_ADDITIONAL_DATA_SIZE];


Would it be viable to factor out some of the duplication between encode() and from_bytes() in writing out the additional_data?

djc · 2020-10-05T18:42:47Z

quinn-proto/src/endpoint.rs

                }
                .encode(&*server_config.token_key, &remote, &temp_loc_cid);
-                let mut buf = Vec::new();
+


Nit: moving the initialization of buf around here seems orthogonal to the rest of this change. If it's not too much of hassle, maybe revert it?

kansi · 2020-10-10T09:06:49Z

Linting seems to fail, its mostly suggesting to use matches! macro in place of match something { ... }

djc · 2020-10-10T09:19:57Z

Feel free to ignore, I'll fix that up in a separate PR.

kansi · 2020-10-10T09:22:06Z

okay, then I guess the PR can approved and merged.

djc

I took the liberty of rebasing this on top of current main to deal with some new conflicts, and split out my formatting nit into a separate commit. Thanks for working on this!

djc · 2020-10-12T09:29:58Z

Merged despite some new clippy problems which are addressed in #864.

kansi · 2020-10-12T09:47:06Z

@djc @Ralith Thank you for all the support on this, really appreciate it 🙂

Ralith · 2020-10-12T19:13:35Z

Thanks for your contribution! It's great to have this checked off, and it's super cool that you were willing to take it on as a learning project!

djc reviewed Jul 29, 2020

View reviewed changes

quinn-proto/src/config.rs Outdated Show resolved Hide resolved

kansi requested a review from djc July 30, 2020 13:09

Ralith requested changes Aug 1, 2020

View reviewed changes

kansi commented Aug 4, 2020

View reviewed changes

quinn-proto/src/config.rs Outdated Show resolved Hide resolved

kansi commented Aug 7, 2020

View reviewed changes

quinn-proto/src/config.rs Outdated Show resolved Hide resolved

kansi commented Aug 7, 2020

View reviewed changes

quinn-proto/src/config.rs Show resolved Hide resolved

Ralith requested changes Aug 8, 2020

View reviewed changes

kansi commented Aug 12, 2020

View reviewed changes

quinn-proto/src/token.rs Outdated Show resolved Hide resolved

djc reviewed Aug 12, 2020

View reviewed changes

quinn-proto/src/token.rs Outdated Show resolved Hide resolved

quinn-proto/src/crypto/ring.rs Show resolved Hide resolved

quinn-proto/src/crypto/ring.rs Outdated Show resolved Hide resolved

quinn-proto/src/token.rs Outdated Show resolved Hide resolved

kansi force-pushed the 783-encrypt-retry-token branch 2 times, most recently from fe47254 to fb41600 Compare August 27, 2020 20:28

kansi force-pushed the 783-encrypt-retry-token branch from fb41600 to 964c005 Compare September 20, 2020 07:49

Ralith requested changes Sep 27, 2020

View reviewed changes

quinn-proto/src/lib.rs Outdated Show resolved Hide resolved

quinn-proto/src/token.rs Outdated Show resolved Hide resolved

quinn-proto/src/crypto.rs Outdated Show resolved Hide resolved

kansi force-pushed the 783-encrypt-retry-token branch from 964c005 to 4022e8d Compare October 4, 2020 08:49

Ralith requested changes Oct 4, 2020

View reviewed changes

quinn-proto/src/token.rs Outdated Show resolved Hide resolved

quinn-proto/src/crypto.rs Outdated Show resolved Hide resolved

djc changed the base branch from master to main October 4, 2020 19:29

kansi force-pushed the 783-encrypt-retry-token branch from cfc5948 to 01134c5 Compare October 5, 2020 18:27

Ralith previously approved these changes Oct 5, 2020

View reviewed changes

djc reviewed Oct 5, 2020

View reviewed changes

kansi dismissed Ralith’s stale review via 74a6296 October 10, 2020 08:56

djc force-pushed the 783-encrypt-retry-token branch from 74a6296 to d29bea3 Compare October 12, 2020 09:16

kansi and others added 2 commits October 12, 2020 11:19

Encrypt retry token (fixes quinn-rs#783)

99d69f1

Tweak formatting

6c5a219

djc force-pushed the 783-encrypt-retry-token branch from d29bea3 to 6c5a219 Compare October 12, 2020 09:20

djc approved these changes Oct 12, 2020

View reviewed changes

djc merged commit 0630fc9 into quinn-rs:main Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#783] Encrypt retry token #833

[#783] Encrypt retry token #833

kansi commented Jul 29, 2020 •

edited

Loading

djc left a comment

kansi commented Jul 29, 2020 •

edited

Loading

Ralith left a comment •

edited

Loading

djc left a comment

djc commented Aug 12, 2020

kansi commented Aug 22, 2020 •

edited

Loading

kansi commented Aug 27, 2020 •

edited

Loading

djc commented Aug 27, 2020

Ralith commented Sep 19, 2020

DemiMarie commented Sep 20, 2020

Ralith commented Sep 21, 2020

DemiMarie commented Sep 21, 2020

Ralith commented Sep 21, 2020

Ralith left a comment

Ralith left a comment •

edited

Loading

Ralith left a comment

djc left a comment

djc Oct 5, 2020

kansi Oct 10, 2020

djc Oct 5, 2020

kansi commented Oct 10, 2020

djc commented Oct 10, 2020

kansi commented Oct 10, 2020

djc left a comment

djc commented Oct 12, 2020

kansi commented Oct 12, 2020

Ralith commented Oct 12, 2020

[#783] Encrypt retry token #833

[#783] Encrypt retry token #833

Conversation

kansi commented Jul 29, 2020 • edited Loading

djc left a comment

Choose a reason for hiding this comment

kansi commented Jul 29, 2020 • edited Loading

Ralith left a comment • edited Loading

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

djc commented Aug 12, 2020

kansi commented Aug 22, 2020 • edited Loading

kansi commented Aug 27, 2020 • edited Loading

djc commented Aug 27, 2020

Ralith commented Sep 19, 2020

DemiMarie commented Sep 20, 2020

Ralith commented Sep 21, 2020

DemiMarie commented Sep 21, 2020

Ralith commented Sep 21, 2020

Ralith left a comment

Choose a reason for hiding this comment

Ralith left a comment • edited Loading

Choose a reason for hiding this comment

Ralith left a comment

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

djc Oct 5, 2020

Choose a reason for hiding this comment

kansi Oct 10, 2020

Choose a reason for hiding this comment

djc Oct 5, 2020

Choose a reason for hiding this comment

kansi commented Oct 10, 2020

djc commented Oct 10, 2020

kansi commented Oct 10, 2020

djc left a comment

Choose a reason for hiding this comment

djc commented Oct 12, 2020

kansi commented Oct 12, 2020

Ralith commented Oct 12, 2020

kansi commented Jul 29, 2020 •

edited

Loading

kansi commented Jul 29, 2020 •

edited

Loading

Ralith left a comment •

edited

Loading

kansi commented Aug 22, 2020 •

edited

Loading

kansi commented Aug 27, 2020 •

edited

Loading

Ralith left a comment •

edited

Loading