Add support for client-side QUIC 0-RTT #16260

DavidSchinazi · 2021-04-30T23:10:22Z

This PR introduces EnvoyQuicSessionCache, which is responsible
for saving QUIC+TLS session tickets which are required for 0-RTT.

I've confirmed that the new integration test from this PR would
reproduce the 0-RTT crash that we saw previouslybut had since
been fixed.

Risk Level: Low
Testing: Local
Docs Changes: None
Release Notes: None
Platform Specific Features: None

Closes #16234

This PR introduces EnvoyQuicSessionCache, which is responsible for saving QUIC+TLS session tickets which are required for 0-RTT. I've confirmed that the new integration test from this PR would reproduce the 0-RTT crash that we saw previouslybut had since been fixed. Signed-off-by: David Schinazi <[email protected]>

DavidSchinazi · 2021-04-30T23:11:21Z

/assign @danzh2010

Signed-off-by: David Schinazi <[email protected]>

source/common/quic/envoy_quic_session_cache.h

DavidSchinazi · 2021-05-03T17:17:21Z

/retest

repokitteh-read-only · 2021-05-03T17:17:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16260 (comment) was created by @DavidSchinazi.

see: more, trace.

danzh2010

Nice, this should also address #16146.

danzh2010 · 2021-05-02T20:04:31Z

source/common/quic/envoy_quic_session_cache.cc

+constexpr size_t kMaxSessionCacheEntries = 1024;
+
+// Returns false if the SSL |session| doesn't exist or it is expired at |system_time|.
+bool isSessionValid(SSL_SESSION* session, SystemTime system_time) {


nits: if you make this a private member function, it can directly access system_time_ and no need to pass in system_time.

There is no system_time_ member. I think you might be confusing it with time_source_ which is different - we want to avoid computing the time from the time source too often.

Right, I meant to access time_source_ directly. time_source_.systemTime() is always called before isSessionValid(). For readability, I think either renaming system_time to now here or making this function a class member.

Makes sense. I've renamed system_time to now.

danzh2010 · 2021-05-03T17:17:14Z

source/common/quic/envoy_quic_session_cache.h

+
+    SSL_SESSION* peekSession();
+
+    bssl::UniquePtr<SSL_SESSION> sessions[2];


Why size of 2?

Because that's sufficient in practice, it's the value Chrome uses.

Mind commenting about the reason? Otherwise a queue-like data structure might be more preferred.

Added a comment.

mind adding a bit more detail for those of with less QUIC-savvy? is it that session information changes infrequently? If so why do we need two?

danzh2010 · 2021-05-03T17:20:39Z

source/common/quic/envoy_quic_session_cache.cc

+void EnvoyQuicSessionCache::createAndInsertEntry(const quic::QuicServerId& server_id,
+                                                 bssl::UniquePtr<SSL_SESSION> session,
+                                                 const quic::TransportParameters& params,
+                                                 const quic::ApplicationState* application_state) {


It's a bit confusing about the life time of application_state. Can you comment that it's now owned?

The input parameter isn't owned. The fact that it's a const pointer makes the ownership model clear to me. Am I missing something?

sorry, typo "not" for "now". If we create ApplicationState instance for each entry here, why does doApplicationStatesMatch() compare pointers?
And Lookup() also create ApplicationState for return value. Probably QUICHE somehow requires duplication this object.

sorry, typo "not" for "now".

I don't think we need to comment that a const pointer is not owned, as that's very common usage.

If we create ApplicationState instance for each entry here, why does doApplicationStatesMatch() compare pointers?

doApplicationStatesMatch() dereferences the pointers and compares the values. It looks at pointers because they can be nullptr.

And Lookup() also create ApplicationState for return value. Probably QUICHE somehow requires duplication this object.

I'm not sure what you're asking? QUICHE passes us a const pointer and we need to make a copy that we own.

danzh2010 · 2021-05-03T17:24:53Z

source/common/quic/envoy_quic_session_cache.cc

+  ASSERT(cache_.size() < kMaxSessionCacheEntries);
+  Entry entry;
+  entry.pushSession(std::move(session));
+  entry.params = std::make_unique<quic::TransportParameters>(params);
+  if (application_state != nullptr) {
+    entry.application_state = std::make_unique<quic::ApplicationState>(*application_state);
+  }


nits: consider wrapping these into Entry constructor.

Given that this is a struct which needs a default constructor, I think this is overall simpler?

source/common/quic/envoy_quic_session_cache.cc

danzh2010 · 2021-05-03T17:39:36Z

source/common/quic/envoy_quic_session_cache.cc

+namespace Quic {
+namespace {
+
+constexpr size_t kMaxSessionCacheEntries = 1024;


Envoy doesn't use kCamelCaseConstantName convention for const. MAX_SESSION_CACHE_SIZE instead?
Do this needs to be configuration? @alyssawilk

I went with MaxSessionCacheEntries which is preferred by the Envoy style guide because it doesn't conflict with macros

we'll want config eventually but that can be a TODO (either for David, against QUIC upstream, I vote for the former =P) with a comment that the hard-coded value is arbitrary

Also @RyanTheOptimist look, another bounded cache! now that I look they're everywhere =P

I can file an issue to make this configurable once this lands. Though I don't think it's a priority

I'm thinking more that having configurable 0-rtt config and integration tests is worthwhile for envoy mobile :-)

What's a configurable 0-RTT config?
This PR adds an integration test.

Signed-off-by: David Schinazi <[email protected]>

danzh2010 · 2021-05-03T21:49:20Z

source/common/quic/envoy_quic_session_cache.cc

+void EnvoyQuicSessionCache::createAndInsertEntry(const quic::QuicServerId& server_id,
+                                                 bssl::UniquePtr<SSL_SESSION> session,
+                                                 const quic::TransportParameters& params,
+                                                 const quic::ApplicationState* application_state) {


sorry, typo "not" for "now". If we create ApplicationState instance for each entry here, why does doApplicationStatesMatch() compare pointers?
And Lookup() also create ApplicationState for return value. Probably QUICHE somehow requires duplication this object.

danzh2010 · 2021-05-03T21:51:02Z

source/common/quic/envoy_quic_session_cache.cc

+constexpr size_t kMaxSessionCacheEntries = 1024;
+
+// Returns false if the SSL |session| doesn't exist or it is expired at |system_time|.
+bool isSessionValid(SSL_SESSION* session, SystemTime system_time) {


Right, I meant to access time_source_ directly. time_source_.systemTime() is always called before isSessionValid(). For readability, I think either renaming system_time to now here or making this function a class member.

danzh2010 · 2021-05-03T21:54:25Z

source/common/quic/envoy_quic_session_cache.h

+
+    SSL_SESSION* peekSession();
+
+    bssl::UniquePtr<SSL_SESSION> sessions[2];


Mind commenting about the reason? Otherwise a queue-like data structure might be more preferred.

source/common/quic/envoy_quic_session_cache.cc

Signed-off-by: David Schinazi <[email protected]>

alyssawilk

this looks fantastic - it'll be great to have 0-rtt working and even better to regression-proof that crash :-)

source/common/quic/envoy_quic_session_cache.cc

alyssawilk · 2021-05-04T14:31:04Z

source/common/quic/envoy_quic_session_cache.cc

+
+constexpr size_t MaxSessionCacheEntries = 1024;
+
+// Returns false if the SSL |session| doesn't exist or it is expired at |now|.


Envoy doesn't usually do ||

alyssawilk · 2021-05-04T14:42:14Z

source/common/quic/envoy_quic_session_cache.cc

+
+EnvoyQuicSessionCache::~EnvoyQuicSessionCache() = default;
+
+void EnvoyQuicSessionCache::Insert(const quic::QuicServerId& server_id,


The duplication between this and QuicClientSessionCache is a bit of a bummer.

Optionally, as a follow-up, maybe we can implement more of the base logic in the quiche class and either template away time or have pure virtuals for time bits. but that'd be a follow-up no matter what and should wait until we've sorted out any upstream scalability problems.

What's QuicClientSessionCache?

https://chromium.googlesource.com/chromium/src/+/master/net/quic/quic_client_session_cache.cc :-)

Ah right, yes we're going to have duplication between Chrome and Envoy :)

Yep, but modulo "can we refactor away the time abstraction" seems reasonable to stick this one in quiche at some point.

alyssawilk · 2021-05-04T14:43:03Z

source/common/quic/envoy_quic_session_cache.cc

+}
+
+std::unique_ptr<quic::QuicResumptionState>
+EnvoyQuicSessionCache::Lookup(const quic::QuicServerId& server_id, const SSL_CTX* /*ctx*/) {


drive by question - why is SSL_CTX unused here and in quiche? Should we clean up upstream?

Some clients may want to use different CCL_CTX'es. We could remove it but it isn't causing any issues. Either way that would be a QUICHE CL.

yeah, sorry, quiche == upstream in this context. Not something you'd do in situ but if it's not used anywhere might be worth adding our quiche clean up list.

alyssawilk · 2021-05-04T14:46:09Z

source/common/quic/envoy_quic_session_cache.cc

+                                                 bssl::UniquePtr<SSL_SESSION> session,
+                                                 const quic::TransportParameters& params,
+                                                 const quic::ApplicationState* application_state) {
+  prune();


we walk the entire list on insertion? Maybe make that clear in prune comments()
Also definitely worth a comment up by that 1024 that it should stay small and why. eek!

O(1024) is negligible compared to the cryptographic operations required to create a new connection.

Added comments though

alyssawilk · 2021-05-04T14:59:08Z

source/common/quic/envoy_quic_session_cache.h

+
+    SSL_SESSION* peekSession();
+
+    bssl::UniquePtr<SSL_SESSION> sessions[2];


mind adding a bit more detail for those of with less QUIC-savvy? is it that session information changes infrequently? If so why do we need two?

alyssawilk · 2021-05-04T14:59:40Z

source/common/quic/envoy_quic_session_cache.h

+    // already stored.
+    void pushSession(bssl::UniquePtr<SSL_SESSION> session);
+
+    // Retrieves the latest session from the entry, meanwhile removing it.


Retrieves and removes the latest session...?

alyssawilk · 2021-05-04T15:02:03Z

source/common/quic/envoy_quic_session_cache.h

+    std::unique_ptr<quic::ApplicationState> application_state;
+  };
+
+  // Remove all entries that are no longer valid. If after that we are still at the size limit, also


Optionally
If after that we are still at the size limit, also remove ->
If all entries were valid but the cache is at its size limit, instead removes

alyssawilk · 2021-05-04T15:05:15Z

test/common/quic/envoy_quic_session_cache_test.cc

+public:
+  // From TimeSource.
+  SystemTime systemTime() override { return fake_time_; }
+  MonotonicTime monotonicTime() override { return MonotonicTime(); }


mind commenting why you can't just use simtime's monotonicTime too?

test/common/quic/envoy_quic_session_cache_test.cc

Signed-off-by: David Schinazi <[email protected]>

alyssawilk

LG modulo one more | nit!

alyssawilk · 2021-05-04T17:39:28Z

source/common/quic/envoy_quic_session_cache.cc

+
+EnvoyQuicSessionCache::~EnvoyQuicSessionCache() = default;
+
+void EnvoyQuicSessionCache::Insert(const quic::QuicServerId& server_id,


https://chromium.googlesource.com/chromium/src/+/master/net/quic/quic_client_session_cache.cc :-)

alyssawilk · 2021-05-04T17:40:04Z

source/common/quic/envoy_quic_session_cache.cc

+}
+
+std::unique_ptr<quic::QuicResumptionState>
+EnvoyQuicSessionCache::Lookup(const quic::QuicServerId& server_id, const SSL_CTX* /*ctx*/) {


yeah, sorry, quiche == upstream in this context. Not something you'd do in situ but if it's not used anywhere might be worth adding our quiche clean up list.

alyssawilk · 2021-05-04T17:41:26Z

source/common/quic/envoy_quic_session_cache.h

+    Entry(Entry&&) noexcept;
+    ~Entry();
+
+    // Adds a new |session| onto sessions, dropping the oldest one if two are


ditto | here and below

Signed-off-by: David Schinazi <[email protected]>

danzh2010 · 2021-05-04T15:21:14Z

source/common/quic/envoy_quic_session_cache.cc

+  if (state == other) {
+    return true;
+  }


I was wondering why we compare the pointer of two different objects. But it seems to be only possible when both state and other are nullptr? I slightly prefer to make it explicit instead.

Yes, this is needed to avoid a dereference when they're both nullptr. I personally prefer this style of comparison because it is correct for all inputs. The fact that callers won't ever send the same non-null pointer as both parameters is an implementation details that doesn't need to be visible in this function.

danzh2010 · 2021-05-04T15:27:47Z

source/common/quic/envoy_quic_session_cache.cc

+  return session_time <= now_u64 + 1 && now_u64 < session_expiration;
+}
+
+bool doApplicationStatesMatch(const quic::ApplicationState* state, quic::ApplicationState* other) {


nits: making other const as well?

Signed-off-by: David Schinazi <[email protected]>

alyssawilk

LGTM, modulo Dan's sign-off

danzh2010 · 2021-05-04T22:26:09Z

test/common/quic/envoy_quic_session_cache_test.cc

+constexpr uint32_t kTimeout = 1000;
+const quic::QuicVersionLabel kFakeVersionLabel = 0x01234567;
+const quic::QuicVersionLabel kFakeVersionLabel2 = 0x89ABCDEF;
+const uint64_t kFakeIdleTimeoutMilliseconds = 12012;
+const uint8_t kFakeStatelessResetTokenData[16] = {0x90, 0x91, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97,
+                                                  0x98, 0x99, 0x9A, 0x9B, 0x9C, 0x9D, 0x9E, 0x9F};
+const uint64_t kFakeMaxPacketSize = 9001;
+const uint64_t kFakeInitialMaxData = 101;
+const bool kFakeDisableMigration = true;
+const auto kCustomParameter1 = static_cast<quic::TransportParameters::TransportParameterId>(0xffcd);
+const char* kCustomParameter1Value = "foo";
+const auto kCustomParameter2 = static_cast<quic::TransportParameters::TransportParameterId>(0xff34);
+const char* kCustomParameter2Value = "bar";


nits: renaming these const according to Envoy naming convention. (some of them are only used once, plz consider inlining them)

Renamed the constants to the Envoy naming convention. I kept the ones used only once because they make the code more readable.

Signed-off-by: David Schinazi <[email protected]>

danzh2010

LGTM, thanks for adding this!

This PR introduces EnvoyQuicSessionCache, which is responsible for saving QUIC+TLS session tickets which are required for 0-RTT. Risk Level: Low Testing: unit tests, integration test Docs Changes: None Release Notes: None Platform Specific Features: None Closes envoyproxy#16234 Signed-off-by: David Schinazi <[email protected]> Signed-off-by: Gokul Nair <[email protected]>

repokitteh-read-only bot assigned danzh2010 Apr 30, 2021

DavidSchinazi added 2 commits April 30, 2021 17:22

fix tidy

12e4a08

Signed-off-by: David Schinazi <[email protected]>

more tidy

b295998

Signed-off-by: David Schinazi <[email protected]>

rojkov reviewed May 3, 2021

View reviewed changes

source/common/quic/envoy_quic_session_cache.h Show resolved Hide resolved

danzh2010 reviewed May 3, 2021

View reviewed changes

alyssawilk self-assigned this May 3, 2021

review comment

04e5ce1

Signed-off-by: David Schinazi <[email protected]>

danzh2010 reviewed May 3, 2021

View reviewed changes

review comments

22c4e3d

Signed-off-by: David Schinazi <[email protected]>

alyssawilk reviewed May 4, 2021

View reviewed changes

review comments

fe07df8

Signed-off-by: David Schinazi <[email protected]>

alyssawilk reviewed May 4, 2021

View reviewed changes

tweak comment

65c0883

Signed-off-by: David Schinazi <[email protected]>

danzh2010 reviewed May 4, 2021

View reviewed changes

const

8a59130

Signed-off-by: David Schinazi <[email protected]>

alyssawilk previously approved these changes May 4, 2021

View reviewed changes

danzh2010 reviewed May 4, 2021

View reviewed changes

constant casing change

1ed0f28

Signed-off-by: David Schinazi <[email protected]>

DavidSchinazi dismissed alyssawilk’s stale review via 1ed0f28 May 4, 2021 22:44

danzh2010 approved these changes May 5, 2021

View reviewed changes

alyssawilk approved these changes May 5, 2021

View reviewed changes

alyssawilk merged commit 691487d into envoyproxy:main May 5, 2021


		SSL_SESSION* peekSession();

		bssl::UniquePtr<SSL_SESSION> sessions[2];


		constexpr size_t MaxSessionCacheEntries = 1024;

		// Returns false if the SSL \|session\| doesn't exist or it is expired at \|now\|.


		EnvoyQuicSessionCache::~EnvoyQuicSessionCache() = default;

		void EnvoyQuicSessionCache::Insert(const quic::QuicServerId& server_id,

Add support for client-side QUIC 0-RTT #16260

Add support for client-side QUIC 0-RTT #16260

Conversation

DavidSchinazi commented Apr 30, 2021 • edited Loading

DavidSchinazi commented Apr 30, 2021

DavidSchinazi commented May 3, 2021

repokitteh-read-only bot commented May 3, 2021

danzh2010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidSchinazi May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 left a comment

Choose a reason for hiding this comment

DavidSchinazi commented Apr 30, 2021 •

edited

Loading

DavidSchinazi May 3, 2021 •

edited

Loading