alts: add gRPC TSI socket #4153

lizan · 2018-08-15T03:39:14Z

Signed-off-by: Lizan Zhou zlizan@google.com

Description:
3rd PR for #3429, add transport socket implementation.

Risk Level: Low (not enabled in main)
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Lizan Zhou <zlizan@google.com>

danzh2010

Thanks for putting things together! I haven't read the test code yet, but overall it looks good!

danzh2010 · 2018-08-16T15:47:58Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+void TsiSocket::setTransportSocketCallbacks(Envoy::Network::TransportSocketCallbacks& callbacks) {
+  callbacks_ = &callbacks;
+
+  handshaker_ = handshaker_factory_(callbacks.connection().dispatcher());


For internal usage, can you pass down callbacks_->connection().localAddress() and callbacks_->connection().remoteAddress() as arguments to handshaker_factory_?
And also can you move handshaker creation to sometime later. e.g. doHandshake(). Because we don't need it till then, and by that time connection's local and remote address should have been set.

danzh2010 · 2018-08-16T16:09:17Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+      std::string err;
+      bool peer_validated = handshake_validator_(peer, err);
+      if (peer_validated) {
+        ENVOY_CONN_LOG(info, "TSI: Handshake validation succeeded.", callbacks_->connection());


Are we sure that we want to log peer validation result for each handshake? If not, maybe down grade to debug or trace?

danzh2010 · 2018-08-16T16:57:03Z

source/extensions/transport_sockets/alts/tsi_socket.h

+namespace TransportSockets {
+namespace Alts {
+
+typedef std::function<TsiHandshakerPtr(Event::Dispatcher&)> HandshakerFactory;


Can you also comment this?

danzh2010 · 2018-08-16T18:48:44Z

test/extensions/transport_sockets/alts/tsi_socket_test.cc

+TEST_F(TsiSocketTest, HandshakeWithoutValidationAndTransferData) {
+  initialize(nullptr, nullptr);
+
+  client_.write_buffer_.add("hello from client");


Please add a const for "hello from client"

danzh2010 · 2018-08-16T18:49:28Z

test/extensions/transport_sockets/alts/tsi_socket_test.cc

+  auto validator = [](const tsi_peer&, std::string&) { return true; };
+  initialize(validator, validator);
+
+  client_.write_buffer_.add("hello from client");


danzh2010 · 2018-08-16T18:51:21Z

test/extensions/transport_sockets/alts/tsi_socket_test.cc

+}
+
+TEST_F(TsiSocketTest, HandshakeWithoutValidationAndTransferData) {
+  initialize(nullptr, nullptr);


comment: pass in null validator to skip peer validation.

danzh2010 · 2018-08-16T19:18:44Z

test/extensions/transport_sockets/alts/tsi_socket_test.cc

+  EXPECT_EQ("hello from client", server_.read_buffer_.toString());
+}
+
+TEST_F(TsiSocketTest, HandshakeWithSucessfulValidationAndTransferData) {


This test looks very similar to the one above. Can you extend doHandshakeAndExpectSuccess() to successfulHandshakeAndTransferData() so that the shared code between these 2 tests can be in one place?

danzh2010 · 2018-08-16T19:19:28Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+
+  if (status != TSI_INCOMPLETE_DATA && status != TSI_OK) {
+    ENVOY_CONN_LOG(debug, "TSI: Handshake failed: status: {}", callbacks_->connection(), status);
+    return Network::PostIoAction::Close;


Is this code covered in test?

danzh2010 · 2018-08-16T19:20:27Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+        tsi_handshaker_result_get_unused_bytes(handshaker_result, &unused_bytes, &unused_byte_size);
+    ASSERT(status == TSI_OK);
+    if (unused_byte_size > 0) {
+      raw_read_buffer_.add(unused_bytes, unused_byte_size);


Is this code covered in test?

danzh2010 · 2018-08-16T19:22:02Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+                 callbacks_->connection(), enumToInt(result.action_), result.bytes_processed_,
+                 result.end_stream_read_);
+  if (result.action_ == Network::PostIoAction::Close && result.bytes_processed_ == 0) {
+    return result;


covered in test?

Q: What if result.actions_ == Network::PostIoAction::Close but result.bytes_processed_ > 0? It seems possible in RawBufferSocket.

It will go through this method, if handshake is not complete it will still call doHandshake and process the bytes read from RawBufferSocket, if handshake was complete, then it will use frame protector to unprotect the bytes and return Close, the unprotected buffer will be added in buffer.

It makes sense that if it happens after handshake completion, we want to hand the decrypted payload to upper layer. But I'm not sure if we still want to move forward handshake in this case. Because handshake will likely fail because of invalid input or even worse still wait for a complete message?

It is still a valid case. For example, a client send CLIENT_FINISHED with some data (this will be processed as unused data in handshake), with end_stream. In this case we should still proceed with handshake, process the data, and decrypt the unused data to upper layer.

receive end_stream won't cause RawBufferSocket::doRead() to return Close. So it should proceed as you described above.
But if server got a read error before its receives the whole CLIENT_FINISHED message, it will pass whatever it gets to handshaker. And Handshaker will return TSI_INCOMPLETE_DATA and doHandshake() return KeepOpen, but handshake_complete_ is still false. According to the following code block:
if (action == Network::PostIoAction::Close || !handshake_complete_) {
return {action, 0, false};
}
This doRead() will return {Network::PostIoAction::KeepOpen, 0, false}. Then server will still wait for handshake to finish.

Can you add a test for this case by making the mocked raw_buffer_socket_ to return partial CLIENT_FINISHED message and Close? The expectation is that doRead() should return Close.

danzh2010 · 2018-08-16T19:26:09Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+
+  if (!handshake_complete_) {
+    Network::PostIoAction action = doHandshake();
+    if (action == Network::PostIoAction::Close || !handshake_complete_) {


Is this condition ever satisfied? It seems that doHandshake() always return Network::PostIoAction::KeepOpen.

I'll leave this uncovered until next PR to follow up on synchronous TSI optimization.

Ah, that's for synchronous handshake! Makes sense.

danzh2010 · 2018-08-16T19:27:32Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+Network::IoResult TsiSocket::doWrite(Buffer::Instance& buffer, bool end_stream) {
+  if (!handshake_complete_) {
+    Network::PostIoAction action = doHandshake();
+    if (action == Network::PostIoAction::Close) {


see comment in doRead()

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2018-08-23T20:24:23Z

@danzh2010 ping?

danzh2010

Sorry for the delay. LGTM once my question is addressed.

danzh2010 · 2018-08-23T21:19:44Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+                 callbacks_->connection(), enumToInt(result.action_), result.bytes_processed_,
+                 result.end_stream_read_);
+  if (result.action_ == Network::PostIoAction::Close && result.bytes_processed_ == 0) {
+    return result;


It makes sense that if it happens after handshake completion, we want to hand the decrypted payload to upper layer. But I'm not sure if we still want to move forward handshake in this case. Because handshake will likely fail because of invalid input or even worse still wait for a complete message?

Signed-off-by: Lizan Zhou <zlizan@google.com>

dnoe · 2018-08-28T13:17:23Z

Assigning @htuch for senior review.

Signed-off-by: Lizan Zhou <zlizan@google.com>

danzh2010 · 2018-08-30T14:53:13Z

LGTM. Thanks for adding check to read error and end stream!

htuch

Overall, this looks very solid and close to ready to ship. A bunch of nits, clarity improvements and coverage request comments.

htuch · 2018-08-30T22:07:13Z

source/extensions/transport_sockets/alts/BUILD

+    hdrs = [
+        "tsi_socket.h",
+    ],
+    repository = "@envoy",


Why do you need this line?

htuch · 2018-08-30T22:12:49Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+  raw_buffer_socket_->setTransportSocketCallbacks(*noop_callbacks_);
+}
+
+std::string TsiSocket::protocol() const { return ""; }


htuch · 2018-08-30T22:15:35Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+    raw_write_buffer_.move(*next_result->to_send_);
+  }
+
+  if (status == TSI_OK && handshaker_result != nullptr) {


What if status == TSI_OK && handshaker_results == nullptr? I see various error handling below, but it's not immediately clear how/why it covers these cases. Can we put in some more ASSERTS or make this more robust?

status == TSI_OK && handshaker_results == nullptr is a common case when a handshake is in progress, i.e. need to read/write more data. It doesn't follow in any error handlings below, just need more calls to doHandshake.

htuch · 2018-08-30T22:17:16Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+        ENVOY_CONN_LOG(debug, "TSI: Handshake validation succeeded.", callbacks_->connection());
+      } else {
+        ENVOY_CONN_LOG(info, "TSI: Handshake validation failed: {}", callbacks_->connection(), err);
+        tsi_peer_destruct(&peer);


Can we use RAII via https://github.com/envoyproxy/envoy/blob/master/source/common/common/cleanup.h to make this more robust/clearer?

htuch · 2018-08-30T22:17:38Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+      if (peer_validated) {
+        ENVOY_CONN_LOG(debug, "TSI: Handshake validation succeeded.", callbacks_->connection());
+      } else {
+        ENVOY_CONN_LOG(info, "TSI: Handshake validation failed: {}", callbacks_->connection(), err);


Warning or error log levels?

Actually downgraded this to debug level, this is consistent with what SslSocket does.

htuch · 2018-08-30T22:17:50Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+    }
+    if (handshake_validator_) {
+      std::string err;
+      bool peer_validated = handshake_validator_(peer, err);


Nit: const bool

htuch · 2018-08-30T22:18:13Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+
+    status =
+        tsi_handshaker_result_get_unused_bytes(handshaker_result, &unused_bytes, &unused_byte_size);
+    ASSERT(status == TSI_OK);


Can you add a comment on why this must always be the case?

htuch · 2018-08-30T22:18:58Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+    tsi_frame_protector* frame_protector;
+    status =
+        tsi_handshaker_result_create_frame_protector(handshaker_result, NULL, &frame_protector);
+    ASSERT(status == TSI_OK);


htuch · 2018-08-30T22:24:34Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+                   callbacks_->connection(), enumToInt(result.action_), result.bytes_processed_,
+                   result.end_stream_read_);
+    if (result.action_ == Network::PostIoAction::Close && result.bytes_processed_ == 0) {
+      return result;


Missing coverage.

htuch · 2018-08-30T22:24:50Z

source/extensions/transport_sockets/alts/tsi_socket.cc

+  if (!handshake_complete_) {
+    Network::PostIoAction action = doHandshake();
+    if (action == Network::PostIoAction::Close) {
+      return {action, 0, false};


Missing coverage.

Signed-off-by: Lizan Zhou <zlizan@google.com>

htuch · 2018-08-31T00:47:12Z

@lizan LGTM; need to fix format.

Signed-off-by: Lizan Zhou <zlizan@google.com>

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <piotrsikora@google.com>

alts: add gRPC TSI socket

ce89fd9

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan self-assigned this Aug 15, 2018

lizan requested a review from danzh2010 August 15, 2018 03:39

lizan added 2 commits August 15, 2018 02:14

more tests, fix asan

62f4339

Signed-off-by: Lizan Zhou <zlizan@google.com>

fix format

9d8c0d6

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan mentioned this pull request Aug 15, 2018

Add an unused implementation NoOpTransportSocketCallbacks #4172

Closed

danzh2010 reviewed Aug 16, 2018

View reviewed changes

lizan mentioned this pull request Aug 21, 2018

Add an unused implementation NoOpTransportSocketCallbacks #4181

Merged

lizan added 3 commits August 21, 2018 15:53

Merge remote-tracking branch 'upstream/master' into alts_socket

149eb1b

address comment

10dc809

Signed-off-by: Lizan Zhou <zlizan@google.com>

more tests

79c5082

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan requested a review from alyssawilk August 23, 2018 20:24

danzh2010 reviewed Aug 23, 2018

View reviewed changes

lizan added 2 commits August 23, 2018 15:13

prepend

aeab41f

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_socket

f98b4e6

dnoe assigned htuch and lizan and unassigned lizan Aug 28, 2018

lizan added 3 commits August 30, 2018 01:21

handle end of stream and read error more rigidly

2687a25

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_socket

2e3d5c1

Merge remote-tracking branch 'upstream/master' into alts_socket

40152c6

danzh2010 previously approved these changes Aug 30, 2018

View reviewed changes

htuch suggested changes Aug 30, 2018

View reviewed changes

lizan added 2 commits August 30, 2018 17:30

address comments

39c373a

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_socket

f73fb09

lizan dismissed danzh2010’s stale review via f73fb09 August 31, 2018 00:30

nits

f13f64a

Signed-off-by: Lizan Zhou <zlizan@google.com>

fix format

fc0b98e

Signed-off-by: Lizan Zhou <zlizan@google.com>

htuch approved these changes Aug 31, 2018

View reviewed changes

lizan merged commit 1212423 into envoyproxy:master Aug 31, 2018

alts: add gRPC TSI socket #4153

alts: add gRPC TSI socket #4153

Conversation

lizan commented Aug 15, 2018

danzh2010 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

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

lizan commented Aug 23, 2018

danzh2010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe commented Aug 28, 2018

danzh2010 commented Aug 30, 2018

htuch 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

htuch commented Aug 31, 2018