Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Macos system roots implementation #16246

Closed
wants to merge 36 commits into from
Closed

Macos system roots implementation #16246

wants to merge 36 commits into from

Conversation

maliksaafir
Copy link

@maliksaafir maliksaafir commented Aug 4, 2018 •
edited
Loading

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
Copy link 

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

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
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@maliksaafir maliksaafir force-pushed the macos-system-roots-clean-history branch from 6593233 to e10f67b Compare August 8, 2018 00:27
@maliksaafir maliksaafir changed the title [WIP] Macos system roots implementation Macos system roots implementation Aug 8, 2018
@maliksaafir maliksaafir mentioned this pull request Aug 8, 2018
Closed
@grpc-testing
Copy link 

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

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
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

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
Copy link 

[trickle] No significant performance differences

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

[microbenchmarks] No significant performance differences

@maliksaafir 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
@tdbhacks @maliksaafir
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.
@tdbhacks @maliksaafir
Handling fread return value
3fbe00d
@tdbhacks @maliksaafir
Initial build failures fixes
1e1f4eb 
- Removed instances of d_type, now using POSIX-compliant stat;
- Not including dirent.h on Windows anymore.
@tdbhacks @maliksaafir
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.
@maliksaafir
namespace fix
763a672 
- should fix build errors on non-linux systems
- slight formatting
@tdbhacks @maliksaafir
Various fixes, more to come
2fc6b04 
- 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.
@tdbhacks @maliksaafir
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.
@maliksaafir
memory leak fixes
dcfd6d5 
- add missing gpr_frees and grpc_slice_unrefs
@maliksaafir
remove const, was failing build on linux
687f52b
@tdbhacks @maliksaafir
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 maliksaafir force-pushed the macos-system-roots-clean-history branch from f409732 to fd40338 Compare August 10, 2018 17:12
@grpc-testing
Copy link 

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

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
Copy link 

[trickle] No significant performance differences

@maliksaafir
avoid building on iOS due to library discrepancies
458b881 
- TODO for compatibility with iOS Security library
@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

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
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@coryan coryan mentioned this pull request Sep 6, 2018
Closed
@yang-g
Copy link
Member

yang-g commented Feb 12, 2019

Seems obsolete, closing.

@yang-g yang-g closed this Feb 12, 2019
@ywh233
Copy link
Contributor

ywh233 commented Mar 6, 2019

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

@nicolasnoble
Copy link
Member

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

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jiangtaoli2016 jiangtaoli2016 Awaiting requested review from jiangtaoli2016

Assignees

@jiangtaoli2016 jiangtaoli2016

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@maliksaafir @grpc-testing @yang-g @ywh233 @nicolasnoble @justinburke @jiangtaoli2016 @kokoro-team @tdbhacks