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

Add a New ConfigurableX509TrustManager Class #6805

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
134 changes: 134 additions & 0 deletions netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
@@ -0,0 +1,134 @@
/*
* Copyright 2020 The gRPC Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.grpc.netty;

import io.grpc.netty.TlsOptions.VerificationAuthType;
import java.net.Socket;
import java.security.KeyStore;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLParameters;
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509ExtendedTrustManager;

public class ConfigurableX509TrustManager extends X509ExtendedTrustManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Javadoc comments for public class?
  2. Can it be marked final?

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 would expect some other users to further extend this class. In that case, it is better to not make it final?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


private TlsOptions tlsOptions;

public ConfigurableX509TrustManager(TlsOptions tlsOptions) {
this.tlsOptions = tlsOptions;
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s, Socket socket)
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
throws CertificateException {

}

@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s, SSLEngine sslEngine)
throws CertificateException {
checkTrusted(x509Certificates, s, sslEngine, false);
}

@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
throws CertificateException {

}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s, Socket socket)
throws CertificateException {

}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s, SSLEngine sslEngine)
throws CertificateException {
checkTrusted(x509Certificates, s, sslEngine, true);
}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
throws CertificateException {

}

@Override
public X509Certificate[] getAcceptedIssuers() {
return new X509Certificate[0];
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ZhenLian ZhenLian Apr 16, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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'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.

}

private void checkTrusted(X509Certificate[] x509Certificates, String s, SSLEngine sslEngine,
boolean isClient) throws CertificateException {
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
VerificationAuthType authType = this.tlsOptions.getVerificationAuthType();
if (authType == VerificationAuthType.CertificateAndHostNameVerification
|| authType == VerificationAuthType.CertificateVerification) {
if (x509Certificates == null || x509Certificates.length == 0) {
throw new CertificateException(
"Client side requires certificate but got null or empty certificates");
}
KeyStore ks;
try {
ks = this.tlsOptions.getTrustedCerts();
} catch (Exception e) {
throw new CertificateException("Function getTrustedCerts fails, error: " + e.getMessage());
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
}
X509ExtendedTrustManager delegateManager = null;
try {
final TrustManagerFactory tmf = TrustManagerFactory
.getInstance(TrustManagerFactory.getDefaultAlgorithm());
tmf.init(ks);
TrustManager[] tms = tmf.getTrustManagers();
// Iterate over the returned trust managers, looking for an instance of X509TrustManager.
// If found, use that as the delegate trust manager.
for (int i = 0; i < tms.length; i++) {
if (tms[i] instanceof X509ExtendedTrustManager) {
delegateManager = (X509ExtendedTrustManager) tms[i];
break;
}
}
if (delegateManager == null) {
throw new CertificateException(
"Instance delegateX509TrustManager is null. Failed to initialize");
}
} catch (Exception e) {
throw new CertificateException("Failed to initialize delegateX509TrustManager, error: "
+ e.getMessage());
}
if (isClient) {
String algorithm = authType == VerificationAuthType.CertificateAndHostNameVerification
? "HTTPS" : "";
SSLParameters sslParams = sslEngine.getSSLParameters();
sslParams.setEndpointIdentificationAlgorithm(algorithm);
sslEngine.setSSLParameters(sslParams);
delegateManager.checkServerTrusted(x509Certificates, s, sslEngine);
} else {
delegateManager.checkClientTrusted(x509Certificates, s, sslEngine);
}
}
// Perform custom check
try {
this.tlsOptions.verifyPeerCertificate(x509Certificates, s, sslEngine);
} catch (Exception e) {
throw new CertificateException("Custom authorization check fails, error: " + e.getMessage());
}
}
}
62 changes: 62 additions & 0 deletions netty/src/main/java/io/grpc/netty/TlsOptions.java
@@ -0,0 +1,62 @@
/*
* Copyright 2020 The gRPC Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.grpc.netty;

import java.security.KeyStore;
import java.security.cert.X509Certificate;
import javax.net.ssl.SSLEngine;

// TlsOptions contains different options users could choose. In a nutshell, it provides three main
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
// features users could customize:
// 1. choose different levels of peer verification by specifying |VerificationAuthType|
// 2. provide custom peer verification check by inheriting |verifyPeerCertificate|
// 3. change the trust CA certificate bundle by inheriting |getTrustedCerts|
public abstract class TlsOptions {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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. Updated accordingly.
Regarding the name of this class, would TlsConfig sound better?

// VerificationAuthType contains set of verification levels users can choose to customize
// their checks against its peer.
// Note we don't have hostname check on server side. Choosing CertificateAndHostNameVerification
// has the same effect as choosing CertificateVerification on server side, in terms of peer
// endpoint check.
public enum VerificationAuthType {
// Default option: performs certificate verification and hostname verification.
CertificateAndHostNameVerification,
// Performs certificate verification, but skips hostname verification.
// Users are responsible for verifying peer's identity via custom check callback.
CertificateVerification,
// Skips both certificate and hostname verification.
// Users are responsible for verifying peer's identity and peer's certificate via custom
// check callback.
SkipAllVerification,
}

private VerificationAuthType verificationType;

public TlsOptions(VerificationAuthType verificationType) {
this.verificationType = verificationType;
}

public VerificationAuthType getVerificationAuthType() {
return this.verificationType;
}

// used to perform custom peer authorization checking
abstract void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType,
SSLEngine engine) throws Exception;
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved

// used to perform trust CA certificates reloading
abstract KeyStore getTrustedCerts() throws Exception;
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
}
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved