8073182: keytool may generate duplicate extensions
authorweijun
Wed, 25 Feb 2015 18:30:29 +0800
changeset 29111 e9103f166a4a
parent 29110 ea89fdd8a5d5
child 29112 df7e4edb6566
8073182: keytool may generate duplicate extensions Reviewed-by: mullan
jdk/src/java.base/share/classes/sun/security/tools/keytool/Main.java
jdk/test/sun/security/tools/keytool/KeyToolTest.java
--- a/jdk/src/java.base/share/classes/sun/security/tools/keytool/Main.java	Wed Feb 25 18:30:07 2015 +0800
+++ b/jdk/src/java.base/share/classes/sun/security/tools/keytool/Main.java	Wed Feb 25 18:30:29 2015 +0800
@@ -3765,41 +3765,55 @@
         }
     }
 
+    // Add an extension into a CertificateExtensions, always using OID as key
+    private static void setExt(CertificateExtensions result, Extension ex)
+            throws IOException {
+        result.set(ex.getId(), ex);
+    }
+
     /**
      * Create X509v3 extensions from a string representation. Note that the
      * SubjectKeyIdentifierExtension will always be created non-critical besides
      * the extension requested in the <code>extstr</code> argument.
      *
-     * @param reqex the requested extensions, can be null, used for -gencert
-     * @param ext the original extensions, can be null, used for -selfcert
+     * @param requestedEx the requested extensions, can be null, used for -gencert
+     * @param existingEx the original extensions, can be null, used for -selfcert
      * @param extstrs -ext values, Read keytool doc
      * @param pkey the public key for the certificate
      * @param akey the public key for the authority (issuer)
      * @return the created CertificateExtensions
      */
     private CertificateExtensions createV3Extensions(
-            CertificateExtensions reqex,
-            CertificateExtensions ext,
+            CertificateExtensions requestedEx,
+            CertificateExtensions existingEx,
             List <String> extstrs,
             PublicKey pkey,
             PublicKey akey) throws Exception {
 
-        if (ext != null && reqex != null) {
+        if (existingEx != null && requestedEx != null) {
             // This should not happen
             throw new Exception("One of request and original should be null.");
         }
-        if (ext == null) ext = new CertificateExtensions();
+        // A new extensions always using OID as key
+        CertificateExtensions result = new CertificateExtensions();
+        if (existingEx != null) {
+            for (Extension ex: existingEx.getAllExtensions()) {
+                setExt(result, ex);
+            }
+        }
         try {
             // name{:critical}{=value}
             // Honoring requested extensions
-            if (reqex != null) {
+            if (requestedEx != null) {
                 for(String extstr: extstrs) {
                     if (extstr.toLowerCase(Locale.ENGLISH).startsWith("honored=")) {
                         List<String> list = Arrays.asList(
                                 extstr.toLowerCase(Locale.ENGLISH).substring(8).split(","));
                         // First check existence of "all"
                         if (list.contains("all")) {
-                            ext = reqex;    // we know ext was null
+                            for (Extension ex: requestedEx.getAllExtensions()) {
+                                setExt(result, ex);
+                            }
                         }
                         // one by one for others
                         for (String item: list) {
@@ -3828,9 +3842,9 @@
                                     type = item;
                                 }
                             }
-                            String n = reqex.getNameByOid(findOidForExtName(type));
+                            String n = findOidForExtName(type).toString();
                             if (add) {
-                                Extension e = reqex.get(n);
+                                Extension e = requestedEx.get(n);
                                 if (!e.isCritical() && action == 0
                                         || e.isCritical() && action == 1) {
                                     e = Extension.newExtension(
@@ -3838,9 +3852,9 @@
                                             !e.isCritical(),
                                             e.getExtensionValue());
                                 }
-                                ext.set(n, e);
+                                setExt(result, e);
                             } else {
-                                ext.delete(n);
+                                result.delete(n);
                             }
                         }
                         break;
@@ -3902,8 +3916,7 @@
                                 }
                             }
                         }
-                        ext.set(BasicConstraintsExtension.NAME,
-                                new BasicConstraintsExtension(isCritical, isCA,
+                        setExt(result, new BasicConstraintsExtension(isCritical, isCA,
                                 pathLen));
                         break;
                     case 1:     // KU
@@ -3931,7 +3944,7 @@
                             KeyUsageExtension kue = new KeyUsageExtension(ok);
                             // The above KeyUsageExtension constructor does not
                             // allow isCritical value, so...
-                            ext.set(KeyUsageExtension.NAME, Extension.newExtension(
+                            setExt(result, Extension.newExtension(
                                     kue.getExtensionId(),
                                     isCritical,
                                     kue.getExtensionValue()));
@@ -3969,8 +3982,7 @@
                                     v.add(new ObjectIdentifier("1.3.6.1.5.5.7.3." + p));
                                 }
                             }
-                            ext.set(ExtendedKeyUsageExtension.NAME,
-                                    new ExtendedKeyUsageExtension(isCritical, v));
+                            setExt(result, new ExtendedKeyUsageExtension(isCritical, v));
                         } else {
                             throw new Exception(rb.getString
                                     ("Illegal.value.") + extstr);
@@ -3991,13 +4003,11 @@
                                 gnames.add(createGeneralName(t, v));
                             }
                             if (exttype == 3) {
-                                ext.set(SubjectAlternativeNameExtension.NAME,
-                                        new SubjectAlternativeNameExtension(
-                                            isCritical, gnames));
+                                setExt(result, new SubjectAlternativeNameExtension(
+                                        isCritical, gnames));
                             } else {
-                                ext.set(IssuerAlternativeNameExtension.NAME,
-                                        new IssuerAlternativeNameExtension(
-                                            isCritical, gnames));
+                                setExt(result, new IssuerAlternativeNameExtension(
+                                        isCritical, gnames));
                             }
                         } else {
                             throw new Exception(rb.getString
@@ -4047,11 +4057,9 @@
                                         oid, createGeneralName(t, v)));
                             }
                             if (exttype == 5) {
-                                ext.set(SubjectInfoAccessExtension.NAME,
-                                        new SubjectInfoAccessExtension(accessDescriptions));
+                                setExt(result, new SubjectInfoAccessExtension(accessDescriptions));
                             } else {
-                                ext.set(AuthorityInfoAccessExtension.NAME,
-                                        new AuthorityInfoAccessExtension(accessDescriptions));
+                                setExt(result, new AuthorityInfoAccessExtension(accessDescriptions));
                             }
                         } else {
                             throw new Exception(rb.getString
@@ -4071,10 +4079,9 @@
                                 String v = item.substring(colonpos+1);
                                 gnames.add(createGeneralName(t, v));
                             }
-                            ext.set(CRLDistributionPointsExtension.NAME,
-                                    new CRLDistributionPointsExtension(
-                                        isCritical, Collections.singletonList(
-                                        new DistributionPoint(gnames, null, null))));
+                            setExt(result, new CRLDistributionPointsExtension(
+                                    isCritical, Collections.singletonList(
+                                    new DistributionPoint(gnames, null, null))));
                         } else {
                             throw new Exception(rb.getString
                                     ("Illegal.value.") + extstr);
@@ -4112,7 +4119,7 @@
                         } else {
                             data = new byte[0];
                         }
-                        ext.set(oid.toString(), new Extension(oid, isCritical,
+                        setExt(result, new Extension(oid, isCritical,
                                 new DerValue(DerValue.tag_OctetString, data)
                                         .toByteArray()));
                         break;
@@ -4122,18 +4129,16 @@
                 }
             }
             // always non-critical
-            ext.set(SubjectKeyIdentifierExtension.NAME,
-                    new SubjectKeyIdentifierExtension(
-                        new KeyIdentifier(pkey).getIdentifier()));
+            setExt(result, new SubjectKeyIdentifierExtension(
+                    new KeyIdentifier(pkey).getIdentifier()));
             if (akey != null && !pkey.equals(akey)) {
-                ext.set(AuthorityKeyIdentifierExtension.NAME,
-                        new AuthorityKeyIdentifierExtension(
-                        new KeyIdentifier(akey), null, null));
+                setExt(result, new AuthorityKeyIdentifierExtension(
+                                new KeyIdentifier(akey), null, null));
             }
         } catch(IOException e) {
             throw new RuntimeException(e);
         }
-        return ext;
+        return result;
     }
 
     /**
--- a/jdk/test/sun/security/tools/keytool/KeyToolTest.java	Wed Feb 25 18:30:07 2015 +0800
+++ b/jdk/test/sun/security/tools/keytool/KeyToolTest.java	Wed Feb 25 18:30:29 2015 +0800
@@ -1194,6 +1194,12 @@
         assertTrue(!b.getExtension(new ObjectIdentifier("1.2.3")).isCritical());
         assertTrue(b.getExtension(new ObjectIdentifier("1.2.4")).isCritical());
 
+        // 8073182: keytool may generate duplicate extensions
+        testOK("", pre+"dup -ext bc=2 -ext 2.5.29.19=30030101FF -ext bc=3");
+        ks = loadStore("x.jks", "changeit", "JKS");
+        X509CertImpl dup = (X509CertImpl)ks.getCertificate("dup");
+        assertTrue(dup.getBasicConstraints() == 3);
+
         remove("x.jks");
         remove("test.req");
         remove("test.cert");