RFR [14] JDK-8226374 Restrict signature algorithms and named groups

[Sean Mullan]

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