Added AOSP OkHostnameVerifier to have usable default ConscryptHostnameVerifier #867

d-reidenbach · 2020-07-24T23:39:02Z

initial PR just to go over the path in how I am using the new verifier and plumbing it through the two platforms.

common/src/main/java/org/conscrypt/TrustManagerImpl.java

veblush

Thanks for the work! It seems right to me. Probably tests for OkHostnameVerifier need to be added as well? If this is ready to run, I can verify this with gRPC to check whether it can work with gRPC TLS which wasn't working with Conscrypt 2.4.

daulet · 2020-07-28T13:51:32Z

I think tests have to be part of this PR: (1) negative test to verify we don't have accept all verifier and (2) positive one to verify this new HostnameVerifier.

prbprbprb · 2020-07-28T14:47:17Z

I think tests have to be part of this PR: (1) negative test to verify we don't have accept all verifier and (2) positive one to verify this new HostnameVerifier.

Yeah, Danny is planning to bring over the unit test for OkHostnameVerifier which covers most of the requirements but it has a few dependencies on other AOSP code that need some work.

common/src/main/java/org/conscrypt/Conscrypt.java

veblush · 2020-07-29T00:40:32Z

This PR still can't allow gRPC to connect to GCS server with the same NoRouteToHostException error. Test was done with this PR, gRPC Java 1.30.x and Netty 4.1.51. Netty was upgraded deliberately to get netty/netty#10375 not to have error Unable to access wrapped TrustManager with java.lang.NoSuchFieldException.

To see why OkHostnameVerifier rejects the host, logger is added to the class like

     public boolean verify(String host, SSLSession session) {
+       logger.info("verify: host=" + host);
         try {
             Certificate[] certificates = session.getPeerCertificates();
             return verify(host, (X509Certificate) certificates[0]);
         } catch (SSLException e) {
+           logger.log(Level.INFO, "verify exception", e);
             return false;
         }

And the log is as follows (full log is conscrypt-pr-netty4.1.51.txt)

Jul 29, 2020 12:21:22 AM org.conscrypt.OkHostnameVerifier verify
INFO: verify: host=storage.googleapis.com
Jul 29, 2020 12:21:22 AM org.conscrypt.OkHostnameVerifier verify
INFO: verify exception
javax.net.ssl.SSLPeerUnverifiedException: peer not verified
	at io.grpc.netty.shaded.io.netty.handler.ssl.ReferenceCountedOpenSslEngine$DefaultOpenSslSession.getPeerCertificates(ReferenceCountedOpenSslEngine.java:2373)
	at io.grpc.netty.shaded.io.netty.handler.ssl.ExtendedOpenSslSession.getPeerCertificates(ExtendedOpenSslSession.java:130)
	at org.conscrypt.OkHostnameVerifier.verify(OkHostnameVerifier.java:81)
	at org.conscrypt.TrustManagerImpl.checkTrusted(TrustManagerImpl.java:407)
	at org.conscrypt.TrustManagerImpl.getTrustedChainForServer(TrustManagerImpl.java:356)
	at org.conscrypt.TrustManagerImpl.checkServerTrusted(TrustManagerImpl.java:369)
	at io.grpc.netty.shaded.io.netty.handler.ssl.OpenSslTlsv13X509ExtendedTrustManager.checkServerTrusted(OpenSslTlsv13X509ExtendedTrustManager.java:223)
	at io.grpc.netty.shaded.io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify(ReferenceCountedOpenSslClientContext.java:261)
	at io.grpc.netty.shaded.io.netty.handler.ssl.ReferenceCountedOpenSslContext$AbstractCertificateVerifier.verify(ReferenceCountedOpenSslContext.java:700)
	at io.grpc.netty.shaded.io.netty.internal.tcnative.SSL.readFromSSL(Native Method)
	at io.grpc.netty.shaded.io.netty.handler.ssl.ReferenceCountedOpenSslEngine.readPlaintextData(ReferenceCountedOpenSslEngine.java:595)
	at io.grpc.netty.shaded.io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1202)
	at io.grpc.netty.shaded.io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1324)
	at io.grpc.netty.shaded.io.netty.handler.ssl.SslHandler$SslEngineType$1.unwrap(SslHandler.java:201)
	at io.grpc.netty.shaded.io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1380)
	at io.grpc.netty.shaded.io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1275)
	at io.grpc.netty.shaded.io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1322)
	at io.grpc.netty.shaded.io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:501)
	at io.grpc.netty.shaded.io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:440)
	at io.grpc.netty.shaded.io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
	at io.grpc.netty.shaded.io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.grpc.netty.shaded.io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.grpc.netty.shaded.io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
	at io.grpc.netty.shaded.io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
	at io.grpc.netty.shaded.io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.grpc.netty.shaded.io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.grpc.netty.shaded.io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
	at io.grpc.netty.shaded.io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:792)
	at io.grpc.netty.shaded.io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:475)
	at io.grpc.netty.shaded.io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:378)
	at io.grpc.netty.shaded.io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
	at io.grpc.netty.shaded.io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.grpc.netty.shaded.io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.lang.Thread.run(Thread.java:748)

veblush · 2020-07-29T00:54:19Z

SSLPeerUnverifiedException: peer not verified was thrown at ReferenceCountedOpenSslEngine because the peer certificates is empty. It isn't clear to me why there was no certificate.

@ejona86 Can you take a look at this?

veblush · 2020-07-29T02:34:21Z

From my code-read, it seems that there are some mismatch between Netty and Conscrypt.

Netty inits the peer's certificates after the handshake finishes. (code) In this case, however, the process of handshake is in progress so certs cannot be there and calling getPeerCertificates() ends up returning empty.

Conscrypt seems to follow this semantics; TrustManagerImpl.checkTrusted passes certs as a parameter along with session, indicating that validator needs to use them rather than fetching it from session. But this is dropped from ConscryptHostnameVerifier since it doesn't have the parameter for certs in the verify method. (code) and it has to get certs from session, which looks wrong.

I think that a solution is adding cert parameter to ConscryptHostnameVerifier.verify (code) method so that OkHostnameVerifier can use it directly.

@prbprbprb @daulet WDTY?

ejona86 · 2020-07-29T14:51:57Z

Netty inits the peer's certificates after the handshake finishes

This is required, as getPeerCertificates() returns verified certificates. The documentation has: "SSLPeerUnverifiedException - if the peer's identity has not been verified". It is dangerous to mix verified and unverified certificates in one API.

prbprbprb · 2020-07-29T15:31:16Z

This is true, but presumably the code calling checkServerTrusted() also has verified certificates because it either supplied them itself or got them via getPeerCertificates().

On the one hand, passing the handshake session to the hostname verifier guarantees it looks at the leaf certificate even if the calling code only passes in a partial chain¹, but I also suspect the shape of the internal APIs were driven by the (ab)use of HttpsUrlConnection.HostnameVerifier on Android. @kruton may remember as he wrote some of this code originally.

We can refactor this later if it turns out to be wrong but I don't want to introduce a behaviour change that might cause Android app compat issues as part of this CL.

I can't think of a valid use case for this, but the API does allow for it

prbprbprb · 2020-07-29T17:17:43Z

Oh sorry, somehow my email thread didn't include Esun's exception. It sounds like Netty's SSLSession implementation is the same as Conscrypt's and only throws if certificates are not present. But that raises the question as to why Netty is calling checkServerTrusted() before it has received certificates, or more likely before it's added them to the handshake SSLSession?

I hadn't really considered the scenario that stack trace suggests where you are using Netty's SSLEngine and SSLSession implementations but passing them into Conscrypt's X509ExtendedTrustManager.

It looks like they're maybe calling back into Java from native code to verify certificates (maybe as a prelude to adding them to the handshake SSLContext), but they're also checking whether the certificate is trusted at the same time (otherwise why are they calling checkServerTrusted. That approach is different to Conscrypt's where certificates are verified by the native layer, then once they are all received the entire chain is checked for trustworthiness.

I'm fairly confident the Conscrypt approach is correct (I don't think it's unreasonable to expect that an SSLEngine passed into checkServerTrusted() to have a valid handshake SSLSession), but it makes me wonder how Conscrypt and Netty have ever worked together, unless people are installing no-op hostname verifiers.

veblush · 2020-07-29T20:55:33Z

As a proof-of-concept (veblush@b8e0514), I tried to change the signature of ConscryptHostnameVerifier.verify like

public interface ConscryptHostnameVerifier {
  boolean verify(X509Certificate[] certs, String hostname, SSLSession session);
}

and made OkHostnameVerifier to use the first value of certs instead of getting it from session and it worked now with gRPC! I like to change the class in this way but it's up to you to measure how invasive this would be. I briefly checked AOSP, github, and google3, there doesn't seem to be a case using ConscryptHostnameVerifier directly.

There is another way to detour this; Introducing ConscryptHostnameVerifier2 with the new verify function and making OkHostnameVerifier implement ConscryptHostnameVerifier2 and TrustManagerImpl prefer ConscryptHostnameVerifier2 over the original. In this way, we can detour the breaking change in the public API at the cost of having two interfaces.

public interface ConscryptHostnameVerifier {
  boolean verify(String hostname, SSLSession session);
}

public interface ConscryptHostnameVerifier2 {
  boolean verify(X509Certificate[] certs, String hostname, SSLSession session);
}

OkHostnameVerifier implements ConscryptHostnameVerifier, ConscryptHostnameVerifier2 { ... }

TrustManagerImpl.checkTrusted() {
  ...
  ConscryptHostnameVerifier verifier = getHttpsVerifier();
  if (verifier instanceof ConscryptHostnameVerifier) {
    // Use V2
    ((ConscryptHostnameVerifier2)verifier).verify(certs, hostname, session);
  } else {
    // Use V1
    verifier.verify(hostname, session)
  }
  ...
}

veblush · 2020-07-29T21:09:42Z

@normanmaurer Can you please check #867 (comment) about how Netty and Conscrypt work together?

prbprbprb · 2020-07-30T16:53:42Z

As a proof-of-concept (veblush@b8e0514), I tried to change the signature of ConscryptHostnameVerifier.verify like

Yeah, that's the way I'd do it too...introducing yet another verifier abstraction will lead to future pain.

On the face of it this is quite attractive - no behaviour change on Android (because wrapHostnameVerifier() can just drop the certificate array for now) and it gives us a path to eventually stop depending on Android's peculiarities.

However it leads to some ambiguity with user-supplied ConscryptHostnameVerifier classes (which you note, are not very prevalent ;) ) because depending on whether the TLS transport is Netty or Conscrypt the SSLSession passed in may or may not be complete.

So I do have some more questions before we commit to this:

Is the calling pattern in your original stack trace deliberate, or maybe some misconfiguration? i.e. it has what looks like a Netty SSLSEngine calling back into Conscrypt's trust manager and passing one of Netty's SSLSession objects.
From the fact that your source change worked, it sounds like Netty is only calling checkServerTrusted() once, for the whole certificate chain. Is it maybe a bug that they're not populating the certificate list in the handshake SSLSession first?

veblush · 2020-07-31T00:17:12Z

I don't know much about the detail on how Netty and Conscrypt works to answer your questions correctly. AFAIK, configuration should be correct. I guess it's expected that the Conscrypt's trust manager is called by Netty because Conscrypt was given as a default security provider by the my benchmark's init code.

Security.insertProviderAt(Conscrypt.newProvider(), 1);

FYI, we used to use following code to detour this problem but it has its own problem which cannot support TLS 1.3.

Security.insertProviderAt(Conscrypt.newProviderBuilder().provideTrustManager(false).build(), 1);

ejona86 · 2020-07-31T00:22:57Z

Is the calling pattern in your original stack trace deliberate, or maybe some misconfiguration? i.e. it has what looks like a Netty SSLSEngine calling back into Conscrypt's trust manager and passing one of Netty's SSLSession objects.

In that test Conscrypt is the very first security provider, to improve performance HTTP clients. So it is the default TLS provider or purpose and it just so happens to be the default TrustManager provider.

Because the default TLS provider has been classically poor, gRPC uses netty_tcnative if it is available. Long-term we'd like to change that, but it has been the long-standing behavior. By default, Netty will use the default TrustManager.

That means gRPC will select netty_tcnative and Netty will use Conscrypt's TrustManager with it. I'd say "everything is working as intended" since the user did specify Conscrypt as the first security provider, and gRPC is not yet favoring Conscrypt if both Conscrypt and tcnative are available.

From the fact that your source change worked, it sounds like Netty is only calling checkServerTrusted() once, for the whole certificate chain. Is it maybe a bug that they're not populating the certificate list in the handshake SSLSession first?

Isn't it only supposed to be called once? No, it is not a bug that SSLSession lacks the certificate list. As I said earlier it is mandatory per the API. If Conscrypt is setting the certificate chain in SSLSession before the TrustManager processes the chain, then it seems that could be considered a security vulnerability. (Not very severe of a vulnerability on OpenJDK, since it's unlikely for there to be broken code on the JDK since the JDK will throw in that situation. On Android it would be a bigger problem.)

prbprbprb · 2020-07-31T15:44:16Z

Because the default TLS provider has been classically poor, gRPC uses netty_tcnative if it is available. Long-term we'd like to change that, but it has been the long-standing behavior. By default, Netty will use the default TrustManager.

Fair enough.

Isn't it only supposed to be called once?

Yup, I was correcting my speculation above that maybe Netty was calling once for each peer certificate

No, it is not a bug that SSLSession lacks the certificate list. As I said earlier it is mandatory per the API. If Conscrypt is setting the certificate chain in SSLSession before the TrustManager processes the chain, then it seems that could be considered a security vulnerability. (Not very severe of a vulnerability on OpenJDK, since it's unlikely for there to be broken code on the JDK since the JDK will throw in that situation. On Android it would be a bigger problem.)

I think there's some confusion here. Absolutely certificates can't go into the final SSLSession unless they're trusted. But the handshake SSLSession is only used for the duration of the TLS handshake and is needed to store session parameters during negotiation.

Unhelpfully the JSSE ref guide doesn't say whether those parameters may include certificates under consideration, just "In some cases, parameters negotiated during the handshake are needed later in the handshake to make decisions about trust". However I tested on the RI (openjdk 8 and 11) and neither of these populate the certificates in the handshake session, so this looks like another Consrypt "quirk", possibly added just to support the way hostname verification works on Android. There's no vulnerability though, the peer certificates still won't get into the final SSLSession unless they're trusted.

Anyway, sorry if this seems like a lot of angsting over a small detail, but (1) eventually ConscrypyHostnameVerifier ought to be a public API on Android and once we do that it'll be hard to change it again for a long time and (2) as you note, any mistakes in this area can lead to vulnerabilities.

I think the upshot is:-

What Netty is doing is reasonable and consistent with the RI and so Conscrypt should support it, i.e. the default ConscryptHostnameVerifier should prefer the certificate list passed to checkServerTrusted() when it's available.
Ideally we should do the same on Android (both in the platform itself and the unbundled library), but for app compat reasons we will have to keep supporting the HttpsUrlConnection.HostnameVerifier way of injecting a hostname verifier. We can (hopefully!) deprecate that at the next API version bump, but we'll need to support apps targeting older API versions for years to come.

tl;dr for Daniel: We should make the interface look like this and the default implementation (used on unbundled android and openjdk) should only look at the certs argument:

public interface ConscryptHostnameVerifier {
  boolean verify(X509Certificate[] certs, String hostname, SSLSession session);
}

common/src/main/java/org/conscrypt/Conscrypt.java

common/src/test/java/org/conscrypt/TrustManagerImplTest.java

prbprbprb

Thanks! Sorry for the delay reviewing, I thought we were still waiting for the HostnameVerifierTest, I didn't notice that Github was just hiding the diff :/

common/src/main/java/org/conscrypt/OkHostnameVerifier.java

common/src/test/java/org/conscrypt/TrustManagerImplTest.java

prbprbprb

Thanks for all your work on this!

veblush · 2020-08-12T18:24:58Z

Danny, thank you for your work!
Pete, can you merge this and import it so that we can see the result?

veblush reviewed Jul 27, 2020

View reviewed changes

common/src/main/java/org/conscrypt/TrustManagerImpl.java Show resolved Hide resolved

veblush approved these changes Jul 27, 2020

View reviewed changes

prbprbprb requested changes Jul 28, 2020

View reviewed changes

common/src/main/java/org/conscrypt/Conscrypt.java Outdated Show resolved Hide resolved

d-reidenbach force-pushed the trust branch from b8009c1 to 11542e7 Compare August 4, 2020 04:28

d-reidenbach requested a review from prbprbprb August 4, 2020 05:04

d-reidenbach added 11 commits August 4, 2020 05:11

Copied in AOSP OKHostnameVerifier and used it in TrustManagerImpl

95889a8

Connected Platforms and removed default initialization

db7d7da

Cleaned up code

6cf7ad7

Fixed Typos

b5c6d87

Added TrustManagerTesting as well as third non bound platform

ed0bc29

removed import for ubuntu integration test

6003260

Added HostnameVerifierTest

64d055a

Fixed AOSP style and added HostnameVerifierTest

a3d93d3

Fixed Style for travis ci

566b507

Updated verify method to allow certificates to be passed in directly

3d21617

Cleaned up comments

52a8043

d-reidenbach added 3 commits August 4, 2020 05:11

fixed merge conflict

e961420

Fixed style for travis

8345727

update for warnings

878f0ce

d-reidenbach force-pushed the trust branch from 61bee28 to 878f0ce Compare August 4, 2020 05:32

prbprbprb requested changes Aug 4, 2020

View reviewed changes

d-reidenbach force-pushed the trust branch 3 times, most recently from 4df5eda to ab41c26 Compare August 5, 2020 23:44

Fixed test and updated style.

faaf7b2

d-reidenbach force-pushed the trust branch from ab41c26 to faaf7b2 Compare August 6, 2020 17:04

d-reidenbach added 2 commits August 6, 2020 18:21

iffy integration test

e2653a7

Iffy testing

9634556

prbprbprb requested changes Aug 10, 2020

View reviewed changes

Upgraded TrustManagerImplTest as well as suppressed warnings

d61af15

d-reidenbach force-pushed the trust branch from 202ca98 to d61af15 Compare August 10, 2020 20:53

Remove commented out code and simplify slightly

840aa5e

prbprbprb approved these changes Aug 10, 2020

View reviewed changes

Flaky Integration Commit

b45dbe8

Merge branch 'master' into trust

c209bc9

prbprbprb merged commit 8ccdfe5 into google:master Aug 13, 2020

veblush mentioned this pull request Aug 20, 2020

Upgrading Conscrypt to 2.5.1 grpc/grpc-java#7342

Merged

voidzcy mentioned this pull request Sep 16, 2020

Revert "SECURITY.md: add instruction for disabling Conscrypt's defaul… grpc/grpc-java#7428

Merged

veblush mentioned this pull request Sep 30, 2020

[BEAM-5748] Upgrade conscrypt to latest version apache/beam#12965

Merged

4 tasks

prbprbprb mentioned this pull request Oct 8, 2020

Spike in "Trust anchor for certification path not found " errors #881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added AOSP OkHostnameVerifier to have usable default ConscryptHostnameVerifier #867

Added AOSP OkHostnameVerifier to have usable default ConscryptHostnameVerifier #867

d-reidenbach commented Jul 24, 2020

veblush left a comment

daulet commented Jul 28, 2020

prbprbprb commented Jul 28, 2020

veblush commented Jul 29, 2020

veblush commented Jul 29, 2020 •

edited

veblush commented Jul 29, 2020 •

edited

ejona86 commented Jul 29, 2020

prbprbprb commented Jul 29, 2020 •

edited

prbprbprb commented Jul 29, 2020

veblush commented Jul 29, 2020 •

edited

veblush commented Jul 29, 2020

prbprbprb commented Jul 30, 2020

veblush commented Jul 31, 2020 •

edited

ejona86 commented Jul 31, 2020

prbprbprb commented Jul 31, 2020

prbprbprb left a comment

prbprbprb left a comment

veblush commented Aug 12, 2020

Added AOSP OkHostnameVerifier to have usable default ConscryptHostnameVerifier #867

Added AOSP OkHostnameVerifier to have usable default ConscryptHostnameVerifier #867

Conversation

d-reidenbach commented Jul 24, 2020

veblush left a comment

Choose a reason for hiding this comment

daulet commented Jul 28, 2020

prbprbprb commented Jul 28, 2020

veblush commented Jul 29, 2020

veblush commented Jul 29, 2020 • edited

veblush commented Jul 29, 2020 • edited

ejona86 commented Jul 29, 2020

prbprbprb commented Jul 29, 2020 • edited

prbprbprb commented Jul 29, 2020

veblush commented Jul 29, 2020 • edited

veblush commented Jul 29, 2020

prbprbprb commented Jul 30, 2020

veblush commented Jul 31, 2020 • edited

ejona86 commented Jul 31, 2020

prbprbprb commented Jul 31, 2020

prbprbprb left a comment

Choose a reason for hiding this comment

prbprbprb left a comment

Choose a reason for hiding this comment

veblush commented Aug 12, 2020

veblush commented Jul 29, 2020 •

edited

veblush commented Jul 29, 2020 •

edited

prbprbprb commented Jul 29, 2020 •

edited

veblush commented Jul 29, 2020 •

edited

veblush commented Jul 31, 2020 •

edited