quiche: make quic proof source and crypto stream pluggable #16658

danzh2010 · 2021-05-25T15:10:25Z

Commit Message: make quic proof source and crypto streams extensions. Add config for default ones. If not specified in config, the default ones will be used.

Risk Level: low
Testing: existing tests passed
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only · 2021-05-25T15:10:43Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16658 was opened by danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-05-25T15:54:40Z

/assign @alyssawilk

Signed-off-by: Dan Zhang <danzh@google.com>

api/envoy/config/listener/v3/quic_config.proto

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist · 2021-05-27T16:35:45Z

api/envoy/config/listener/v3/quic_config.proto

@@ -48,4 +63,12 @@ message QuicProtocolOptions {
  // bound by 6000, regardless of this field or how many connections there are.
  google.protobuf.UInt32Value packets_to_read_to_connection_count_ratio = 5
      [(validate.rules).uint32 = {gte: 1}];
+
+  // The crypto server stream implementation used for this listener.
+  // If not specified the :ref:`QUICHE defaul one<envoy_v3_api_field_extensions.quic.v3.CryptoServerStreamConfig>` will be used.


nit: should this say "default" instead of "defaul"?

api/envoy/config/listener/v3/quic_config.proto

RyanTheOptimist · 2021-05-27T16:37:24Z

api/envoy/config/listener/v3/quic_config.proto

@@ -48,4 +63,12 @@ message QuicProtocolOptions {
  // bound by 6000, regardless of this field or how many connections there are.
  google.protobuf.UInt32Value packets_to_read_to_connection_count_ratio = 5
      [(validate.rules).uint32 = {gte: 1}];
+
+  // The crypto server stream implementation used for this listener.


Is this the C++ name of a class, or something else?

Yes, added quic class names instead.

Signed-off-by: Dan Zhang <danzh@google.com>

adisuissa

API looks good, but one question to verify that this is intentional:
Why do you set the extension name to be different than the extension path in both extensions (e.g., you use envoy.quic.server.crypto_stream.quiche instead of envoy.quic.crypto_stream)?

danzh2010 · 2021-06-09T14:37:55Z

API looks good, but one question to verify that this is intentional:
Why do you set the extension name to be different than the extension path in both extensions (e.g., you use envoy.quic.server.crypto_stream.quiche instead of envoy.quic.crypto_stream)?

Yes, that's intentional. "server" and "quiche" is the unique to this extensions. We might add client or non-quiche-default extensions in the future.

danzh2010 · 2021-06-09T14:55:54Z

/retest

repokitteh-read-only · 2021-06-09T14:55:58Z

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

🐱

Caused by: a #16658 (comment) was created by @danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-06-09T18:06:19Z

API looks good, but one question to verify that this is intentional:
Why do you set the extension name to be different than the extension path in both extensions (e.g., you use envoy.quic.server.crypto_stream.quiche instead of envoy.quic.crypto_stream)?

Yes, that's intentional. "server" and "quiche" is the unique to this extensions. We might add client or non-quiche-default extensions in the future.

And I will fix their ordering as a follow up.

adisuissa

/lgtm api

danzh2010 · 2021-06-09T20:30:17Z

Can this be merged?

phlax · 2021-06-09T20:35:53Z

api/envoy/config/listener/v3/quic_config.proto

 import "envoy/config/core/v3/protocol.proto";

+import "google/protobuf/any.proto";


this is unused - could you remove it

phlax

@danzh2010 ive added a couple of docs nits

phlax · 2021-06-09T20:36:51Z

api/envoy/config/listener/v3/quic_config.proto

@@ -48,4 +50,14 @@ message QuicProtocolOptions {
  // bound by 6000, regardless of this field or how many connections there are.
  google.protobuf.UInt32Value packets_to_read_to_connection_count_ratio = 5
      [(validate.rules).uint32 = {gte: 1}];
+
+  // Configure which implementation of quic::QuicCryptoClientStreamBase to be used for this listener.


could you surround quic::QuicCryptoClientStreamBase with `` as its a literal

phlax · 2021-06-09T20:37:20Z

api/envoy/config/listener/v3/quic_config.proto

+  // [#extension-category: envoy.quic.server.crypto_stream]
+  core.v3.TypedExtensionConfig crypto_stream_config = 6;
+
+  // Configure which implementation of quic::ProofSource to be used for this listener.


please make quic::ProofSource a literal also

htuch

/lgtm api

Signed-off-by: Dan Zhang <danzh@google.com>

@danzh2010

nits addressed - thanks @danzh2010

adisuissa · 2021-06-10T07:52:29Z

/lgtm api

alyssawilk

LGTM! Looks like CI flaked though - I'll kick off another run but feel free to ping me if you notice it's green before I do :-)

moved

…y#16658) Commit Message: make quic proof source and crypto streams extensions. Add config for default ones. If not specified in config, the default ones will be used. Risk Level: low Testing: existing tests passed Part of envoyproxy#2557 Co-authored-by: Dan Zhang <danzh@google.com>

danzh1989 added 9 commits May 14, 2021 17:05

build no handshake stuff

4570f2d

Signed-off-by: Dan Zhang <danzh@google.com>

build dispatcher

b5821a0

Signed-off-by: Dan Zhang <danzh@google.com>

build listener

e772105

Signed-off-by: Dan Zhang <danzh@google.com>

making tests pass

5a83a9c

Signed-off-by: Dan Zhang <danzh@google.com>

revert debug logging

587facd

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quiche_fuzz

108700c

Signed-off-by: Dan Zhang <danzh@google.com>

add client crypto stream factory

75c569f

Signed-off-by: Dan Zhang <danzh@google.com>

add proof source factory

547e36b

Signed-off-by: Dan Zhang <danzh@google.com>

rename some classes

cef927c

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added the api label May 25, 2021

repokitteh-read-only bot assigned htuch May 25, 2021

Merge branch 'master' into quiche_fuzz

d2c7eb5

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned alyssawilk May 25, 2021

alyssawilk added the waiting label May 25, 2021

danzh1989 added 2 commits May 25, 2021 15:22

Merge branch 'master' into quiche_fuzz

653a451

Signed-off-by: Dan Zhang <danzh@google.com>

use absl::optional

150befd

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label May 25, 2021

htuch reviewed May 26, 2021

View reviewed changes

api/envoy/config/listener/v3/quic_config.proto Outdated Show resolved Hide resolved

alyssawilk added the waiting label May 26, 2021

modify docs

083daee

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label May 27, 2021

danzh1989 added 2 commits May 27, 2021 11:48

Merge branch 'master' into quiche_fuzz

55c023c

Signed-off-by: Dan Zhang <danzh@google.com>

fix extension status

8b38134

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk assigned RyanTheOptimist May 27, 2021

danzh1989 added 2 commits May 27, 2021 12:30

fix toctree reference

22137cc

Signed-off-by: Dan Zhang <danzh@google.com>

fix docs cross reference

563ca96

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist reviewed May 27, 2021

View reviewed changes

fix CI

467a808

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed alyssawilk’s stale review via 257fd65 June 8, 2021 16:12

fix doc reference

d2164a0

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 changed the title ~~quiche: extend quic proof source and crypto streams~~ quiche: make quic proof source and crypto stream pluggable Jun 8, 2021

fix crypto_stream doc reference

d6f6647

Signed-off-by: Dan Zhang <danzh@google.com>

adisuissa reviewed Jun 9, 2021

View reviewed changes

alyssawilk added the waiting label Jun 9, 2021

Merge branch 'master' into quiche_fuzz

0471c5c

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Jun 9, 2021

adisuissa reviewed Jun 9, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jun 9, 2021

phlax reviewed Jun 9, 2021

View reviewed changes

phlax previously requested changes Jun 9, 2021

View reviewed changes

htuch reviewed Jun 9, 2021

View reviewed changes

address comments

58441b8

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added the api label Jun 9, 2021

danzh1989 added 2 commits June 9, 2021 19:34

Merge branch 'master' into quiche_fuzz

9c95845

Signed-off-by: Dan Zhang <danzh@google.com>

format

7465ec6

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the api label Jun 10, 2021

alyssawilk approved these changes Jun 10, 2021

View reviewed changes

alyssawilk merged commit beb5a93 into envoyproxy:main Jun 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quiche: make quic proof source and crypto stream pluggable #16658

quiche: make quic proof source and crypto stream pluggable #16658

danzh2010 commented May 25, 2021

repokitteh-read-only bot commented May 25, 2021

danzh2010 commented May 25, 2021

RyanTheOptimist May 27, 2021

danzh2010 May 27, 2021

RyanTheOptimist May 27, 2021

danzh2010 May 27, 2021

adisuissa left a comment

danzh2010 commented Jun 9, 2021

danzh2010 commented Jun 9, 2021

repokitteh-read-only bot commented Jun 9, 2021

danzh2010 commented Jun 9, 2021

adisuissa left a comment

danzh2010 commented Jun 9, 2021

phlax Jun 9, 2021

danzh2010 Jun 9, 2021

phlax left a comment

phlax Jun 9, 2021 •

edited

Loading

danzh2010 Jun 9, 2021

phlax Jun 9, 2021

danzh2010 Jun 9, 2021

htuch left a comment

adisuissa commented Jun 10, 2021

alyssawilk left a comment

		import "envoy/config/core/v3/protocol.proto";

		import "google/protobuf/any.proto";

quiche: make quic proof source and crypto stream pluggable #16658

quiche: make quic proof source and crypto stream pluggable #16658

Conversation

danzh2010 commented May 25, 2021

repokitteh-read-only bot commented May 25, 2021

danzh2010 commented May 25, 2021

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

adisuissa left a comment

Choose a reason for hiding this comment

danzh2010 commented Jun 9, 2021

danzh2010 commented Jun 9, 2021

repokitteh-read-only bot commented Jun 9, 2021

danzh2010 commented Jun 9, 2021

adisuissa left a comment

Choose a reason for hiding this comment

danzh2010 commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlax left a comment

Choose a reason for hiding this comment

phlax Jun 9, 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

htuch left a comment

Choose a reason for hiding this comment

adisuissa commented Jun 10, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

phlax Jun 9, 2021 •

edited

Loading