Eliminate gRPC insecure build by yihuazhang · Pull Request #25586 · grpc/grpc

yihuazhang · 2021-03-01T16:43:02Z

This PR is a part of the effort for removing gRPC insecure build so that all gRPC targets can depend on secure builds. Another purpose of this PR is to refactor the BUILD file so that for the targets that do not rely on SSL, they will not link libssl targets.

After this PR, there will be the following continuous efforts.

Refactor gRPC C++ so that each C++ credential target will have its own build target.
Clean up BUILD file by removing unnecessary targets.

This change is

yihuazhang · 2021-03-01T16:46:00Z

@jiangtaoli2016 F.Y.I.

jtattermusch · 2021-03-02T10:28:16Z

@yihuazhang what is the motivation for getting rid of the insecure targets?

I added myself as a reviewer as I'd like to check that the BUILD / cmake / other build systems changes make sense - please let me know once this is ready for review (and please wait for LGTM from me).

jtattermusch · 2021-03-02T10:31:14Z

@yihuazhang can you please also provide a list of the "public" insecure targets that you're planning to remove?

markdroth · 2021-03-02T15:10:36Z

@jtattermusch This is something we've been wanting to do for a while. For context, see the internal design doc at go/higher-security-for-grpc.

yihuazhang · 2021-03-02T18:29:20Z

Thanks @jtattermusch for chiming in and sorry for lacking the context. The doc Mark pointed contains more details about the motivation of this PR. PLMK if you have any questions.

Regarding the insecure builds, I think grpc_transport_chttp2_client_insecure and grpc_transport_chttp2_server_insecure are the ones that we want to deprecate (@markdroth please correct me if I am wrong), because for insecure channel creations (at both client and server sides), we want to build it in the same we build a secure channel by passing in a credential object to the secure channel creation API.

jtattermusch · 2021-03-03T10:09:54Z

Thanks @jtattermusch for chiming in and sorry for lacking the context. The doc Mark pointed contains more details about the motivation of this PR. PLMK if you have any questions.

Regarding the insecure builds, I think grpc_transport_chttp2_client_insecure and grpc_transport_chttp2_server_insecure are the ones that we want to deprecate (@markdroth please correct me if I am wrong), because for insecure channel creations (at both client and server sides), we want to build it in the same we build a secure channel by passing in a credential object to the secure channel creation API.

Oh I see, I thought you're going to be removing the grpc_unsecure and grpc++_unsecure targets which are "publicly" visible in our other build systems (such as cmake etc.) and removing them would involve a bit more work. If you're only including some of the "internal" targets (that users wouldn't depend on directly) this might not require any additional work in the non-bazel build systems.
Just to be sure, I'd like to do a quick review once this has been LGTMd by someone, just to make sure this doesn't have some negative consequences on non-bazel builds (as said given the circumstances I'm not expecting that to be the case, but better safe than sorry).

yihuazhang · 2021-03-03T16:36:54Z

It seems the failed tests are not related to this PR.

F.Y.I. @markdroth, the PR is ready for a review. No rush though (I expect you might be busy due to perf).

@jtattermusch SG. I will let you know once Mark approved this PR.

markdroth

Thanks for doing this, Yihua! I've been wanting to see this change for a long time.

Jan, your review will be most welcome. I'm sure you can help us spot anything we might inadvertantly break here.

Please let me know if you have any questions. Thanks!

Reviewed 12 of 27 files at r1, 3 of 17 files at r2, 8 of 8 files at r3, 5 of 8 files at r4.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jtattermusch and @yihuazhang)

BUILD, line 130 at r3 (raw file):

]

GRPC_SECURE_PUBLIC_HDRS = [

This list can probably go away now, and grpc_security.h can be added to GRPC_PUBLIC_HDRS instead.

BUILD, line 134 at r3 (raw file):

]

# TODO(ctiller): layer grpc atop grpc_unsecure, layer grpc++ atop grpc++_unsecure

This TODO can go away now.

BUILD, line 300 at r3 (raw file):

grpc_cc_library(
    name = "grpc_unsecure",

We need to clean up the conditional compilation structure that we currently have for secure vs. insecure builds.

We have the grpc_unsecure target for insecure builds and the grpc_secure target for secure builds. The two targets actually build a slightly different set of files. Some specifics:

The insecure build includes init_unsecure.cc and grpc_unsecure_plugin_registry.cc, whereas the secure build includes init_secure.cc and grpc_plugin_registry.cc.
Both secure and insecure builds depend on grpc_transport_chttp2_client_insecure and grpc_transport_chttp2_server_insecure (which define grpc_insecure_channel_create() and grpc_server_add_insecure_http2_port()), but the secure build additionally depends on grpc_transport_chttp2_client_secure and grpc_transport_chttp2_server_secure (which define grpc_server_add_secure_http2_port() and grpc_secure_channel_create()).

I think what we want here is to eliminate the conditional compilation and make the secure target a simple layer on top of the insecure target that adds the dependencies for the credential types (which bring in the SSL library dependency).

Here are some specific changes we should make:

Move the code from init_secure.cc directly into init.cc, and remove init_secure.cc and init_unsecure.cc. Also remove the relevant declarations in init.h, since those functions can now be static inside of init.cc.
Move the code from grpc_plugin_registry.cc into init.cc and remove both grpc_plugin_registry.cc and grpc_unsecure_plugin_registry.cc.
Eliminate grpc_insecure_channel_create() and grpc_server_add_insecure_http2_port() from the API and rename grpc_server_add_secure_http2_port() and grpc_secure_channel_create() to just grpc_server_add_http2_port() and grpc_channel_create(). This will require a gRFC. As part of this, we should probably move their declarations from grpc_security.h to grpc.h. And instead of the code being in src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc and src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, it should be moved to the existing src/core/ext/transport/chttp2/client/chttp2_connector.cc and src/core/ext/transport/chttp2/server/chttp2_server.cc files.

BUILD, line 1313 at r3 (raw file):

grpc_cc_library(
    name = "grpc_lb_policy_grpclb",

Here's another case where we have some conditional compilation for insecure builds that needs to be cleaned up.

We currently have two targets for grpclb, one for insecure builds (this one) and another for secure builds (the next one, grpc_lb_policy_grpclb_secure). The only difference between the two is that the insecure one uses the file grpclb_channel.cc, whereas the secure one uses the file grpclb_channel_secure.cc. Both files provide the interface defined in grpclb_channel.h, but with different implementations for the two different builds. And we have the build dependencies set up such that insecure builds and secure builds depend on the right version of this target.

This difference is no longer needed now that insecure builds are going away. Instead, let's do the following:

Move the code from grpclb_channel_secure.cc directly into grpclb.cc.
Remove the files grpclb_channel.h, grpclb_channel.cc, and grpclb_channel_secure.cc.
Remove the grpc_lb_policy_grpclb_secure target.
Remove the grpc_lb_policy_grpclb_secure dependency from the grpc target, and add grpc_lb_policy_grpclb as a dependency of grpc_common.
Change the grpc_lb_policy_grpclb target to depend on grpc_transport_chttp2_client_secure instead of grpc_transport_chttp2_client_insecure.

BUILD, line 1847 at r3 (raw file):

grpc_cc_library(
    name = "grpc_secure_base",

Suggest calling this grpc_security_base.

BUILD, line 1860 at r3 (raw file):

        "src/core/lib/security/transport/server_auth_filter.cc",
        "src/core/lib/security/transport/tsi_error.cc",
        "src/core/tsi/transport_security.cc",

These TSI files are already present in the tsi target, and they should not be in more than one build target. I think we still need the tsi target, because there are internal callers like GFE that depend on TSI without necessarily depending on gRPC, so we can't just remove that target. So we'll need to make this target depend on the tsi target.

Of course, we can't just depend on the tsi target as it currently exists, because that would pull in the SSL library dependency that we're trying to eliminate here. To avoid this, I suggest the following:

Move the "base" TSI files (like src/core/tsi/transport_security.cc) into a new target called tsi_base, which can be a dependency of both this target and the tsi target.
For each credential type, move the TSI files into a new target called tsi_${type}_credentials, which can be a dependency of both the tsi target and the corresponding grpc_${type}_credentials target. (For example, a file like src/core/tsi/fake_transport_security.cc would be in a target called tsi_fake_credentials, which would be a dependency of both the tsi target and the grpc_fake_credentials target, so the grpc_fake_credentials target would not need to directly include that .cc file.)

BUILD, line 1944 at r3 (raw file):

    name = "grpc_alts_credentials",
    srcs = [
        "src/core/ext/upb-generated/src/proto/grpc/gcp/altscontext.upb.c",

These upb files should not be repeated here. Instead, add a dependency on the existing alts_upb target.

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

#define GRPC_ARG_SECURITY_CONNECTOR "grpc.security_connector"
#define GRPC_SSL_URL_SCHEME "https"

This seems like the wrong place for this definition, since there is nothing SSL-specific in this file. I think this should be kept where it was originally, in ssl_utils.h. Was there some reason you needed to move it?

yihuazhang · 2021-03-03T18:32:51Z

Thanks @markdroth for the detailed feedback, and sorry for the delayed action!

Regarding the comments - Eliminate grpc_insecure_channel_create() and grpc_server_add_insecure_http2_port() from the API..., don't we break existing users by directly removing or renaming functions? Should not the correct course of action be mark the API as deprecated, introduce the new API, and migrate existing internal call sites to use the new API? I would prefer to do it later in the follow-up PR as I want to keep the changes in this PR not resulting in any external call site change.

markdroth · 2021-03-03T19:50:58Z

The C-core API is not a public API, and we don't offer a backward-compatibility guarantee for it. We are required to publish a gRFC for C-core API changes, but we are not required to make non-breaking changes.

In general, the only callers of the C-core API should be C++ and the wrapped languages, so we are in control of all the call sites. We should be able to update them as part of this PR.

If there's some reason that it's hard to update the callers at the same time, I'm not opposed to keeping the old APIs for a short time to ease the transition (e.g., to allow us to spread the changes out across multiple PRs). But I don't want to be left supporting duplicate APIs for a long period of time.

Also, note that there are subtle behavioral differences between grpc_insecure_channel_create() and grpc_secure_channel_create() with insecure creds when it comes to how call creds are handled, as we discussed recently. We'd probably have to do a bit of work to preserve the old behavior of grpc_insecure_channel_create() once this PR goes in, and I'm not sure it's really worth the effort to do so.

yihuazhang · 2021-03-04T00:31:47Z

Ah you are right. Now I remember that we do not need to maintain backward compatibility for C-core surface API changes.
I briefly checked their call sites: 1) most of them are in third_party/grpc, 2) some of them are in net/grpc for which I can prepare for an internal patch, and 3) one or two are third_party non-grpc related for which i think we can break them.

After we change all wrapped languages to use the new way of creating an insecure channel, I am not sure if it is going to break any of their internal tests that rely on the subtle difference between old and new behaviors you mentioned (e.g., when communicating call credentials on the channel created via the old API, the call credentials will get ignored but RPC still succeeds while RPC will fail when communicating the call credentials over the channel created via the new API).

markdroth · 2021-03-04T14:35:30Z

one or two are third_party non-grpc related for which i think we can break them.

We usually have to update these callers as well. But we can deal with that when we're ready to try importing this change.

After we change all wrapped languages to use the new way of creating an insecure channel, I am not sure if it is going to break any of their internal tests that rely on the subtle difference between old and new behaviors you mentioned (e.g., when communicating call credentials on the channel created via the old API, the call credentials will get ignored but RPC still succeeds while RPC will fail when communicating the call credentials over the channel created via the new API).

Yes. We'll need to update any affected tests as part of this PR.

yihuazhang · 2021-06-24T17:53:31Z

I apologize for taking so long to address the comments. I finally got time working on it, and it is my top priority now.

While working on it, I found the following two issues:

If we rename grpc_secure_channel_create() to grpc_channel_create(), it conflicts with the same API defined in src/core/lib/surface/channel.h. I do not see a problem of using the existing name - grpc_secure_channel_create() as we are passing credential objects to it.
I found another insecure channel creation API - grpc_insecure_channel_create_from_fd defined in src/core/ext/transport/chttp2/client/insecure/channel_create_posix.cc. Should we remove it as well?

markdroth · 2021-06-24T18:48:09Z

If we rename grpc_secure_channel_create() to grpc_channel_create(), it conflicts with the same API defined in src/core/lib/surface/channel.h. I do not see a problem of using the existing name - grpc_secure_channel_create() as we are passing credential objects to it.

The existing grpc_channel_create() function in src/core/lib/surface/channel.h is basically the underlying functionality for both grpc_secure_channel_create() and grpc_insecure_channel_create(). I suggest renaming it to grpc_channel_create_internal() so that we can use grpc_channel_create() for the C-core API.

I found another insecure channel creation API - grpc_insecure_channel_create_from_fd defined in src/core/ext/transport/chttp2/client/insecure/channel_create_posix.cc. Should we remove it as well?

I don't think we can remove this, since there are still use-cases where people want to create a channel from an fd. But since we always assume that the fd is insecure in this case, we can just change the implementation of this function to do the same thing as grpc_channel_create() with insecure creds.

yihuazhang

Reviewable status: 12 of 40 files reviewed, 8 unresolved discussions (waiting on @jtattermusch and @markdroth)

BUILD, line 130 at r3 (raw file):