Added system roots feature to load roots from OS trust store #16083

tdbhacks · 2018-07-20T23:46:31Z

Added a flag-guarded feature that allows gRPC to load TLS/SSL
roots from the OS trust store. This is the Linux-specific
implementation of such feature.

This change is

jiangtaoli2016 · 2018-07-20T23:51:09Z

Thanks @tdbhacks for this PR.
@yihuazhang Could you please take the first pass?
@markdroth Could you please check grpc coding standards?
I will be out of office next week.

grpc-testing · 2018-07-21T00:15:19Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   668,635       Core (>)        666,642

 1,978,398      Total (>)      1,976,225

***************FRAMEWORKS****************
  New size                      Old size
 3,612,130       Core (>)      3,609,774

10,772,783      Total (>)     10,770,427

grpc-testing · 2018-07-21T00:42:27Z

****************************************************************

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.4% +2.64Ki [None]                                                         +12.4Ki  +0.1%
      +0.4% +2.59Ki [Unmapped]                                                     +12.4Ki  +0.1%
      +6.0%     +60 [None]                                                               0  [ = ]
      [NEW]     +16 CSWTCH.93                                                          +16  [NEW]
   +18% +1.74Ki src/core/lib/security/security_connector/security_connector.cc +1.73Ki   +18%
      [NEW]    +487 grpc_core::DefaultSslRootStore::CreateRootCertsBundle             +487  [NEW]
       +85%    +373 grpc_core::DefaultSslRootStore::ComputePemRootCerts               +373   +85%
      [NEW]    +328 grpc_core::DefaultSslRootStore::GetDirectoryTotalSize             +328  [NEW]
      [NEW]    +207 grpc_core::DefaultSslRootStore::GetAbsoluteFilePath               +207  [NEW]
      [NEW]    +172 grpc_core::DefaultSslRootStore::GetSystemRootCerts                +172  [NEW]
      [NEW]     +87 grpc_core::DefaultSslRootStore::GetValidCertsDirectory             +87  [NEW]
      [NEW]     +40 grpc_core::DefaultSslRootStore::linux_cert_files_                  +40  [NEW]
      [NEW]     +40 grpc_core::DefaultSslRootStore::linux_cert_directories_            +40  [NEW]
      +5.0%     +28 [Unmapped]                                                         +28  +5.0%
      [NEW]     +14 grpc_core::DefaultSslRootStore::DetectPlatform                     +14  [NEW]
      [NEW]      +4 grpc_core::DefaultSslRootStore::platform_                            0  [ = ]

  +0.3% +4.38Ki TOTAL                                                          +14.1Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-07-21T00:50:23Z

[trickle] No significant performance differences

grpc-testing · 2018-07-21T01:04:15Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   668,693       Core (>)        666,642

 1,978,456      Total (>)      1,976,225

***************FRAMEWORKS****************
  New size                      Old size
 3,612,130       Core (>)      3,609,774

10,772,781      Total (>)     10,770,426

grpc-testing · 2018-07-21T01:34:54Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-07-23T17:50:45Z

****************************************************************

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.4% +2.77Ki [None]                                                         +13.3Ki  +0.1%
      +0.4% +2.74Ki [Unmapped]                                                     +13.3Ki  +0.1%
      +2.8%     +28 [None]                                                               0  [ = ]
      [NEW]     +16 CSWTCH.95                                                          +16  [NEW]
   +19% +1.91Ki src/core/lib/security/security_connector/security_connector.cc +1.91Ki   +19%
      [NEW]    +577 grpc_core::DefaultSslRootStore::CreateRootCertsBundle             +577  [NEW]
      [NEW]    +416 grpc_core::DefaultSslRootStore::GetDirectoryTotalSize             +416  [NEW]
       +85%    +373 grpc_core::DefaultSslRootStore::ComputePemRootCerts               +373   +85%
      [NEW]    +207 grpc_core::DefaultSslRootStore::GetAbsoluteFilePath               +207  [NEW]
      [NEW]    +172 grpc_core::DefaultSslRootStore::GetSystemRootCerts                +172  [NEW]
      [NEW]     +87 grpc_core::DefaultSslRootStore::GetValidCertsDirectory             +87  [NEW]
      [NEW]     +40 grpc_core::DefaultSslRootStore::linux_cert_files_                  +40  [NEW]
      [NEW]     +40 grpc_core::DefaultSslRootStore::linux_cert_directories_            +40  [NEW]
      +4.7%     +26 [Unmapped]                                                         +26  +4.7%
      [NEW]     +14 grpc_core::DefaultSslRootStore::DetectPlatform                     +14  [NEW]
      [NEW]      +4 grpc_core::DefaultSslRootStore::platform_                            0  [ = ]

  +0.3% +4.68Ki TOTAL                                                          +15.2Ki  +0.2%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-07-23T17:59:19Z

[trickle] No significant performance differences

grpc-testing · 2018-07-23T18:13:33Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
   668,765       Core (>)        666,642

 1,978,560      Total (>)      1,976,225

***************FRAMEWORKS****************
  New size                      Old size
 3,612,178       Core (>)      3,609,774

10,772,828      Total (>)     10,770,425

grpc-testing · 2018-07-23T18:42:28Z

[microbenchmarks] No significant performance differences

markdroth

My main concern here is that the platform-specificity is being handled the wrong way. The way this PR is now, it won't even build on non-Linux platforms.

Please let me know if you have any questions.

Reviewed 7 of 8 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @tdbhacks and @yihuazhang)

src/core/lib/security/security_connector/security_connector.h, line 45 at r3 (raw file):

  PLATFORM_WINDOWS,
  PLATFORM_TEST
} grpc_platform;

This enum doesn't make sense. We already know what platform we're built on, so let's just compile in the right implementation for the platform. There should be no need for callers to tell us this at run-time.

For an example of how to do this, see:

https://github.com/grpc/grpc/blob/master/src/core/lib/iomgr/gethostname.h

The API is the same on all platforms, but there's a different file to implement it for each platform:

https://github.com/grpc/grpc/blob/master/src/core/lib/iomgr/gethostname_host_name_max.cc
https://github.com/grpc/grpc/blob/master/src/core/lib/iomgr/gethostname_sysconf.cc

And there's a default implementation used if we don't have one for the platform:

https://github.com/grpc/grpc/blob/master/src/core/lib/iomgr/gethostname_fallback.cc

The implementations are selected via the GRPC_POSIX_SYSCONF or GRPC_POSIX_HOST_NAME_MAX preprocessor macros, which are defined here:

https://github.com/grpc/grpc/blob/master/src/core/lib/iomgr/port.h

I think we should use a similar structure for the feature in this PR.

src/core/lib/security/security_connector/security_connector.h, line 275 at r3 (raw file):

  // Returns a grpc_slice containing OS-specific root certificates.
  // Protected for testing.
  static grpc_slice GetSystemRootCerts();

None of these methods should be present here. Instead, there should be a single interface called something like LoadSystemRootCerts(), with different implementations for each platform. That function can then be called from the code here.

src/core/lib/security/security_connector/security_connector.h, line 326 at r3 (raw file):

  // List of possible Linux certificate files and directories.
  static const char* linux_cert_files_[];

These three constants should be in a platform-specific file.

src/core/lib/security/security_connector/security_connector.cc, line 64 at r3 (raw file):

#endif

/* --- Flag to enable/disable system root certificates feature --- */

Is it the intent that if you want to disable this, you define GRPC_USE_SYSTEM_SSL_ROOTS to some other environment variable name that is not expected to actually be defined? If so, this seems very cumbersome. It's also not guaranteed to work, because you have no guarantee that any other environment variable name does not actually exist.

Instead, I suggest having an environment variable whose value is a bool to enable or disable this behavior. We will always check this environment variable. Initially, it can default to off, so people need to opt in -- if it's not set, nothing changes. Later, when we're confident this is working right, we can consider changing the default to on, so that people need to explicitly opt out.

You can evaluate the boolean value with gpr_is_true(), defined here:

https://github.com/grpc/grpc/blob/master/src/core/lib/gpr/string.h#L117

src/core/lib/security/security_connector/security_connector.cc, line 1261 at r3 (raw file):

    case PLATFORM_LINUX: {
      grpc_slice valid_bundle_slice = grpc_empty_slice();
      size_t num_cert_files_ = sizeof(DefaultSslRootStore::linux_cert_files_);

I think you want to use GPR_ARRAY_SIZE() here.

src/core/lib/security/security_connector/security_connector.cc, line 1286 at r3 (raw file):

  // TODO: this function can return a slice to avoid opening the dir twice.
  DIR* directory;
  const char* custom_dir = gpr_getenv(GRPC_SYSTEM_SSL_ROOTS_DIR);

Why are we reading this env var again, if we've already done so above?

src/core/lib/security/security_connector/security_connector.cc, line 1292 at r3 (raw file):

  size_t num_cert_dirs_ = sizeof(DefaultSslRootStore::linux_cert_directories_);
  for (size_t i = 0; i < num_cert_dirs_; i++) {
    directory = opendir(linux_cert_directories_[i]);

It's cheaper to stat() and then use S_ISDIR() on the result than it is to actually open the directory.

src/core/lib/security/security_connector/security_connector.cc, line 1308 at r3 (raw file):

  size_t valid_file_dir_len = strlen(valid_file_dir);
  size_t file_name_len = strlen(file_entry_name);
  char* absolute_path =

Why not use gpr_asprintf() here?

Or better yet, create a buffer of size MAXPATHLEN and write into that with snprintf(), so that we don't need to do any allocation.

src/core/lib/security/security_connector/security_connector.cc, line 1313 at r3 (raw file):

  strcat(absolute_path, "/");
  strcat(absolute_path, file_entry_name);
  return grpc_slice_new(absolute_path, valid_file_dir_len + file_name_len + 2,

Why return this as a slice? Seems like a string would work fine, since that's the form you actually need to use it in.

src/core/lib/security/security_connector/security_connector.cc, line 1327 at r3 (raw file):

    struct stat dir_entry_stat;
    const char* file_entry_name = directory_entry->d_name;
    file_path = GetAbsoluteFilePath(directory_path, file_entry_name);

We're creating a new slice in each iteration through the loop, but only unreffing it after we exit the loop. This is a memory leak for all but the last slice we see.

src/core/lib/security/security_connector/security_connector.cc, line 1328 at r3 (raw file):

    const char* file_entry_name = directory_entry->d_name;
    file_path = GetAbsoluteFilePath(directory_path, file_entry_name);
    int stat_return = lstat(grpc_slice_to_c_string(file_path), &dir_entry_stat);

This is another memory leak: we are not freeing the value returned from grpc_slice_to_c_string().

src/core/lib/security/security_connector/security_connector.cc, line 1335 at r3 (raw file):

      continue;
    }
    cert_file = fopen(grpc_slice_to_c_string(file_path), "rb");

Why open the file and seek to the end instead of just looking at dir_entry_stat.st_size?

src/core/lib/security/security_connector/security_connector.cc, line 1366 at r3 (raw file):

    struct stat dir_entry_stat;
    const char* file_entry_name = directory_entry->d_name;
    file_path = GetAbsoluteFilePath(found_cert_dir, file_entry_name);

Same comments as above regarding the memory leaks on this line and the next one.

src/core/lib/security/security_connector/security_connector.cc, line 1374 at r3 (raw file):

      continue;
    }
    cert_file = fopen(grpc_slice_to_c_string(file_path), "rb");

Since you're not trying to do any line buffering here, stdio doesn't buy you anything. Suggest using the unix file API directly (i.e., open(2) and read(2) instead of fopen(3) and fread(3)).

src/core/lib/security/security_connector/security_connector.cc, line 1377 at r3 (raw file):

    if (cert_file != nullptr) {
      // Read file into bundle.
      fseek(cert_file, 0, SEEK_END);

As above, no need to seek to the end to get the size; just use dir_entry_stat.st_size.

tdbhacks · 2018-07-23T21:22:44Z

Yes, we figured that out as soon as we sent the PR. Thank you for pointing us in the right direction, we will take a look into what you provided and change our implementation accordingly. Thanks!

yihuazhang · 2018-07-24T22:52:23Z

@tdbhacks, here is another example (i.e., check gcp environment) of a single-interface with multiple platform-specific implementations that Mark pointed out.

Interface: grpc_alts_is_running_on_gcp() https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/alts/check_gcp_environment.h#L55
Linux-specific implementation: https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/alts/check_gcp_environment_linux.cc
Windows-specific implementation: https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/alts/check_gcp_environment_linux.cc
no-op implementation: https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/alts/check_gcp_environment_linux.cc

You could also use GPR_LINUX, GPR_WINDOWS, and GPR_APPLE macros to identify a corresponding platform.

yihuazhang

My major concern is that it is not clear to me how you select a root cert when multiple sources are available for it. Currently, four sources are available: 1) a default root cert directory, 2) custom root cert directory, 3) loading from OS, and 4) load from a gRPC source. I think we need to have a clear justification on their selection orders. If the goal is to get an up-to-date cert, shouldn't we retrieve the certs from all sources and compare their timestamps?

Other things that worth consideration:

when reading certs from OS, there seems existing multiple sources and the current implementation simply takes the first one that is readable which does not make sense to me. Are they guaranteed to have same contents always?
Have you considered caching the root cert that has been previously retrieved?

tdbhacks

To answer the first one of your two questions, we only care about the first valid source because the list of directories we have is simply a list of places in which the OS could be storing root certs. This is distribution-specific, so we don't expect a specific distribution to be storing its root certs in more than one location. Regarding timestamps and caching, those are good points. I guess our assumption was that the OS (at least as far as Linux is concerned) is always going to have the most updated trust store. But it's something worth discussing, thanks for pointing this out.

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @tdbhacks, @markdroth, and @jiangtaoli2016)

src/core/lib/security/security_connector/security_connector.h, line 45 at r3 (raw file):