tls: use custom instead of socket based bio by florincoras · Pull Request #12911 · envoyproxy/envoy

florincoras · 2020-09-01T03:16:45Z

Signed-off-by: Florin Coras fcoras@cisco.com

Risk Level: Medium
Testing: unit tests
Docs Changes: n/a
Release Notes: tls: switched from using socket BIOs to using custom BIOs that know how to interact with IoHandles. The feature can be disabled by setting runtime feature envoy.reloadable_features.tls_use_io_handle_bio to false.

Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras · 2020-09-01T03:18:40Z

cc @antoniovicente @mattklein123

antoniovicente · 2020-09-01T04:10:51Z

Please mention issue #12899 in the PR description.

I need to look at it more closely but it seems more or less right. It may be worth adding a separate header for BIO_new_io_handle

lizan

+1 to a separate header for BIO, can we get for coverage for https://storage.googleapis.com/envoy-pr/12911/coverage/source/extensions/transport_sockets/tls/io_handle_bio.cc.gcov.html ?

lizan · 2020-09-01T04:44:57Z

source/extensions/transport_sockets/tls/io_handle_bio.cc

+
+namespace {
+
+static inline Envoy::Network::IoHandle* bio_io_handle(BIO* bio) {


static doesn't do anything in unnamed namespace :)

Thanks for that! Wanted to maintain the "typical" BIO formatting but even clang_tidy is complaining now. Is there any macro/comment that we could use to disable clang_tidy for this file? Do we want that or should I rename everything?

I'm fine with keeping the naming, but not static. You can add NOLINT or NOLINTNEXTLINE for each method.

Thanks and done!

- added io_handle_bio header file - removed static keyword for functions in unnamed namespace - added simple unit tests for io_handle_bio member functions Signed-off-by: Florin Coras <fcoras@cisco.com>

lizan · 2020-09-01T23:58:42Z

source/extensions/transport_sockets/tls/BUILD

    name = "ssl_socket_lib",
-    srcs = ["ssl_socket.cc"],
-    hdrs = ["ssl_socket.h"],
+    srcs = [


Can io_handle_bio.{cc,h} in a separate envoy_cc_library?

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente

Thanks for the cleanup. One less need for direct access to the fd.

And sorry for the delay in review, it has been a busy week.

antoniovicente · 2020-09-01T04:20:26Z

source/extensions/transport_sockets/tls/ssl_socket.cc

-  // socket BIO which relies on access to a file descriptor.
-  BIO* bio = BIO_new_socket(callbacks_->ioHandle().fdDoNotUse(), 0);
+  // Use custom BIO that reads from/writes to IoHandle
+  BIO* bio = BIO_new_io_handle(&callbacks_->ioHandle());


Consider protecting with a runtime feature to allow for fast rollback if we find some problem despite it being relatively simple and as far I can tell, correct.

Okay, let me add that. But, since I'm not familiar with them, will switching be possible at runtime or does the code have to be recompiled with the feature moved from enabled to disabled list?

My understanding is that it is controllable via reloadable config. The default is to use the new code path. Since this call only happens once per connection, during initialization of the transport socket object, it is safe for the runtime feature to change during runtime.

antoniovicente · 2020-09-03T04:02:58Z

source/extensions/transport_sockets/tls/io_handle_bio.cc

+  case BIO_C_GET_FD:
+    ret = -1;
+    *reinterpret_cast<void**>(ptr) = b->ptr;
+    break;


Do we really need to implement BIO_C_SET_FD and BIO_C_GET_FD?

It may be good to add ASSERT(false) for cases that we do not expect to ever happen at the proxy. This includes: BIO_CTRL_RESET, BIO_C_FILE_SEEK, BIO_C_FILE_TELL, BIO_CTRL_INFO, BIO_C_GET_FD, BIO_C_SET_FD:

I've used the socket bio as an example (since we're faking one) and those are the only control messages it implements. BIO_C_SET_FD is the one I've used for setting the ptr but I guess we could change that.
What would you prefer, keep following the socket model or implement things differently?

Your constructor could set b->ptr directly instead of using BIO_C_SET_FD.

I think your implementation of iohandle BIOs is a great improvement, I'm just working my way through some nits.

Ow. Yes we can, but we need to initialize all the other fields (shutdown, init, num). Should I do that explicitly in the constructor and then ASSERT(false) on BIO_C_SET_FD and BIO_C_GET_FD?

Yes, we'ld need to also initiate other relevant fields like init also in the contructor.

Perfect! Done!

antoniovicente · 2020-09-03T04:04:05Z

source/extensions/transport_sockets/tls/io_handle_bio.h

+const BIO_METHOD* BIO_s_io_handle(void);
+
+// NOLINTNEXTLINE(readability-identifier-naming)
+BIO* BIO_new_io_handle(Envoy::Network::IoHandle* io_handle);


Would it be possible to add some comments for this factory method?

antoniovicente · 2020-09-03T04:04:29Z

source/extensions/transport_sockets/tls/io_handle_bio.h

+namespace Tls {
+
+// NOLINTNEXTLINE(readability-identifier-naming)
+const BIO_METHOD* BIO_s_io_handle(void);


This method seems to only be used in the cc file, should we remove it from the header?

Good point. I exposed it for testing but ended up not using it.

antoniovicente · 2020-09-03T04:11:17Z

source/extensions/transport_sockets/tls/io_handle_bio.cc

+  auto result = bio_io_handle(b)->writev(&slice, 1);
+  BIO_clear_retry_flags(b);
+  if (!result.ok()) {
+    if (result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again) {


We may also want to set retry in the case of Api::IoError::IoErrorCode::Interrupt.

Although it seems that writes returning Interrupt will result in Envoy closing the socket. I don't know how often a EINTR return would happen in practice.

Interesting. I guess we could treat EINTR as EAGAIN here.

antoniovicente · 2020-09-03T04:24:40Z

test/extensions/transport_sockets/tls/io_handle_bio_test.cc

+  ret = bio_->method->ctrl(bio_, BIO_CTRL_GET_CLOSE, 0, nullptr);
+  EXPECT_EQ(ret, 1);
+
+  // avoid BIO_free assert in destructor


The bio_->init = 1 and bio_->shutdown = 1 further down seem to undo the work done by this BIO_CTRL_SET_CLOSE call.

Removed this test. It was crashing only prior to adding the last one.

antoniovicente · 2020-09-03T04:25:37Z

source/extensions/transport_sockets/tls/io_handle_bio.cc

+    b->shutdown = int(num);
+    break;
+  case BIO_CTRL_FLUSH:
+    ret = 1;


test for this case.

I didn't add a test for this because it's exercised by the rest of the boringssl infra in other tests, but let me add a simple one for completion.

antoniovicente

Looks great, thanks for the improvements to the socket APIs.

Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras · 2020-09-03T19:42:51Z

Looks great, thanks for the improvements to the socket APIs.

Thank you for the review and help with this!

N.B. Pushed a fix for clang-tidy's complaints regarding the use of nullptr

mattklein123

Awesome, thanks, one quick high level comment.

/wait

mattklein123 · 2020-09-04T00:18:07Z

source/common/runtime/runtime_features.cc

    "envoy.reloadable_features.preserve_upstream_date",
    "envoy.reloadable_features.stop_faking_paths",
    "envoy.reloadable_features.strict_1xx_and_204_response_headers",
+    "envoy.reloadable_features.tls_use_io_handle_bio",


This should be documented somewhere. Can you add a release note for this and also update the PR description to mention it? This will help us track it for removal later.

Done! Let me know if I missed something because this is my first time doing release notes updates.

Signed-off-by: Florin Coras <fcoras@cisco.com>

mattklein123

Nice!

tls: use custom instead of socket based bio

cedd3af

Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras requested review from PiotrSikora and lizan as code owners September 1, 2020 03:16

florincoras mentioned this pull request Sep 1, 2020

network tls: alternative to using socket BIOs in SslSocket #12899

Closed

lizan reviewed Sep 1, 2020

View reviewed changes

comments

467e88a

- added io_handle_bio header file - removed static keyword for functions in unnamed namespace - added simple unit tests for io_handle_bio member functions Signed-off-by: Florin Coras <fcoras@cisco.com>

mattklein123 assigned lizan and antoniovicente Sep 1, 2020

lizan reviewed Sep 1, 2020

View reviewed changes

move io handle bio to separate lib

6f884db

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente reviewed Sep 3, 2020

View reviewed changes

antoniovicente previously approved these changes Sep 3, 2020

View reviewed changes

florincoras dismissed antoniovicente’s stale review via 8f574fd September 3, 2020 19:40

florincoras force-pushed the tls branch from 5358b9b to 8f574fd Compare September 3, 2020 19:40

more comments

8f574fd

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente previously approved these changes Sep 3, 2020

View reviewed changes

mattklein123 requested changes Sep 4, 2020

View reviewed changes

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

mattklein123 self-assigned this Sep 4, 2020

update release notes

9b8bc32

Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras dismissed antoniovicente’s stale review via 9b8bc32 September 4, 2020 00:49

repokitteh-read-only bot removed the waiting label Sep 4, 2020

florincoras added 2 commits September 3, 2020 18:20

Merge branch 'master' into tls

55878ea

Signed-off-by: Florin Coras <fcoras@cisco.com>

Merge branch 'master' into tls

643a4e4

Signed-off-by: Florin Coras <fcoras@cisco.com>

mattklein123 approved these changes Sep 4, 2020

View reviewed changes

mattklein123 merged commit 1663949 into envoyproxy:master Sep 4, 2020

alyssawilk mentioned this pull request Apr 15, 2021

envoy.reloadable_features.tls_use_io_handle_bio deprecation #16013

Closed


		namespace {

		static inline Envoy::Network::IoHandle* bio_io_handle(BIO* bio) {

Conversation

florincoras commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

florincoras commented Sep 1, 2020

Uh oh!

antoniovicente commented Sep 1, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florincoras Sep 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

florincoras commented Sep 3, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

florincoras commented Sep 1, 2020 •

edited

Loading

florincoras Sep 3, 2020 •

edited

Loading