On 5/15/20 6:11 PM, Xuelei Fan wrote:
New webrev: http://cr.openjdk.java.net/~xuelei/8206925/webrev.04/
*
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
212 SSLLogger.warning(
213 "Too much certificate authorities to
use " +
214 "the certificate_authorities extension");
How about: "The number of CAs exceeds the maximum size of the
certificate_authorities extension"
* test/jdk/sun/security/ssl/X509KeyManager/CertificateAuthorities.java
The test is still using the previous property name:
jdk.tls.client.indicateCertificateAuthorities
* test/jdk/sun/security/ssl/X509TrustManagerImpl/TooMuchCAs.java
Will this test FAIL if we ever exceed the maximum number of CAs? I think
it is important that it does FAIL, as the extension is effectively not
working anymore and could cause compatibility issues. I even think we
would need to try to think of some way to fix it, either by seeing if
some CAs could be excluded - not really sure, hopefully it won't ever
happen but we would want to know about it in advance.
* src/java.base/share/classes/sun/security/ssl/SSLExtension.java
Some typos and wording suggestions:
750 // Note: Plese be careful to enable this extension in
ClientHello.
Typo: Please
757 // untrusted server certificate. If this extension is
enabled in
758 // the ClientHello handshake message, the server does
not send
759 // certificate which is not indicated in the
extension. The server
760 // will just terminate handshake and close the
connection. Then
A few more details would be useful above - how about combining the 2
sentences as:
"If this extension is enabled in the ClientHello handshake message, and
the server's certificate does not chain back to any of the CAs in the
extension, then the server will terminate the handshake and close the
connection."
761 // there is no chance for client to perform the manually
checking.
"the client"
s/manually checking/manual check/
762 // Therefore, enable this extension in ClientHello may
lead to
s/enable/enabling/
767 // maximum TLS record size is 2^14 bytes. In case of
handshake
s/In case of/If the/
768 // message is bigger then maximum TLS record size, it
should be
s/then/than the/
770 // implementations does not allow ClientHello message
bigger than
s/does/do/
s/message/messages/
771 // the maximum TLS record size and aborts connection
immediately
s/aborts connection immediately/will immediately abort the connection/
772 // with a fatal alert. Therefore, if the client accepts
too much
s/accepts too much/trusts too many/
775 // Further more, if the client accepts too much CAs to
meet the
776 // size limit of the extension, enabling this extension
in client
s/Further more/Furthermore/
s/accepts too much CAs to meet the/trusts more CAs such that it exceeds the/
--Sean
On 5/15/2020 5:27 AM, Sean Mullan wrote:
On 5/15/20 1:22 AM, Xuelei Fan wrote:
Alexey has some good point about the size limit of the extension. I
added more comments about the compatibility impact and interop impact
when there is too much CAs to meet the size limits in CSR, source
code and release notes.
New webrev: https://bugs.openjdk.java.net/browse/JDK-8244460
I have not had a chance to add a negative test case yet.
By negative test case, do you mean trying to exceed the maximum number
of CAs? I agree that would be a good test to add, as we may or may not
exceed that number some day, but it would be good to know when/if we do.
Yes. I added a new test case in webrev.04 (TooMuchCAs.java).
Xuelei
--Sean
On 5/14/2020 1:38 PM, Sean Mullan wrote:
For the CSR, why did you check the binary and behavioral boxes for
compatibility risk?
I should not check the boxes. Removed.
Thanks,
Xuelei
Otherwise it looks good, and I added my name as Reviewer. I will
review the updated webrev later.
Please file and add a link to a docs issue to document the new
system property.
--Sean
On 5/13/20 5:20 PM, Xuelei Fan wrote:
On 5/13/2020 2:11 PM, Sean Mullan wrote:
It is not expected to use this extension regularly.
Please let me know if you still prefer to use "enableCAExtension".
Also, it is a bit unfortunate that we have to have a system
property to enable it. Can we not enable it based on whether
the configured X509TrustManager.getAcceptedIssuers returns a
non-empty list?
We can do that on server side, but there are compatibility
impact on client behavior if we did it in client side. See #2
in the "Specification" section.
But doesn't the default JDK PKIX TrustManager throw a fatal
exception and close the connection if the server's certificate
cannot be validated? Could we check if the PKIX TrustManager is
being used?
Yes, the trust manager could throw a fatal exception and close
the connection if the trust cannot be established. The fallback
mechanism is implemented in the customized trust manager, that if
users accept the cert, the cert is trusted, and no exception and
the handshaking continued. It is too later to fallback after the
connection closed.
If a client wants to accept self-signed or untrusted server
certificates, I would have expected them to have to use a custom
X509TrustManager that allows that, and that getAcceptedIssuers()
should return an empty List. Is that not is what is typically
done in practice?
Yes, customized trust manager is used to accept users manually
selection. As the users may also want to accept normal
certificate without manually involved, so getAcceptedIssuers()
should respect those CA as well.
I see. Out of curiosity, have you checked how other
implementations handle this extension? For web browsers, they
typically give the user the option of proceeding if the server
certificate is not trusted. Seems to be a bit of a configuration
dilemma as you may want this extension enabled for certain sites
that have multiple certificates, but not as a general default
because then you wouldn't be able to connect to untrusted sites
(at your own risk of course). I wonder if it would have been
better for the RFC to allow the server to treat this extension
more as a hint, and still return its chain if an acceptable
certificate could not be found.
If it is treated as a hint, then it might be better no have this
extension.
I checked with browsers, the extension is not present in
ClientHello. For JDK, I would not expect a lot use of this
extension in client side. It is just designed to workaround a few
cases, just as you mentioned above.
Xuelei
