quiche: implement certificate verification #12063

danzh2010 · 2020-07-13T20:26:09Z

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

Implement quic::ProofVerifier which consists of cert verification and signature verification.
Cert verification:
Share cert verification code with Extensions::TransportSockets::Tls::ClientContextImpl. And initialize ProofVerifier using Envoy::Ssl::ClientContextConfig protobuf.
Signature verification:
Use quic::CertificateViewer to verify signature.

Risk Level: low, not in use
Testing: added new unit test for cert verifier and test interaction with ProofSource
Part of #9434 #2557

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

danzh2010 · 2020-07-13T20:41:02Z

/assign @PiotrSikora @wu-bin

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

wu-bin

Thanks Dan. I have some comments but as we chatted offline, I'd prefer to get a review from Victor or Nick.

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

wu-bin · 2020-07-14T21:59:04Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

+  size_t payload_size = sizeof(quic::kProofSignatureLabel) + sizeof(uint32_t) + chlo_hash.size() +
+                        server_config.size();
+  auto payload = std::make_unique<char[]>(payload_size);
+  quic::QuicDataWriter payload_writer(payload_size, payload.get(),
+                                      quiche::Endianness::HOST_BYTE_ORDER);
+  bool success =
+      payload_writer.WriteBytes(quic::kProofSignatureLabel, sizeof(quic::kProofSignatureLabel)) &&
+      payload_writer.WriteUInt32(chlo_hash.size()) && payload_writer.WriteStringPiece(chlo_hash) &&
+      payload_writer.WriteStringPiece(server_config);


This seems similar to quic::ProofSourceX509::GetProof, is it possible to reuse that code?

payload generation is actually shared across all ProofSource implementation not just with X509 ProofSource. I think it makes more sense for GetProof() to take the generated payload as argument. But this requires large upstream code refactoring which I don't think worth doing just for the sake of code sharing.

wu-bin · 2020-07-14T22:01:30Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+static X509* parseDERCertificate(const std::string& der_bytes, std::string* error_details) {
+  const uint8_t* data;
+  const uint8_t* orig_data;
+  orig_data = data = reinterpret_cast<const uint8_t*>(der_bytes.data());
+  bssl::UniquePtr<X509> cert(d2i_X509(nullptr, &data, der_bytes.size()));
+  if (!cert.get()) {
+    *error_details = "d2i_X509";
+    return nullptr;
+  }
+  if (data < orig_data || static_cast<size_t>(data - orig_data) != der_bytes.size()) {
+    // Trailing garbage.
+    return nullptr;
+  }
+  return cert.release();
+}


Can we move this function into to quiche and use it from here?

parseDERCertificate() is not used anywhere else in QUICHE. So it would look like a dead function.

No, we don't want the boringssl X509 code in quiche.

wu-bin · 2020-07-14T22:16:33Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+      quic::CertificateView::ParseSingleCertificate(certs[0]);
+  ASSERT(cert_view != nullptr);
+  for (const absl::string_view config_san : cert_view->subject_alt_name_domains()) {
+    if (config_san == hostname) {


Is it possible that config_san is a wildcard domain? If so, should we do regex match instead of ==?

I don't think SAN can be wildcard. But better to leave it to Nick to confirm.

Yes, it could be a wildcard. A regex is the wrong way to match the wildcard. For a given hostname, there is a single wildcard entry that could cover it: replace the leftmost label in the hostname with *. This could be changed to if (config_san == hostname || config_san == wildcard) where wildcard is the substitution described above (assuming there's more than one label in the hostname).

wu-bin · 2020-07-14T22:27:06Z

test/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_test.cc

+  std::unique_ptr<EnvoyQuicProofVerifier> verifier_;
+};
+
+TEST_F(EnvoyQuicProofVerifierTest, VerifyFilterChainSuccess) {


VerifyCertChainSuccess?

Same for other test names.

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

nharper · 2020-07-15T22:48:28Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

+
+  // TODO(danzh) Get the signature algorithm from leaf cert.
+  auto signature_callback = std::make_unique<SignatureCallback>(std::move(callback), chain);
+  ComputeTlsSignature(server_address, client_address, hostname, SSL_SIGN_RSA_PSS_RSAE_SHA256,


Calling ComputeTlsSignature here looks a bit confusing when reading the code. I realize that GetProof and ComputeTlsSignature both do similar things (and ComputeTlsSignature doesn't need to do any processing on the input before signing), but having a helper method for the shared signing code would improve readability here.

nharper · 2020-07-15T22:54:23Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+  if (certs.empty()) {
+    return quic::QUIC_FAILURE;
+  }
+  bssl::UniquePtr<STACK_OF(X509)> intermediates(sk_X509_new_null());


Is there any way we can avoid using the boringssl X.509 code? (It's a mess and should be replaced. The chromium cert verifier is a candidate replacement.)

I make the interface takes X509_STORE_CTX and X509 objects because ContextImpl::doVerifyCertChain() heavily uses X509 code. We can move the conversion of const std::vector<std::string>& certs to X509 into ContextImpl, but basically there is no way to avoid using these boringssl interfaces unless we don't reuse envoy cert verification code. But I think it makes sense to be consistent with tcp SSL socket.

There is a separate issue being sorted out for how to move envoy off of this boringssl code. The current status, AFAIK, is that the chromium cert verifier isn't in a state that can be consumed by other projects yet, so there isn't any readily available replacement. @PiotrSikora and I are discussing, and he has posted #10621 as an option.

I think for this PR, you should leave this as-is using boringssl, and this can be updated when we address the issue in envoy as a whole.

I agree you should reuse the existing envoy cert verification code.

nharper · 2020-07-15T22:55:13Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+static X509* parseDERCertificate(const std::string& der_bytes, std::string* error_details) {
+  const uint8_t* data;
+  const uint8_t* orig_data;
+  orig_data = data = reinterpret_cast<const uint8_t*>(der_bytes.data());
+  bssl::UniquePtr<X509> cert(d2i_X509(nullptr, &data, der_bytes.size()));
+  if (!cert.get()) {
+    *error_details = "d2i_X509";
+    return nullptr;
+  }
+  if (data < orig_data || static_cast<size_t>(data - orig_data) != der_bytes.size()) {
+    // Trailing garbage.
+    return nullptr;
+  }
+  return cert.release();
+}


No, we don't want the boringssl X509 code in quiche.

nharper · 2020-07-15T23:00:47Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+      quic::CertificateView::ParseSingleCertificate(certs[0]);
+  ASSERT(cert_view != nullptr);
+  for (const absl::string_view config_san : cert_view->subject_alt_name_domains()) {
+    if (config_san == hostname) {


Yes, it could be a wildcard. A regex is the wrong way to match the wildcard. For a given hostname, there is a single wildcard entry that could cover it: replace the leftmost label in the hostname with *. This could be changed to if (config_san == hostname || config_san == wildcard) where wildcard is the substitution described above (assuming there's more than one label in the hostname).

nharper · 2020-07-15T23:07:49Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_base.h

+
+// A partial implementation of quic::ProofVerifier which does signature
+// verification using SSL_SIGN_RSA_PSS_RSAE_SHA256.
+class EnvoyQuicProofVerifierBase : public quic::ProofVerifier {


I don't understand why EnvoyQuicProofVerifierBase and EnvoyQuicProofVerifier are separate classes.

Cert chain verification part is missing in base class. In EnvoyQuicProofVerifier it gets root cert from envoy config to verify the chain.

I see that that part is missing in the base class. Why is it missing from the base class instead of combining the two classes into one?

there is another TestProofVerifer inheriting from this base class but doesn't retrieve certs from Envoy infrastructure.

nharper · 2020-07-15T23:18:13Z

source/extensions/transport_sockets/tls/context_impl.h

@@ -101,6 +101,14 @@ class ContextImpl : public virtual Envoy::Ssl::Context {

  std::vector<Ssl::PrivateKeyMethodProviderSharedPtr> getPrivateKeyMethodProviders();

+  // Called by verifyCallback to do the actual cert chain verification.
+  int doVerifyCertChain(X509_STORE_CTX* store_ctx, Ssl::SslExtendedSocketInfo* ssl_extended_info,


My suggestion would be to refactor the cert verification code in ContextImpl to allow for the QUIC code to pass in a cert chain (as a vector of buffers of bytes, not X509 structs) and a hostname so that none of the x509 code needs to be in the ProofVerifier impl. (This might also mean adding an additional function to get error details.)

I agree; it would be nice if all the boringssl X509 code was contained here. But if it makes it messier, I'm ok with leaving it how it is.

I added another interface here for quiche integration code to call. It takes care of X509 CTX stuff

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

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

ggreenway · 2020-07-21T16:48:35Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+  if (certs.empty()) {
+    return quic::QUIC_FAILURE;
+  }
+  bssl::UniquePtr<STACK_OF(X509)> intermediates(sk_X509_new_null());


There is a separate issue being sorted out for how to move envoy off of this boringssl code. The current status, AFAIK, is that the chromium cert verifier isn't in a state that can be consumed by other projects yet, so there isn't any readily available replacement. @PiotrSikora and I are discussing, and he has posted #10621 as an option.

I think for this PR, you should leave this as-is using boringssl, and this can be updated when we address the issue in envoy as a whole.

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

ggreenway · 2020-07-21T17:19:01Z

source/extensions/transport_sockets/tls/context_impl.h

@@ -101,6 +101,14 @@ class ContextImpl : public virtual Envoy::Ssl::Context {

  std::vector<Ssl::PrivateKeyMethodProviderSharedPtr> getPrivateKeyMethodProviders();

+  // Called by verifyCallback to do the actual cert chain verification.
+  int doVerifyCertChain(X509_STORE_CTX* store_ctx, Ssl::SslExtendedSocketInfo* ssl_extended_info,


I agree; it would be nice if all the boringssl X509 code was contained here. But if it makes it messier, I'm ok with leaving it how it is.

source/extensions/transport_sockets/tls/context_impl.h

tools/spelling/spelling_dictionary.txt

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

danzh2010 · 2020-07-30T15:34:48Z

ping?

source/extensions/transport_sockets/tls/context_impl.cc

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

danzh2010 · 2020-07-31T16:57:37Z

ping?

ggreenway

This is looking pretty good.

Please add test coverage for the error cases you're adding. The report says the following are especially in need of more tests:

EnvoyQuicProofSourceBase::GetProof
EnvoyQuicProofSourceBase::Run
EnvoyQuicProofVerifier::VerifyCertChain
EnvoyQuicProofVerifierBase

Minor style note I saw in a few places: when declaring a pure virtual method, envoy style is to use PURE instead of = 0.

There's an open issue right now discussing whether it's safe to use std::string_view due to older supported OS versions for envoy mobile. The outcome isn't finalized yet, but it would be safer to convert all std::string_view into absl::string_view. cc @junr03

/wait

source/extensions/quic_listeners/quiche/envoy_quic_proof_source.cc

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_base.cc

test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.h

test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc

test/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_test.cc

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

ggreenway · 2020-07-31T17:51:34Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

@@ -44,6 +47,43 @@ class CodecClientCallbacksForTest : public Http::CodecClientCallbacks {
  Http::StreamResetReason last_stream_reset_reason_{Http::StreamResetReason::LocalReset};
 };

+std::unique_ptr<QuicClientTransportSocketFactory>


Please add 1 simple integration test validating cert validation failure.

added CertVerificationFailure which failed because of SAN mismatch.

ggreenway · 2020-07-31T21:25:18Z

It is confirmed that std::string_view needs to be changed to absl::string_view. See #12341 for discussion/rationale.

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

danzh2010 · 2020-08-03T16:17:08Z

It is confirmed that std::string_view needs to be changed to absl::string_view. See #12341 for discussion/rationale.

done

ggreenway

This is looking pretty good.

check_format is failing. I'd like to take a look at the coverage report before reviewing further, but coverage doesn't run until formatting passes.

I'd still really appreciate another set of eyes on this, since this is security-critical code. @PiotrSikora ? @envoyproxy/maintainers ? Anyone from google who works on related stuff?

/wait

test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc

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

ggreenway

I think the code looks good overall. Mostly just need to improve error handling test coverage now.

/wait

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

ggreenway · 2020-08-04T16:57:17Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

+
+  std::string error_details;
+  bssl::UniquePtr<X509> cert = parseDERCertificate(chain->certs[0], &error_details);
+  if (cert == nullptr) {


Please add test coverage for the error case. If it can't be tested, should it be changed to an ASSERT?

ggreenway · 2020-08-04T16:57:40Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_source_base.cc

+
+  bssl::UniquePtr<EVP_PKEY> pub_key(X509_get_pubkey(cert.get()));
+  int sign_alg = deduceSignatureAlgorithmFromPublicKey(pub_key.get(), &error_details);
+  if (sign_alg == 0) {


Please add test coverage for the error case.

ggreenway · 2020-08-04T16:59:13Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier.cc

+  bssl::UniquePtr<X509> leaf;
+  for (size_t i = 0; i < certs.size(); i++) {
+    bssl::UniquePtr<X509> cert = parseDERCertificate(certs[i], error_details);
+    if (!cert) {


Add test coverage for the error case

ggreenway · 2020-08-04T16:59:54Z

source/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_base.cc

+    return false;
+  }
+  int sign_alg = deduceSignatureAlgorithmFromPublicKey(cert_view->public_key(), error_details);
+  if (sign_alg == 0) {


Add test coverage for the error case

ggreenway · 2020-08-04T17:03:54Z

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

+  orig_data = data = reinterpret_cast<const uint8_t*>(der_bytes.data());
+  bssl::UniquePtr<X509> cert(d2i_X509(nullptr, &data, der_bytes.size()));
+  if (!cert.get()) {
+    *error_details = "d2i_X509: fail to parse DER";


add test coverage

ggreenway · 2020-08-04T17:04:03Z

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

+    return nullptr;
+  }
+  if (data < orig_data || static_cast<size_t>(data - orig_data) != der_bytes.size()) {
+    *error_details = "There is trailing garbage in DER.";


add test coverage

ggreenway · 2020-08-04T17:04:58Z

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

+  int sign_alg = 0;
+  const int pkey_id = EVP_PKEY_id(public_key);
+  switch (pkey_id) {
+  case EVP_PKEY_EC: {


Add test coverage for this case, including the error code for wrong curve/group

ggreenway · 2020-08-04T17:05:36Z

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

+    }
+#else
+    if (rsa_key_length < 2048 / 8) {
+      *error_details =


test coverage

ggreenway · 2020-08-04T17:06:20Z

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

+  } break;
+  default:
+    *error_details =
+        "Invalid leaf cert, only RSA and ECDSA certificates are supported in FIPS mode";


nit: this also applies to non-fips mode now; fix the error string

ggreenway · 2020-08-04T17:58:19Z

I'm not sure what happened in clang_tidy; all the errors were in standard library headers. Maybe try merging from master to see if it goes away?

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

ggreenway

This looks great, modulo one very minor nit. Waiting for the coverage build to finish so I can take a look, but I think you covered all the cases that were lacking.

ggreenway · 2020-08-05T21:25:39Z

test/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_test.cc

@@ -166,6 +194,43 @@ TEST_F(EnvoyQuicProofVerifierTest, VerifyProofFailureInvalidLeafCert) {
  EXPECT_EQ("Invalid leaf cert.", error_details);
 }

+TEST_F(EnvoyQuicProofVerifierTest, VerifyProofFailureUnsupportedRsaKey) {


nit: the test uses an unsupported EC curve, but the name says unsupported RSA key. Please fix the test name.

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

ggreenway · 2020-08-06T20:23:39Z

/azp run

azure-pipelines · 2020-08-06T20:23:48Z

Azure Pipelines successfully started running 1 pipeline(s).

danzh1989 added 8 commits July 1, 2020 16:31

add verifier impl and proof source details

0045de4

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

no empty filter chain

105e714

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

Merge branch 'master' into certverify

811a141

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

revert cert verification

a0d8975

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

add back cert verification

6e599bf

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

comment

67fad57

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

Merge branch 'master' into certverify

af27dd7

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

Merge branch 'master' into certverify

eae447a

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

danzh2010 requested review from alyssawilk, lizan, mattklein123 and PiotrSikora as code owners July 13, 2020 20:26

add comment

010661c

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

repokitteh-read-only bot assigned PiotrSikora and wu-bin Jul 13, 2020

PiotrSikora requested a review from ggreenway July 13, 2020 21:13

danzh1989 added 5 commits July 14, 2020 12:04

spell

7ac6b81

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

remove redudant check

562bbd2

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

fix asan

66e0383

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

fix sigalgs

5c0dd47

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

format

ae75699

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

wu-bin reviewed Jul 14, 2020

View reviewed changes

danzh1989 added 2 commits July 15, 2020 13:15

fix windows

1b85e44

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

sign alg

bcad52c

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

nharper suggested changes Jul 15, 2020

View reviewed changes

danzh1989 added 3 commits July 20, 2020 16:21

share sign function

2675461

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

wildcard

f19000a

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

update comment

4c2d222

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

ggreenway requested changes Jul 21, 2020

View reviewed changes

danzh1989 added 3 commits July 28, 2020 18:09

Merge branch 'master' into certverify

03f319e

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

fix unuse param

c60059a

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

fix sig alg deduction

29e1f29

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

ggreenway reviewed Jul 30, 2020

View reviewed changes

source/extensions/transport_sockets/tls/context_impl.cc Show resolved Hide resolved

use string_view

bb9b501

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

ggreenway requested changes Jul 31, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 31, 2020

corner cases

333b896

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

repokitteh-read-only bot removed the waiting label Jul 31, 2020

ggreenway requested changes Aug 3, 2020

View reviewed changes

test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 3, 2020

format

99aa268

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

repokitteh-read-only bot removed the waiting label Aug 3, 2020

fix test failure

fc121e7

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

ggreenway requested changes Aug 4, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 4, 2020

add more test

c3229ed

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

repokitteh-read-only bot removed the waiting label Aug 5, 2020

Merge branch 'master' into certverify

405fdb9

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

ggreenway requested changes Aug 5, 2020

View reviewed changes

fix test name

0f7f7b8

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

ggreenway approved these changes Aug 6, 2020

View reviewed changes

ggreenway merged commit 43b110a into envoyproxy:master Aug 7, 2020

quiche: implement certificate verification #12063

quiche: implement certificate verification #12063

Conversation

danzh2010 commented Jul 13, 2020 • edited Loading

danzh2010 commented Jul 13, 2020

wu-bin 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

danzh2010 Jul 15, 2020 • 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

danzh2010 commented Jul 30, 2020

danzh2010 commented Jul 31, 2020

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Jul 31, 2020

danzh2010 commented Aug 3, 2020

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway 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

ggreenway commented Aug 4, 2020

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Aug 6, 2020

azure-pipelines bot commented Aug 6, 2020

danzh2010 commented Jul 13, 2020 •

edited

Loading

danzh2010 Jul 15, 2020 •

edited

Loading