adoptium-support icon indicating copy to clipboard operation
adoptium-support copied to clipboard

Not all key types are passed to alias selection in SSL Client

Open thomasfox opened this issue 5 years ago • 27 comments

Platform: adoptopenjdk-8-hotspot (8u272-b10-3)

Architecture: linux amd64

We are creating a HTTPS connection to a server using client authentication. While connecting, the client explicitly chooses to present one specific entry in the client keystore by overriding X509KeyManager's chooseClientAlias method. However, in openjdk 8u272-b10-3, only the key type "EC" is presented when the chooseClientAlias method is called from the SSL code, whereas in openjdk 8u265-b01-3, the three key types "RSA", "DSA" and "EC" were presented. Because the used key is a RSA key, the client cannot authenticate any more in openjdk 8u272-b10-3.

Attached are a test case to reproduce the problem, together with the keystores and truststores used (nothing special about them apart from the keystore containing the alias "localhost"). The relevant line in the output is key types : [...], which prints the key types passed from the JDK SSL code to the custom key manager.

Desired behavior is to get called with the three key types "RSA", "DSA" and "EC" again. SslAliasChoosing.zip

Output when running the test in 8u272-b10-3 testSSL start key types : [EC] aliases for keyType EC are null valid aliases : [] testSSL stop

Output when running the test in 8u265-b01-3 testSSL start key types : [RSA, DSA, EC] aliases for keyType RSA are [localhost] aliases for keyType DSA are null aliases for keyType EC are null valid aliases : [localhost] [ [ Version: V3 Subject: CN=localhost, O=Seitenbau GmbH, L=Konstanz, ST=BW, C=DE .... ] handler was called Test successful testSSL stop

thomasfox avatar Oct 27 '20 08:10 thomasfox

@thomasfox Thanks for the detailed report and reproducer. Would you mind testing with https://adoptopenjdk.net/upstream.html?variant=openjdk8&jvmVariant=hotspot? That would help narrow down the source of the problem.

aahlenst avatar Oct 27 '20 09:10 aahlenst

One thing to note: 8u272 added suppprt for TLSv1.3 which wasn't available before. So it's possible that this altered the behaviour, for example because the server offers different cipher suites when negotiating TLSv1.3 in comparison to TLSv1.2. You can disable specific TLS versions and see if it changes something: https://www.java.com/en/configure_crypto.html

aahlenst avatar Oct 27 '20 10:10 aahlenst

@aahlenst, thanks for the quick reply. Sorry, I do not understand the question about the other build to check. The build you are pointing to is the one I thought I was using (installed via add-apt-repository --yes https://adoptopenjdk.jfrog.io/adoptopenjdk/deb/ && apt-get update && apt-get install -y adoptopenjdk-8-hotspot which currently points to version 8u272-b10-3).

On 8u272-b10-3: I eplicitly disabled TLSv1.3 by using SSLContext sslContext = SSLContext.getInstance("TLSv1.2") on the client, this changes nothing (from https://www.java.com/en/configure_crypto.html I got the impression that TLSv1.2 is the default anyway). I checked that TLSv1.3 is really disabled by printing Arrays.toString(((SSLSocket) socket).getEnabledProtocols() on the server during the handshake, this prints [TLSv1.2, TLSv1.1, TLSv1]. Same for explicitly using TLSv1.3 by using SSLContext sslContext = SSLContext.getInstance("TLSv1.3") on the client,

Then going back to TLSv1.1 using SSLContext sslContext = SSLContext.getInstance("TLSv1.1") on the client fixes the behaviour.

On 8u265-b01-3: On the contrary, explicitly using TLSv1.2 by using SSLContext sslContext = SSLContext.getInstance("TLSv1.2") on 8u265-b01-3 does not produce the problem. I was not sure whether TLS v1.2 is really enabled but using System.setProperty("javax.net.debug", "all") and looking in the output I see lines like main, WRITE: TLSv1.2 Handshake, length = 185 so it seems this is enabled.

So, after all, it seems to me that the problem ist in the TLSv1.2 (and probably TLSv1.3) implementation of 8u272-b10-3 but not in the TLSv1.2 implementation of 8u265-b01-3

thomasfox avatar Oct 27 '20 12:10 thomasfox

The build you are pointing to is the one I thought I was using.

No, they are not identical. https://adoptopenjdk.net/upstream.html?variant=openjdk8&jvmVariant=hotspot are builds made by the OpenJDK team at Red Hat. Bugs reported to OpenJDK are only accepted if the problem is reproducible with one of their builds.

aahlenst avatar Oct 27 '20 12:10 aahlenst

Problem reproduced on Windows build JDK 8u272-b10 from https://adoptopenjdk.net/upstream.html?variant=openjdk8&jvmVariant=hotspot for TLS 1.2. Verified that TLSv1.1 works as expected on this build (same result as above).

I need to withdraw the TLS1.3 statement for 8u272-b10 from above, the SSL debug output says "Negotiated protocol version: TLSv1.2" although I tried to enable TLSv1.3 using SSLContext.getInstance("TLSv1.3") on both server and client

thomasfox avatar Oct 27 '20 13:10 thomasfox

The new behaviour of 8u272 is consistent with OpenJDK 11 and 15.

This is not my area of expertise because I'm not that familiar with TLS handshakes and the current spec. This might be specific enough to warrant an inquiry on https://mail.openjdk.java.net/mailman/listinfo/net-dev if you don't want to ask on StackOverflow first.

aahlenst avatar Oct 27 '20 15:10 aahlenst

Asked at https://mail.openjdk.java.net/mailman/listinfo/net-dev for the contract of the javax.net.ssl.X509KeyManager.chooseClientAlias method

thomasfox avatar Oct 29 '20 05:10 thomasfox

I realized I probably sent you to the wrong list because net-dev explicitly states it's not about SSL. Please try security-dev and sorry for the misdirection.

aahlenst avatar Dec 03 '20 16:12 aahlenst

Closing due to lack of activity.

aahlenst avatar Dec 25 '20 11:12 aahlenst

Von: "Stefan Fritz" [email protected] An: "AdoptOpenJDK/openjdk-support" [email protected] CC: "Thomas Fox" [email protected], "Mention" [email protected] Gesendet: Dienstag, 9. Februar 2021 08:44:06 Betreff: Re: [AdoptOpenJDK/openjdk-support] Not all key types are passed to alias selection in SSL Client (#200)

[ https://github.com/thomasfox | @thomasfox ] Have you been able to solve this? Would appreciate it if you could share your findings/solution.


There are no definitive findings, I'm afraid.

Mailing to the net-dev list yielded no result at all.

Mailing to the security list on 28th Dec 2020 with the following email: I have a question regarding the contract of the method javax.net.ssl.X509KeyManager.chooseClientAlias(String[] keyType, Principal[] issuers, Socket socket). This method gets called by SSL code on a client when a SSL connection is opened to a server, the server requests client authentication and the client wants to choose which one of the available keys is used for the authentication. The question is whether the SSL code should pass all possible key types in the argument keyType, or should it pass only one (probably the favoured key type???) in the argument keyType? The javadoc of the argument says "keyType - the key algorithm type name(s), ordered with the most-preferred key type first.", which leaves a little room to interpretation (is it "all acceptable key algorithm type name(s)" or "a subset of all allowed key algorithm type name(s)"?).

Background is that the argument keyType passed by the SSL code is different between (A) openjdk 8u272 TLSv1.2 on the one hand and (B) openjdk 8u272 TLSv1.1, openjdk 8u265 TLSv1.1 and openjdk 8u265 TLSv1.2 on the other hand. In the former case (A), ["EC"] is passed as keyType, whereas in the latter case (B), ["RSA", "DSA", "EC"] is passed as key type, with no other changes except from JDK and TLS version. Note that in all cases, at least the key type "RSA" is acceptable as a key type. The question is if the behavior (A) is a bug (see [1]), because it does not contain the acceptable key type "RSA" (did not check whether DSA keys are also acceptable).

One implementation (Apache HttpClient 4.5.13 org.apache.http.conn.ssl.SSLContextBuilder.KeyManagerDelegate, [2]) uses the passed key types to find matching keys (it iterates over the passed key types and asks a delegate KeyManger for aliases for that key type). In case (1) this strategy fails if the keystore only contains a RSA key because RSA keys are never queried, although the RSA key can be used to authenticate to the server. If (A) were correct, how could the implementation guess that it can also return a RSA key?

Thanks,

Thomas Fox

[1] [ https://github.com/AdoptOpenJDK/openjdk-support/issues/200 | https://github.com/AdoptOpenJDK/openjdk-support/issues/200 ] [2] [ https://hc.apache.org/httpcomponents-client-ga/httpclient/xref/org/apache/http/conn/ssl/SSLContextBuilder.html#L221 | https://hc.apache.org/httpcomponents-client-ga/httpclient/xref/org/apache/http/conn/ssl/SSLContextBuilder.html#L221 ] </mail to security list>

yielded the following reply (I'm hope its ok to publish the reply here, therefore leaving out the author ) I haven't been following what OpenJDK has been doing recently, but IIRC, the original call includes all of the server's requested key types, copied directly from the CertificateRequest message. See:

[ https://tools.ietf.org/rfcmarkup?doc=2246#section-7.4.4 | https://tools.ietf.org/rfcmarkup?doc=2246#section-7.4.4 ]

Have a look at the current code:

[ https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/ssl/CertificateRequest.java | https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/ssl/CertificateRequest.java ]

See the T10CertificateRequestMessage(line 182)/T10CertificateRequestConsumer(346) where the message is read off the wire, then is passed to chooseClientAlias(369) after checking for duplicates() and whether the algorithm is available.

It's similar but more involved for TLS12.

Which kind of supports that this is a bug, but does not explicitly say so. Asking whether someone can confirm this is a bug yielded no further reply.

So, after all, I'm still convinced this is a bug, but until now I did not get anybody to second my opinion. And this is the current status, and I have no Idea how to change it. Our current solution is not to upgrade to a newer openjdk build, but this will be not practical any more in the future. Another solution would be to completely ignore the key types passed by the JDK in the callback, and use a static list (but again, not a very convincing solution).

Thomas Fox

thomasfox avatar Feb 14 '21 18:02 thomasfox

@thomasfox If you have the same problem with Oracle JDK, ideally the version from jdk.java.net, submit your case on https://bugreport.java.com/bugreport/. Write it solely mentioning Oracle JDK. That might reach the right people.

aahlenst avatar Feb 14 '21 21:02 aahlenst

I did a bit of debugging. Not being an SSL expert, this is the situation as it looks to me:

OpenJDK 8u265-b01

  • Key types are determined in sun.security.ssl.ClientHandshaker#serverHelloDone The key types are read from the certificate request. All types which are in the certificate request are added and are then passed to the chooseClientAlias method

OpenJDK 8u272-b10

  • Key type is determined in sun.security.ssl.CertificateRequest.T12CertificateRequestConsumer#choosePossession. All signature schemes are read from the request. These are iterated over. For the first acceptable scheme (in my case ECDSA_SECP521R1_SHA512), the key type (in my case EC) is determined and then this key type is passed via sun.security.ssl.X509Authentication#createPossession and sun.security.ssl.X509Authentication.X509PossessionGenerator#createClientPossession to the chooseClientAlias method. In my opinion this is wrong because there can be (and are, in my case) other acceptable schemes with other acceptable key types. Instead of going for the first acceptable scheme, all schemes should be processed and the key types extracted from them. Comparing to the current openjdk source code, this code does not seem to have changed.

thomasfox avatar Feb 15 '21 06:02 thomasfox

Thanks Thomas, for us the cause was that our implementation of chooseClientAlias threw an exception instead of returning null. Changing this solved it.

sfritz-aurea avatar Feb 15 '21 06:02 sfritz-aurea

Thanks, @sfritz-aurea, for the hint. Actually, this was also the case in our application, and returning null resolves the issue for the application. Doing so is required in the javadoc of the method [1] (The method is then called several times for a single key resolution, with all allowed key types. I simply did not have the idea of such a behaviour. I did not realize this as this code was masked by Apache HttpComponent's code, which javadoc did not mention this issue at all)

However, In my opinion, there is still room for improvement, in the JDK. The javadoc [1] says:

String[] keyType - the key algorithm type name(s), ordered with the most-preferred key type first.

The parameter type and description kind of suggests, though it does not explicitly say so, that all possible key types are passed to this method, which was the case in the past, and is also the case for TLS 1.1.

The javadoc of the method could be improved insofar as to mention that the method can be called several times with different parameters for a single resolution, and that the client should not assume that all possible key types are passed to a single call of the method. @aahlenst, What do you think of this, and if you agree, what would be the best way to propose such a documentation change?

[1] https://docs.oracle.com/en/java/javase/15/docs/api/java.base/javax/net/ssl/X509KeyManager.html

thomasfox avatar Feb 16 '21 11:02 thomasfox

The parameter type and description kind of suggests, though it does not explicitly say so, that all possible key types are passed to this method, which was the case in the past, and is also the case for TLS 1.1.

Reading this, I think it makes more sense to change the behaviour.

What do you think of this, and if you agree, what would be the best way to propose such a documentation change?

If you cannot come up with a patch yourself, try security-dev again or file a report against OpenJDK 15 or 16 on https://bugreport.java.com/bugreport/ referring only to the download on http://jdk.java.net/. If the fix is applied to a development version, we can help to organize backports. Please post any links you get here.

aahlenst avatar Feb 16 '21 13:02 aahlenst

Submitted a jdk bug to https://bugreport.java.com/bugreport/, internal review id is 9069117

thomasfox avatar Feb 17 '21 17:02 thomasfox

JDK Bug is available here: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8262186 @aahlenst should I then reopen this issue?

thomasfox avatar Feb 23 '21 11:02 thomasfox

@thomasfox Thanks!

Tracking bug at OpenJDK is at https://bugs.openjdk.java.net/browse/JDK-8262186.

aahlenst avatar Feb 23 '21 11:02 aahlenst

We are marking this issue as stale because it has not been updated for a while. This is just a way to keep the support issues queue manageable. It will be closed soon unless the stale label is removed by a committer, or a new comment is made.

github-actions[bot] avatar May 07 '22 00:05 github-actions[bot]

Fixed in Java 18 but not yet backported.

karianna avatar May 09 '22 11:05 karianna

We are marking this issue as stale because it has not been updated for a while. This is just a way to keep the support issues queue manageable. It will be closed soon unless the stale label is removed by a committer, or a new comment is made.

github-actions[bot] avatar Aug 08 '22 00:08 github-actions[bot]

Checked status and is still fixed in Java 18 but backporting has not occured.

karianna avatar Aug 08 '22 13:08 karianna

We are marking this issue as stale because it has not been updated for a while. This is just a way to keep the support issues queue manageable. It will be closed soon unless the stale label is removed by a committer, or a new comment is made.

github-actions[bot] avatar Nov 07 '22 00:11 github-actions[bot]

We are marking this issue as stale because it has not been updated for a while. This is just a way to keep the support issues queue manageable. It will be closed soon unless the stale label is removed by a committer, or a new comment is made.

github-actions[bot] avatar Feb 06 '23 00:02 github-actions[bot]

We are marking this issue as stale because it has not been updated for a while. This is just a way to keep the support issues queue manageable. It will be closed soon unless the stale label is removed by a committer, or a new comment is made.

github-actions[bot] avatar May 13 '23 00:05 github-actions[bot]

We are marking this issue as stale because it has not been updated for a while. This is just a way to keep the support issues queue manageable. It will be closed soon unless the stale label is removed by a committer, or a new comment is made.

github-actions[bot] avatar Aug 28 '23 00:08 github-actions[bot]

Backport to Oracle JDK 17 only is here. https://bugs.openjdk.org/browse/JDK-8307322

karianna avatar Aug 28 '23 03:08 karianna