RFR 8242184: CRL generation error with RSASSA-PSS

[Weijun Wang]

> On Apr 8, 2020, at 11:46 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> Hi Max,
> 
> Not all of the comments are related to the changes in the webrev, just notice some PSS related inconsistency and thought I will ask:
> 
> <AlgorithmId.java>
> 
> - For RSASSA-PSS keys, existing code in getDefaultAlgorithmParameterSpec(...) (line 1121) decides the default based on key size. I think we should consider checking if the key contains PSS parameters. If present, use it as default.

Correct.

> 
> - In the checkKeyAndSigAlgMatch(...) method (line 1027), don't we need to add checking for RSASSA-PSS signature? RSASSA-PSS sig can take both RSA and RSASSA-PSS keys.
> 
> - The getEncAlgFromSigAlg(...) method just returns the key algorithm as result when the specified signature algorithm does not contain "with". As RSASSA-PSS signature can use both RSASSA-PSS and RSA keys, should it still return key algorithm?
> 
> - The makeSigAlg(...) method does not work for RSASSA-PSS. 

These are for JAR file signing. The support for RSASSA-PSS is very poor in this area. I am thinking about fixing these in "8242068: Signed JAR support for RSASSA-PSS and EdDSA".

> 
> <X509CRLImpl.java>
> 
> - The sign() methods of X509CertImpl class do not generate default parameters automatically. Have you considered adding a sign() method to X509CRLImpl class which has extra signature       parameter spec argument and move the default parameter call to the caller instead of inside X509CRLImpl? I think it's more suitable for the caller to generate the default unless there are many callers all need the same functionality.

But this X509CertImpl method is not used anywhere. If only for JDK internal use, I'd rather sacrifice this flexibility and introduce a method like

    public static Signature fromKey(String sigAlg, Key Privatekey, String provider)
            throws NoSuchAlgorithmException, NoSuchProviderException,
                   InvalidKeyException{
        Signature sigEngine = (provider == null || provider.isEmpty())
                ? Signature.getInstance(sigAlg)
                : Signature.getInstance(sigAlg, provider);
        AlgorithmParameterSpec params = SignatureUtil
                .getDefaultAlgorithmParameterSpec(sigAlg, key);
        try {
            SignatureUtil.initSignWithParam(sigEngine, key, params,
                    null);
        } catch (InvalidAlgorithmParameterException e) {
            throw new AssertionError("getDefaultAlgorithmParameterSpec", e);
        }
        return s;
    }

There are quite some places that need this pattern. If necessary later, we can add a nullable AlgorithmParameterSpec argument.

Thanks,
Max

> 
> Thanks,
> 
> Valerie
> 
> On 4/6/2020 8:11 PM, Weijun Wang wrote:
>> Please review the fix at
>> 
>>    
>> http://cr.openjdk.java.net/~weijun/8242184/webrev.00/
>> 
>> 
>> The major change is inside X509CRLImpl.java to allow params setting and reading.
>> 
>> I also take this chance to:
>> 
>> 1. Provide a default -sigalg for "keytool -genkeypair -keyalg rsassa-pss".
>> 
>> 2. Revert a former change in X509CertImpl.java, which might be a safer call.
>> 
>> Thanks,
>> Max
>> 
>>