8255536: Remove the directsign property and option

wangweij · wangweij · commit a7563207f640 · 2020-10-30T13:29:41.000Z
Reviewed-by: mullan
diff --git a/src/java.base/share/classes/sun/security/pkcs/PKCS7.java b/src/java.base/share/classes/sun/security/pkcs/PKCS7.java
@@ -28,9 +28,6 @@
 import java.io.*;
 import java.math.BigInteger;
 import java.net.URI;
-import java.security.interfaces.EdECPrivateKey;
-import java.security.spec.InvalidParameterSpecException;
-import java.security.spec.PSSParameterSpec;
 import java.util.*;
 import java.security.cert.X509Certificate;
 import java.security.cert.CertificateException;
diff --git a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
@@ -34,7 +34,6 @@
 import sun.security.pkcs.PKCS9Attributes;
 import sun.security.timestamp.HttpTimestamper;
 import sun.security.tools.PathList;
-import sun.security.tools.jarsigner.TimestampedSigner;
 import sun.security.util.Event;
 import sun.security.util.ManifestDigester;
 import sun.security.util.SignatureFileVerifier;
@@ -122,7 +121,6 @@ public static class Builder {
         String tSADigestAlg;
         boolean sectionsonly = false;
         boolean internalsf = false;
-        boolean directsign = false;
         String altSignerPath;
         String altSigner;
 
@@ -358,10 +356,6 @@ public Builder eventHandler(BiConsumer<String,String> handler) {
          * <li>"sectionsonly": "true" if the .SF file only contains the hash
          * value for each section of the manifest and not for the whole
          * manifest, "false" otherwise. Default "false".
-         * <li>"directsign": "true" if the signature is calculated on the
-         * content directly, "false" if it's calculated on signed attributes
-         * which itself is calculated from the content and stored in the
-         * signer's SignerInfo. Default "false".
          * </ul>
          * All property names are case-insensitive.
          *
@@ -395,9 +389,6 @@ public Builder setProperty(String key, String value) {
                 case "sectionsonly":
                     this.sectionsonly = parseBoolean("sectionsonly", value);
                     break;
-                case "directsign":
-                    this.directsign = parseBoolean("directsign", value);
-                    break;
                 case "altsignerpath":
                     altSignerPath = value;
                     break;
@@ -510,7 +501,6 @@ public JarSigner build() {
     private final String tSADigestAlg;
     private final boolean sectionsonly; // do not "sign" the whole manifest
     private final boolean internalsf; // include the .SF inside the PKCS7 block
-    private final boolean directsign;
 
     @Deprecated(since="16", forRemoval=true)
     private final String altSignerPath;
@@ -561,9 +551,12 @@ private JarSigner(JarSigner.Builder builder) {
         this.altSigner = builder.altSigner;
         this.altSignerPath = builder.altSignerPath;
 
-        this.directsign = this.altSigner != null
-                ? true
-                : builder.directsign;
+        // altSigner cannot support modern algorithms like RSASSA-PSS and EdDSA
+        if (altSigner != null
+                && !sigalg.toUpperCase(Locale.ENGLISH).contains("WITH")) {
+            throw new IllegalArgumentException(
+                    "Customized ContentSigner is not supported for " + sigalg);
+        }
     }
 
     /**
@@ -666,8 +659,6 @@ public String getProperty(String key) {
                 return Boolean.toString(sectionsonly);
             case "altsignerpath":
                 return altSignerPath;
-            case "directsign":
-                return Boolean.toString(directsign);
             case "altsigner":
                 return altSigner;
             default:
@@ -855,20 +846,7 @@ private void sign0(ZipFile zipFile, OutputStream os)
         sf.write(baos);
         byte[] content = baos.toByteArray();
 
-        // Use new method if directSign is false or it's a modern
-        // algorithm not supported by existing ContentSigner.
-        // Make this always true after we remove ContentSigner.
-        boolean useNewMethod = !directsign
-                || !sigalg.toUpperCase(Locale.ENGLISH).contains("WITH");
-
-        // For newer sigalg without "with", always use the new PKCS7
-        // generateToken method. Otherwise, use deprecated ContentSigner.
-        if (useNewMethod) {
-            if (altSigner != null) {
-                throw new IllegalArgumentException(directsign
-                        ? ("Customized ContentSigner is not supported for " + sigalg)
-                        : "Customized ContentSigner does not support authenticated attributes");
-            }
+        if (altSigner == null) {
             Function<byte[], PKCS9Attributes> timestamper = null;
             if (tsaUrl != null) {
                 timestamper = s -> {
@@ -889,7 +867,7 @@ private void sign0(ZipFile zipFile, OutputStream os)
             }
             // We now create authAttrs in block data, so "direct == false".
             block = PKCS7.generateNewSignedData(sigalg, sigProvider, privateKey, certChain,
-                    content, internalsf, directsign, timestamper);
+                    content, internalsf, false, timestamper);
         } else {
             Signature signer = SignatureUtil.fromKey(sigalg, privateKey, sigProvider);
             signer.update(content);
@@ -901,9 +879,7 @@ private void sign0(ZipFile zipFile, OutputStream os)
                             tSADigestAlg, signature,
                             signer.getAlgorithm(), certChain, content, zipFile);
             @SuppressWarnings("removal")
-            ContentSigner signingMechanism = (altSigner != null)
-                    ? loadSigningMechanism(altSigner, altSignerPath)
-                    : new TimestampedSigner();
+            ContentSigner signingMechanism = loadSigningMechanism(altSigner, altSignerPath);
             block = signingMechanism.generateSignedData(
                     params,
                     !internalsf,
diff --git a/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java b/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -163,7 +163,6 @@ public static void main(String args[]) throws Exception {
     boolean debug = false; // debug
     boolean signManifest = true; // "sign" the whole manifest
     boolean externalSF = true; // leave the .SF out of the PKCS7 block
-    boolean directSign = false; // sign SF directly or thru signedAttrs
     boolean strict = false;  // treat warnings as error
     boolean revocationCheck = false; // Revocation check flag
 
@@ -473,8 +472,6 @@ String[] parseArgs(String args[]) throws Exception {
                 signManifest = false;
             } else if (collator.compare(flags, "-internalsf") ==0) {
                 externalSF = false;
-            } else if (collator.compare(flags, "-directsign") ==0) {
-                directSign = true;
             } else if (collator.compare(flags, "-verify") ==0) {
                 verify = true;
             } else if (collator.compare(flags, "-verbose") ==0) {
@@ -663,9 +660,6 @@ static void fullusage() {
         System.out.println(rb.getString
                 (".internalsf.include.the.SF.file.inside.the.signature.block"));
         System.out.println();
-        System.out.println(rb.getString
-                (".directsign.sign.the.SF.file.directly.no.signerinfo.signedattributes"));
-        System.out.println();
         System.out.println(rb.getString
                 (".sectionsonly.don.t.compute.hash.of.entire.manifest"));
         System.out.println();
@@ -1773,7 +1767,6 @@ void signJar(String jarName, String alias)
 
         builder.setProperty("sectionsOnly", Boolean.toString(!signManifest));
         builder.setProperty("internalSF", Boolean.toString(!externalSF));
-        builder.setProperty("directsign", Boolean.toString(directSign));
 
         FileOutputStream fos = null;
         try {
diff --git a/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java b/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java
@@ -101,8 +101,6 @@ public class Resources extends java.util.ListResourceBundle {
                 "                            (This option is deprecated and will be removed in a future release.)"},
         {".internalsf.include.the.SF.file.inside.the.signature.block",
                 "[-internalsf]               include the .SF file inside the signature block"},
-        {".directsign.sign.the.SF.file.directly.no.signerinfo.signedattributes",
-                "[-directsign]               sign the .SF file directly (no SignerInfo signedAttributes)"},
         {".sectionsonly.don.t.compute.hash.of.entire.manifest",
                 "[-sectionsonly]             don't compute hash of entire manifest"},
         {".protected.keystore.has.protected.authentication.path",
diff --git a/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/TimestampedSigner.java b/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/TimestampedSigner.java
diff --git a/test/jdk/jdk/security/jarsigner/Properties.java b/test/jdk/jdk/security/jarsigner/Properties.java
@@ -75,15 +75,10 @@ public static void main(String[] args) throws Exception {
         Asserts.assertTrue(sf.startsWith("Signature-Version"));
 
         // There is a SignedAttributes
-        byte[] d0 = sign(jsb.setProperty("directsign", "false"));
+        byte[] d0 = sign(jsb);
         Asserts.assertTrue(DerUtils.innerDerValue(d0, "10403")
                 .isContextSpecific((byte)0));
 
-        // There is no SignedAttributes
-        byte[] d1 = sign(jsb.setProperty("directsign", "true"));
-        Asserts.assertFalse(DerUtils.innerDerValue(d1, "10403")
-                .isContextSpecific((byte)0));
-
         // Has a hash for the whole manifest
         byte[] s0 = sign(jsb.setProperty("sectionsonly", "false"));
         sf = new String(DerUtils.innerDerValue(s0, "10210").getOctetString());
diff --git a/test/jdk/jdk/security/jarsigner/Spec.java b/test/jdk/jdk/security/jarsigner/Spec.java
@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 8056174 8242068
+ * @bug 8056174 8242068 8255536
  * @summary Make sure JarSigner impl conforms to spec
  * @library /test/lib
  * @modules java.base/sun.security.tools.keytool
@@ -70,6 +70,9 @@ public static void main(String[] args) throws Exception {
         sun.security.tools.keytool.Main.main(
                 ("-keystore ks -storepass changeit -keypass changeit -dname" +
                         " CN=DSA -alias d -genkeypair -keyalg dsa").split(" "));
+        sun.security.tools.keytool.Main.main(
+                ("-keystore ks -storepass changeit -keypass changeit -dname" +
+                        " CN=Ed25519 -alias e -genkeypair -keyalg Ed25519").split(" "));
 
         char[] pass = "changeit".toCharArray();
 
@@ -127,8 +130,6 @@ public static void main(String[] args) throws Exception {
         iae(()->b1.setProperty("sectionsonly", "OK"));
         npe(()->b1.setProperty("sectionsonly", null));
         npe(()->b1.setProperty("altsigner", null));
-        iae(()->b1.setProperty("directsign", "OK"));
-        npe(()->b1.setProperty("directsign", null));
         npe(()->b1.eventHandler(null));
 
         // default values
@@ -146,7 +147,6 @@ public static void main(String[] args) throws Exception {
         assertTrue(js2.getProperty("tsapolicyid") == null);
         assertTrue(js2.getProperty("internalsf").equals("false"));
         assertTrue(js2.getProperty("sectionsonly").equals("false"));
-        assertTrue(js2.getProperty("directsign").equals("false"));
         assertTrue(js2.getProperty("altsigner") == null);
         uoe(()->js2.getProperty("invalid"));
 
@@ -163,7 +163,6 @@ public static void main(String[] args) throws Exception {
                 .setProperty("tsapolicyid", "1.2.3.4")
                 .setProperty("internalsf", "true")
                 .setProperty("sectionsonly", "true")
-                .setProperty("directsign", "true")
                 .setProperty("altsigner", "MyContentSigner")
                 .eventHandler(myeh);
         JarSigner js3 = b3.build();
@@ -176,7 +175,6 @@ public static void main(String[] args) throws Exception {
         assertTrue(js3.getProperty("tsapolicyid").equals("1.2.3.4"));
         assertTrue(js3.getProperty("internalsf").equals("true"));
         assertTrue(js3.getProperty("sectionsonly").equals("true"));
-        assertTrue(js3.getProperty("directsign").equals("true"));
         assertTrue(js3.getProperty("altsigner").equals("MyContentSigner"));
         assertTrue(js3.getProperty("altsignerpath") == null);
 
@@ -208,6 +206,14 @@ public static void main(String[] args) throws Exception {
         assertTrue(JarSigner.Builder
                 .getDefaultSignatureAlgorithm(kpg.generateKeyPair().getPrivate())
                 .equals("SHA512withECDSA"));
+
+        // altsigner does not support modern algorithms
+        JarSigner.Builder b4 = new JarSigner.Builder(
+                (PrivateKey)ks.getKey("e", pass),
+                CertificateFactory.getInstance("X.509")
+                        .generateCertPath(Arrays.asList(ks.getCertificateChain("e"))));
+        b4.setProperty("altsigner", "MyContentSigner");
+        iae(() -> b4.build());
     }
 
     interface RunnableWithException {
