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
Changes from 1 commit
0914bcb
39a2363
e7b18a0
624edfd
1bb3e43
89b5c4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All new APIs should be marked as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this could be implemented, but using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
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()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Since this is intended to be extended with custom behavior, "options" doesn't really fit as a name. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, The other option is to guarantee that the verification type does not change over time and make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SG. Updated accordingly. |
||
// 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
?There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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
.