Skip to content

Commit 0d53851

Browse files
anniefrchzanniefrchz
committed
Clean up code and slight refactor for handshake internals functions
1 parent b9392aa commit 0d53851

File tree

3 files changed

+44
-60
lines changed

3 files changed

+44
-60
lines changed

‎src/core/tsi/ssl_transport_security.cc‎

Lines changed: 40 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,8 @@ struct tsi_ssl_handshaker : public tsi_handshaker,
189189
// Will be set if there is a pending call to tsi_handshaker_next(),
190190
// or nullopt if not.
191191
std::optional<HandshakerNextArgs> handshaker_next_args ABSL_GUARDED_BY(mu);
192-
const void MaybeSetError(std::string error)
193-
ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu) {
192+
void MaybeSetError(std::string error) const
193+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
194194
if (!handshaker_next_args.has_value()) return;
195195
if (handshaker_next_args->error_ptr == nullptr) return;
196196
*handshaker_next_args->error_ptr = std::move(error);
@@ -208,7 +208,7 @@ struct tsi_ssl_handshaker : public tsi_handshaker,
208208

209209
static std::pair<tsi_result, std::optional<HandshakerNextArgs>>
210210
ssl_handshaker_next_async(tsi_ssl_handshaker* self)
211-
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&self->mu);
211+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu);
212212

213213
struct tsi_ssl_handshaker_result {
214214
tsi_handshaker_result base;
@@ -455,13 +455,8 @@ enum ssl_private_key_result_t TlsPrivateKeySignWrapper(
455455
&result,
456456
[&](absl::StatusOr<std::string>* status_or_string)
457457
ABSL_NO_THREAD_SAFETY_ANALYSIS {
458-
if (status_or_string->ok()) {
459-
handshaker->signed_bytes = std::move(*status_or_string);
460-
return TlsPrivateKeyOffloadComplete(ssl, out, out_len, max_out);
461-
} else {
462-
handshaker->MaybeSetError(status_or_string->status().ToString());
463-
return ssl_private_key_failure;
464-
}
458+
handshaker->signed_bytes = std::move(*status_or_string);
459+
return TlsPrivateKeyOffloadComplete(ssl, out, out_len, max_out);
465460
},
466461
[&](std::shared_ptr<grpc_core::PrivateKeySigner::AsyncSigningHandle>*
467462
async_handler) ABSL_NO_THREAD_SAFETY_ANALYSIS {
@@ -2035,11 +2030,11 @@ static const tsi_handshaker_result_vtable handshaker_result_vtable = {
20352030

20362031
static tsi_result ssl_handshaker_result_create(
20372032
tsi_ssl_handshaker* handshaker, unsigned char* unused_bytes,
2038-
size_t unused_bytes_size, tsi_handshaker_result** handshaker_result,
2039-
std::string* error) {
2033+
size_t unused_bytes_size, tsi_handshaker_result** handshaker_result)
2034+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
20402035
if (handshaker == nullptr || handshaker_result == nullptr ||
20412036
(unused_bytes_size > 0 && unused_bytes == nullptr)) {
2042-
if (error != nullptr) *error = "invalid argument";
2037+
if (handshaker != nullptr) handshaker->MaybeSetError("invalid argument");
20432038
return TSI_INVALID_ARGUMENT;
20442039
}
20452040
tsi_ssl_handshaker_result* result =
@@ -2060,11 +2055,11 @@ static tsi_result ssl_handshaker_result_create(
20602055
// --- tsi_handshaker methods implementation. ---
20612056

20622057
static tsi_result ssl_handshaker_get_bytes_to_send_to_peer(
2063-
tsi_ssl_handshaker* impl, unsigned char* bytes, size_t* bytes_size,
2064-
std::string* error) {
2058+
tsi_ssl_handshaker* impl, unsigned char* bytes, size_t* bytes_size)
2059+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
20652060
int bytes_read_from_ssl = 0;
20662061
if (bytes == nullptr || bytes_size == nullptr || *bytes_size > INT_MAX) {
2067-
if (error != nullptr) *error = "invalid argument";
2062+
impl->MaybeSetError("invalid argument");
20682063
return TSI_INVALID_ARGUMENT;
20692064
}
20702065
GRPC_CHECK_LE(*bytes_size, static_cast<size_t>(INT_MAX));
@@ -2073,7 +2068,7 @@ static tsi_result ssl_handshaker_get_bytes_to_send_to_peer(
20732068
if (bytes_read_from_ssl < 0) {
20742069
*bytes_size = 0;
20752070
if (!BIO_should_retry(impl->network_io)) {
2076-
if (error != nullptr) *error = "error reading from BIO";
2071+
impl->MaybeSetError("error reading from BIO");
20772072
impl->result = TSI_INTERNAL_ERROR;
20782073
return impl->result;
20792074
} else {
@@ -2092,8 +2087,8 @@ static tsi_result ssl_handshaker_get_result(tsi_ssl_handshaker* impl) {
20922087
return impl->result;
20932088
}
20942089

2095-
static tsi_result ssl_handshaker_do_handshake(tsi_ssl_handshaker* impl,
2096-
std::string* error) {
2090+
static tsi_result ssl_handshaker_do_handshake(tsi_ssl_handshaker* impl)
2091+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
20972092
if (ssl_handshaker_get_result(impl) != TSI_HANDSHAKE_IN_PROGRESS) {
20982093
impl->result = TSI_OK;
20992094
return impl->result;
@@ -2130,10 +2125,9 @@ static tsi_result ssl_handshaker_do_handshake(tsi_ssl_handshaker* impl,
21302125
LOG(INFO) << "Handshake failed with error "
21312126
<< tsi::SslErrorString(ssl_result) << ": " << err_str
21322127
<< verify_result_str;
2133-
if (error != nullptr && error->empty()) {
2134-
*error = absl::StrCat(tsi::SslErrorString(ssl_result), ": ", err_str,
2135-
verify_result_str);
2136-
}
2128+
impl->MaybeSetError(absl::StrCat(
2129+
tsi::SslErrorString(ssl_result), ": ", err_str, verify_result_str,
2130+
": ", impl->signed_bytes.status().ToString()));
21372131
impl->result = TSI_PROTOCOL_FAILURE;
21382132
return impl->result;
21392133
}
@@ -2142,24 +2136,24 @@ static tsi_result ssl_handshaker_do_handshake(tsi_ssl_handshaker* impl,
21422136
}
21432137

21442138
static tsi_result ssl_handshaker_process_bytes_from_peer(
2145-
tsi_ssl_handshaker* impl, const unsigned char* bytes, size_t* bytes_size,
2146-
std::string* error) {
2139+
tsi_ssl_handshaker* impl, const unsigned char* bytes, size_t* bytes_size)
2140+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
21472141
int bytes_written_into_ssl_size = 0;
21482142
if (bytes == nullptr || bytes_size == nullptr || *bytes_size > INT_MAX) {
2149-
if (error != nullptr) *error = "invalid argument";
2143+
impl->MaybeSetError("invalid argument");
21502144
return TSI_INVALID_ARGUMENT;
21512145
}
21522146
GRPC_CHECK_LE(*bytes_size, static_cast<size_t>(INT_MAX));
21532147
bytes_written_into_ssl_size =
21542148
BIO_write(impl->network_io, bytes, static_cast<int>(*bytes_size));
21552149
if (bytes_written_into_ssl_size < 0) {
21562150
LOG(ERROR) << "Could not write to memory BIO.";
2157-
if (error != nullptr) *error = "could not write to memory BIO";
2151+
impl->MaybeSetError("could not write to memory BIO");
21582152
impl->result = TSI_INTERNAL_ERROR;
21592153
return impl->result;
21602154
}
21612155
*bytes_size = static_cast<size_t>(bytes_written_into_ssl_size);
2162-
return ssl_handshaker_do_handshake(impl, error);
2156+
return ssl_handshaker_do_handshake(impl);
21632157
}
21642158

21652159
static void ssl_handshaker_destroy(tsi_handshaker* self) {
@@ -2171,11 +2165,11 @@ static void ssl_handshaker_destroy(tsi_handshaker* self) {
21712165
// |bytes_remaining|.
21722166
static tsi_result ssl_bytes_remaining(tsi_ssl_handshaker* impl,
21732167
unsigned char** bytes_remaining,
2174-
size_t* bytes_remaining_size,
2175-
std::string* error) {
2168+
size_t* bytes_remaining_size)
2169+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
21762170
if (impl == nullptr || bytes_remaining == nullptr ||
21772171
bytes_remaining_size == nullptr) {
2178-
if (error != nullptr) *error = "invalid argument";
2172+
if (impl != nullptr) impl->MaybeSetError("invalid argument");
21792173
return TSI_INVALID_ARGUMENT;
21802174
}
21812175
// Attempt to read all of the bytes in SSL's read BIO. These bytes should
@@ -2193,9 +2187,8 @@ static tsi_result ssl_bytes_remaining(tsi_ssl_handshaker* impl,
21932187
<< "Failed to read the expected number of bytes from SSL object.";
21942188
gpr_free(*bytes_remaining);
21952189
*bytes_remaining = nullptr;
2196-
if (error != nullptr) {
2197-
*error = "Failed to read the expected number of bytes from SSL object.";
2198-
}
2190+
impl->MaybeSetError(
2191+
"Failed to read the expected number of bytes from SSL object.");
21992192
return TSI_INTERNAL_ERROR;
22002193
}
22012194
*bytes_remaining_size = static_cast<size_t>(bytes_read);
@@ -2206,16 +2199,15 @@ static tsi_result ssl_bytes_remaining(tsi_ssl_handshaker* impl,
22062199
// By doing that, we drain SSL bio buffer used to hold handshake data.
22072200
// This API needs to be repeatedly called until all handshake data are
22082201
// received from SSL.
2209-
static tsi_result ssl_handshaker_write_output_buffer(tsi_handshaker* self,
2210-
size_t* bytes_written,
2211-
std::string* error) {
2212-
tsi_ssl_handshaker* impl = static_cast<tsi_ssl_handshaker*>(self);
2202+
static tsi_result ssl_handshaker_write_output_buffer(tsi_ssl_handshaker* impl,
2203+
size_t* bytes_written)
2204+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
22132205
tsi_result status = TSI_OK;
22142206
size_t offset = *bytes_written;
22152207
do {
22162208
size_t to_send_size = impl->outgoing_bytes_buffer_size - offset;
22172209
status = ssl_handshaker_get_bytes_to_send_to_peer(
2218-
impl, impl->outgoing_bytes_buffer + offset, &to_send_size, error);
2210+
impl, impl->outgoing_bytes_buffer + offset, &to_send_size);
22192211
offset += to_send_size;
22202212
if (status == TSI_INCOMPLETE_DATA) {
22212213
impl->outgoing_bytes_buffer_size *= 2;
@@ -2228,7 +2220,7 @@ static tsi_result ssl_handshaker_write_output_buffer(tsi_handshaker* self,
22282220
}
22292221

22302222
static tsi_result ssl_handshaker_next_impl(tsi_ssl_handshaker* self)
2231-
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&self->mu) {
2223+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
22322224
// If there are received bytes, process them first.
22332225
tsi_result status = TSI_OK;
22342226
size_t bytes_written = 0;
@@ -2248,19 +2240,16 @@ static tsi_result ssl_handshaker_next_impl(tsi_ssl_handshaker* self)
22482240
size_t bytes_written_to_openssl =
22492241
remaining_bytes_to_write_to_openssl_size;
22502242
status = ssl_handshaker_process_bytes_from_peer(
2251-
self, remaining_bytes_to_write_to_openssl, &bytes_written_to_openssl,
2252-
self->handshaker_next_args->error_ptr);
2243+
self, remaining_bytes_to_write_to_openssl, &bytes_written_to_openssl);
22532244
// As long as the BIO is full, drive the SSL handshake to consume bytes
22542245
// from the BIO. If the SSL handshake returns any bytes, write them to
22552246
// the peer.
22562247
while (status == TSI_DRAIN_BUFFER) {
2257-
status = ssl_handshaker_write_output_buffer(
2258-
self, &bytes_written, self->handshaker_next_args->error_ptr);
2248+
status = ssl_handshaker_write_output_buffer(self, &bytes_written);
22592249
if (status != TSI_OK) {
22602250
return status;
22612251
}
2262-
status = ssl_handshaker_do_handshake(
2263-
self, self->handshaker_next_args->error_ptr);
2252+
status = ssl_handshaker_do_handshake(self);
22642253
}
22652254
// Move the pointer to the first byte not yet successfully written to
22662255
// the BIO.
@@ -2283,17 +2272,15 @@ static tsi_result ssl_handshaker_next_impl(tsi_ssl_handshaker* self)
22832272
// During the PrivateKeyOffload signature, an empty call to
22842273
// ssl_handshaker_do_handshake needs to be forced after the async offload
22852274
// has completed.
2286-
status = ssl_handshaker_do_handshake(self,
2287-
self->handshaker_next_args->error_ptr);
2275+
status = ssl_handshaker_do_handshake(self);
22882276
#endif
22892277
}
22902278

22912279
if (status != TSI_OK) {
22922280
return status;
22932281
}
22942282
// Get bytes to send to the peer, if available.
2295-
status = ssl_handshaker_write_output_buffer(
2296-
self, &bytes_written, self->handshaker_next_args->error_ptr);
2283+
status = ssl_handshaker_write_output_buffer(self, &bytes_written);
22972284
if (status != TSI_OK) {
22982285
return status;
22992286
}
@@ -2309,8 +2296,7 @@ static tsi_result ssl_handshaker_next_impl(tsi_ssl_handshaker* self)
23092296
// bytes from the peer that must be processed.
23102297
unsigned char* unused_bytes = nullptr;
23112298
size_t unused_bytes_size = 0;
2312-
status = ssl_bytes_remaining(self, &unused_bytes, &unused_bytes_size,
2313-
self->handshaker_next_args->error_ptr);
2299+
status = ssl_bytes_remaining(self, &unused_bytes, &unused_bytes_size);
23142300
if (status != TSI_OK) {
23152301
return status;
23162302
}
@@ -2323,8 +2309,7 @@ static tsi_result ssl_handshaker_next_impl(tsi_ssl_handshaker* self)
23232309
}
23242310
status = ssl_handshaker_result_create(
23252311
self, unused_bytes, unused_bytes_size,
2326-
&self->handshaker_next_args->handshaker_result,
2327-
self->handshaker_next_args->error_ptr);
2312+
&self->handshaker_next_args->handshaker_result);
23282313
if (status == TSI_OK) {
23292314
// Indicates that the handshake has completed and that a
23302315
// handshaker_result has been created.
@@ -2350,7 +2335,7 @@ static tsi_result ssl_handshaker_next_impl(tsi_ssl_handshaker* self)
23502335
// For example, this would be called from the key signer's callback.
23512336
static std::pair<tsi_result, std::optional<HandshakerNextArgs>>
23522337
ssl_handshaker_next_async(tsi_ssl_handshaker* self)
2353-
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&self->mu) {
2338+
ABSL_EXCLUSIVE_LOCKS_REQUIRED(&tsi_ssl_handshaker::mu) {
23542339
if (self->is_shutdown || !self->handshaker_next_args.has_value()) {
23552340
return {TSI_HANDSHAKE_SHUTDOWN, std::nullopt};
23562341
}

‎src/cpp/common/tls_certificate_provider.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,11 +148,11 @@ absl::Status InMemoryCertificateProvider::UpdateIdentityKeyCertPair(
148148
std::vector<IdentityKeyOrSignerCertPair>
149149
identity_key_or_signer_cert_pairs) {
150150
GRPC_CHECK(!identity_key_or_signer_cert_pairs.empty());
151-
auto pairs_core_or =
151+
auto pairs_core =
152152
CreatePairsCore(std::move(identity_key_or_signer_cert_pairs));
153-
if (!pairs_core_or.ok()) return pairs_core_or.status();
153+
if (!pairs_core.ok()) return pairs_core.status();
154154
return grpc_tls_certificate_provider_in_memory_set_identity_certificate(
155-
c_provider_, *pairs_core_or)
155+
c_provider_, *pairs_core)
156156
? absl::OkStatus()
157157
: absl::InternalError("Unable to update identity certificate");
158158
}

‎test/core/test_util/tls_utils.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,7 @@ PemKeyCertPairList MakeCertKeyPairs(absl::string_view private_key,
7373
if (private_key.empty() && certs.empty()) {
7474
return {};
7575
}
76-
return PemKeyCertPairList{PemKeyCertPair(
77-
std::string(private_key.data(), private_key.length()), certs)};
76+
return PemKeyCertPairList{PemKeyCertPair(std::string(private_key), certs)};
7877
}
7978

8079
std::string GetFileContents(const std::string& path) {

0 commit comments

Comments
 (0)