tls: support async cert validation by danzh2010 · Pull Request #21417 · envoyproxy/envoy

danzh2010 · 2022-05-24T02:53:49Z

Commit Message: change TLS transport socket to use SSL_CTX_set_custom_verify() instead of SSL_CTX_set_verify() and change CertValidator interface to support async cert validation. Also change EnvoyQuicCertVerifier to use the new async interfaces.

This change is needed for envoyproxy/envoy-mobile#1575. Envoy Mobile allows certificates to be verified by the OS-provided certificate verifier. And these verification can be very slow and so blocking the network thread while the verification happens is not an option. Instead, the verification should be performed asynchronously on a different thread. (This is how the cert verification works in Chrome, which is what we're modeling this implementation on).

Risk Level: high, change boring SSL interface used
Testing: added new unit tests and integration tests
Docs Changes: release note
Release Notes: documented tls transport changes.
Runtime guard: envoy.reloadable_features.tls_async_cert_validation

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

soulxu · 2022-05-24T13:41:17Z

source/extensions/transport_sockets/tls/context_impl.cc

+  Ssl::ValidateResult result = cert_validator_->doCustomVerifyCertChain(
+      *cert_chain, nullptr, extended_socket_info, transport_socket_options, SSL_get_SSL_CTX(ssl),
+      ech_name_override, SSL_is_server(ssl), error_details, out_alert);
+  if (result != Ssl::ValidateResult::Pending && extended_socket_info != nullptr) {


seems extended_socket_info can't be nullptr. otherwise the code here doesn't check the nullptr https://github.com/envoyproxy/envoy/pull/21417/files#diff-2b39f212c2f760ae547351a46f92ad08c0ec84c556d678ebe9388b9d008e7313R462

but I could be wrong, still learning this part of code.

Thanks for pointing it out! I changed it into ASSERT.

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

danzh2010 · 2022-05-24T15:46:14Z

/assign @RyanTheOptimist

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

RyanTheOptimist

There's a lot going on in this PR, as you mentioned :) If there are any opportunities to break out refactors into smaller PRs, I would strongly recommend doing so to expedite getting this landed. Here's a first pass at reviewing source/.

changelogs/current.yaml

envoy/ssl/ssl_socket_extended_info.h

source/common/quic/envoy_quic_proof_verifier.cc

source/common/quic/envoy_quic_proof_verifier.h

source/extensions/transport_sockets/tls/cert_validator/cert_validator.h

source/extensions/transport_sockets/tls/cert_validator/default_validator.cc

ggreenway · 2022-05-25T16:57:25Z

High level question to start, since it wasn't obvious from skimming through the code: why does this need to become async?

/wait-any

RyanTheOptimist · 2022-05-25T17:13:26Z

High level question to start, since it wasn't obvious from skimming through the code: why does this need to become async?

/wait-any

Oh, great question. We should discuss that in the PR and potentially link to envoyproxy/envoy-mobile#1575. This PR lays the ground work for eventually allowing certificates to be verified by the OS-provided certificate verifier. Annoyingly, these verifications can be very slow and so blocking the network thread while the verification happens is not an option. Instead, the verification will be performed on a different thread. (This is how the cert verification works in Chrome, which is what we're modeling this implementation on). Hopefully that helps?

yanavlasov · 2022-05-25T19:24:18Z

High level question to start, since it wasn't obvious from skimming through the code: why does this need to become async?
/wait-any

Oh, great question. We should discuss that in the PR and potentially link to envoyproxy/envoy-mobile#1575. This PR lays the ground work for eventually allowing certificates to be verified by the OS-provided certificate verifier. Annoyingly, these verifications can be very slow and so blocking the network thread while the verification happens is not an option. Instead, the verification will be performed on a different thread. (This is how the cert verification works in Chrome, which is what we're modeling this implementation on). Hopefully that helps?

Another use case is support for cloud HSM, that require RPC for key operations.

ggreenway · 2022-05-25T19:29:28Z

High level question to start, since it wasn't obvious from skimming through the code: why does this need to become async?
/wait-any

Oh, great question. We should discuss that in the PR and potentially link to envoyproxy/envoy-mobile#1575. This PR lays the ground work for eventually allowing certificates to be verified by the OS-provided certificate verifier. Annoyingly, these verifications can be very slow and so blocking the network thread while the verification happens is not an option. Instead, the verification will be performed on a different thread. (This is how the cert verification works in Chrome, which is what we're modeling this implementation on). Hopefully that helps?

Another use case is support for cloud HSM, that require RPC for key operations.

This change is to validation; I think it would be a different change to do async signing.

ggreenway

/wait

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

daixiang0 · 2022-08-01T02:11:06Z

source/extensions/transport_sockets/tls/cert_validator/default_validator.cc

+      if (error_details != nullptr) {
+        *error_details = error;
+      }
+      ENVOY_LOG(debug, error);


Is it error_details since error is fixed?

Using either error or error_details should be fine. What do you mean by error being fixed?

I think error is not changed since:

if (error_details != nullptr) { *error_details = error; }

daixiang0 · 2022-08-01T02:11:46Z

source/extensions/transport_sockets/tls/cert_validator/default_validator.cc

+      if (error_details != nullptr) {
+        *error_details = error;
+      }
+      ENVOY_LOG(debug, error);


Is it error_details since error is fixed?

danzh2010 · 2022-08-01T21:16:27Z

PTAL @ggreenway

ggreenway · 2022-08-01T23:19:42Z

/wait-any

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

danzh2010 · 2022-08-02T15:04:14Z

clang-tidy is still complaining the unrelated missing include in source/common/common/logger.h.
PTAL @ggreenway

ggreenway

LGTM!

I'm not sure why clang-tidy decided to look at logger.h in this PR.

ggreenway · 2022-08-02T19:06:40Z

clang-tidy error is a tool failure; I don't understand why it's producing this error.

Writing fixes to clang-tidy-fixes.yaml ...
clang-tidy check failed, potentially fixed by clang-apply-replacements:
Diagnostics:
- BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
  DiagnosticMessage:
    FileOffset: 303
    FilePath: ./source/common/common/logger.h
    Message: '''source/common/common/logger_impl.h'' file not found'
    Ranges:
    - FileOffset: 303
      FilePath: ./source/common/common/logger.h
      Length: 36
    Replacements: []
  DiagnosticName: clang-diagnostic-error
  Level: Error
MainSourceFile: ''

danzh2010 · 2022-08-02T19:40:14Z

@RyanTheOptimist This is good to merge. Thanks!

RyanTheOptimist · 2022-08-02T20:09:28Z

Alas, I can't merge 'cause of the spurious clang tidy error. @alyssawilk, can you?

alyssawilk · 2022-08-03T13:40:16Z

alas by now it needs a main merge, but I'm happy to force merge once that's sorted

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

danzh2010 · 2022-08-03T18:31:15Z

The PR is sync'ed @alyssawilk

Both things the comment says are inaccurate: After envoyproxy#23320, this mechanism no longer uses the X509_STORE callback, but a flag. After envoyproxy#21417, Envoy is also not (always) using SSL_CTX_set_cert_verify_callback, but a different one. Just remove the comment altogether, as it doesn't seem to provide any value. All it's saying is that, because Envoy still uses `store` at the end of the day, configuring things on `store` does stuff. But the whole function configures things on `store`, so it's not specific to that block anyway. Signed-off-by: David Benjamin <davidben@google.com>

Both things the comment says are inaccurate: After #23320, this mechanism no longer uses the X509_STORE callback, but a flag. After #21417, Envoy is also not (always) using SSL_CTX_set_cert_verify_callback, but a different one. Just remove the comment altogether, as it doesn't seem to provide any value. All it's saying is that, because Envoy still uses `store` at the end of the day, configuring things on `store` does stuff. But the whole function configures things on `store`, so it's not specific to that block anyway. Signed-off-by: David Benjamin <davidben@google.com>

danzh1989 added 14 commits August 16, 2021 13:07

switch to github

bc25efa

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

rename namespace

47683cb

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

async cert verify

daebbc9

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

remove unused interface

39af664

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

ssl socket unit tests

2cefcbb

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

Merge branch 'master' into sslsocket

32b1afe

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

add integration tests

525c1cc

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

ssl transport socket integration test

1430ad5

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

quic tests

6f5894a

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

add integration test

caa5e8f

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

Merge branch 'master' into sslsocket

5875baf

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

add comments

6b7cc90

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

add runtime guard

de9c7fa

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

release note

1d5711f

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

danzh2010 requested review from ggreenway and lizan as code owners May 24, 2022 02:53

htuch assigned ggreenway May 24, 2022

soulxu reviewed May 24, 2022

View reviewed changes

fix CI

a7a0f09

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

repokitteh-read-only bot assigned RyanTheOptimist May 24, 2022

fix REGISTER_FACTORY_TEST_ALLOWLIST

91f2c5d

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

yanavlasov self-assigned this May 24, 2022

RyanTheOptimist reviewed May 25, 2022

View reviewed changes

repokitteh-read-only bot added the waiting:any label May 25, 2022

repokitteh-read-only bot removed the waiting:any label May 25, 2022

repokitteh-read-only bot removed the waiting label Jul 22, 2022

ggreenway requested changes Jul 25, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 25, 2022

return value

3fd3e4c

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

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

daixiang0 reviewed Aug 1, 2022

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Aug 1, 2022

always store leaf cert

458b1c8

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

ggreenway previously approved these changes Aug 2, 2022

View reviewed changes

danzh1989 added 3 commits August 3, 2022 13:53

Merge remote-tracking branch 'upstream/main'

d3102d6

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

Merge branch 'master' into sslsocket

e0f26d5

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

format

b1519ab

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

danzh2010 dismissed ggreenway’s stale review via b1519ab August 3, 2022 15:15

alyssawilk merged commit a95d1b7 into envoyproxy:main Aug 3, 2022

davidben mentioned this pull request Nov 4, 2022

tls: remove stale comment #23846

Merged

alyssawilk mentioned this pull request Apr 24, 2023

envoy_reloadable_features_tls_async_cert_validation deprecation #26908

Closed

Conversation

danzh2010 commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 May 24, 2022

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented May 24, 2022

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway commented May 25, 2022

Uh oh!

RyanTheOptimist commented May 25, 2022

Uh oh!

yanavlasov commented May 25, 2022

Uh oh!

ggreenway commented May 25, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

daixiang0 Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

danzh2010 Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

daixiang0 Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

daixiang0 Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Aug 1, 2022

Uh oh!

ggreenway commented Aug 1, 2022

Uh oh!

danzh2010 commented Aug 2, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Aug 2, 2022

Uh oh!

danzh2010 commented Aug 2, 2022

Uh oh!

RyanTheOptimist commented Aug 2, 2022

Uh oh!

alyssawilk commented Aug 3, 2022

Uh oh!

danzh2010 commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

danzh2010 commented May 24, 2022 •

edited

Loading

soulxu May 24, 2022 •

edited

Loading