Macos system roots implementation by maliksaafir · Pull Request #16246 · grpc/grpc

maliksaafir · 2018-08-04T00:28:55Z

This implements the utilization of MacOS system root certificates. It's currently missing the implementation of a feature to remove distrusted certs from the cert pool gathered by the code.

grpc-testing · 2018-08-06T17:34:56Z

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

libgrpc.so

     VM SIZE                                                                           FILE SIZE
 ++++++++++++++ GROWING                                                             ++++++++++++++
  +0.3% +1.89Ki [None]                                                              +28.0Ki  +0.3%
      +0.3% +1.92Ki [Unmapped]                                                          +28.0Ki  +0.3%
      [NEW]     +16 CSWTCH.83                                                               +16  [NEW]
  [NEW] +1.31Ki src/core/lib/security/security_connector/load_system_roots_linux.cc +1.31Ki  [NEW]
      [NEW]    +773 grpc_core::CreateRootCertsBundle                                       +773  [NEW]
      [NEW]    +225 grpc_core::LoadSystemRootCerts                                         +225  [NEW]
      [NEW]    +170 grpc_core::GetSystemRootCerts                                          +170  [NEW]
      [NEW]     +68 grpc_core::GetAbsoluteFilePath                                          +68  [NEW]
      [NEW]     +40 grpc_core::linux_cert_files_                                            +40  [NEW]
      [NEW]     +40 grpc_core::linux_cert_directories_                                      +40  [NEW]
      [NEW]     +29 [Unmapped]                                                              +29  [NEW]
  +1.6%    +160 src/core/lib/security/security_connector/security_connector.cc         +160  +1.6%
       +38%    +168 grpc_core::DefaultSslRootStore::ComputePemRootCerts                    +168   +38%

  +0.2% +3.36Ki TOTAL                                                               +29.5Ki  +0.3%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-08-06T17:43:05Z

[trickle] No significant performance differences

grpc-testing · 2018-08-06T18:26:59Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-08-09T22:07:57Z

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

libgrpc.so

     VM SIZE                                                                           FILE SIZE
 ++++++++++++++ GROWING                                                             ++++++++++++++
  +0.2% +1.79Ki [None]                                                              +27.9Ki  +0.3%
      +0.3% +1.71Ki [Unmapped]                                                          +27.8Ki  +0.3%
      [NEW]     +40 grpc_core::(anonymous namespace)::kLinuxCertFiles                       +40  [NEW]
      [NEW]     +40 grpc_core::(anonymous namespace)::kLinuxCertDirectories                 +40  [NEW]
      [NEW]     +16 CSWTCH.83                                                               +16  [NEW]
  [NEW] +1.43Ki src/core/lib/security/security_connector/load_system_roots_linux.cc +1.43Ki  [NEW]
      [NEW]    +921 grpc_core::CreateRootCertsBundle                                       +921  [NEW]
      [NEW]    +426 grpc_core::LoadSystemRootCerts                                         +426  [NEW]
      [NEW]    +105 grpc_core::GetAbsoluteFilePath                                         +105  [NEW]
      [NEW]     +14 [Unmapped]                                                              +14  [NEW]
  +1.6%    +160 src/core/lib/security/security_connector/security_connector.cc         +160  +1.6%
       +38%    +168 grpc_core::DefaultSslRootStore::ComputePemRootCerts                    +168   +38%

  +0.2% +3.38Ki TOTAL                                                               +29.5Ki  +0.3%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-08-09T22:14:54Z

[trickle] No significant performance differences

grpc-testing · 2018-08-09T22:59:12Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-08-10T00:01:37Z

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

libgrpc.so

     VM SIZE                                                                           FILE SIZE
 ++++++++++++++ GROWING                                                             ++++++++++++++
  +0.2% +1.79Ki [None]                                                              +27.9Ki  +0.3%
      +0.3% +1.71Ki [Unmapped]                                                          +27.8Ki  +0.3%
      [NEW]     +40 grpc_core::(anonymous namespace)::kLinuxCertFiles                       +40  [NEW]
      [NEW]     +40 grpc_core::(anonymous namespace)::kLinuxCertDirectories                 +40  [NEW]
      [NEW]     +16 CSWTCH.83                                                               +16  [NEW]
  [NEW] +1.43Ki src/core/lib/security/security_connector/load_system_roots_linux.cc +1.43Ki  [NEW]
      [NEW]    +921 grpc_core::CreateRootCertsBundle                                       +921  [NEW]
      [NEW]    +426 grpc_core::LoadSystemRootCerts                                         +426  [NEW]
      [NEW]    +105 grpc_core::GetAbsoluteFilePath                                         +105  [NEW]
      [NEW]     +14 [Unmapped]                                                              +14  [NEW]
  +1.6%    +160 src/core/lib/security/security_connector/security_connector.cc         +160  +1.6%
       +38%    +168 grpc_core::DefaultSslRootStore::ComputePemRootCerts                    +168   +38%

  +0.2% +3.38Ki TOTAL                                                               +29.5Ki  +0.3%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-08-10T00:07:50Z

[trickle] No significant performance differences

grpc-testing · 2018-08-10T00:54:09Z

[microbenchmarks] No significant performance differences

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.

- Removed instances of d_type, now using POSIX-compliant stat; - Not including dirent.h on Windows anymore.

- Now using suggested structure to handle platform specificity; - Addressed first wave of review comments and suggestions.

- should fix build errors on non-linux systems - slight formatting

- Fixed wrong include statement that was causing Windows failures - Added system roots flag to grpc_security_constants.h - Removed MacOS and Windows references, focusing on Linux now - Removed old unit test from security_connector_test.cc - Regenerated project files and added new files to BUILD, which should address Bazel build failures.

- One of the unit tests introduced in security_connector_test is now Linux-specific as a way to temporarily address the issue while we investigate its failure on MacOS. - Minor comment improvements.

- add missing gpr_frees and grpc_slice_unrefs

gpr_getenv() was causing memory leaks. Freed variables accordingly. After a quick check, valgrind does not show any memory leaks now.

- slight formatting, reformatting - finish testing TODO

- fix fallback preprocessor check - remove duplicate build rule

grpc-testing · 2018-08-10T17:37:16Z

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

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]    +576  +0.0%

  [ = ]       0 TOTAL     +576  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-08-10T17:44:28Z

[trickle] No significant performance differences

- TODO for compatibility with iOS Security library

grpc-testing · 2018-08-10T18:28:53Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-08-10T20:04:50Z

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

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]    +576  +0.0%

  [ = ]       0 TOTAL     +576  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-08-10T20:12:24Z

[trickle] No significant performance differences

grpc-testing · 2018-08-10T20:56:34Z

[microbenchmarks] No significant performance differences

yang-g · 2019-02-12T20:31:47Z

Seems obsolete, closing.

ywh233 · 2019-03-06T23:14:22Z

I wonder why this is closed without merging. Is there still a plan to use system trust store on Mac?

nicolasnoble · 2019-03-07T00:16:30Z

This is mainly a cleanup-close. Code that has been rotting since 8+ months probably needs revisiting.

justinburke added the kokoro:force-run label Aug 6, 2018

kokoro-team removed the kokoro:force-run label Aug 6, 2018

maliksaafir force-pushed the macos-system-roots-clean-history branch from 6593233 to e10f67b Compare August 8, 2018 00:27

maliksaafir changed the title ~~[WIP] Macos system roots implementation~~ Macos system roots implementation Aug 8, 2018

maliksaafir mentioned this pull request Aug 8, 2018

MacOS System Root Certificates Feature Needs Filtering #16286

Closed

justinburke added the kokoro:force-run label Aug 9, 2018

kokoro-team removed the kokoro:force-run label Aug 9, 2018

justinburke added the kokoro:force-run label Aug 9, 2018

kokoro-team removed the kokoro:force-run label Aug 9, 2018

maliksaafir force-pushed the macos-system-roots-clean-history branch from 361ffcf to 562a1e5 Compare August 10, 2018 00:24

maliksaafir force-pushed the macos-system-roots-clean-history branch from 562a1e5 to f409732 Compare August 10, 2018 15:57

tdbhacks and others added 10 commits August 10, 2018 10:03

Added system roots feature to load roots from OS trust store

86368f4

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.

Handling fread return value

3fbe00d

Initial build failures fixes

1e1f4eb

- Removed instances of d_type, now using POSIX-compliant stat; - Not including dirent.h on Windows anymore.

Restructured PR to handle platform specificity + other fixes

190574f

- Now using suggested structure to handle platform specificity; - Addressed first wave of review comments and suggestions.

namespace fix

763a672

- should fix build errors on non-linux systems - slight formatting

System roots unit test now Linux-specific

50f5dd3

- One of the unit tests introduced in security_connector_test is now Linux-specific as a way to temporarily address the issue while we investigate its failure on MacOS. - Minor comment improvements.

memory leak fixes

dcfd6d5

- add missing gpr_frees and grpc_slice_unrefs

remove const, was failing build on linux

687f52b

More memory leaks fixed

b8d6db1

gpr_getenv() was causing memory leaks. Freed variables accordingly. After a quick check, valgrind does not show any memory leaks now.

maliksaafir added 9 commits August 10, 2018 10:12

format fix

d431154

remove comments

0d5b45a

refactoring + new test

0b82ca8

- slight formatting, reformatting - finish testing TODO

formatting

44ed409

include code from golang core

e4831fc

add linker options for required Apple frameworks

37cb9e8

implement MacOS LoadSystemRoots function

9aad42e

add test for macos system roots

3ed6324

- fix fallback preprocessor check - remove duplicate build rule

reference to gRPC issue grpc#16286

fd40338

maliksaafir force-pushed the macos-system-roots-clean-history branch from f409732 to fd40338 Compare August 10, 2018 17:12

justinburke added the kokoro:force-run label Aug 10, 2018

kokoro-team removed the kokoro:force-run label Aug 10, 2018

avoid building on iOS due to library discrepancies

458b881

- TODO for compatibility with iOS Security library

justinburke added the kokoro:force-run label Aug 10, 2018

kokoro-team removed the kokoro:force-run label Aug 10, 2018

direct iOS builds to the fallback

8ee0624

sreecha requested a review from jiangtaoli2016 August 16, 2018 23:40

coryan mentioned this pull request Sep 6, 2018

Use Windows trust store by default. #16571

Closed

apolcyn assigned jiangtaoli2016 Oct 3, 2018

yang-g closed this Feb 12, 2019

lock Bot locked as resolved and limited conversation to collaborators Jun 5, 2019

Conversation

maliksaafir commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grpc-testing commented Aug 6, 2018

Uh oh!

grpc-testing commented Aug 6, 2018

Uh oh!

grpc-testing commented Aug 6, 2018

Uh oh!

grpc-testing commented Aug 9, 2018

Uh oh!

grpc-testing commented Aug 9, 2018

Uh oh!

grpc-testing commented Aug 9, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

grpc-testing commented Aug 10, 2018

Uh oh!

yang-g commented Feb 12, 2019

Uh oh!

ywh233 commented Mar 6, 2019

Uh oh!

nicolasnoble commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

maliksaafir commented Aug 4, 2018 •

edited

Loading