Add useTransportSecurity in ManagedChannelBuilder #3561

ramaraochavali · 2017-10-10T07:05:06Z

Please refer to issue for more details

grpc-kokoro · 2017-10-10T07:05:08Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

ramaraochavali · 2017-10-20T10:15:54Z

netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java

+   * @throws SSLException
+   */
+  @Override
+  public NettyChannelBuilder trustStore(File trustCertCollectionFile) throws SSLException {


May have to add keyManager also here. But am waiting for your approval on the pattern. then will add support keyManager if needed

I'm not excited that this builds the sslContext all at once, as it means it can't be combined with any other TLS configuration (like keyManager). Although I'm also not excited about adding keyManager to ManagedChannelBuilder; it's a fairly advanced use case and needs extra configuration on server-side as well (like request/require client certs and trust store). Going down this road feels like we end up re-inventing our own TLS context for configuration.

I think I'd be happier with a useTransportSecurity(File trustCerts). But I have reservations about it as well as I feel it is dangerously close to opening a can of worms for other TLS configuration. And at the same time I'm not too bothered by the status quo of needing to use a specific builder when managing your own CA.

Ok. Should I ignore this then?

ejona86 · 2017-11-08T14:42:13Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   *
+   * @return this
+   * @throws UnsupportedOperationException if transport security is not supported.
+   */


Any new methods added to ManagedChannelBuilder must have a default implementation here, otherwise it breaks API stability. Just implement the throwing UnsupportOperationException here.

ok. will add

ejona86 · 2017-11-08T15:09:27Z

netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java

+   * @throws SSLException
+   */
+  @Override
+  public NettyChannelBuilder trustStore(File trustCertCollectionFile) throws SSLException {


I'm not excited that this builds the sslContext all at once, as it means it can't be combined with any other TLS configuration (like keyManager). Although I'm also not excited about adding keyManager to ManagedChannelBuilder; it's a fairly advanced use case and needs extra configuration on server-side as well (like request/require client certs and trust store). Going down this road feels like we end up re-inventing our own TLS context for configuration.

I think I'd be happier with a useTransportSecurity(File trustCerts). But I have reservations about it as well as I feel it is dangerously close to opening a can of worms for other TLS configuration. And at the same time I'm not too bothered by the status quo of needing to use a specific builder when managing your own CA.

ramaraochavali · 2017-11-08T17:03:23Z

should I just ignore the changes of trustStore and just restrict it to useTransportSecurity()

thelinuxfoundation · 2017-11-08T17:03:26Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @ramaraochavali isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

ejona86 · 2017-11-08T18:36:05Z

should I just ignore the changes of trustStore and just restrict it to useTransportSecurity()

@ramaraochavali, that seems good to me. A change with just useTransportSecurity() would be easy to get in.

Signed-off-by: Rama <ramaraochavali@gmail.com>

ramaraochavali · 2017-11-09T09:18:59Z

@ejona86 I removed the rest of the stuff and only retained the useTransportSecurity method.

ejona86

Looks pretty good. Only minor stuff left.

ejona86 · 2017-11-09T17:16:44Z

core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java

 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
 import io.grpc.internal.SharedResourceHolder;
+


We only put extra line between static and non-static imports. Remove here and elsewhere.

ejona86 · 2017-11-09T17:18:00Z

core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java

    }
+
+    @Override
+    public Builder useTransportSecurity() {


This and several other of the implementations seem unnecessary, now that ManagedChannelBuilder. provides a default.

ejona86 · 2017-11-09T17:25:23Z

netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java

      GrpcSslContexts.ensureAlpnAndH2Enabled(sslContext.applicationProtocolNegotiator());
    }
    this.sslContext = sslContext;
+    this.negotiationType = NegotiationType.TLS;


While I see the value in this, doing things implicitly can lead to trouble. We mostly try to have the builder be "dumb setters." Since I'm conflicted on this, let's just leave it out for now.

ejona86 · 2017-11-09T17:26:34Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   *
+   * @return this
+   * @throws UnsupportedOperationException if transport security is not supported.
+   */


We mark new APIs with @ExperimentalApi("https://link/to/github/issue"). Create a new issue for this API to become stable and add it as part of the annotation here.

ejona86 · 2017-11-09T17:27:03Z

okay to test

ejona86 · 2017-11-09T20:35:00Z

@ramaraochavali, Soo, since you started this we've swapped CLAs from Google's to CNCF. Would you be willing to go through the process again with the Linux Foundation?

(I'm working on getting the Google CLA GitHub status removed for new PRs.)

ramaraochavali · 2017-11-10T04:43:50Z

@ejona86 addressed review comments and signed with CNCF Linux foundation. Please check

ejona86

Looks great! Thanks!

dapengzhang0 · 2017-11-10T18:28:29Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

+   * Makes the client use TLS.
+   *
+   * @return this
+   * @throws UnsupportedOperationException if transport security is not supported.


also adding * @since 1.9.0 in the doc would be nice

Done. I went ahead and did that.

added ssl related methods for ManagedChannelBuilder

61d7b04

ramaraochavali changed the title ~~added ssl related methods for ManagedChannelBuilder~~ Added ssl related methods for ManagedChannelBuilder Oct 10, 2017

ramaraochavali mentioned this pull request Oct 10, 2017

SSL Related Methods in ManagedChannelBuilder #3556

Closed

ramaraochavali added 4 commits October 11, 2017 13:04

added useTransportSecurity to ManagedChannelBuilder

b51495b

added trustStore method

65a0661

fixed test compilation errors and Styling issues

92eb975

corrected styles

89d6ba2

ramaraochavali changed the title ~~Added ssl related methods for ManagedChannelBuilder~~ SSL Related Methods in ManagedChannelBuilder Oct 13, 2017

ramaraochavali commented Oct 20, 2017

View reviewed changes

ejona86 reviewed Nov 8, 2017

View reviewed changes

core netty added useTransportSecurity api in ManagedChannelBuilder

4f93c97

Signed-off-by: Rama <ramaraochavali@gmail.com>

ramaraochavali changed the title ~~SSL Related Methods in ManagedChannelBuilder~~ Add useTransportSecurity in ManagedChannelBuilder Nov 9, 2017

ejona86 reviewed Nov 9, 2017

View reviewed changes

ramaraochavali added 3 commits November 10, 2017 10:00

fixed imports, removed call from sslContext, removed unnecessary impl

f83e3bf

core netty http fixed imports,javadoc

328e584

core netty http changed order of method

6968583

fixed javadoc issue

106d21f

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 10, 2017

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 10, 2017

ejona86 approved these changes Nov 10, 2017

View reviewed changes

ejona86 requested a review from dapengzhang0 November 10, 2017 18:17

dapengzhang0 approved these changes Nov 10, 2017

View reviewed changes

Added since 1.9.0

cd3cf22

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 10, 2017

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 10, 2017

ejona86 merged commit 17e1ad9 into grpc:master Nov 10, 2017

ramaraochavali deleted the feature/uniform-ssl-context branch November 14, 2017 16:16

zhangkun83 added this to the 1.9 milestone Jan 9, 2018

zhangkun83 added Type: Feature Type: API Change and removed Type: Feature labels Jan 9, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Add useTransportSecurity in ManagedChannelBuilder #3561

Add useTransportSecurity in ManagedChannelBuilder #3561

Uh oh!

Conversation

ramaraochavali commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grpc-kokoro commented Oct 10, 2017

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!

ramaraochavali commented Nov 8, 2017

Uh oh!

thelinuxfoundation commented Nov 8, 2017

Uh oh!

ejona86 commented Nov 8, 2017

Uh oh!

ramaraochavali commented Nov 9, 2017

Uh oh!

ejona86 left a comment

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!

ejona86 commented Nov 9, 2017

Uh oh!

ejona86 commented Nov 9, 2017

Uh oh!

ramaraochavali commented Nov 10, 2017

Uh oh!

ejona86 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ramaraochavali commented Oct 10, 2017 •

edited

Loading