RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v6]

[Valerie Peng]

On Tue, 13 Oct 2020 13:34:27 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Major points in CSR at https://bugs.openjdk.java.net/browse/JDK-8245274:
>> 
>> - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in jarsigner
>> 
>> - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a signed JAR) are reused for new signature
>>   algorithms
>> 
>> - A new JarSigner property "directsign"
>> 
>> - Updating the jarsigner tool doc
>> 
>> Major code changes:
>> 
>> - Always use the signature algorithm directly as SignerInfo::signatureAlgorithm. We used to use the encryption algorithm
>>   there like RSA, DSA, and EC. Now it's always SHA1withRSA or RSASSA-PSS.
>> 
>> - Move signature related utilities methods from AlgorithmId.java to SignatureUtil.java
>> 
>> - Add new SignatureUtil methods fromKey() and fromSignature() to simplify creating Signature and getting its AlgorithmId
>> 
>> - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing
>> 
>> - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of all old and new signature algorithms
>> 
>> - Mark all -altsign related code deprecated and they can be removed once ContentSigner is removed
>
> Weijun Wang has refreshed the contents of this pull request, and previous commits have been removed. The incremental
> views will show differences compared to the previous content of the PR.

src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 113:

> 111:     }
> 112:
> 113:     public AlgorithmId(ObjectIdentifier oid, DerValue params)

Now that this is public, can we add a javadoc spec for it?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 50:

> 48:
> 49:     /**
> 50:      * Convent OID.1.2.3.4 or 1.2.3.4 to the matched stdName.

Convent => Convert? matched => matching?
Or maybe just "Match OID..... to the stdName"

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 53:

> 51:      *
> 52:      * @param algName input, could be in any form
> 53:      * @return the original name is it does not look like an OID,

is => if?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 54:

> 52:      * @param algName input, could be in any form
> 53:      * @return the original name is it does not look like an OID,
> 54:      *         the matched name for an OID, or the OID itself

Maybe use "the OID value"? The term "itself" reminds me of the specified argument somehow. Just nit.

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 94:

> 92:      * @return an AlgorithmParameterSpec object
> 93:      * @throws ProviderException
> 94:      */

Well, I am a bit unsure about your changes to this method. With your change, it returns default parameter spec (instead
of null) when the specified AlgorithmParameters object is null. This may not be desirable for all cases? Existing
callers would have to check for (params != null) before calling this method. The javadoc description also seems a bit
strange with the to-be-converted AlgorithmParameters object being optional. Maybe add a separate method like
`getParamSpecWithDefault` on top of this method or add a separate boolean argument `useDefault`?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 164:

> 162:             }
> 163:         } else {
> 164:             paramSpec = getDefaultAlgorithmParameterSpec(sigName, null);

Same comment here as for the getParamSpec(String, AlgorithmParameters) above. Given that there are two methods named
getParamSpec(...), maybe add a boolean argument to indicate whether to return default value if the to-be-converted
object is null?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 290:

> 288:      * that must be initialized with a AlgorithmParameterSpec now.
> 289:      */
> 290:     public static AlgorithmParameterSpec getDefaultAlgorithmParameterSpec(

Maybe we can just use "getDefaultParamSpec" to match other methods' name in this class. Just a suggestion.

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 511:

> 509:     }
> 510:
> 511:     // Same values for RSA and DSA

Update the comment with "Return the default message digest algorithm with the same security strength as the specified
IFC/FFC key size"?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 499:

> 497:     }
> 498:
> 499:     // Values from SP800-57 part 1 rev 4 tables 2 and 3

Update the comment with "Return the default message digest algorithm with the same security strength as the specified
EC key size"?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 367:

> 365:      * @param sigEngine the signature object
> 366:      * @param key the private key
> 367:      * @return the AlgorithmIA, not null

IA => Id?

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 268:

> 266:
> 267:     /**
> 268:      * Extracts the digest algorithm names from a signature

names => name

src/java.base/share/classes/sun/security/util/SignatureUtil.java line 205:

> 203:             try {
> 204:                 sha512 = AlgorithmId.get(KnownOIDs.SHA_512.stdName());
> 205:                 shake256 = AlgorithmId.get(KnownOIDs.SHAKE256.stdName());

For these two, why not just do `new AlgorithmId(ObjectIdentifier.of(ko))` where ko = KnownOIDs.SHA_512,
KnownOIDs.SHAKE256? More direct this way.

-------------

PR: https://git.openjdk.java.net/jdk/pull/322