quiche: add fake implemention of QuicProofSource and QuicProofVerifier by danzh2010 · Pull Request #7328 · envoyproxy/envoy

danzh2010 · 2019-06-19T20:52:37Z

This pair will be replaced by actual EnvoyQuicProofSource/Verifier. Before that, we use faked ones to integrate with quiche code.
Update quiche tarball to commit 8f3a576515ada564d3e2b560220649f49a21dec1

Risk Level: low, temporary use, not to be used in production
Testing: added new tests for the faked behavior. Since boringssl QUIC interface is not present in FIPS yet, quic targets are disabled in compile_option_ci using nofips tag.
Part of #2557

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

This reverts commit 9914fb4. Signed-off-by: Dan Zhang <danzh@google.com>

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

danzh2010 · 2019-06-19T20:53:41Z

/assign @wu-bin @alyssawilk

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

danzh2010 · 2019-06-20T14:30:06Z

/retest

repokitteh-read-only · 2019-06-20T14:30:12Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

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

see: more, trace.

wu-bin

Thanks Dan. LGTM modulo some nits.

wu-bin · 2019-06-20T14:27:33Z

source/extensions/quic_listeners/quiche/envoy_quic_fake_proof_source.h

+namespace Quic {
+
+// A dummy implementation of quic::ProofSource which returns a dummy cert and
+// a fake signature for given QUIC server config.


nit: for a given QUIC server config.

wu-bin · 2019-06-20T14:47:14Z

source/extensions/quic_listeners/quiche/envoy_quic_fake_proof_verifier.h

+              std::unique_ptr<quic::ProofVerifierCallback> callback) override {
+    if (VerifyCertChain(hostname, certs, "", cert_sct, context, error_details, details,
+                        std::move(callback)) == quic::QUIC_SUCCESS &&
+        signature == absl::StrCat("Dummy signature for { ", server_config, " }")) {


Can we put the StrCat call into a function, and use it from here and EnvoyQuicFakeProofSource::ComputeTlsSignature?

I thought about this, but ideally ProofSource and ProofVerifier shouldn't depends on each other and it's not worth to add another library for helper function used by fake implementations.

wu-bin · 2019-06-20T14:50:27Z

source/extensions/quic_listeners/quiche/envoy_quic_fake_proof_verifier.h

+                  const quic::ProofVerifyContext* /*context*/, std::string* /*error_details*/,
+                  std::unique_ptr<quic::ProofVerifyDetails>* /*details*/,
+                  std::unique_ptr<quic::ProofVerifierCallback> /*callback*/) override {
+    std::string cert = absl::StrCat("Dummy cert from ", hostname);


Similar to above, put "Dummy cert from {}" and "Dummy timestamp" into functions, use them from here and EnvoyQuicFakeProofSource.

wu-bin · 2019-06-20T14:56:23Z

source/extensions/quic_listeners/quiche/envoy_quic_fake_proof_verifier.h

+namespace Envoy {
+namespace Quic {
+
+// A dummy implementation of quic::ProofVerifier which approves the certs and


nit: For consistency, how about replace all mentions of "dummy" to "fake", in fake_proof_verifier and fake_proof_source?

wu-bin · 2019-06-20T14:59:31Z

tools/spelling_dictionary.txt

 canonicalizer
 cardinality
 casted
+chlo


nit: This is not needed, see the beginning of this file. Adding "CHLO" will also accept "chlo" or "Chlo".

wu-bin · 2019-06-20T15:19:20Z

test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc

+  std::vector<std::string> expected_certs_;
+};
+
+class EnvoyQuicProofSourceTest : public ::testing::Test {


nit: How about EnvoyQuicFakeProofSourceTest?

wu-bin · 2019-06-20T15:21:19Z

test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc

+TEST_F(EnvoyQuicProofSourceTest, TestGetProof) {
+  auto callback = std::make_unique<TestGetProofCallback>(expected_signature_, "Dummy timestamp",
+                                                         expected_certs_);
+  proof_source_.GetProof(server_address_, hostname_, server_config_, version_, chlo_hash_,


Can we also check that "callback" is actually executed by GetProof?

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

danzh2010 · 2019-06-20T18:51:00Z

/retest

repokitteh-read-only · 2019-06-20T18:51:04Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

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

see: more, trace.

wu-bin · 2019-06-20T18:54:40Z

bazel/external/quiche.BUILD

 )

+envoy_cc_library(
+    name = "quic_core_crypto_proof_source_interface",


nit: Please keep the libraries name in sync with their internal names. Same for quic_core_crypto_proof_verifier_interface.

Switched to quic_core_crypto_crypto_handshake_lib after sync'ing #7340.

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

danzh2010 · 2019-06-27T19:15:33Z

compile_time_options ci fixed by disabling all quic related tests via tag nofips. PTAL

wu-bin

Thanks Dan. This looks good to me, but I'm not familiar with ssl, can you get a second eye on the "nofips" filtering?

danzh2010 · 2019-06-27T20:45:30Z

/assign @mattklein123 @htuch

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

mattklein123

LGTM from a skim with a question. Thank you!

/wait-any

mattklein123 · 2019-06-29T17:12:46Z

ci/do_ci.sh

  # Building all the dependencies from scratch to link them against libc++.
  echo "Building..."
-  bazel build ${BAZEL_BUILD_OPTIONS} ${COMPILE_TIME_OPTIONS} -c dbg //source/exe:envoy-static
+  bazel build ${BAZEL_BUILD_OPTIONS} ${COMPILE_TIME_OPTIONS} -c dbg //source/exe:envoy-static --build_tag_filters=-nofips


Sorry what is the TLDR on all the nofips changes?

QUIC relies on some boringssl hooks that aren't in the (current) fips boringssl release.
I suggested to Dan that we just opt QUIC out of the FIPS build because of that. I didn't realize that our debug build used the fips boringssl build though :-/

I think this is the "compile time options" build.

What is preventing us from just building/testing QUIC in the normal build at this point?

I just documented the issue in #7433, but basically is what Alyssa explained.

nofip tag is only excluded from compile-time-options build. And in normal build we can still run and test QUIC integration code.

OK. Are we building QUIC in the normal release build now though? Or is it only compile time options?

Yes, we build and test QUIC in all CI's except for compile-time-options.

OK sounds good.

danzh1989 added 9 commits May 8, 2019 12:12

add fake proof source/verifier

3253b11

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

move to test

9914fb4

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

Revert "move to test"

2ea58dd

This reverts commit 9914fb4. Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into fakeproof

a8aa262

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

add tests

6b5315b

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

actually add test file

02e4fd9

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

Merge branch 'master' into fakeproof

6f8c2e4

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

revert flag change

58f3842

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

more comment

8fec99d

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners June 19, 2019 20:52

repokitteh-read-only bot assigned alyssawilk and wu-bin Jun 19, 2019

danzh1989 added 2 commits June 19, 2019 16:57

add words to dict

15e5e33

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

fix clang_tidy

72c2f84

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

wu-bin reviewed Jun 20, 2019

View reviewed changes

s/dummy/fake

25998ab

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

wu-bin previously approved these changes Jun 20, 2019

View reviewed changes

Merge branch 'master' into fakeproof

2fd6df0

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

danzh2010 dismissed wu-bin’s stale review via 2fd6df0 June 21, 2019 22:24

danzh1989 added 5 commits June 21, 2019 18:36

update spell_dict

aacc588

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

add nofips

f68e5dd

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

fix build exlucsion

12ef3c7

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

add more nofips

6029aeb

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

add build_tests_only

14f1e79

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

remove build_tag_filter for test

2fccff1

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

wu-bin previously approved these changes Jun 27, 2019

View reviewed changes

repokitteh-read-only bot assigned htuch and mattklein123 Jun 27, 2019

update tarball

ef7cf28

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

danzh2010 dismissed wu-bin’s stale review via ef7cf28 June 27, 2019 20:56

htuch removed their assignment Jun 28, 2019

mattklein123 reviewed Jun 29, 2019

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Jun 29, 2019

mattklein123 added the waiting:any label Jul 1, 2019

repokitteh-read-only bot removed the waiting:any label Jul 1, 2019

danzh2010 mentioned this pull request Jul 1, 2019

re-enable QUICHE tests when boringssl FIPS support of QUIC interface #7433

Closed

mattklein123 approved these changes Jul 1, 2019

View reviewed changes

mattklein123 merged commit cf1954e into envoyproxy:master Jul 1, 2019

Conversation

danzh2010 commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Jun 19, 2019

Uh oh!

danzh2010 commented Jun 20, 2019

Uh oh!

repokitteh-read-only bot commented Jun 20, 2019

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Jun 20, 2019

Uh oh!

repokitteh-read-only bot commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Jun 27, 2019

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Jun 27, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danzh2010 commented Jun 19, 2019 •

edited

Loading