tls: store SslSocket::transport_socket_options_ in SSL object #21679

danzh2010 · 2022-06-13T16:15:38Z

Commit Message: Store a pointer to shared pointer SslSocket::transport_socket_options_ in SSL object via SSL_set_app_data() for both Client and Server ContextImpl. The old behavior is storing the pointer to the object pointed by shared pointer SslSocket::transport_socket_options_, and only for ClientContextImpl.

This change make it possible the caller of SSL_get_app_data() can extend the life time of the object pointed by SslSocket::transport_socket_options_.

Additional Description: interface change to ContextImpl::newSsl()
Risk Level: high, behavioral change in client cert validation, but no external behavioral change.
Testing: existing ssl socket tests pass
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Runtime guard: N/A

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

danzh2010 · 2022-06-13T21:29:13Z

/assign @RyanTheOptimist

RyanTheOptimist

This looks like a nice small, PR. WOO HOO! :) How hard would it be to verify the new (server-side) behavior in unit tests?

RyanTheOptimist · 2022-06-13T21:58:02Z

source/extensions/transport_sockets/tls/context_impl.cc

+    auto transport_socket_options_shared_ptr_ptr =
+        static_cast<const Network::TransportSocketOptionsConstSharedPtr*>(SSL_get_app_data(ssl));
+    ASSERT(transport_socket_options_shared_ptr_ptr);
+    transport_socket_options = (*transport_socket_options_shared_ptr_ptr).get();


Is the only difference between these two branches the ASSERT()? I can't quite seem to convince myself of it.

the new code path dereferences the pointer twice.

Hm. So line 441 calls SSL_get_app_data(ssl) and casts it to TransportSocketOptionsConstSharedPtr*. but link 446 calls the same method and casts it to TransportSocketOptions*. This is confusing to me. What is the actual object stored in SSL_get_app_data? Maybe the type of the object is different in the old/new codepath?

Right, in the old path SSL_set_app_data() takes the TransportSocketOptions* which is from TransportSocketOptionsConstSharedPtr, but the in the new path SSL_set_app_data() takes a pointer to TransportSocketOptionsConstSharedPtr itself.

Ah, I see. Thanks for the explanation!

Storing a pointer to a shared pointer sets off my spidey sense a bit. A shared pointer implies the that object is owned by whoever has the last shared pointer to it, whereas a pointer implies that the object is owned elsewhere. But I guess SSL_set_app_data() requires a raw pointer and we can't put a smart pointer in there, right? And I guess this is safe because we require the caller to own Options and make it outlive the context, right?

Oh, I see from the PR description that you want the shared_ptr so that you can extend the lifetime of the object via what's stored in the SSL. Can you elaborate on the use case for this?

Given that this PR doesn't yet have any external behavior change, it might simplify to remove the runtime guard.

But I guess SSL_set_app_data() requires a raw pointer and we can't put a smart pointer in there, right?

Yes

And I guess this is safe because we require the caller to own Options and make it outlive the context, right?

SSL_get_app_data() is called within the life time of a ssl socket, so it's safe. And after retrieving the pointer to the shared_ptr, the async cert validator is responsible to extends the life time of the socket option.

Oh, I see from the PR description that you want the shared_ptr so that you can extend the lifetime of the object via what's stored in the SSL. Can you elaborate on the use case for this?

The ability to extend the life time of socket options will be very helpful in async cert validation. Given that ssl socket and context can be gone in the middle.

Given that this PR doesn't yet have any external behavior change, it might simplify to remove the runtime guard.

I wasn't quite sure about the runtime guard policy so I used one just in case, given that it's risky behavioral change. That being said, if I don't need to runtime guard the change, ssl socket will always storing the same type into SSL object. So your concern about storing different types in different conditions won't exist. Which one is better?

I think it's better in this case to not runtime guard it; it makes the code simpler, which reduces the risk of a logic error.

Okay, reverted runtime guard. PTAL!

lizan

xref: #9867

We might want to create a struct for SSL_set/get_app_data.

lizan · 2022-06-14T17:11:23Z

source/extensions/transport_sockets/tls/context_impl.cc

@@ -606,7 +622,9 @@ bssl::UniquePtr<SSL> ClientContextImpl::newSsl(const Network::TransportSocketOpt
  }

  if (options && !options->verifySubjectAltNameListOverride().empty()) {
-    SSL_set_app_data(ssl_con.get(), options);
+    if (!tls_async_cert_validation_) {
+      SSL_set_app_data(ssl_con.get(), options.get());


Let's always set pointer to the shared ptr? That simplifies the SSL_get_app_data logic.

This if condition is the runtime guard, so is the call site of SSL_get_app_data(). With the guard being true, newSsl() will always set pointer to the shared ptr.

danzh2010 · 2022-06-14T19:20:36Z

xref: #9867

We might want to create a struct for SSL_set/get_app_data.

I'm not sure about the direction of xref: #9867. I agree we should use only one interface for the same purpose. But SSL_set_ex_data() allows the SSL object to points to multiple places with different indexes, while SSL_set_app_data() only allows one. We could combine extended socket info and transport socket option into a struct, but we also could use SSL_set_ex_data() with different indexes for them. To me SSL_set_ex_data() seems to provide more flexibility and we don't need to refactor ssl socket interfaces.

Anyway, can I keep this PR as it before we clearly know which direction to go?

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

danzh2010 · 2022-06-15T15:12:33Z

How hard would it be to verify the new (server-side) behavior in unit tests?

Added a unit test for that.

RyanTheOptimist

Thanks for adding a test!

RyanTheOptimist · 2022-06-16T14:58:24Z

source/extensions/transport_sockets/tls/context_impl.cc

+    auto transport_socket_options_shared_ptr_ptr =
+        static_cast<const Network::TransportSocketOptionsConstSharedPtr*>(SSL_get_app_data(ssl));
+    ASSERT(transport_socket_options_shared_ptr_ptr);
+    transport_socket_options = (*transport_socket_options_shared_ptr_ptr).get();


Hm. So line 441 calls SSL_get_app_data(ssl) and casts it to TransportSocketOptionsConstSharedPtr*. but link 446 calls the same method and casts it to TransportSocketOptions*. This is confusing to me. What is the actual object stored in SSL_get_app_data? Maybe the type of the object is different in the old/new codepath?

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

RyanTheOptimist · 2022-06-22T14:25:14Z

/wait

danzh2010 · 2022-06-22T16:18:38Z

/wait

What's the action item?

RyanTheOptimist · 2022-06-22T16:45:08Z

/wait

What's the action item?

I asked a question last week about the casting on line 441/446 and I don't see a reply to it. #21679 (comment)

Oh, bizarre! Man, github is just the worst UI in history. I don't see a reply on the main page, but if click to the file I see a response there. I'm so sorry about that. I'll take a look now.

RyanTheOptimist

I'm really sorry for the delay! I think this LGTM, but I'd like your confirmation on a question before merging.

RyanTheOptimist · 2022-06-22T16:54:29Z

source/extensions/transport_sockets/tls/context_impl.cc

+    auto transport_socket_options_shared_ptr_ptr =
+        static_cast<const Network::TransportSocketOptionsConstSharedPtr*>(SSL_get_app_data(ssl));
+    ASSERT(transport_socket_options_shared_ptr_ptr);
+    transport_socket_options = (*transport_socket_options_shared_ptr_ptr).get();


Ah, I see. Thanks for the explanation!

Storing a pointer to a shared pointer sets off my spidey sense a bit. A shared pointer implies the that object is owned by whoever has the last shared pointer to it, whereas a pointer implies that the object is owned elsewhere. But I guess SSL_set_app_data() requires a raw pointer and we can't put a smart pointer in there, right? And I guess this is safe because we require the caller to own Options and make it outlive the context, right?

ggreenway

/wait

ggreenway · 2022-06-22T16:59:06Z

source/extensions/transport_sockets/tls/context_impl.cc

+    auto transport_socket_options_shared_ptr_ptr =
+        static_cast<const Network::TransportSocketOptionsConstSharedPtr*>(SSL_get_app_data(ssl));
+    ASSERT(transport_socket_options_shared_ptr_ptr);
+    transport_socket_options = (*transport_socket_options_shared_ptr_ptr).get();


I think it would be better to just store the raw pointer, not a pointer to smartptr, if we have to guarantee the lifetime already.

ggreenway · 2022-06-22T17:00:34Z

source/extensions/transport_sockets/tls/context_impl.cc

  // We use the first certificate for a new SSL object, later in the
  // SSL_CTX_set_select_certificate_cb() callback following ClientHello, we replace with the
  // selected certificate via SSL_set_SSL_CTX().
-  return bssl::UniquePtr<SSL>(SSL_new(tls_contexts_[0].ssl_ctx_.get()));
+  auto ssl_con = bssl::UniquePtr<SSL>(SSL_new(tls_contexts_[0].ssl_ctx_.get()));
+  if (tls_async_cert_validation_) {


Can we remove this branch and set the app data unconditionally? It makes it easier to reason about what data is set and when, especially since we lose all type-safety when stashing things in the SSL.

ggreenway · 2022-06-22T17:03:48Z

source/extensions/transport_sockets/tls/context_impl.cc

+    transport_socket_options = (*transport_socket_options_shared_ptr_ptr).get();
+  } else {
+    transport_socket_options =
+        static_cast<const Network::TransportSocketOptions*>(SSL_get_app_data(ssl));


I'm not a fan of storing different types here depending on options. It makes it harder to reason about (given lack of type safety), and much harder to sort it out in gdb if it crashes. I think it would be better to just have 2 indexes, and store the appropriate data in each index. Then if the wrong one is read, it will be a nullptr instead of a valid pointer of the wrong type.

Are you suggesting to switch to SSL_get_ex_data() with a non-zero index completely?

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

danzh2010 · 2022-06-24T15:34:20Z

sync'ed with main and removed runtime guard. PTAL

danzh2010 · 2022-06-27T15:09:12Z

Ping for a review after merge!

…roxy#21679) Store a pointer to shared pointer SslSocket::transport_socket_options_ in SSL object via SSL_set_app_data() for both Client and Server ContextImpl. The old behavior is storing the pointer to the object pointed by shared pointer SslSocket::transport_socket_options_, and only for ClientContextImpl. This change make it possible the caller of SSL_get_app_data() can extend the life time of the object pointed by SslSocket::transport_socket_options_. Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

danzh1989 added 6 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>

Merge remote-tracking branch 'origin/master'

aec3b1c

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

Merge remote-tracking branch 'upstream/main'

9d72076

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

SSL app data change

5bb9914

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

guard old behavior

167c7c9

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

danzh2010 requested review from lizan and ggreenway as code owners June 13, 2022 16:15

fix build

e337f5a

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

repokitteh-read-only bot assigned RyanTheOptimist Jun 13, 2022

RyanTheOptimist reviewed Jun 13, 2022

View reviewed changes

lizan reviewed Jun 14, 2022

View reviewed changes

test for server side change

7c4afa7

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

RyanTheOptimist reviewed Jun 16, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main'

a25e5ac

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

repokitteh-read-only bot added the waiting label Jun 22, 2022

RyanTheOptimist previously approved these changes Jun 22, 2022

View reviewed changes

ggreenway requested changes Jun 22, 2022

View reviewed changes

remove runtime guard

2cf90ac

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

danzh2010 dismissed RyanTheOptimist’s stale review via 2cf90ac June 23, 2022 15:36

repokitteh-read-only bot removed the waiting label Jun 23, 2022

danzh1989 added 3 commits June 23, 2022 15:47

format

b75f0e7

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

Merge remote-tracking branch 'upstream/main'

cdb1893

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

Merge branch 'master' into transportsocketoption

80c8bca

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

fix test

d8e3712

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

ggreenway approved these changes Jun 27, 2022

View reviewed changes

ggreenway merged commit f14eee8 into envoyproxy:main Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: store SslSocket::transport_socket_options_ in SSL object #21679

tls: store SslSocket::transport_socket_options_ in SSL object #21679

danzh2010 commented Jun 13, 2022 •

edited

Loading

danzh2010 commented Jun 13, 2022

RyanTheOptimist left a comment

RyanTheOptimist Jun 13, 2022

danzh2010 Jun 13, 2022

RyanTheOptimist Jun 16, 2022

danzh2010 Jun 16, 2022

RyanTheOptimist Jun 22, 2022

ggreenway Jun 22, 2022

danzh2010 Jun 22, 2022

danzh2010 Jun 22, 2022

ggreenway Jun 22, 2022

danzh2010 Jun 23, 2022

lizan left a comment

lizan Jun 14, 2022

danzh2010 Jun 14, 2022

danzh2010 commented Jun 14, 2022

danzh2010 commented Jun 15, 2022

RyanTheOptimist left a comment

RyanTheOptimist Jun 16, 2022

RyanTheOptimist commented Jun 22, 2022

danzh2010 commented Jun 22, 2022

RyanTheOptimist commented Jun 22, 2022

RyanTheOptimist left a comment

RyanTheOptimist Jun 22, 2022

ggreenway left a comment

ggreenway Jun 22, 2022

ggreenway Jun 22, 2022

ggreenway Jun 22, 2022

danzh2010 Jun 22, 2022

danzh2010 commented Jun 24, 2022

danzh2010 commented Jun 27, 2022

tls: store SslSocket::transport_socket_options_ in SSL object #21679

tls: store SslSocket::transport_socket_options_ in SSL object #21679

Conversation

danzh2010 commented Jun 13, 2022 • edited Loading

danzh2010 commented Jun 13, 2022

RyanTheOptimist 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

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Jun 14, 2022

danzh2010 commented Jun 15, 2022

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Jun 22, 2022

danzh2010 commented Jun 22, 2022

RyanTheOptimist commented Jun 22, 2022

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway 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

danzh2010 commented Jun 24, 2022

danzh2010 commented Jun 27, 2022

danzh2010 commented Jun 13, 2022 •

edited

Loading