Webrev has been uploaded to cr.openjdk.java.net
<http://cr.openjdk.java.net>:
*
http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/webrev.03/
*
http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/webrev.03/8046295.webrev.03.zip
Kind regards,
Martin.-
On Tue, Jul 18, 2017 at 2:12 PM, Martin Balao <mba...@redhat.com
<mailto:mba...@redhat.com>> wrote:
Hi,
Given that 1) Trusted CA Indication TLS Extension does not appear to
be widely implemented today and 2) it will be replaced by
Certificate Authorities TLS extension in TLS 1.3, it looks more
beneficial to me supporting only the latter -and avoid paying the
cost for a larger code-base-.
Here it is my proposal for Certificate Authorities TLS extension:
*
http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03/
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03/>
(browse online)
*
http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03.zip
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03.zip>
(download)
Implementation based on TLS 1.3 - draft 20 [1]. Patch based on
JDK-10 (rev 76ff72bb6a4a).
Pending / blocked (in descending priority order):
High
* The extension applies only to TLSv1.3+
* Blocked because TLSv1.3 is still not supported in JSSE.
* Impact: the extension cannot be used in TLS 1.2 (high impact on
the client-side).
* Action: replace "useTLS12PlusSpec" with "useTLS13PlusSpec" in
the patch when available.
Medium
* Server can send the CertificateAuthorities extension to the
client in a CertificateRequest message (feature)
* Blocked by: Server is still not able to send
EncryptedExtensions message in CertificateRequest
* Impact: feature not supported on the server side. The extension
can still work in production environments. (medium).
* Action: implement EncryptedExtensions message in
CertificateRequest and then implement this feature.
Low
* Update documentation to refer the final TLS 1.3 RFC (draft 20 is
currently referred)
* Blocked by: publication of the final TLS 1.3 RFC
* Impact: documentation is not accurate. (low)
* Action: replace
"https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4
<https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4>"
with the final link in the patch.
* Update bug id in "CertificateAuthoritiesClientTest.java" and
"CertificateAuthoritiesServerTest.java"
* Blocked by: there is no bug id for Certificate Authorities TLS
extension
* Impact: internal tests (very low).
* Action: replace "@bug 8046295" with the new bug id in the
patch. Open a new bug id for Certificate Authorities TLS extension.
Look forward to your comments.
Kind regards,
Martin.-
--
[1] -
https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4
<https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4>
On Tue, Jun 20, 2017 at 9:33 PM, Xuelei Fan <xuelei....@oracle.com
<mailto:xuelei....@oracle.com>> wrote:
Hi Martin,
The TLS 1.3 spec is replacing the Trusted CA Indication
(trusted_ca_keys) extension with a new Certificate Authorities
(certificate_authorities) extension. See more details about the
specification in the TLS 1.3 draft:
https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4
<https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4>
Both serves a similar purpose, but the trusted_ca_keys extension
will not be used in TLS 1.3 any more. The "trusted_ca_keys"
extension will only be used for legacy protocol versions (TLS
1.2/1.1/1.0).
There are two options to me:
1. Supports the certificate_authorities, but not trusted_ca_keys
extension.
It is acceptable to me as trusted_ca_keys is for legacy use only
and the certificate_authorities extension is the future. Plus,
the certificate_authorities extension can also be used for TLS
1.2 and previous versions.
2. Supports both the certificate_authorities and trusted_ca_keys
extensions.
As far as I know, I did not see much benefit of this option
unless the trusted_ca_keys extension is widely used in practice.
If I did not miss something, the APIs you designed can still be
used for the certificate_authorities extension, with a little
bit update.
What do you think?
Thanks & Regards,
Xuelei
On 6/15/2017 12:05 PM, Martin Balao wrote:
Hi Xuelei,
The new webrev.02 is ready:
*
http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/>
(browse online)
*
http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip>
(zip, download)
The following changes have been implemented since the
previous webrev.01:
* s/getUseTrustedCAIndication() methods in
SSLEngine/SSLSocket and in SSLEngineImpl/SSLSocketImpl
removed. s/getSSLParameters is now the only way to set or
get the use of the Trusted CA Indication extension. An
exception is no longer thrown if trying to disable the
extension for a server, but the change takes no effect as
the extension is mandatory for servers. X509KeyManagerImpl
modified to use SSLParameters to get information regarding
if Trusted CA Indication is enabled and should guide the
certificate choice.
* TrustedAuthorityIndicator.IdentifierType has been moved
from enum to String, to follow JSSE conventions. I
understand how important is to be consistent. However, I
still believe that an enum is a better fit for this value
and does not prevent from future extension. We are choosing
from a closed set (strictly defined by the RFC) and that's
what enum allows to express. From the client point of
view/API, it's very handy that the type gives you
information regarding the allowed choices for the parameter.
You don't necessarily have to look for implementation
details or documentation but you can just leverage on the
strongly typed language. It's also likely that enums are
faster for comparisons than strings, but that's not the main
point here.
* Removed X509Certificate from TrustedAuthorityIndicator
class (method and property). It was there for informational
purposes (when TrustedAuthorityIndicator was built from a
certificate by a client and the whole extension indicators
converted to String).
* "equals" and "hashCode" methods moved from
TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl
class.
* "getLength" method removed from
TrustedAuthorityIndicator class. It's possible to get the
encoded buffer and the length from there.
* "getData" method renamed to "getEncoded" in
TrustedAuthorityIndicator class.
* "trustedAuthorityEncodedData" renamed to "encodedData"
in TrustedAuthorityIndicator and
TrustedAuthorityIndicatorImpl classes
* "identifier" and "encodedData" instance variables moved
from TrustedAuthorityIndicator to
TrustedAuthorityIndicatorImpl class.
* "getEncoded" and "getIdentifier" are now abstract
methods in TrustedAuthorityIndicator, and their
implementation is in TrustedAuthorityIndicatorImpl class.
* "getIdentifier" method renamed to "getType" in
TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl
classes ("identifier" instance variable and parameter in
TrustedAuthorityIndicatorImpl class renamed to "type").
* Test cases (server and client) updated to reflect the
new interface (enabling the use of the extension through
SSLParameters)
However, some changes are still not implemented and I have
some concerns:
1) I still believe that identifier type information has to
be on TrustedAuthorityIndicator class somehow, and
implementations restricted on what they can return as part
of "getType" method. This is strictly specified by the RFC
TrustedAuthorityIndicator class represents, and I find
desirable that any implementation is enforced to be
compliant to that. If we remove all of that (including the
enum), TrustedAuthorityIndicator looks too generic and does
not reflect (in my opinion) what it really is. It'd also be
chaotic if different implementations call pre-agreed type as
"preagreed", "pre-agreed", "PRE_AGREED", etc. I prefer
stricter and more explicit interfaces.
2) I agree that type mappings can be seen as part of an
implementation, but they were in TrustedAuthorityIndicator
(as protected) because every implementation is highly likely
to need them and we can avoid the necessity for repeated
code/mappings. The same for "type" and "encodedData"
variables or even "hashCode" and "equals" methods. That's
why I was thinking more of an abstract class and not an
interface, as it happens (in example) with SNIServerName.
3) I think that "implies" method on
TrustedAuthorityIndicator should be also part of the
class/interface, because that's the whole point of a Trusted
Authority Information: to allow queries for a given
certificate. This is, in fact, the only thing a server wants
from one of these objects. My concern is that if we remove
this requirement for an implementation, the interface looks
more like a byte buffer holder.
I'd appreciate if you can re-consider these items.
Thanks,
Martin.-
On Wed, Jun 14, 2017 at 7:17 PM, Xuelei Fan
<xuelei....@oracle.com <mailto:xuelei....@oracle.com>
<mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>> wrote:
Hi Martin,
The big picture of the design looks pretty good to me,
except a few
comment about the JSSE conventions. I appreciate it
very much. By
the way, I need more time to look into the details of the
specification and implementation.
In order to keep the APIs simple and small,
SSLParameters is
preferred as the only configuration port for common
cases. I may
suggest to remove the s/getUseTrustedCAIndication()
methods in
SSLEngine/SSLSocket.
The identify type is defined as an enum
TrustedAuthorityIndicator.IdentifierType. In the
future, if more
type is added, we need to update the specification by
adding a new
enum item. Enum is preferred in JDK, but for good
extensibility, in
general JSSE does not use enum in public APIs for
extensible
properties. I may suggest to use String (or
integer/byte, I prefer
to use String) as the type. The standard trusted authority
indicator algorithm (identifier) can be documented in
the "Java
Cryptography Architecture Standard Algorithm Name
Documentation"[1].
In TrustedAuthorityIndicator class, some methods, like
getIdentifierTypeFromCode(),
getCodeFromIdentifierType(), implies(),
getLength(), equals() and hashCode() look more like
implementation
logic. I may suggest remove them from public APIs.
I did not see the benefit to have X509Certificate in the
TrustedAuthorityIndicator class. The class is mainly
used for
server side certificate selection. X509Certificate
could be unknown
for a indicator. I may suggestion remove the related
methods and
properties.
After that, as there is no requirement to instantiate
TrustedAuthorityIndicator class in application code,
looks like it
may be enough to use an interface to represent a
trusted authorities:
public interface TrustedAuthorityIndicator {
// identifier type, standard algorithm name
String/int/Byte getType();
// identifier
byte[] getEncoded();
}
What do you think?
Thanks & Regards,
Xuelei
[1]
https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
<https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>
<https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
<https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>>
On 6/13/2017 3:41 PM, Martin Balao wrote:
Hi Xuelei,
The new webrev.01 is ready:
*
http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>>
(browse online)
*
http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
<http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>>
(zip, download)
The following changes have been implemented since
the previous
webrev.00:
* Pre-agreed support removed from server-side
* Unnecessary overhead and minium benefits for
JSSE.
* Enabling the use of Trusted CA Indication
extension for
clients through TrustManager objects was reverted.
Trusted CA
Indication extension can now be enabled through: 1)
SSLEngine,
2) SSLSocket, or 3) SSLParameters (which can be
applied to both
SSLEngine and SSLSocket objects). Trusted CA Indication
extension is mandatory for servers.
* SunX509KeyManagerImpl old key manager
("SunX509" algorithm)
is now out of scope. This key manager does not
support other TLS
extensions as Server Name Indication (SNI), which
is far more
relevant than Trusted CA Indication. The new
X509KeyManagerImpl
key manager ("PKIX" algorithm) is now in scope.
* Client requested indications are now an
ExtendedSSLSession
attribute. ServerHandshaker gets the information
from the Client
Hello message (now parsed by
TrustedCAIndicationExtension class
instead of TrustedAuthorityIndicator) and sets it
in the
ExtendedSSLSession (SSLSessionImpl object). The key
manager
(i.e.: X509KeyManagerImpl), when choosing a server
alias, may
now get the information from the ExtendedSSLSession
object and
guide the certificate selection based on it.
* In order to allow multiple key managers to
use Trusted
Authority Indicators information and to allow
multiple Trusted
Authority Indicators implementations,
TrustedAuthorityIndicator
has now been split in an abstract class
(TrustedAuthorityIndicator, located in
javax.net.ssl) and an
implementation class
(TrustedAuthorityIndicatorImpl, located in
sun.security.ssl). No coupling was added between
javax.net.ssl
and sun.security.ssl packages.
* Documentation extended and improved.
* Test cases (server and client) updated to
reflect the new
interface and supported key manager.
Look forward to your new review!
Kind regards,
Martin.-
On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan
<xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>
<mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>>>
wrote:
I'm OK to use SSLParameters. Thank you very
much for
considering a
new design.
Xuelei
On 6/9/2017 1:10 PM, Martin Balao wrote:
Hi Xuelei,
I didn't notice that some of the SSLSocket
contructors
did not
establish the connection, so SSLParameters
can be
effective for
Trusted CA Indication. This was an invalid
argument on
my side,
sorry.
As for the configuration to enable the
extension, it's
probably
not necessary on the Server side because
-as you
mentioned- it
is mandatory and there is no harm in
supporting it.
However, it
has to be configurable on the Client side
because -as we
previously discussed- the client may cause
a handshake
failure
if the server does not support the
extension. I'd
prefer the
Client configuring the SSLSocket through
SSLParameters
instead
of a system-wide property -which has even
more impact
than the
TrustManager approach-. Would this work
for you?
> In JSSE, the benefits pre_agreed
option can get by
customizing the key/trust manager, so I
did not see too
much
benefits to support this option in JDK
I understand your point and will remove
support for
"pre_agreed".
On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
<xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>
<mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>>
<mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>
<mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>
<mailto:xuelei....@oracle.com <mailto:xuelei....@oracle.com>
<mailto:xuelei....@oracle.com
<mailto:xuelei....@oracle.com>>>>>
wrote:
On 6/8/2017 8:36 PM, Xuelei Fan wrote:
The trusted authorities can be
get from client
trust
manager. Server can choose the
best matching of
server
certificate of the
client requested trusted authorities.
>
I missed the point that the key
manager need to
know the client
requested trusted authorities for the
choosing. So may
need a new
SSLSession attribute (See similar
method in
ExtendedSSLSession).
Xuelei
Yes, an attribute on SSLSession may do the
job (both
when Key
Manager receives a SSLSocket and a SSLEngine).
Kind regards,
Martin.-
