On 7/25/19 11:45 AM, Xuelei Fan wrote:
I updated the CSR and webrev per the review comments accordingly.
webrev: http://cr.openjdk.java.net/~xuelei/8226374/webrev.02/
Not done reviewing yet, but here are a couple of comments so far:
* test/jdk/sun/security/ssl/CipherSuite/RestrictNamedGroup.java
96 } catch (Exception exp) {
97 continue;
98 }
Can you check for an expected exception here? Same comment on lines
111-113 of RestrictSignatureScheme.java
* src/java.base/share/classes/sun/security/ssl/ECDHServerKeyExchange.java
114 if ((namedGroup == null) || (!namedGroup.isAvailable)) {
You don't do this check for null and isAvailable in other places, for
example ECDHClientKeyExchange.ECDHEClientKeyExchangeConsumer.consume() -
should you?
CSR: https://bugs.openjdk.java.net/browse/JDK-8227445
In the CSR, the Notes 1-3 are very similar. I only think you need to
mention that the names will be standardized in a subsequent CSR - and
maybe you can point or add a link to the issue that has been filed for
that. I don't think you need the other 2 notes.
--Sean
Thanks,
Xuelei
On 7/12/2019 9:14 AM, Xuelei Fan wrote:
On 7/12/2019 5:24 AM, Sean Mullan wrote:
On 7/11/19 11:56 AM, Xuelei Fan wrote:
Also, in the CSR you list all the different signature algorithms
that could be disabled, but you use the TLS names, and not the
standard JCE names. I found this a bit confusing, since if you
added those exact TLS names to jdk.tls.disabledAlgorithms, I don't
think it will work, or if it does we need additional changes to the
jdk.tls.disabledAlgorithms definition - and maybe that is what we
should do? Also, I don't think it is possible to disable
individual RSASSA-PSS algorithms, I think you can just disable all
or none of them because the parameters are specified separately and
not part of the standard JCE name. Similar to other algorithms -
how would I just disable ecdsa_secp256r1_sha256 and nothing else?
Is that an issue?
Yes, it is an issue now. The AlgorithmConstraints is able to accept
parameters, but the current jdk.tls.disabledAlgorithms property
cannot. That's also why I used the TLS names
(ecdsa_secp256r1_sha256, rsa_pss_rsae_sha256, etc) rather than
standard names (SHA256withECDSA, RSASSA-PSS).
I agree with you that it is confusing. The use of
rsa_pss_rsae_sha256 may be fine, but the name "dsa_sha256" rather
then "SHA256withDSA" could be confusing.
I was planned to add TLS signature algorithms into the standard
names, as we will do for the named groups. But it looks like a
duplicate of the crypto Signature algorithms.
I would lean towards this option. We could extend the
jdk.tls.disabledAlgorithms property to allow you to specify TLS
signature schemes as defined in
https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-signaturescheme
We would also need to add a section to the Standard Names
specification listing these schemes.
The reason I like this option is because it fits well with the TLS
namespace, and it is more flexible than the JCE names and we can more
easily restrict new TLS signature schemes that are defined later.
I agreed that is is more straightforward. Okay, let's go with option.
Between file a new enhancement for Standard Names documentation or
update the doc this time, do you have a preference? Maybe, we can
update the doc together with JDKJDK-8210755?
https://bugs.openjdk.java.net/browse/JDK-8210755
Thanks,
Xuelei
