TLS Session Keys export for GRPC C++

[Vignesh2208]

This PR adds the TLS Key export logic for GRPC C++ which allows decrypting packet captures using tools like wireshark.

Tracking Github issue: #24944

Relevant GRFC: grpc/proposal#252

@ctiller, @ZhenLian, @yihuazhang

---

This change is 

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Vignesh Babu (dec3fe1, 9ed3d20, 913dd73)

[ZhenLian]

Thanks for making the change! I left some high-level comments.
This PR contains multiple file changes, and it spans multiple layers. Since this is a newly added feature and no one is using it, it is probably better reviewed if we can divide it into several PRs, instead of one huge PR(for example, we can have the TSI changes first, and then the core changes, and then C++ changes on top of it).
Feel free to let me know if you have any questions/concerns. Thank you so much!

[ZhenLian]

I am a bit confused on why we need a TlsKeyLoggerContainer in addition to TlsKeyLogger...can we make them into one class?

[Vignesh2208]

I have renamed the classes here for more clarity. There are two classes: TlsKeyLogFileWriter and TlsKeyLogger. TlsKeyLogger is bound to a tls key log configuration i.e on object of TlsKeyLogger exists for each specified configuration. On the other hand one instance of TlsKeyLogFileWriter exists for each unique log file. So a two TlsKeyLoggers may share the same TlsKeyLogFileWriter instance if their key log configuration specifies the same log file. Having two separate classes like this allows the key log configuration (with addition of new entries) to grow over time while the TlsKeyLogFileWriter class can remain unchanged.

[Vignesh2208]

Reviewable status: 7 of 42 files reviewed, 21 unresolved discussions (waiting on @ctiller, @markdroth, @yihuazhang, and @ZhenLian)

---

src/core/lib/security/security_connector/tls/tls_security_connector.h, line 29 at r10 (raw file):

Previously, ZhenLian wrote…

I think using is slightly preferable to typedef, per https://google.github.io/styleguide/cppguide.html#Aliases

Done.

---

src/core/tsi/ssl_transport_security.h, line 84 at r7 (raw file):

Previously, ZhenLian wrote…

Same question as grpc_tls_session_key_logger: what was the reason for defining these opaque types here? Sometimes we need to do that because certain header files have to be pure C, and can't include C++ classes. In those scenarios, we have to define an opaque type, and do a type casting later on.

But this header file doesn't belong to that directory. Can we just use the C++ types instead?
(there are a few places following the similar patterns here...)

This is removed in latest commit. I used the C++ types.

---

src/core/tsi/ssl_transport_security.h, line 84 at r10 (raw file):

Previously, ZhenLian wrote…

I still felt like it was unnecessary to re-declare a new type for this here. I was not sure why the ssl_session_cache implementation was doing that way, but if there were some caveats, we'd better document them out.
Can we try to use its original type(TlsSessionKeyLogger) directly in this file, to avoid doing type-casting, please? If something goes wrong, we at least know the reason why we should design structs like this, which could be useful for our future implementations.
Thank you so much!

This is removed in latest commit. I used the C++ types.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 49 at r10 (raw file):

Previously, ZhenLian wrote…

What is the reason that we define Ref() and Unref() for TlsSessionKeyLoggerCache? Can its caller use grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> directly?

TlsSessionKeyLogger class needs TlsSessionKeyLoggerCache object to be alive for its lifetime. So it calls Ref() and Unref() methods on the cache object. We can change TlsSessionKeyLoggerCache into a RefCounted class as well. However, the object of the cache will be a static global instance. Currently its defined in ssl_key_logging.cc as per snipper below. If we are to make TlsSessionKeyLoggerCache as a RefCounted object, we would have to declare static grpc_core::RefcountedPtr<::tsi::TlsSessionKeyLoggerCache> g_cache_instance_. However this is not allowed because a static global object cannot have non default destructor. So to keep it simple, I defined TlsSessionKeyLoggerCache as a normal class with explicit Ref, Unref methods.

Code snippet:

static ::tsi::TlsSessionKeyLoggerCache* g_cache_instance_

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 77 at r10 (raw file):

Previously, ZhenLian wrote…

Can we just define this class as a nested class of TlsSessionKeyLoggerCache?

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 84 at r10 (raw file):

Previously, ZhenLian wrote…

I think TlsSessionKeyLogger needs TlsSessionKeyLoggerCache to always be available through its lifetime? If so, should we use grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> cache here?

I explained in a previous comment why TlsSessionKeyLoggerCache is not implemented as a RefcountedPtr. We would be forced to use a weird global definition like: (the current impl avoids this but instead uses explicit Ref and Unref methods)

Code snippet:

static grpc_core::RefcountedPtr<::tsi::TlsSessionKeyLoggerCache>* g_cache_instance_. // Note that g_cache_instance_ is a pointer to RefcountedPtr. This is because global variables cannot have non default destructor. So they can only be pointers.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 85 at r4 (raw file):

Previously, ZhenLian wrote…

Is "\r\n" going to work on Linux and MacOS as well? If so, this is probably OK.
I think a better approach might be to determine what the current system is(not sure if it is possible), or ask users to configure what kind of system they want their logs to be in, as a input parameter.
@ctiller @markdroth would you mind suggesting some other options as well?

I tested this on Linux VM, Windows VM and Mac machine and it seems to work

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 42 at r10 (raw file):

Previously, ZhenLian wrote…

Can we create a ref-counted instance instead of a plain pointer here?

Discussed in a previous comment. I can make the change if you think it might not be confusing.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 138 at r10 (raw file):

Previously, ZhenLian wrote…

I think the comments are a bit out-of-date?

Thanks for pointing it out. I removed them.

[ZhenLian]

This is looking pretty good! Thanks again for the effort to add session key logging into gRPC. I believe it will be very beneficial to users who have some problems setting up mTLS and hence have to debug their session keys!

Reviewed 11 of 35 files at r11, 24 of 24 files at r12, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @ctiller, @markdroth, @Vignesh2208, and @yihuazhang)

---

include/grpc/grpc_security.h, line 1279 at r12 (raw file):

/**
 * EXPERIMENTAL API - Subject to change.
 * Associates a session key logging config with a

nit: the comment needs to be updated as we don't have "key logging config" anymore

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 49 at r10 (raw file):

Previously, Vignesh2208 (Vignesh Babu) wrote…

TlsSessionKeyLogger class needs TlsSessionKeyLoggerCache object to be alive for its lifetime. So it calls Ref() and Unref() methods on the cache object. We can change TlsSessionKeyLoggerCache into a RefCounted class as well. However, the object of the cache will be a static global instance. Currently its defined in ssl_key_logging.cc as per snipper below. If we are to make TlsSessionKeyLoggerCache as a RefCounted object, we would have to declare static grpc_core::RefcountedPtr<::tsi::TlsSessionKeyLoggerCache> g_cache_instance_. However this is not allowed because a static global object cannot have non default destructor. So to keep it simple, I defined TlsSessionKeyLoggerCache as a normal class with explicit Ref, Unref methods.

I see. Thank you so much for the explanation!

---

test/cpp/end2end/tls_key_export_test.cc, line 110 at r12 (raw file):

}

int CountOccurancesInFileContents(std::string file_contents,

nit: s/Occurances/Occurrences

[Vignesh2208]

Reviewable status: 22 of 42 files reviewed, 15 unresolved discussions (waiting on @ctiller, @markdroth, @Vignesh2208, @yihuazhang, and @ZhenLian)

---

include/grpc/grpc_security.h, line 1279 at r12 (raw file):

Previously, ZhenLian wrote…

nit: the comment needs to be updated as we don't have "key logging config" anymore

Updated the comment. thanks for pointing it out.

---

test/cpp/end2end/tls_key_export_test.cc, line 110 at r12 (raw file):

Previously, ZhenLian wrote…

nit: s/Occurances/Occurrences

Done.

[Vignesh2208]

@markdroth Just wanted to send a gentle reminder on this one. Could you ptal when you have some time ? Thanks!

[markdroth]

This is looking much simpler now! There are a few details to be cleaned up, but the overall structure looks very good.

Please let me know if you have any questions. Thanks!

Reviewed 9 of 35 files at r11, 6 of 24 files at r12, 20 of 20 files at r13, all commit messages.
Reviewable status: all files reviewed, 38 unresolved discussions (waiting on @ctiller, @Vignesh2208, @yihuazhang, and @ZhenLian)

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h, line 28 at r13 (raw file):

#include <grpc/grpc_security.h>

#include "src/core/lib/gpr/env.h"

I don't think this include is needed anymore.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h, line 34 at r13 (raw file):

#include "src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.h"
#include "src/core/lib/security/security_connector/ssl_utils.h"
#include "src/core/tsi/ssl/key_logging/ssl_key_logging.h"

I don't think this one is needed either.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 103 at r13 (raw file):

    grpc_tls_credentials_options* options, const char* path) {
  if (!tsi_tls_session_key_logging_supported() || options == nullptr ||
      path == nullptr) {

I think that if path is null, we don't want to return; instead, we want to set the path to the empty string. That way, the caller has a way to unset the option after setting it if they want to.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 109 at r13 (raw file):

      "grpc_tls_credentials_options_set_tls_session_key_log_config(options=%p)",
      1, (options));

Please remove unnecessary blank line.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 112 at r13 (raw file):

  // Tls session key logging is assumed to be enabled if the specified log
  // file is non-empty.
  if (strlen(path) > 0) {

I think this can just be if (path != nullptr).

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 266 at r13 (raw file):

          overridden_target_name == nullptr ? "" : overridden_target_name),
      ssl_session_cache_(ssl_session_cache) {
  if (options_ != nullptr) {

No need to check whether options_ is null. That will never happen here, because it's checked when the TlsCreds object is created.

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 267 at r13 (raw file):

      ssl_session_cache_(ssl_session_cache) {
  if (options_ != nullptr) {
    std::string tls_session_key_log_file_path =

This should be a const reference to avoid making an unnecessary copy of the string.

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 584 at r13 (raw file):

                                     std::move(server_creds)),
      options_(std::move(options)) {
  if (options_ != nullptr) {

No need to check whether options_ is null.

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 585 at r13 (raw file):

      options_(std::move(options)) {
  if (options_ != nullptr) {
    std::string tls_session_key_log_file_path =

This should be a const reference.

---

src/core/tsi/ssl_transport_security.cc, line 2009 at r13 (raw file):

#if OPENSSL_VERSION_NUMBER >= 0x10101000 && !defined(LIBRESSL_VERSION_NUMBER)
  if (options->key_logger != nullptr) {
    // Unref is manually called on factory destruction

This comment is a little misleading. We are actually storing this as a RefCountedPtr<>, not as a raw pointer, so we're not really unreffing it manually.

It's true that we are creating and destroying the object with gpr_malloc() and gpr_free() instead of new and delete, which means that we need to manually clean up the elements, but that's a separate problem. And when we eventually fix that, we will not remember to remove the comment here.

I suggest just removing this comment.

---

src/core/tsi/ssl_transport_security.cc, line 2011 at r13 (raw file):

    // Unref is manually called on factory destruction
    impl->key_logger =
        reinterpret_cast<TlsSessionKeyLogger*>(options->key_logger)->Ref();

I don't think the reinterpret_cast<> is necessary here. That field is already the right type, so you're just casting it back to its own type here.

---

src/core/tsi/ssl_transport_security.cc, line 2162 at r13 (raw file):

  if (options->key_logger != nullptr) {
    // Unref is manually called on factory destruction.

Same as above: Please remove this comment.

---

src/core/tsi/ssl_transport_security.cc, line 2164 at r13 (raw file):

    // Unref is manually called on factory destruction.
    impl->key_logger =
        reinterpret_cast<TlsSessionKeyLogger*>(options->key_logger)->Ref();

Same as above: no need for the cast.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 63 at r7 (raw file):

Previously, Vignesh2208 (Vignesh Babu) wrote…

The enums in this case would be the same. I didn't understand how we could implement a static_cast here. Could you point me to an example ? Thanks.

It looks like this is no longer relevant to this PR, but for your information, here's an example of using a static cast to ensure that two enums have the same values:

grpc/src/core/ext/xds/xds_api.h

Line 123 in 6b95e97

static_assert(static_cast<ResourceMetadata::ClientResourceStatus>(

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 49 at r10 (raw file):

Previously, ZhenLian wrote…

I see. Thank you so much for the explanation!

There is no need to use RefCountedPtr<> for the global static pointer, because that pointer should not hold a ref. Refs should be held only by active TlsSessionKeyLogger objects, and it can use RefCountedPtr<> for those refs. The global static pointer should be a raw pointer, not holding a ref, and the TlsSessionKeyLoggerCache dtor can reset the raw pointer to null.

There's no reason to reinvent the wheel here. Let's just make this use RefCounted<>.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 1 at r13 (raw file):

/*

Please use C++-style comments.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 62 at r13 (raw file):

   public:
    // Instantiates a TlsSessionKeyLogger instance bound to a specific path.
    explicit TlsSessionKeyLogger(std::string tls_session_key_log_file_path,

No need for explicit, since there are two arguments here.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 63 at r13 (raw file):

    // Instantiates a TlsSessionKeyLogger instance bound to a specific path.
    explicit TlsSessionKeyLogger(std::string tls_session_key_log_file_path,
                                 TlsSessionKeyLoggerCache* cache);

This parameter should be of type RefCountedPtr<TslSessionKeyLoggerCache>.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 77 at r13 (raw file):

   private:
    FILE* fd_;

Please add an ABSL_GUARDED_BY(lock_) annotation here, so that the compiler can enforce the lock ownership.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 80 at r13 (raw file):

    grpc_core::Mutex lock_;  // protects appends to file
    std::string tls_session_key_log_file_path_;
    TlsSessionKeyLoggerCache* cache_;

This data member should be of type RefCountedPtr<TlsSessionKeyLoggerCache>.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 1 at r13 (raw file):

/*

Please use C++-style comments.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 33 at r13 (raw file):

using TlsSessionKeyLogger = tsi::TlsSessionKeyLoggerCache::TlsSessionKeyLogger;

static gpr_once g_cache_init = GPR_ONCE_INIT;

Please use an anonymous namespace instead of declaring individual symbols as static.

Also, you can put the anonymous namespace inside of the tsi namespace, so that you don't need to say ::tsi:: in front of the types.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 42 at r13 (raw file):

  g_tls_session_key_log_cache_mu = new grpc_core::Mutex();
  grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);
  g_cache_instance_ = new ::tsi::TlsSessionKeyLoggerCache();

No need to create this here, because it will be done in TlsSessionKeyLoggerCache::Get() instead.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 47 at r13 (raw file):

namespace tsi {

TlsSessionKeyLoggerCache ::TlsSessionKeyLogger::TlsSessionKeyLogger(

Please remove the space before the ::.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 49 at r13 (raw file):

TlsSessionKeyLoggerCache ::TlsSessionKeyLogger::TlsSessionKeyLogger(
    std::string tls_session_key_log_file_path, TlsSessionKeyLoggerCache* cache)
    : fd_(nullptr),

No need to initialize this to null, since you're going to unconditionally set it to a different value below.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 51 at r13 (raw file):

    : fd_(nullptr),
      tls_session_key_log_file_path_(std::move(tls_session_key_log_file_path)),
      cache_(cache) {

Once you convert to using RefCountedPtr<> for this, you should use std::move() here.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 54 at r13 (raw file):

  GPR_ASSERT(!tls_session_key_log_file_path_.empty());
  GPR_ASSERT(cache_ != nullptr);
  cache_->Ref();

Once you convert to using RefCountedPtr<> for this, this won't be needed.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 69 at r13 (raw file):

  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);
    cache_->tls_session_key_logger_map_.erase(tls_session_key_log_file_path_);

Just as you're removing the entry from the cache here, I think you should add the entry to the cache in the ctor. That way, creation and destruction are more symmetrical, and the logic in TlsSessionKeyLoggerCache::Get() will be a bit simpler.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 71 at r13 (raw file):

    cache_->tls_session_key_logger_map_.erase(tls_session_key_log_file_path_);
  }
  cache_->Unref();

This shouldn't be needed.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 77 at r13 (raw file):

    SSL_CTX* /* ssl_context */, const std::string& session_keys_info) {
  grpc_core::MutexLock lock(&lock_);

Please remove blank lines within functions.

Same thing throughout.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 82 at r13 (raw file):

  // Append to key log file under lock
  bool err;
  err = (fwrite((session_keys_info + "\r\n").c_str(), sizeof(char),

This can be combined with the previous line.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 82 at r13 (raw file):

  // Append to key log file under lock
  bool err;
  err = (fwrite((session_keys_info + "\r\n").c_str(), sizeof(char),

No need for the outer parens here.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 99 at r13 (raw file):

TlsSessionKeyLoggerCache::~TlsSessionKeyLoggerCache() {
  grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);
  g_cache_instance_ = nullptr;

Just as you're unsetting the global pointer in the dtor, I suggest that you set it in the ctor (g_cache_instance_ = this). That way, creation and destruction will be symmetrical, and the logic in TlsSessionKeyLoggerCache::Get() can be a bit simpler.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 108 at r13 (raw file):

  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);
    if (g_cache_instance_ == nullptr) {

As part of converting the cache to use RefCountedPtr<>, I think you can inline CreateTlsSessionKeyLogger() within this method. The combined logic can look something like this:

// Create the cache if it doesn't already exist.
RefCountedPtr<TlsSessionKeyLoggerCache> cache;
if (g_cache_instance_ == nullptr) {
  // This will automatically set g_cache_instance.
  cache = MakeRefCounted<TlsSessionKeyLoggerCache>();
} else {
  cache = g_cache_instance->Ref();
}
// Check cache for entry.
auto it = cache->tls_session_key_logger_map_.find(tls_session_key_log_file_path);
if (it != cache->tls_session_key_logger_map_.end()) return it->second.Ref();
// Not found in cache, so create new entry.
// This will automatically add itself to tls_session_key_logger_map_.
return grpc_core::MakeRefCounted<TlsSessionKeyLogger>(
    std::move(tls_session_key_log_file_path), std::move(cache));
}

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 112 at r13 (raw file):

    }
    return g_cache_instance_->CreateTlsSessionKeyLogger(
        tls_session_key_log_file_path);

std::move()

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 119 at r13 (raw file):

TlsSessionKeyLoggerCache::CreateTlsSessionKeyLogger(
    std::string tls_session_key_log_file_path) {
  if (tls_session_key_log_file_path.empty()) {

This check can move into the start of TlsSessionKeyLoggerCache::Get(), before you even acquire the lock.

---

test/cpp/end2end/tls_key_export_test.cc, line 32 at r13 (raw file):

#include <grpcpp/support/channel_arguments.h>

#include "src/core/lib/gpr/env.h"

This include doesn't appear to be needed.

---

test/cpp/end2end/tls_key_export_test.cc, line 123 at r13 (raw file):

class TlsKeyLoggingEnd2EndTest : public ::testing::TestWithParam<TestScenario> {
 protected:
  TlsKeyLoggingEnd2EndTest() = default;

No need to declare this if you don't need it to do anything.

[Vignesh2208]

Thanks for taking a look. I have modified the code as per your suggestions. Could you let me know if any other changes are required. Thanks!

Reviewable status: 15 of 42 files reviewed, 38 unresolved discussions (waiting on @ctiller, @markdroth, @yihuazhang, and @ZhenLian)

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h, line 28 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think this include is needed anymore.

Done.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h, line 34 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think this one is needed either.

Done.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 103 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think that if path is null, we don't want to return; instead, we want to set the path to the empty string. That way, the caller has a way to unset the option after setting it if they want to.

Done.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 109 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please remove unnecessary blank line.

Done.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 112 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this can just be if (path != nullptr).

Replaced it with inline checking in the line options->set_tls_session_key_log_file_path

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 266 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to check whether options_ is null. That will never happen here, because it's checked when the TlsCreds object is created.

Done.

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 267 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should be a const reference to avoid making an unnecessary copy of the string.

Done.

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 584 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to check whether options_ is null.

Done.

---

src/core/lib/security/security_connector/tls/tls_security_connector.cc, line 585 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should be a const reference.

Done.

---

src/core/tsi/ssl_transport_security.cc, line 2009 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This comment is a little misleading. We are actually storing this as a RefCountedPtr<>, not as a raw pointer, so we're not really unreffing it manually.

It's true that we are creating and destroying the object with gpr_malloc() and gpr_free() instead of new and delete, which means that we need to manually clean up the elements, but that's a separate problem. And when we eventually fix that, we will not remember to remove the comment here.

I suggest just removing this comment.

Done.

---

src/core/tsi/ssl_transport_security.cc, line 2011 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think the reinterpret_cast<> is necessary here. That field is already the right type, so you're just casting it back to its own type here.

Done.

---

src/core/tsi/ssl_transport_security.cc, line 2162 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same as above: Please remove this comment.

Done.

---

src/core/tsi/ssl_transport_security.cc, line 2164 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same as above: no need for the cast.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 63 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

It looks like this is no longer relevant to this PR, but for your information, here's an example of using a static cast to ensure that two enums have the same values:

grpc/src/core/ext/xds/xds_api.h

Line 123 in 6b95e97

static_assert(static_cast<ResourceMetadata::ClientResourceStatus>(

Thanks for the info

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 49 at r10 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

There is no need to use RefCountedPtr<> for the global static pointer, because that pointer should not hold a ref. Refs should be held only by active TlsSessionKeyLogger objects, and it can use RefCountedPtr<> for those refs. The global static pointer should be a raw pointer, not holding a ref, and the TlsSessionKeyLoggerCache dtor can reset the raw pointer to null.

There's no reason to reinvent the wheel here. Let's just make this use RefCounted<>.

Thanks for pointing it out. I hadn't thought of that. I made the changes as per your suggestions.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 1 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please use C++-style comments.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 62 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need for explicit, since there are two arguments here.

Removed

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 63 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This parameter should be of type RefCountedPtr<TslSessionKeyLoggerCache>.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 77 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please add an ABSL_GUARDED_BY(lock_) annotation here, so that the compiler can enforce the lock ownership.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.h, line 80 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This data member should be of type RefCountedPtr<TlsSessionKeyLoggerCache>.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 1 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please use C++-style comments.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 33 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please use an anonymous namespace instead of declaring individual symbols as static.

Also, you can put the anonymous namespace inside of the tsi namespace, so that you don't need to say ::tsi:: in front of the types.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 42 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to create this here, because it will be done in TlsSessionKeyLoggerCache::Get() instead.

I moved it to the ::Get() method.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 47 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please remove the space before the ::.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 49 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to initialize this to null, since you're going to unconditionally set it to a different value below.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 51 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Once you convert to using RefCountedPtr<> for this, you should use std::move() here.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 54 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Once you convert to using RefCountedPtr<> for this, this won't be needed.

Removed.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 69 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Just as you're removing the entry from the cache here, I think you should add the entry to the cache in the ctor. That way, creation and destruction are more symmetrical, and the logic in TlsSessionKeyLoggerCache::Get() will be a bit simpler.

I put this logic (inserting into the map) now in the constructor

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 71 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This shouldn't be needed.

Removed.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 77 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please remove blank lines within functions.

Same thing throughout.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 82 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can be combined with the previous line.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 82 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need for the outer parens here.

Removed

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 99 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Just as you're unsetting the global pointer in the dtor, I suggest that you set it in the ctor (g_cache_instance_ = this). That way, creation and destruction will be symmetrical, and the logic in TlsSessionKeyLoggerCache::Get() can be a bit simpler.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 108 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

As part of converting the cache to use RefCountedPtr<>, I think you can inline CreateTlsSessionKeyLogger() within this method. The combined logic can look something like this:

// Create the cache if it doesn't already exist.
RefCountedPtr<TlsSessionKeyLoggerCache> cache;
if (g_cache_instance_ == nullptr) {
  // This will automatically set g_cache_instance.
  cache = MakeRefCounted<TlsSessionKeyLoggerCache>();
} else {
  cache = g_cache_instance->Ref();
}
// Check cache for entry.
auto it = cache->tls_session_key_logger_map_.find(tls_session_key_log_file_path);
if (it != cache->tls_session_key_logger_map_.end()) return it->second.Ref();
// Not found in cache, so create new entry.
// This will automatically add itself to tls_session_key_logger_map_.
return grpc_core::MakeRefCounted<TlsSessionKeyLogger>(
    std::move(tls_session_key_log_file_path), std::move(cache));
}

I modified the logic to be like this.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 112 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

std::move()

This function is now removed entirely.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 119 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This check can move into the start of TlsSessionKeyLoggerCache::Get(), before you even acquire the lock.

Done.

---

test/cpp/end2end/tls_key_export_test.cc, line 32 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This include doesn't appear to be needed.

Done.

---

test/cpp/end2end/tls_key_export_test.cc, line 123 at r13 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to declare this if you don't need it to do anything.

Removed.

[markdroth]

This is very close now! There's only one significant remaining issue, which is the one about the race condition.

Please let me know if you have any questions. Thanks!

Reviewed 22 of 27 files at r14, 5 of 8 files at r15, 3 of 3 files at r16, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ctiller, @Vignesh2208, @yihuazhang, and @ZhenLian)

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 117 at r16 (raw file):

  // file is non-empty.
  gpr_log(GPR_INFO, "Enabling TLS session key logging with keys stored at: %s",
          path ? path : "");

Please use path != nullptr to emphasize to the reader that this is a pointer, not a bool.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 117 at r16 (raw file):

  // file is non-empty.
  gpr_log(GPR_INFO, "Enabling TLS session key logging with keys stored at: %s",
          path ? path : "");

If path is null, we should log something like "" instead of the empty string.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 118 at r16 (raw file):

  gpr_log(GPR_INFO, "Enabling TLS session key logging with keys stored at: %s",
          path ? path : "");
  options->set_tls_session_key_log_file_path(path ? path : "");

Please use path != nullptr to emphasize to the reader that this is a pointer, not a bool.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 60 at r16 (raw file):

            grpc_error_std_string(error).c_str());
  }
  cache_->tls_session_key_logger_map_.insert(

I think this can be written as:

cache_->tls_session_key_logger_map_.emplace(tls_session_key_log_file_path_, this);

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 66 at r16 (raw file):

TlsSessionKeyLoggerCache::TlsSessionKeyLogger::~TlsSessionKeyLogger() {
  grpc_core::MutexLock lock(&lock_);

I suggest putting these two lines in their own scope, just so that we release the lock in this key logger before we acquire the global lock. (Shouldn't actually matter in this case, but in general it's better to be defensive and avoid potential lock inversion problems.)

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 69 at r16 (raw file):

  if (fd_ != nullptr) fclose(fd_);
  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);

I just realized that there's a race condition here. Consider the following sequence of events:

1. Thread 1 drops the last ref to a key logger. Ref-count goes to zero and the dtor is invoked.
2. Thread 2 calls TlsSessionKeyLoggerCache::Get() for the same path. It acquires the global lock, finds the entry in the map, and calls Ref() on it. This increases the ref-count from 0 back to 1, and the caller thinks that it is holding a ref to an existing key logger.
3. Thread 1, still in the dtor, now acquires the global lock and removes the entry. The dtor now returns and the object is destroyed.

This becomes a use-after-free bug.

I think the right solution is to change TlsSessionKeyLoggerCache::Get() to use RefIfNonZero() instead of Ref(). Instead of this:

      return it->second->Ref();

We can say this:

      auto key_logger = it->second->RefIfNonZero();
      if (key_logger != nullptr) return key_logger;

Then the code here in the TlsSessionKeyLogger dtor can remove the cache entry only if it still points to this object:

auto it = cache_->tls_session_key_logger_map_.find(tls_session_key_log_file_path_);
if (it != cache_->tls_session_key_logger_map_.end() &&
    it->second == this) {
  cache_->tls_session_key_logger_map_.erase(it);
}

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 95 at r16 (raw file):

TlsSessionKeyLoggerCache::TlsSessionKeyLoggerCache() {
  // constructor is already called under the lock g_tls_session_key_log_cache_mu

You can use a lock annotation to have the compiler enforce this.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 110 at r16 (raw file):

    return nullptr;
  }
  GPR_DEBUG_ASSERT(g_tls_session_key_log_cache_mu != nullptr);

This should probably move up to be right after the gpr_once_init(), since it's verifying the result of that call.

[Vignesh2208]

Thanks for taking a look. I addressed your comments.

Reviewable status: 21 of 42 files reviewed, 10 unresolved discussions (waiting on @ctiller, @markdroth, @yihuazhang, and @ZhenLian)

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 117 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please use path != nullptr to emphasize to the reader that this is a pointer, not a bool.

Done.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 117 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

If path is null, we should log something like "" instead of the empty string.

Done.

---

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc, line 118 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please use path != nullptr to emphasize to the reader that this is a pointer, not a bool.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 60 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this can be written as:

cache_->tls_session_key_logger_map_.emplace(tls_session_key_log_file_path_, this);

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 66 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I suggest putting these two lines in their own scope, just so that we release the lock in this key logger before we acquire the global lock. (Shouldn't actually matter in this case, but in general it's better to be defensive and avoid potential lock inversion problems.)

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 69 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I just realized that there's a race condition here. Consider the following sequence of events:

1. Thread 1 drops the last ref to a key logger. Ref-count goes to zero and the dtor is invoked.
2. Thread 2 calls TlsSessionKeyLoggerCache::Get() for the same path. It acquires the global lock, finds the entry in the map, and calls Ref() on it. This increases the ref-count from 0 back to 1, and the caller thinks that it is holding a ref to an existing key logger.
3. Thread 1, still in the dtor, now acquires the global lock and removes the entry. The dtor now returns and the object is destroyed.

This becomes a use-after-free bug.

I think the right solution is to change TlsSessionKeyLoggerCache::Get() to use RefIfNonZero() instead of Ref(). Instead of this:

      return it->second->Ref();

We can say this:

      auto key_logger = it->second->RefIfNonZero();
      if (key_logger != nullptr) return key_logger;

Then the code here in the TlsSessionKeyLogger dtor can remove the cache entry only if it still points to this object:

auto it = cache_->tls_session_key_logger_map_.find(tls_session_key_log_file_path_);
if (it != cache_->tls_session_key_logger_map_.end() &&
    it->second == this) {
  cache_->tls_session_key_logger_map_.erase(it);
}

Thanks for catching this. Fixed it as per your suggestions

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 95 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

You can use a lock annotation to have the compiler enforce this.

Done.

---

src/core/tsi/ssl/key_logging/ssl_key_logging.cc, line 110 at r16 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should probably move up to be right after the gpr_once_init(), since it's verifying the result of that call.

Done.

[markdroth]

This looks great!

Reviewed 21 of 21 files at r17, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ctiller, @yihuazhang, and @ZhenLian)