[TLS creds] fix cancel_check_peer() to actually work (#34434)

The `cancel_check_peer()` method is [always called with a non-OK
status](866fc41067/src/core/lib/security/transport/security_handshaker.cc (L560)),
since it's used only in cancellation cases. However, the implementation
of this method for TLS creds was bailing out if the status was non-OK,
meaning that `cancel_check_peer()` was never actually cancelling the
verification request. This bug seems to have been introduced back in
#25631, when the method was initially implemented.

I don't think we actually have any async verifier implementations today,
so this isn't actually causing a problem. I discovered this bug as part
of #34426, which was triggering the core e2e `no_logging` test to fail.
That test is designed to ensure that we don't generate any logs while
processing individual RPCs, since that would be bad for performance and
would flood logfiles. My PR caused a connection attempt to be cancelled
during the test, which triggered the error log that I am removing in
this PR.

Note that with this PR, the TLS creds `cancel_check_peer()` methods are
not actually doing anything with the status. Ideally, they should be
passing the status through to the verifier's `Cancel()` method, but we
apparently didn't add a parameter for that, which means that although
cancellation will work now, it will not properly pass through the right
error message. At some point, we should fix this and add tests covering
cancellation of async verifier requests to prove that the error message
is propagated correctly.
pull/34444/head
Mark D. Roth 1 year ago committed by GitHub
parent 60f25c289b
commit 7f555bd9a1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 2
      src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.h
  2. 16
      src/core/lib/security/security_connector/tls/tls_security_connector.cc

@ -50,6 +50,8 @@ struct grpc_tls_certificate_verifier
absl::Status* sync_status) = 0;
// Operations that will be performed when a request is cancelled.
// This is only needed when in async mode.
// TODO(roth): This needs to take an absl::Status argument so that we
// can pass the cancellation status through to the check_peer callback.
virtual void Cancel(grpc_tls_custom_verification_check_request* request) = 0;
// Compares this grpc_tls_certificate_verifier object with \a other.

@ -389,13 +389,7 @@ void TlsChannelSecurityConnector::check_peer(
}
void TlsChannelSecurityConnector::cancel_check_peer(
grpc_closure* on_peer_checked, grpc_error_handle error) {
if (!error.ok()) {
gpr_log(GPR_ERROR,
"TlsChannelSecurityConnector::cancel_check_peer error: %s",
StatusToString(error).c_str());
return;
}
grpc_closure* on_peer_checked, grpc_error_handle /*error*/) {
auto* verifier = options_->certificate_verifier();
if (verifier != nullptr) {
grpc_tls_custom_verification_check_request* pending_verifier_request =
@ -674,13 +668,7 @@ void TlsServerSecurityConnector::check_peer(
}
void TlsServerSecurityConnector::cancel_check_peer(
grpc_closure* on_peer_checked, grpc_error_handle error) {
if (!error.ok()) {
gpr_log(GPR_ERROR,
"TlsServerSecurityConnector::cancel_check_peer error: %s",
StatusToString(error).c_str());
return;
}
grpc_closure* on_peer_checked, grpc_error_handle /*error*/) {
auto* verifier = options_->certificate_verifier();
if (verifier != nullptr) {
grpc_tls_custom_verification_check_request* pending_verifier_request =

Loading…
Cancel
Save