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 all commits
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
152 changes: 152 additions & 0 deletions netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
@@ -0,0 +1,152 @@
/*
* 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 static com.google.common.base.Preconditions.checkNotNull;

import io.grpc.ExperimentalApi;
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;

/**
* ConfigurableX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to choose
* different level of peer checking mechanisms, as well as some customized check. It could also be
* used to reload trust certificate bundle client/server uses.
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/XXXX")
public final class ConfigurableX509TrustManager extends X509ExtendedTrustManager {

private TlsOptions tlsOptions;

public ConfigurableX509TrustManager(TlsOptions tlsOptions) {
this.tlsOptions = checkNotNull(tlsOptions, "tlsOptions");
}

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

@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 {
checkTrusted(x509Certificates, s, null, false);
}

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

@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 {
checkTrusted(x509Certificates, s, null, true);
}

// getAcceptedIssuers returns an empty list because if we don't do so, Netty will use the
// certificates returned here to do some additional checks, while we want the function
// checkClientTrusted or checkServerTrusted to take charge of the whole trust verification.
@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 checkingServer) throws CertificateException {
VerificationAuthType authType = this.tlsOptions.getVerificationAuthType();
if (authType == VerificationAuthType.CertificateAndHostNameVerification
|| authType == VerificationAuthType.CertificateVerification) {
if (x509Certificates == null || x509Certificates.length == 0) {
throw new CertificateException(
"Want certificate verification but got null or empty certificates");
}
KeyStore ks;
try {
ks = this.tlsOptions.getTrustedCerts();
} catch (Exception e) {
throw new CertificateException("Failed loading trusted certs", e);
}
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", e);
}
if (checkingServer) {
if (authType == VerificationAuthType.CertificateAndHostNameVerification
&& sslEngine == null) {
throw new CertificateException(
"SSLEngine is null. Couldn't check host name");
}
if (sslEngine != null) {
String algorithm = authType == VerificationAuthType.CertificateAndHostNameVerification
? "HTTPS" : "";
SSLParameters sslParams = sslEngine.getSSLParameters();
sslParams.setEndpointIdentificationAlgorithm(algorithm);
Copy link
Member

Choose a reason for hiding this comment

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

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?

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.

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!

Copy link
Member

Choose a reason for hiding this comment

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

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

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", e);
}
}
}
84 changes: 84 additions & 0 deletions netty/src/main/java/io/grpc/netty/TlsOptions.java
@@ -0,0 +1,84 @@
/*
* 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.ExperimentalApi;
import java.io.IOException;
import java.security.KeyStore;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import javax.net.ssl.SSLEngine;

/**
* TlsOptions contains different options users could choose. In a nutshell, it provides three main
* 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|
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/XXX")
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,
}

/**
* sub-classes extend this function to select proper {@code VerificationAuthType}.
* @return the selected VerificationAuthType
*/
abstract VerificationAuthType getVerificationAuthType();

/**
* sub-classes extend this function to perform custom peer identity checking.
* @param peerCertChain the certificate chain sent from the peer
* @param authType the key exchange algorithm used
* @param engine the engine used for this connection. This parameter can be null, which indicates
* that implementations need not check the ssl parameters
* @throws CertificateException exception thrown when performing custom peer identity check
*/
abstract void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType,
SSLEngine engine) throws CertificateException;

/**
* sub-classes extend this function to perform trust certificate bundle reloading.
* @return A KeyStore containing the trust certificate bundle that will be used for the following
* connections.
* @throws IOException exception thrown when performing trust certificate bundle reloading
*/
abstract KeyStore getTrustedCerts() throws IOException;
}