> On Apr 8, 2020, at 11:46 AM, Valerie Peng <valerie.p...@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
>>
>>
