Add a New ConfigurableX509TrustManager Class #6805

ZhenLian · 2020-03-05T03:42:24Z

This PR contains the following three classes:

TlsOptions.java: an option set by users to specify some advanced features when performing TLS handshake, both on client side and on server side. These features include:
- choose different levels of peer verification
- provide custom peer verification check
- change the trust CA certificate bundle
ConfigurableX509TrustManager.java: a class that extends X509ExtendedTrustManager and takes TlsOptions as parameter. Users need to feed ConfigurableX509TrustManager when creating GrpcSslContexts, in order to use the advanced features.
AdvancedTlsTest.java: integration tests.

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

sanjaypujare · 2020-03-05T05:15:07Z

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

+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509ExtendedTrustManager;
+
+public class ConfigurableX509TrustManager extends X509ExtendedTrustManager {


Javadoc comments for public class?

Can it be marked final?

I would expect some other users to further extend this class. In that case, it is better to not make it final?

When would a user extend the class instead of delegating to the class? Since all the protected+public methods are public in X509ExtendedTrustManager, I can't figure out any purpose to extending this class. As a general rule it is much better to mark things final.

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

sanjaypujare · 2020-03-05T05:26:24Z

I can see configurable trust manager being useful but this PR has no gRPC specific code nor netty specific code - but just an implementation of X509ExtendedTrustManager that can work for anyone wanting to customize Java's trust manager. Should this code be here?

ZhenLian · 2020-03-05T19:11:52Z

@sanjaypujare
Thanks for bringing this up! I also considered this problem before but this place is the best place I could find.
To my understanding, configurable trust manager is a class dealing with the trust behaviors when performing TLS handshake, and it might be most relevant to the Netty layer, if we put it somewhere in the grpc-java repo.
As far as I know, it might not easy to add this implementation directly into some "standard" Java libraries. Putting in Netty repo might be another potential alternative, and I could ask in their community.
@ejona86 Could you please make some suggestions on this? Thanks in advance!

sanjaypujare · 2020-03-05T19:19:48Z

There is nothing netty specific in this PR either. I think the best place might be an example in grpc where we include these classes with a concrete example of configurable trust manager.

ZhenLian · 2020-03-05T19:39:03Z

Are you suggesting somewhere here? I am OK with the place, as long as users can actually use these classes.
And when saying putting it in Netty repo, I meant here. Similar as Netty, We could also have a new util directory somewhere in grpc-java.

sanjaypujare · 2020-03-05T20:39:20Z

I looked at the Netty link you posted and it does have util classes so this could fit right in there (and will have more audience than just gRPC).

For grpc examples you will need to create a proper example which includes client and server. The added advantage is that you can then show ConfigurableX509TrustManager use on both sides.

I talked to @ejona86 offline and he'll add his comments too.

ZhenLian · 2020-03-11T20:53:36Z

@ejona86 I couldn't add you as the reviewer but feel free to comment on this PR.
@jiangtaoli2016 FYI.

ejona86 · 2020-04-02T16:36:55Z

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

+ * level of peer checking mechanisms, as well as some customized check. It could also be used to
+ * reload trust certificate bundle client/server uses.
+ */
+public class ConfigurableX509TrustManager extends X509ExtendedTrustManager {


All new APIs should be marked as @ExperimentalApi. You can add the annotation to each of the public classes.

Thanks for the suggestions! Shall I also create an issue to keep track of it, like this one: #1710?

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

ejona86 · 2020-04-02T16:45:21Z

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

+ * 2. provide custom peer verification check by inheriting |verifyPeerCertificate|
+ * 3. change the trust CA certificate bundle by inheriting |getTrustedCerts|
+ */
+public abstract class TlsOptions {


So this is expected to be extended by users? It seems it would be best to remove private VerificationAuthType verificationType; and just have users implement the getVerificationAuthType() method directly.

Since this is intended to be extended with custom behavior, "options" doesn't really fit as a name.

Sorry, I'd like to discuss a bit more about this here. Making it an interface seems a reasonable common practice, but the implication becomes slightly different. Before, I want users to set a VerificationAuthType at the construction time, and this can be expressed by making it a construction function of an abstract class; If using interface, users may need to read the comments to know they need to set VerificationAuthType. The method might be more like setVerificationAuthType instead of getVerificationAuthType if using interface. May I know your opinions?
Also, does TlsConfig sound like a better name for you?

I was not suggesting to make it an interface; keep it as an abstract class. We use abstract classes so we can add methods to them later.

No, even if the user is implementing it, getVerificationAuthType() is a proper name for it. I don't think that would cause a problem for Java programmers, especially those that would be implementing this.

The other option is to guarantee that the verification type does not change over time and make getVerificationAuthType() final. The problem now is that it can be overridden but it is also required for the constructor.

SG. Updated accordingly.
Regarding the name of this class, would TlsConfig sound better?

ejona86 · 2020-04-02T16:45:45Z

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

+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509ExtendedTrustManager;
+
+public class ConfigurableX509TrustManager extends X509ExtendedTrustManager {


When would a user extend the class instead of delegating to the class? Since all the protected+public methods are public in X509ExtendedTrustManager, I can't figure out any purpose to extending this class. As a general rule it is much better to mark things final.

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

ejona86 · 2020-04-02T16:56:21Z

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

+
+  @Override
+  public X509Certificate[] getAcceptedIssuers() {
+    return new X509Certificate[0];


It looks like this could be implemented, but using KeyStore.aliases() and KeyStore.getCertificate(alias).

Implementing it will cause one test to fail, and I think the cause is probably https://github.com/netty/netty/blob/netty-4.1.48.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java#L153-L164. It looks like if setting this, Netty internally will use it to do some additional checks, which is not the behavior I want. I might want checkClientTrusted and checkServerTrusted to cover all the trust behaviors we might have during the authentication.

Hmm... That's sort of worrisome. Go ahead and put a comment here saying why it is empty.

Actually... this looks like it is only for certificate chain verification. What does it break?

I've summarized my findings in this Gist: https://gist.github.com/ZhenLian/50c73f146493ac2fe9746d196082679b.
If I changed getAcceptedIssuers to the one named change.java in my Gist, one of the tests I wrote would fail, with the error message named failure.log.
That is actually an expected behavior under my current implementation. The failing test is basically to test under the scenario that server chooses to SkipAllVerification, and client sends a bad certificate, in mTLS. This should be OK because server chooses to skip all verification(in real situations, we want application to provide additional custom checks, but since this is test, we just test the simple behavior). But this test won't pass if we have additional check in getAcceptedIssuers.

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

ejona86 · 2020-04-02T17:01:48Z

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

+          String algorithm = authType == VerificationAuthType.CertificateAndHostNameVerification
+              ? "HTTPS" : "";
+          SSLParameters sslParams = sslEngine.getSSLParameters();
+          sslParams.setEndpointIdentificationAlgorithm(algorithm);


It is surprising this is being set, since these variables are normally set elsewhere are read within the trust manager as configuration. This looks suspicious. It will also permanently change the parameters, which seems concerning. Have you verified that it is safe to disable endpoint identification here and it not impact any other code?

Yes, thanks for the reminding! I have tested it out locally and it seems good. I also added some integration tests(basicClientSideIntegrationTest and basicServerSideIntegrationTest), and it looks like this EndpointIdentificationAlgorithm behaves as expected, without impacting other parts of the code. Please let me know if you find anything suspicious, thanks!

"I tested it once" isn't what I really meant by verify. But since this on the engine, it is probably okay.

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

ZhenLian · 2020-04-16T21:25:01Z

@ejona86
Hi Eric, thank you so much for your comments! Apologize for the late responses since I was busy with other stuffs. I left a few more comments on your review. Please take a look when you have time and let me know your opinions.
By the way, this PR keeps failing on the codecov, and from the report it is complaining about the coverage of some override functions(https://codecov.io/gh/grpc/grpc-java/compare/e29561fbca3eb7a536b38d0e0b91c95caef94700...624edfdae8c0feee597cf3399579b798519ff5cf/diff#D2-47...82). I added a unit test(basicConfigurableX509TrustManagerTest) and tried to explicit call these uncovered functions, but the codecov is still complaining about it. Locally, it looks like these functions are indeed invoked and can be referenced to the correct function I want them to be through IDE. I have no idea what's going on. Can you provide any guidance on this please?
Thanks again for taking a look into this ;-)

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

ejona86 · 2020-04-17T23:47:58Z

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

+
+  @Override
+  public X509Certificate[] getAcceptedIssuers() {
+    return new X509Certificate[0];


Hmm... That's sort of worrisome. Go ahead and put a comment here saying why it is empty.

ZhenLian · 2020-04-29T05:46:49Z

I've updated the tests and now the coverage issue is gone. I've also added some more comments since your last review @ejona86 Could you please take a look when you have time please? Thanks!

ZhenLian · 2020-05-07T21:14:10Z

@ejona86 Friendly ping :-)

ejona86 · 2021-08-12T22:02:50Z

Is #8175 intended to replace this PR?

ZhenLian · 2021-08-13T14:55:18Z

@ejona86
Ah yes, let me just close this PR in favor of #8175

Add a New ConfigurableX509TrustManager Class

0914bcb

sanjaypujare reviewed Mar 5, 2020

View reviewed changes

netty/src/main/java/io/grpc/netty/TlsOptions.java Outdated Show resolved Hide resolved

sanjaypujare reviewed Mar 5, 2020

View reviewed changes

netty/src/main/java/io/grpc/netty/TlsOptions.java Outdated Show resolved Hide resolved

sanjaypujare reviewed Mar 5, 2020

View reviewed changes

netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java Outdated Show resolved Hide resolved

make class comments to JavaDoc comments

39a2363

ZhenLian force-pushed the zhen_advancedtls_trust_1 branch from af2d9ca to 39a2363 Compare March 5, 2020 18:55

ZhenLian requested a review from sanjaypujare March 5, 2020 19:13

ejona86 reviewed Apr 2, 2020

View reviewed changes

fix first round of feedback

e7b18a0

ZhenLian force-pushed the zhen_advancedtls_trust_1 branch 2 times, most recently from 88eba1a to d45e51a Compare April 16, 2020 19:14

add more tests

624edfd

ZhenLian force-pushed the zhen_advancedtls_trust_1 branch from d45e51a to 624edfd Compare April 16, 2020 20:31

ejona86 mentioned this pull request Apr 17, 2020

Server cert hot reloading #5335

Closed

ejona86 reviewed Apr 17, 2020

View reviewed changes

ZhenLian added 2 commits April 20, 2020 17:36

fix second round of feedbacks

1bb3e43

improve test coverage rate

89b5c4f

ZhenLian force-pushed the zhen_advancedtls_trust_1 branch from 82781db to 89b5c4f Compare April 29, 2020 05:19

ZhenLian requested a review from ejona86 April 29, 2020 05:44

ZhenLian closed this Aug 13, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a New ConfigurableX509TrustManager Class #6805

Add a New ConfigurableX509TrustManager Class #6805

ZhenLian commented Mar 5, 2020

sanjaypujare Mar 5, 2020

ZhenLian Mar 5, 2020

ejona86 Apr 2, 2020

sanjaypujare commented Mar 5, 2020

ZhenLian commented Mar 5, 2020

sanjaypujare commented Mar 5, 2020

ZhenLian commented Mar 5, 2020

sanjaypujare commented Mar 5, 2020

ZhenLian commented Mar 11, 2020

ejona86 Apr 2, 2020

ZhenLian Apr 16, 2020

ejona86 Apr 2, 2020

ZhenLian Apr 16, 2020

ejona86 Apr 17, 2020

ZhenLian Apr 21, 2020

ejona86 Apr 2, 2020

ejona86 Apr 2, 2020

ZhenLian Apr 16, 2020 •

edited

ejona86 Apr 17, 2020

ejona86 Apr 17, 2020

ZhenLian Apr 21, 2020

ejona86 Apr 2, 2020

ZhenLian Apr 16, 2020 •

edited

ejona86 Apr 17, 2020

ZhenLian commented Apr 16, 2020

ejona86 Apr 17, 2020

ZhenLian commented Apr 29, 2020

ZhenLian commented May 7, 2020

ejona86 commented Aug 12, 2021

ZhenLian commented Aug 13, 2021

Add a New ConfigurableX509TrustManager Class #6805

Add a New ConfigurableX509TrustManager Class #6805

Conversation

ZhenLian commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaypujare commented Mar 5, 2020

ZhenLian commented Mar 5, 2020

sanjaypujare commented Mar 5, 2020

ZhenLian commented Mar 5, 2020

sanjaypujare commented Mar 5, 2020

ZhenLian commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenLian Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenLian Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenLian commented Apr 16, 2020

Choose a reason for hiding this comment

ZhenLian commented Apr 29, 2020

ZhenLian commented May 7, 2020

ejona86 commented Aug 12, 2021

ZhenLian commented Aug 13, 2021

ZhenLian Apr 16, 2020 •

edited

ZhenLian Apr 16, 2020 •

edited