Fix ASN.1 encoding of "cn" OtherName in CertGen/CertUtil (elastic/x-pack-elasticsearch#2858)

Certgen was generating "other name" SANs without the explicit [0] tag that is required.
This was masked by the fact that the JRE X.509 classes always wrap the "other name" name-value in a [0] tag  (even if it already has one)

Also switched to a UTF8 String from an IA5 string to match the configuration being used for testing in openssl.

Original commit: elastic/x-pack-elasticsearch@1b87964ec7

plugin/src/main/java/org/elasticsearch/xpack/ssl/CertUtils.java

@@ -51,13 +51,19 @@ import java.util.stream.Collectors;
 
 import org.bouncycastle.asn1.ASN1Encodable;
 import org.bouncycastle.asn1.ASN1ObjectIdentifier;
+import org.bouncycastle.asn1.ASN1TaggedObject;
+import org.bouncycastle.asn1.BERSequence;
+import org.bouncycastle.asn1.BERTaggedObject;
 import org.bouncycastle.asn1.DERIA5String;
 import org.bouncycastle.asn1.DERSequence;
+import org.bouncycastle.asn1.DERTaggedObject;
+import org.bouncycastle.asn1.DERUTF8String;
 import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers;
 import org.bouncycastle.asn1.pkcs.PrivateKeyInfo;
 import org.bouncycastle.asn1.x500.X500Name;
 import org.bouncycastle.asn1.x509.AuthorityKeyIdentifier;
 import org.bouncycastle.asn1.x509.BasicConstraints;
+import org.bouncycastle.asn1.x509.DisplayText;
 import org.bouncycastle.asn1.x509.Extension;
 import org.bouncycastle.asn1.x509.ExtensionsGenerator;
 import org.bouncycastle.asn1.x509.GeneralName;
@@ -509,12 +515,12 @@ public class CertUtils {
     /**
      * Creates an X.509 {@link GeneralName} for use as a <em>Common Name</em> in the certificate's <em>Subject Alternative Names</em>
      * extension. A <em>common name</em> is a name with a tag of {@link GeneralName#otherName OTHER}, with an object-id that references
-     * the {@link #CN_OID cn} attribute, and a DER encoded IA5 (ASCII) string for the name.
+     * the {@link #CN_OID cn} attribute, an explicit tag of '0', and a DER encoded UTF8 string for the name.
      * This usage of using the {@code cn} OID as a <em>Subject Alternative Name</em> is <strong>non-standard</strong> and will not be
      * recognised by other X.509/TLS implementations.
      */
     static GeneralName createCommonName(String cn) {
-        final ASN1Encodable[] sequence = { new ASN1ObjectIdentifier(CN_OID), new DERIA5String(cn) };
+        final ASN1Encodable[] sequence = { new ASN1ObjectIdentifier(CN_OID), new DERTaggedObject(true, 0, new DERUTF8String(cn)) };
         return new GeneralName(GeneralName.otherName, new DERSequence(sequence));
     }
 }

plugin/src/main/java/org/elasticsearch/xpack/ssl/RestrictedTrustManager.java

@@ -23,7 +23,9 @@ import java.util.stream.Collectors;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.bouncycastle.asn1.ASN1ObjectIdentifier;
+import org.bouncycastle.asn1.ASN1Primitive;
 import org.bouncycastle.asn1.ASN1Sequence;
+import org.bouncycastle.asn1.ASN1String;
 import org.bouncycastle.asn1.ASN1TaggedObject;
 import org.bouncycastle.asn1.DERTaggedObject;
 import org.elasticsearch.common.logging.Loggers;
@@ -126,13 +128,35 @@ public final class RestrictedTrustManager extends X509ExtendedTrustManager {
                 .map(pair -> pair.get(1))
                 .map(value -> {
                     ASN1Sequence seq = ASN1Sequence.getInstance(value);
-                    assert seq.size() == 2 : "Incorrect sequence length for 'other name'";
+                    if (seq.size() != 2) {
+                        String message = "Incorrect sequence length for 'other name' [" + seq + "]";
+                        assert false : message;
+                        logger.warn(message);
+                        return null;
+                    }
                     final String id = ASN1ObjectIdentifier.getInstance(seq.getObjectAt(0)).getId();
                     if (CertUtils.CN_OID.equals(id)) {
-                        final ASN1TaggedObject object = DERTaggedObject.getInstance(seq.getObjectAt(1));
+                        ASN1TaggedObject tagged = DERTaggedObject.getInstance(seq.getObjectAt(1));
-                        final String cn = object.getObject().toString();
+                        // The JRE's handling of OtherNames is buggy.
+                        // The internal sun classes go to a lot of trouble to parse the GeneralNames into real object
+                        // And then java.security.cert.X509Certificate just turns them back into bytes
+                        // But in doing so, it ends up wrapping the "other name" bytes with a second tag
+                        // Specifically: sun.security.x509.OtherName(DerValue) never decodes the tagged "nameValue"
+                        // But: sun.security.x509.OtherName.encode() wraps the nameValue in a DER Tag.
+                        // So, there's a good chance that our tagged nameValue contains... a tagged name value.
+                        if (tagged.getObject() instanceof ASN1TaggedObject) {
+                            tagged = (ASN1TaggedObject) tagged.getObject();
+                        }
+                        final ASN1Primitive nameValue = tagged.getObject();
+                        if (nameValue instanceof ASN1String) {
+                            final String cn = ((ASN1String) nameValue).getString();
                             logger.trace("Read cn [{}] from ASN1Sequence [{}]", cn, seq);
                             return cn;
+                        } else {
+                            logger.warn("Certificate [{}] has 'otherName' [{}] with unsupported name-value type [{}]",
+                                    certificate.getSubjectDN(), seq, nameValue.getClass().getSimpleName());
+                            return null;
+                        }
                     } else {
                         logger.debug("Certificate [{}] has 'otherName' [{}] with unsupported object-id [{}]",
                                 certificate.getSubjectDN(), seq, id);

plugin/src/test/java/org/elasticsearch/xpack/ssl/CertificateGenerateToolTests.java

@@ -45,6 +45,7 @@ import org.bouncycastle.asn1.ASN1ObjectIdentifier;
 import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.ASN1String;
 import org.bouncycastle.asn1.DEROctetString;
+import org.bouncycastle.asn1.DERTaggedObject;
 import org.bouncycastle.asn1.pkcs.Attribute;
 import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers;
 import org.bouncycastle.asn1.x509.Extension;
@@ -482,8 +483,11 @@ public class CertificateGenerateToolTests extends ESTestCase {
                 assertThat(seq.size(), equalTo(2));
                 assertThat(seq.getObjectAt(0), instanceOf(ASN1ObjectIdentifier.class));
                 assertThat(seq.getObjectAt(0).toString(), equalTo(CertUtils.CN_OID));
-                assertThat(seq.getObjectAt(1), instanceOf(ASN1String.class));
+                assertThat(seq.getObjectAt(1), instanceOf(DERTaggedObject.class));
-                assertThat(seq.getObjectAt(1).toString(), Matchers.isIn(certInfo.commonNames));
+                DERTaggedObject taggedName = (DERTaggedObject) seq.getObjectAt(1);
+                assertThat(taggedName.getTagNo(), equalTo(0));
+                assertThat(taggedName.getObject(), instanceOf(ASN1String.class));
+                assertThat(taggedName.getObject().toString(), Matchers.isIn(certInfo.commonNames));
             } else {
                 fail("unknown general name with tag " + generalName.getTagNo());
             }

plugin/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java

@@ -50,6 +50,7 @@ import org.apache.lucene.util.IOUtils;
 import org.bouncycastle.asn1.ASN1ObjectIdentifier;
 import org.bouncycastle.asn1.ASN1Sequence;
 import org.bouncycastle.asn1.ASN1String;
+import org.bouncycastle.asn1.ASN1TaggedObject;
 import org.bouncycastle.asn1.DEROctetString;
 import org.bouncycastle.asn1.pkcs.Attribute;
 import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers;
@@ -797,8 +798,10 @@ public class CertificateToolTests extends ESTestCase {
                 assertThat(seq.size(), equalTo(2));
                 assertThat(seq.getObjectAt(0), instanceOf(ASN1ObjectIdentifier.class));
                 assertThat(seq.getObjectAt(0).toString(), equalTo(CertUtils.CN_OID));
-                assertThat(seq.getObjectAt(1), instanceOf(ASN1String.class));
+                assertThat(seq.getObjectAt(1), instanceOf(ASN1TaggedObject.class));
-                assertThat(seq.getObjectAt(1).toString(), Matchers.isIn(certInfo.commonNames));
+                ASN1TaggedObject tagged = (ASN1TaggedObject) seq.getObjectAt(1);
+                assertThat(tagged.getObject(), instanceOf(ASN1String.class));
+                assertThat(tagged.getObject().toString(), Matchers.isIn(certInfo.commonNames));
             } else {
                 fail("unknown general name with tag " + generalName.getTagNo());
             }