Create verify_peer_options when creating ssl credentials to support a peer verification callback by JackOfMostTrades · Pull Request #15274 · grpc/grpc

JackOfMostTrades · 2018-05-04T16:19:05Z

This PR is split from #12656 to include just the updates to core components.

grpc-testing · 2018-05-04T16:57:35Z

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%    +160 [None]                                                         +1.73Ki  +0.0%
      +0.0%    +160 [Unmapped]                                                     +1.73Ki  +0.0%
      [NEW]     +16 CSWTCH.81                                                          +16  [NEW]
  +3.5%    +344 src/core/lib/security/security_connector/security_connector.cc    +344  +3.5%
      +282%    +336 ssl_channel_check_peer                                            +336  +282%
      +1.3%      +8 grpc_ssl_channel_security_connector_create                          +8  +1.3%
  +1.9%     +48 src/core/lib/security/credentials/ssl/ssl_credentials.cc           +48  +1.9%
      +6.0%     +24 grpc_ssl_credentials_create                                        +24  +6.0%
       +79%     +22 ssl_destruct                                                       +22   +79%
      +1.5%      +2 [Unmapped]                                                          +2  +1.5%

  +0.0%    +552 TOTAL                                                          +2.12Ki  +0.0%


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

libgrpc++.so

     VM SIZE                                            FILE SIZE
 ++++++++++++++ GROWING                              ++++++++++++++
  +0.2%     +16 src/cpp/client/secure_credentials.cc     +16  +0.2%
      +4.9%     +16 grpc::SslCredentials                     +16  +4.9%

 -------------- SHRINKING                            --------------
  -0.0%     -16 [None]                                    -8  -0.0%

  [ = ]       0 TOTAL                                     +8  +0.0%

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

[trickle] No significant performance differences

grpc-testing · 2018-05-04T17:49:07Z

[microbenchmarks] No significant performance differences

jiangtaoli2016 · 2018-05-04T18:04:09Z

include/grpc/grpc_security.h

Please give proper comments about this struct. Instead of int (*verify_peer_callback)(const char*, const char*, void*), use e.g.,
int (*verify_peer_callback)(const char* target_name, const char* peer_pem, void* userdata)
and explain each parameter and return value.

Also I assume this callback is blocking, right?

Yes, the callback is blocking. I added that clarification (along with a bunch of other detail) to comments.

jiangtaoli2016 · 2018-05-04T18:08:34Z

include/grpc/grpc_security.h

You probably can make const verify_peer_options* verify_options

Also add a comment about this new parameter.

jiangtaoli2016 · 2018-05-04T18:09:16Z

include/grpc/grpc_security.h

Why add stdbool.h here?

Leftover from a prior iteration of this changeset. Removed.

jiangtaoli2016 · 2018-05-04T18:09:54Z

src/core/lib/security/credentials/ssl/ssl_credentials.cc

s/NULL/nullptr

jiangtaoli2016 · 2018-05-04T18:10:30Z

src/core/lib/security/credentials/ssl/ssl_credentials.cc

add const to verify_peer_options*

jiangtaoli2016 · 2018-05-04T18:15:55Z

src/core/lib/security/credentials/ssl/ssl_credentials.cc

In the case verify_options is nullptr, config->verify_options is uninitialized, it is safer to set memset config->verify_options to nullptr.

jiangtaoli2016 · 2018-05-04T18:26:11Z

src/core/lib/security/security_connector/security_connector.cc

You need to run clang-format.
tools/dockerfile/grpc_clang_format/clang_format_all_the_things.sh

jiangtaoli2016 · 2018-05-04T18:28:04Z

src/core/lib/security/security_connector/security_connector.cc

You probably want to also include callback_status code in the error.

jiangtaoli2016 · 2018-05-04T18:30:17Z

test/core/end2end/h2_ssl_cert_test.cc

nit: you need to format this line.

jiangtaoli2016 · 2018-05-04T18:32:02Z

src/core/lib/security/security_connector/security_connector.cc

use static_cast<char*>gpr_malloc(p->value.length + 1)

jiangtaoli2016 · 2018-05-04T19:46:34Z

Thanks much @JackOfMostTrades for contribution, a few comments first.

JackOfMostTrades · 2018-05-04T20:50:54Z

Thanks for the feedback @jiangtaoli2016 ! I've done a first pass at cleanup based on your comments.

jiangtaoli2016

Thanks much for revision. This looks much better. LGTM

jiangtaoli2016 · 2018-05-07T16:39:54Z

@markdroth Please take a look when you have time.

grpc-testing · 2018-05-07T16:42:48Z

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%    +224 [None]                                                         +6.45Ki  +0.1%
      +0.0%    +224 [Unmapped]                                                     +6.45Ki  +0.1%
      [NEW]     +16 CSWTCH.82                                                          +16  [NEW]
  +3.8%    +376 src/core/lib/security/security_connector/security_connector.cc    +376  +3.9%
      +309%    +368 ssl_channel_check_peer                                            +368  +309%
      +1.3%      +8 grpc_ssl_channel_security_connector_create                          +8  +1.3%
  +3.8%     +96 src/core/lib/security/credentials/ssl/ssl_credentials.cc           +96  +3.8%
       +16%     +64 grpc_ssl_credentials_create                                        +64   +16%
       +79%     +22 ssl_destruct                                                       +22   +79%
      +7.3%     +10 [Unmapped]                                                         +10  +7.3%

  +0.0%    +696 TOTAL                                                          +6.91Ki  +0.1%


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

libgrpc++.so

     VM SIZE                                            FILE SIZE
 ++++++++++++++ GROWING                              ++++++++++++++
  +0.2%     +16 src/cpp/client/secure_credentials.cc     +16  +0.2%
      +4.9%     +16 grpc::SslCredentials                     +16  +4.9%

 -------------- SHRINKING                            --------------
  -0.0%     -16 [None]                                     0  [ = ]

  [ = ]       0 TOTAL                                    +16  +0.0%

grpc-testing · 2018-05-07T16:52:17Z

[trickle] No significant performance differences

markdroth · 2018-05-07T17:26:42Z

Please add a test for this new API.

Reviewed 15 of 22 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

grpc-testing · 2018-05-07T17:35:10Z

[microbenchmarks] No significant performance differences

JackOfMostTrades · 2018-05-08T07:56:00Z

@markdroth I've added a new test verify_peer_options.cc which covers this API, including checking that the expected values get passed through and that the callback return value is respected. Let me know if you have any comments on it.

markdroth · 2018-05-08T20:32:44Z

Thanks. The test looks pretty good. I have just a couple of minor comments.

Reviewed 8 of 8 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

build.yaml, line 2756 at r3 (raw file):

  - linux
  secure: true
- name: handshake_verify_peer_options

Please also add this target to test/core/handshake/BUILD.

test/core/handshake/verify_peer_options.cc, line 203 at r3 (raw file):

                           void* userdata) {
  if (target_host != nullptr) {
    GPR_ASSERT((callback_target_host = static_cast<char*>(

Can use gpr_strdup() here.

test/core/handshake/verify_peer_options.cc, line 208 at r3 (raw file):

  }
  if (target_pem != nullptr) {
    GPR_ASSERT((callback_target_pem = static_cast<char*>(

Same here.

Comments from Reviewable

JackOfMostTrades · 2018-05-08T21:09:16Z

Good feedback, thanks. Added a commit with the cleanup.

markdroth · 2018-05-08T21:15:28Z

This looks great! Please squash your commits, and then we can run the tests and make sure everything is happy.

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

JackOfMostTrades · 2018-05-08T21:27:52Z

Cool, commits squashed and pushed.

grpc-testing · 2018-05-08T22:52:53Z

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%    +224 [None]                                                         +6.45Ki  +0.1%
      +0.0%    +224 [Unmapped]                                                     +6.45Ki  +0.1%
      [NEW]     +16 CSWTCH.82                                                          +16  [NEW]
  +3.8%    +376 src/core/lib/security/security_connector/security_connector.cc    +376  +3.9%
      +309%    +368 ssl_channel_check_peer                                            +368  +309%
      +1.3%      +8 grpc_ssl_channel_security_connector_create                          +8  +1.3%
  +3.8%     +96 src/core/lib/security/credentials/ssl/ssl_credentials.cc           +96  +3.8%
       +16%     +64 grpc_ssl_credentials_create                                        +64   +16%
       +79%     +22 ssl_destruct                                                       +22   +79%
      +7.3%     +10 [Unmapped]                                                         +10  +7.3%

  +0.0%    +696 TOTAL                                                          +6.91Ki  +0.1%


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

libgrpc++.so

     VM SIZE                                            FILE SIZE
 ++++++++++++++ GROWING                              ++++++++++++++
  +0.2%     +16 src/cpp/client/secure_credentials.cc     +16  +0.2%
      +4.9%     +16 grpc::SslCredentials                     +16  +4.9%

 -------------- SHRINKING                            --------------
  -0.0%     -16 [None]                                     0  [ = ]

  [ = ]       0 TOTAL                                    +16  +0.0%

grpc-testing · 2018-06-05T17:51:12Z

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%    +192 [None]                                                         +2.41Ki  +0.0%
      +0.0%    +192 [Unmapped]                                                     +2.41Ki  +0.0%
      [NEW]     +16 CSWTCH.82                                                          +16  [NEW]
  +3.9%    +376 src/core/lib/security/security_connector/security_connector.cc    +376  +3.9%
      +309%    +368 ssl_channel_check_peer                                            +368  +309%
      +1.3%      +8 grpc_ssl_channel_security_connector_create                          +8  +1.3%
  +3.8%     +96 src/core/lib/security/credentials/ssl/ssl_credentials.cc           +96  +3.8%
       +16%     +64 grpc_ssl_credentials_create                                        +64   +16%
       +79%     +22 ssl_destruct                                                       +22   +79%
      +7.3%     +10 [Unmapped]                                                         +10  +7.3%

  +0.0%    +664 TOTAL                                                          +2.88Ki  +0.0%


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

libgrpc++.so

     VM SIZE                                            FILE SIZE
 ++++++++++++++ GROWING                              ++++++++++++++
  +0.0%     +16 [None]                                     0  [ = ]
  +0.2%     +16 src/cpp/client/secure_credentials.cc     +16  +0.2%
      +4.9%     +16 grpc::SslCredentials                     +16  +4.9%

  +0.0%     +32 TOTAL                                    +16  +0.0%

grpc-testing · 2018-06-05T18:43:09Z

[microbenchmarks] No significant performance differences

JackOfMostTrades · 2018-06-12T01:27:24Z

The new test looks like it's behaving now, so I've rebased, resolved conflicts, and squashed.

markdroth · 2018-06-12T18:44:43Z

It looks like your commit is accidentally changing the subproject commit for boringssl and boringssl-with-bazel. Can you please fix that?

grpc-testing · 2018-06-12T18:46:07Z

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%    +192 [None]                                                         +2.41Ki  +0.0%
      +0.0%    +192 [Unmapped]                                                     +2.41Ki  +0.0%
      [NEW]     +16 CSWTCH.82                                                          +16  [NEW]
  +3.9%    +376 src/core/lib/security/security_connector/security_connector.cc    +376  +3.9%
      +309%    +368 ssl_channel_check_peer                                            +368  +309%
      +1.3%      +8 grpc_ssl_channel_security_connector_create                          +8  +1.3%
  +3.8%     +96 src/core/lib/security/credentials/ssl/ssl_credentials.cc           +96  +3.8%
       +16%     +64 grpc_ssl_credentials_create                                        +64   +16%
       +79%     +22 ssl_destruct                                                       +22   +79%
      +7.3%     +10 [Unmapped]                                                         +10  +7.3%

  +0.0%    +664 TOTAL                                                          +2.87Ki  +0.0%


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

libgrpc++.so

     VM SIZE                                            FILE SIZE
 ++++++++++++++ GROWING                              ++++++++++++++
  +0.2%     +16 src/cpp/client/secure_credentials.cc     +16  +0.2%
      +4.9%     +16 grpc::SslCredentials                     +16  +4.9%

 -------------- SHRINKING                            --------------
  -0.0%     -16 [None]                                     0  [ = ]

  [ = ]       0 TOTAL                                    +16  +0.0%

grpc-testing · 2018-06-12T18:52:27Z

[trickle] No significant performance differences

grpc-testing · 2018-06-12T19:38:09Z

[microbenchmarks] No significant performance differences

…expose a verification callback option. These options are not yet exposed to languages outside of core.

JackOfMostTrades · 2018-06-12T20:00:53Z

It looks like your commit is accidentally changing the subproject commit for boringssl and boringssl-with-bazel. Can you please fix that?

Oops, fixed that.

grpc-testing · 2018-06-12T20:59:43Z

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%    +192 [None]                                                         +2.41Ki  +0.0%
      +0.0%    +192 [Unmapped]                                                     +2.41Ki  +0.0%
      [NEW]     +16 CSWTCH.82                                                          +16  [NEW]
  +3.9%    +376 src/core/lib/security/security_connector/security_connector.cc    +376  +3.9%
      +309%    +368 ssl_channel_check_peer                                            +368  +309%
      +1.3%      +8 grpc_ssl_channel_security_connector_create                          +8  +1.3%
  +3.8%     +96 src/core/lib/security/credentials/ssl/ssl_credentials.cc           +96  +3.8%
       +16%     +64 grpc_ssl_credentials_create                                        +64   +16%
       +79%     +22 ssl_destruct                                                       +22   +79%
      +7.3%     +10 [Unmapped]                                                         +10  +7.3%

  +0.0%    +664 TOTAL                                                          +2.87Ki  +0.0%


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

libgrpc++.so

     VM SIZE                                            FILE SIZE
 ++++++++++++++ GROWING                              ++++++++++++++
  +0.2%     +16 src/cpp/client/secure_credentials.cc     +16  +0.2%
      +4.9%     +16 grpc::SslCredentials                     +16  +4.9%

 -------------- SHRINKING                            --------------
  -0.0%     -16 [None]                                     0  [ = ]

  [ = ]       0 TOTAL                                    +16  +0.0%

grpc-testing · 2018-06-12T21:07:34Z

[trickle] No significant performance differences

grpc-testing · 2018-06-12T21:51:37Z

[microbenchmarks] No significant performance differences

markdroth · 2018-06-13T17:33:02Z

Known issues: #15710 #14240

This looks to me, but I'll let @jiangtaoli2016 give final approval.

jiangtaoli2016 · 2018-06-19T16:18:26Z

Looks good to me. Thanks @JackOfMostTrades for contribution.

jiangtaoli2016 · 2018-06-19T16:21:12Z

@markdroth Could you merge?

JackOfMostTrades mentioned this pull request May 4, 2018

Add TLS verify callback for Python and Ruby #12656

Closed

jiangtaoli2016 requested review from jiangtaoli2016 and markdroth May 4, 2018 16:54

jiangtaoli2016 self-assigned this May 4, 2018

jiangtaoli2016 added the kokoro:force-run label May 4, 2018

kokoro-team removed the kokoro:force-run label May 4, 2018

jiangtaoli2016 reviewed May 4, 2018

View reviewed changes

JackOfMostTrades force-pushed the verify-callback-core branch from 863ea4b to fe8b465 Compare May 4, 2018 20:53

jiangtaoli2016 reviewed May 7, 2018

View reviewed changes

jiangtaoli2016 added the kokoro:force-run label May 7, 2018

kokoro-team removed the kokoro:force-run label May 7, 2018

JackOfMostTrades force-pushed the verify-callback-core branch from 35bf47f to c219ef9 Compare May 8, 2018 21:27

jiangtaoli2016 added the kokoro:force-run label May 8, 2018

kokoro-team removed the kokoro:force-run label May 8, 2018

JackOfMostTrades force-pushed the verify-callback-core branch 2 times, most recently from 9dea026 to 75a36de Compare June 12, 2018 01:26

markdroth added the kokoro:run label Jun 12, 2018

kokoro-team removed the kokoro:run label Jun 12, 2018

Create verify_peer_options when creating ssl credentials in order to …

68eff58

…expose a verification callback option. These options are not yet exposed to languages outside of core.

JackOfMostTrades force-pushed the verify-callback-core branch from 75a36de to 68eff58 Compare June 12, 2018 19:59

markdroth added the kokoro:run label Jun 12, 2018

kokoro-team removed the kokoro:run label Jun 12, 2018

markdroth approved these changes Jun 19, 2018

View reviewed changes

markdroth merged commit 9e3e646 into grpc:master Jun 19, 2018

JackOfMostTrades mentioned this pull request Jun 19, 2018

Add checkServerIdentity callback grpc/grpc-node#403

Merged

jtattermusch mentioned this pull request Jun 20, 2018

Node tests broken on master #15814

Closed

srini100 mentioned this pull request Jun 28, 2018

Bump to v1.13.0 #15895

Merged

jtattermusch mentioned this pull request Jul 13, 2018

TSAN failure in handshake_verify_peer_options #16009

Closed

markdroth added the release notes: no Indicates if PR should not be in release notes label Jul 19, 2018

lock bot locked as resolved and limited conversation to collaborators Oct 17, 2018

Conversation

JackOfMostTrades commented May 4, 2018

Uh oh!

grpc-testing commented May 4, 2018

Uh oh!

grpc-testing commented May 4, 2018

Uh oh!

grpc-testing commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangtaoli2016 commented May 4, 2018

Uh oh!

JackOfMostTrades commented May 4, 2018

Uh oh!

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

Uh oh!

jiangtaoli2016 commented May 7, 2018

Uh oh!

grpc-testing commented May 7, 2018

Uh oh!

grpc-testing commented May 7, 2018

Uh oh!

markdroth commented May 7, 2018

Uh oh!

grpc-testing commented May 7, 2018

Uh oh!

JackOfMostTrades commented May 8, 2018

Uh oh!

markdroth commented May 8, 2018

Uh oh!

JackOfMostTrades commented May 8, 2018

Uh oh!

markdroth commented May 8, 2018

Uh oh!

JackOfMostTrades commented May 8, 2018

Uh oh!

grpc-testing commented May 8, 2018

Uh oh!

grpc-testing commented Jun 5, 2018

Uh oh!

grpc-testing commented Jun 5, 2018

Uh oh!

JackOfMostTrades commented Jun 12, 2018

Uh oh!

markdroth commented Jun 12, 2018

Uh oh!

grpc-testing commented Jun 12, 2018

Uh oh!

grpc-testing commented Jun 12, 2018

Uh oh!

grpc-testing commented Jun 12, 2018

Uh oh!

JackOfMostTrades commented Jun 12, 2018

Uh oh!