New webrev:
http://cr.openjdk.java.net/~xuelei/8226374/webrev.03/
On 7/25/2019 11:36 AM, Sean Mullan wrote:
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
As the test is a client-server structure, the exception may be different
for each run (client or server which thread dies first). I updated to
use "SSLException | IllegalStateException". SSLException is the general
exception, but IllegalStateException is the exception thrown by client
or server thread when it dies and failed to wrap the SSLException. The
normal case for the signature schemes and named groups should have been
covered by other test cases, it may not worthy to analysis the exception
stack for exactly exception, which is not reliable if the exception
stack get changed in the future.
* 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?
Good point! Currently, the restriction is only checked for the
supported group extension. I should add more check points in other
places where named groups are used, for example client key exchange and
certificate. Stay tune for the next webrev.
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.
Updated.
Thanks,
Xuelei
--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
