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.

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

Extend local channel/server credentials to support TCP loopback #17370

Merged
merged 1 commit into from Dec 17, 2018

Conversation

yihuazhang
Copy link
Contributor

@yihuazhang yihuazhang commented Dec 3, 2018

The PR enables local channel/server credentials to support both UDS and TCP loopback addresses (IPV4 and IPV6). It will be used as an substituent for insecure channel/server credentials that are used in UDS and TCP loopback connections.

This PR

  1. Modifies grpc_security_connector_check_peer API to further take grpc_endpoint as an argument. An alternative is to pass grpc_endpoint during the handshake which requires the modification of TSI interface.

  2. Applies getsockname to get socket information that will be used to determine if the socket address is type of either UDS or TCP loopback (IPV4 and IPV6).

  3. Updates h2_local.cc to further validate the behavior of local credentials when used in TCP loopback addresses.


This change is Reviewable

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,418      Total (>)      2,020,410

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,174,958      Total (>)     11,174,082

 No significant differences in binary sizes


@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                  FILE SIZE
 ++++++++++++++ GROWING                                                                    ++++++++++++++
  +6.2%    +104 src/core/lib/security/security_connector/local/local_security_connector.cc    +104  +6.2%
      [NEW]    +592 local_check_peer                                                              +592  [NEW]
      +1.5%      +1 [Unmapped]                                                                      +1  +1.5%

 -+-+-+-+-+-+-+ MIXED                                                                      +-+-+-+-+-+-+-
  -0.0%     -40 [None]                                                                     +1.53Ki  +0.0%

  +0.0%     +64 TOTAL                                                                      +1.63Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0 TOTAL     +152  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,418      Total (>)      2,020,410

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,174,416      Total (>)     11,174,077

 No significant differences in binary sizes


@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@yihuazhang yihuazhang added the release notes: no Indicates if PR should not be in release notes label Dec 4, 2018
@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                  FILE SIZE
 ++++++++++++++ GROWING                                                                    ++++++++++++++
  +6.2%    +104 src/core/lib/security/security_connector/local/local_security_connector.cc    +104  +6.2%
      [NEW]    +592 local_check_peer                                                              +592  [NEW]
      +1.5%      +1 [Unmapped]                                                                      +1  +1.5%

 -+-+-+-+-+-+-+ MIXED                                                                      +-+-+-+-+-+-+-
  -0.0%     -40 [None]                                                                     +1.55Ki  +0.0%

  +0.0%     +64 TOTAL                                                                      +1.66Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0 TOTAL     +128  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,019,726      Total (>)      2,019,718

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,154,454      Total (>)     11,154,122

 No significant differences in binary sizes


@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                  FILE SIZE
 ++++++++++++++ GROWING                                                                    ++++++++++++++
  +0.0%      +8 [None]                                                                     +1.57Ki  +0.0%
  +7.1%    +120 src/core/lib/security/security_connector/local/local_security_connector.cc    +120  +7.1%
      [NEW]    +608 local_check_peer                                                              +608  [NEW]
      +1.5%      +1 [Unmapped]                                                                      +1  +1.5%

  +0.0%    +128 TOTAL                                                                      +1.69Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0 TOTAL     +128  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,019,726      Total (>)      2,019,718

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,154,454      Total (>)     11,154,120

 No significant differences in binary sizes


@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

Copy link

@jiangtaoli2016 jiangtaoli2016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Yihua for implementation. Overall, it looks good. Some minor comments.

@@ -209,7 +240,7 @@ grpc_security_status grpc_local_channel_security_connector_create(
c->base.check_call_host = local_check_call_host;
c->base.cancel_check_call_host = local_cancel_check_call_host;
c->base.base.url_scheme =
creds->connect_type == UDS ? GRPC_UDS_URL_SCHEME : nullptr;
creds->connect_type == UDS ? GRPC_UDS_URL_SCHEME : GRPC_SSL_URL_SCHEME;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of Local TCP, we shall not set as GRPC_SSL_URL_SCHEME. It is not https. Leave it as nullptr is fine -- ALTS does not have URL scheme either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for pointing it out.

reinterpret_cast<grpc_sockaddr_in6*>(resolved_addr.addr);
// UDS
if (grpc_is_unix_socket(&resolved_addr)) {
is_endpoint_ok = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: renaming to is_endpoint_local has better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

grpc_auth_context** auth_context,
grpc_closure* on_peer_checked) {
int fd = grpc_endpoint_get_fd(ep);
grpc_resolved_address resolved_addr;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is resolved_addr local address or peer address?

Copy link
Contributor Author

@yihuazhang yihuazhang Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It corresponds to the socket address of a local endpoint. If we need to get the information about the peer's endpoint, getpeername should be used instead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please verify that if local socket address is loopback, then the connection (i.e., peer) must be local.

@@ -178,23 +215,17 @@ grpc_security_status grpc_local_channel_security_connector_create(
"Invalid arguments to grpc_local_channel_security_connector_create()");
return GRPC_SECURITY_ERROR;
}
// Check if local_connect_type is UDS. Only UDS is supported for now.
// Perform sanity check on UDS address.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment that TCP local connection check is during check_peer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

*/
typedef enum { UDS = 0 } grpc_local_connect_type;
typedef enum { UDS = 0, TCP_LOOPBACK } grpc_local_connect_type;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to LOCAL_TCP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. The change is applied.

reinterpret_cast<grpc_sockaddr_in*>(resolved_addr.addr);
grpc_sockaddr_in6* addr6 =
reinterpret_cast<grpc_sockaddr_in6*>(resolved_addr.addr);
// UDS

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc_local_connect_type in credential is not really used to check endpoint. We want to check endpoint is UDS and local_connect_type in base.channel_creds is also UDS to make sure they are consistent. Same for local TCP connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The change is applied.

@yihuazhang
Copy link
Contributor Author

Thanks Jiangtao for the quick review. All comments are addressed. PTAL.

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.0%    +160 [None]                                                                               +10.2Ki  +0.1%
   +26%    +432 src/core/lib/security/security_connector/local/local_security_connector.cc              +432   +26%
      [NEW]    +640 local_check_peer(grpc_security_connector*, tsi_peer, grpc_endpoint*, grpc_auth_conte    +640  [NEW]
      [NEW]    +137 local_server_check_peer                                                                 +137  [NEW]
      [NEW]    +137 local_channel_check_peer                                                                +137  [NEW]
       +35%     +23 [Unmapped]                                                                               +23   +35%

  +0.0%    +592 TOTAL                                                                                +10.6Ki  +0.1%


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

libgrpc++.so

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

  [ = ]       0 TOTAL     +128  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,019,726      Total (>)      2,019,718

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
 3,948,402       Core (>)      3,947,382

11,155,141      Total (>)     11,154,127


@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.0%    +136 [None]                                                                               +6.93Ki  +0.1%
   +30%    +496 src/core/lib/security/security_connector/local/local_security_connector.cc              +496   +30%
      [NEW]    +696 local_check_peer(grpc_security_connector*, tsi_peer, grpc_endpoint*, grpc_auth_conte    +696  [NEW]
      [NEW]    +137 local_server_check_peer                                                                 +137  [NEW]
      [NEW]    +137 local_channel_check_peer                                                                +137  [NEW]
       +48%     +31 [Unmapped]                                                                               +31   +48%

  +0.0%    +632 TOTAL                                                                                +7.41Ki  +0.1%


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

libgrpc++.so

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

  [ = ]       0 TOTAL      +64  +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.0%    +136 [None]                                                                               +10.9Ki  +0.1%
   +30%    +496 src/core/lib/security/security_connector/local/local_security_connector.cc              +496   +30%
      [NEW]    +696 local_check_peer(grpc_security_connector*, tsi_peer, grpc_endpoint*, grpc_auth_conte    +696  [NEW]
      [NEW]    +137 local_server_check_peer                                                                 +137  [NEW]
      [NEW]    +137 local_channel_check_peer                                                                +137  [NEW]
       +48%     +31 [Unmapped]                                                                               +31   +48%

  +0.0%    +632 TOTAL                                                                                +11.4Ki  +0.1%


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

libgrpc++.so

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

  [ = ]       0 TOTAL      +48  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.0%    +136 [None]                                                                               +10.9Ki  +0.1%
   +30%    +496 src/core/lib/security/security_connector/local/local_security_connector.cc              +496   +30%
      [NEW]    +696 local_check_peer(grpc_security_connector*, tsi_peer, grpc_endpoint*, grpc_auth_conte    +696  [NEW]
      [NEW]    +137 local_server_check_peer                                                                 +137  [NEW]
      [NEW]    +137 local_channel_check_peer                                                                +137  [NEW]
       +48%     +31 [Unmapped]                                                                               +31   +48%

  +0.0%    +632 TOTAL                                                                                +11.4Ki  +0.1%


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

libgrpc++.so

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

  [ = ]       0 TOTAL      +48  +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.0%    +136 [None]                                                                               +10.9Ki  +0.1%
   +30%    +496 src/core/lib/security/security_connector/local/local_security_connector.cc              +496   +30%
      [NEW]    +696 local_check_peer(grpc_security_connector*, tsi_peer, grpc_endpoint*, grpc_auth_conte    +696  [NEW]
      [NEW]    +137 local_server_check_peer                                                                 +137  [NEW]
      [NEW]    +137 local_channel_check_peer                                                                +137  [NEW]
       +48%     +31 [Unmapped]                                                                               +31   +48%

  +0.0%    +632 TOTAL                                                                                +11.4Ki  +0.1%


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

libgrpc++.so

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

  [ = ]       0 TOTAL      +48  +0.0%



@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,878      Total (>)      2,020,870

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
 3,971,402       Core (>)      3,970,386

11,178,141      Total (>)     11,177,125


@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,878      Total (>)      2,020,870

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
 3,971,402       Core (>)      3,970,386

11,178,135      Total (>)     11,177,132


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.0%    +144 [None]                                                                               +3.64Ki  +0.0%
      +0.0%    +112 [Unmapped]                                                                           +3.64Ki  +0.0%
      +2.8%     +32 [None]                                                                                     0  [ = ]
  +6.6%    +192 src/core/lib/security/security_connector/local/local_security_connector.cc              +192  +6.6%
      [NEW] +1.19Ki (anonymous namespace)::local_check_peer(grpc_security_connector*, tsi_peer, grpc_end +1.19Ki  [NEW]
      [NEW]     +24 (anonymous namespace)::grpc_local_server_security_connector::check_peer                  +24  [NEW]
      [NEW]     +24 (anonymous namespace)::grpc_local_channel_security_connector::check_peer                 +24  [NEW]
  +0.1%     +16 src/core/lib/security/transport/security_handshaker.cc                                   +16  +0.1%
      +5.6%      +9 [Unmapped]                                                                                +9  +5.6%
      +3.8%      +7 check_peer_locked                                                                         +7  +3.8%

  +0.0%    +352 TOTAL                                                                                +3.84Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0 TOTAL      +80  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,850      Total (>)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,325,193      Total (>)     11,324,729

 No significant differences in binary sizes


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@yihuazhang
Copy link
Contributor Author

Code has been further revised as follows:

  1. Synced with Move security credentials, connectors, and auth context to C++ #17291.
  2. Refactored h2_local end2end test into h2_local_uds, h2_local_ipv4, and h2_local_ipv6, and created local_util that contains a shared util library for those three tests.

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.0%    +144 [None]                                                                               +3.64Ki  +0.0%
      +0.0%    +112 [Unmapped]                                                                           +3.64Ki  +0.0%
      +2.8%     +32 [None]                                                                                     0  [ = ]
  +6.6%    +192 src/core/lib/security/security_connector/local/local_security_connector.cc              +192  +6.6%
      [NEW] +1.19Ki (anonymous namespace)::local_check_peer(grpc_security_connector*, tsi_peer, grpc_end +1.19Ki  [NEW]
      [NEW]     +24 (anonymous namespace)::grpc_local_server_security_connector::check_peer                  +24  [NEW]
      [NEW]     +24 (anonymous namespace)::grpc_local_channel_security_connector::check_peer                 +24  [NEW]
  +0.1%     +16 src/core/lib/security/transport/security_handshaker.cc                                   +16  +0.1%
      +5.6%      +9 [Unmapped]                                                                                +9  +5.6%
      +3.8%      +7 check_peer_locked                                                                         +7  +3.8%

  +0.0%    +352 TOTAL                                                                                +3.84Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0 TOTAL      +80  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,850      Total (>)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,325,193      Total (>)     11,324,734

 No significant differences in binary sizes


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@yihuazhang
Copy link
Contributor Author

Known failures: #13379, #17526

@yihuazhang yihuazhang merged commit 3f7e26e into grpc:master Dec 17, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/core release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants