OF-1100: Fix parsing of 'otherName' records

This commit formalizes the parsing of subjectAltName certificate extensions.

src/java/org/jivesoftware/util/cert/SANCertificateIdentityMapping.java

 package org.jivesoftware.util.cert;
 
+import org.bouncycastle.asn1.*;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.security.cert.CertificateParsingException;
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
@@ -7,32 +11,23 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 
-import org.bouncycastle.asn1.ASN1Encodable;
-import org.bouncycastle.asn1.ASN1InputStream;
-import org.bouncycastle.asn1.ASN1ObjectIdentifier;
-import org.bouncycastle.asn1.ASN1Sequence;
-import org.bouncycastle.asn1.ASN1TaggedObject;
-import org.bouncycastle.asn1.DERTaggedObject;
-import org.bouncycastle.asn1.DERUTF8String;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 /**
  * Certificate identity mapping that uses SubjectAlternativeName as the identity credentials.
  * This implementation returns all subjectAltName entries that are a:
  * <ul>
- *     <li>GeneralName of type otherName with the "id-on-xmppAddr" Object Identifier</li>
- *     <li>GeneralName of type otherName with the "id-on-dnsSRV" Object Identifier</li>
- *     <li>GeneralName of type DNSName</li>
- *     <li>GeneralName of type UniformResourceIdentifier</li>
+ * <li>GeneralName of type otherName with the "id-on-xmppAddr" Object Identifier</li>
+ * <li>GeneralName of type otherName with the "id-on-dnsSRV" Object Identifier</li>
+ * <li>GeneralName of type DNSName</li>
+ * <li>GeneralName of type UniformResourceIdentifier</li>
  * </ul>
  *
  * @author Victor Hong
  * @author Guus der Kinderen, guus@goodbytes.nl
  */
-public class SANCertificateIdentityMapping implements CertificateIdentityMapping {
-	
-	private static final Logger Log = LoggerFactory.getLogger(SANCertificateIdentityMapping.class);
+public class SANCertificateIdentityMapping implements CertificateIdentityMapping
+{
+
+    private static final Logger Log = LoggerFactory.getLogger( SANCertificateIdentityMapping.class );
 
     /**
      * id-on-xmppAddr Object Identifier.
@@ -44,7 +39,7 @@ public class SANCertificateIdentityMapping implements CertificateIdentityMapping
     /**
      * id-on-dnsSRV Object Identifier.
      *
-     * @see <a href="https://www.ietf.org/rfc/rfc4985.txt">RFC 4985</a>
+     * @see <a href="https://tools.ietf.org/html/rfc4985">RFC 4985</a>
      */
     public static final String OTHERNAME_SRV_OID = "1.3.6.1.5.5.7.8.7";
 
@@ -54,22 +49,27 @@ public class SANCertificateIdentityMapping implements CertificateIdentityMapping
      *
      * @param certificate the certificate presented by the remote entity.
      * @return the JID representation of an XMPP entity contained as a SubjectAltName extension
-     *         in the certificate. If none was found then return an empty list.
+     * in the certificate. If none was found then return an empty list.
      */
-	@Override
-	public List<String> mapIdentity(X509Certificate certificate) {
-		List<String> identities = new ArrayList<>();
-        try {
+    @Override
+    public List<String> mapIdentity( X509Certificate certificate )
+    {
+        List<String> identities = new ArrayList<>();
+        try
+        {
             Collection<List<?>> altNames = certificate.getSubjectAlternativeNames();
             // Check that the certificate includes the SubjectAltName extension
-            if (altNames == null) {
+            if ( altNames == null )
+            {
                 return Collections.emptyList();
             }
-            for (List<?> item : altNames) {
-                final Integer type = (Integer) item.get(0);
-                final Object value = item.get(1);
+            for ( List<?> item : altNames )
+            {
+                final Integer type = (Integer) item.get( 0 );
+                final Object value = item.get( 1 ); // this is either a string, or a byte-array that represents the ASN.1 DER encoded form.
                 final String result;
-                switch ( type ) {
+                switch ( type )
+                {
                     case 0:
                         // OtherName: search for "id-on-xmppAddr" or 'sRVName'
                         result = parseOtherName( (byte[]) value );
@@ -88,115 +88,128 @@ public class SANCertificateIdentityMapping implements CertificateIdentityMapping
                         break;
                 }
 
-                if ( result != null ) {
+                if ( result != null )
+                {
                     identities.add( result );
                 }
             }
         }
-        catch (CertificateParsingException e) {
-            Log.error("Error parsing SubjectAltName in certificate: " + certificate.getSubjectDN(), e);
+        catch ( CertificateParsingException e )
+        {
+            Log.error( "Error parsing SubjectAltName in certificate: " + certificate.getSubjectDN(), e );
         }
         return identities;
-	}
-
-	/**
-	 * Returns the short name of mapping.
-	 * 
-	 * @return The short name of the mapping (never null).
-	 */
-	@Override
-	public String name() {
-		return "Subject Alternative Name Mapping";
-	}
+    }
 
     /**
-     * Parses the byte-array representation of a subjectAltName 'otherName' entry, returning the "id-on-xmppAddr" value
-     * when that is in the entry.
+     * Returns the short name of mapping.
+     *
+     * @return The short name of the mapping (never null).
+     */
+    @Override
+    public String name()
+    {
+        return "Subject Alternative Name Mapping";
+    }
+
+    /**
+     * Parses the byte-array representation of a subjectAltName 'otherName' entry.
+     * <p/>
+     * The provided 'OtherName' is expected to have this format:
+     * <pre>{@code
+     * OtherName ::= SEQUENCE {
+     * type-id    OBJECT IDENTIFIER,
+     * value      [0] EXPLICIT ANY DEFINED BY type-id }
+     * }</pre>
      *
      * @param item A byte array representation of a subjectAltName 'otherName' entry (cannot be null).
-     * @return an "id-on-xmppAddr" value (which is expected to be a JID), or null.
+     * @return an xmpp address, or null when the otherName entry does not relate to XMPP (or fails to parse).
      */
-    public static String parseOtherName( byte[] item ) {
-        // Type OtherName found so return the associated value
-        try (ASN1InputStream decoder = new ASN1InputStream(item)) {
-            // Value is encoded using ASN.1 so decode it to get the server's identity
-            Object object = decoder.readObject();
-            ASN1Sequence otherNameSeq;
-            if (object != null && object instanceof ASN1Sequence) {
-                otherNameSeq = (ASN1Sequence) object;
-            } else {
-                return null;
+    public static String parseOtherName( byte[] item )
+    {
+        if ( item == null || item.length == 0 )
+        {
+            return null;
+        }
+
+        try ( final ASN1InputStream decoder = new ASN1InputStream( item ) )
+        {
+            // By specification, OtherName instances must always be an ASN.1 Sequence.
+            final ASN1Primitive object = decoder.readObject();
+            final ASN1Sequence otherNameSeq = (ASN1Sequence) object;
+
+            // By specification, an OtherName instance consists of:
+            // - the type-id (which is an Object Identifier), followed by:
+            // - a tagged value, of which the tag number is 0 (zero) and the value is defined by the type-id.
+            final ASN1ObjectIdentifier typeId = (ASN1ObjectIdentifier) otherNameSeq.getObjectAt( 0 );
+            final ASN1TaggedObject taggedValue = (ASN1TaggedObject) otherNameSeq.getObjectAt( 1 );
+
+            final int tagNo = taggedValue.getTagNo();
+            if ( tagNo != 0 )
+            {
+                throw new IllegalArgumentException( "subjectAltName 'otherName' sequence's second object is expected to be a tagged value of which the tag number is 0. The tag number that was detected: " + tagNo );
             }
-            // Check the object identifier
-            ASN1ObjectIdentifier objectId = (ASN1ObjectIdentifier) otherNameSeq.getObjectAt(0);
-            Log.debug("Parsing otherName for subject alternative names: " + objectId.toString() );
-
-            // Get identity string
-            if ( OTHERNAME_SRV_OID.equals(objectId.getId())) {
-                return parseOtherNameDnsSrv( otherNameSeq );
-            } else if ( OTHERNAME_XMPP_OID.equals(objectId.getId())) {
-                // Not a XMPP otherName
-                return parseOtherNameXmppAddr( otherNameSeq );
-            } else
+            final ASN1Primitive value = taggedValue.getObject();
+
+            switch ( typeId.getId() )
             {
-                Log.debug("Ignoring otherName '{}' that's neither id-on-xmppAddr nor id-on-dnsSRV.", objectId.getId());
-                return null;
+                case OTHERNAME_SRV_OID:
+                    return parseOtherNameDnsSrv( value );
+
+                case OTHERNAME_XMPP_OID:
+                    return parseOtherNameXmppAddr( value );
+
+                default:
+                    Log.debug( "Ignoring subjectAltName 'otherName' type-id '{}' that's neither id-on-xmppAddr nor id-on-dnsSRV.", typeId.getId() );
+                    return null;
             }
         }
-        catch (Exception e) {
-            Log.error("Error decoding subjectAltName", e);
+        catch ( Exception e )
+        {
+            Log.warn( "Unable to parse a byte array (of length {}) as a subjectAltName 'otherName'. It is ignored.", item.length, e );
+            return null;
         }
-        return null;
     }
 
-    public static String parseOtherNameDnsSrv( ASN1Sequence otherNameSeq )
+    /**
+     * Parses a SRVName value as specified by RFC 4985.
+     *
+     * This method parses the argument value as a DNS SRV Resource Record. Only when the parsed value refers to an XMPP
+     * related service, the corresponding DNS domain name is returned (minus the service name).
+     *
+     * @param srvName The ASN.1 representation of the srvName value (cannot be null).
+     * @return an XMPP address value, or null when the record does not relate to XMPP.
+     */
+    public static String parseOtherNameDnsSrv( ASN1Primitive srvName )
     {
-        Log.debug( "Parsing SRVName otherName..." );
-        try {
-            final ASN1Encodable o = otherNameSeq.getObjectAt( 1 );
-            final DERUTF8String derStr = DERUTF8String.getInstance( o );
-            final String value = derStr.getString();
-            if ( value.startsWith( "_xmpp-server." )) {
-                Log.debug( "Found _xmpp-server SRVName otherName" );
-                return value.substring( "_xmpp-server.".length() );
-            }
-            if ( value.startsWith( "_xmpp-client." )) {
-                Log.debug( "Found _xmpp-client SRVName otherName" );
-                return value.substring( "_xmpp-client.".length() );
-            }
-            else
-            {
-                Log.debug( "SRVName otherName '{}' was neither _xmpp-server nor _xmpp-client. It is being ignored.", value );
-                return null;
-            }
-        } catch (IllegalArgumentException ex) {
-            Log.debug("Cannot parse id-on-dnsSRV otherName, likely because of unknown record format.", ex);
+        // RFC 4985 says that this should be a IA5 String. Lets be tolerant and allow all text-based values.
+        final String value = ( (ASN1String) srvName ).getString();
+
+        if ( value.toLowerCase().startsWith( "_xmpp-server." ) )
+        {
+            return value.substring( "_xmpp-server.".length() );
+        }
+        else if ( value.toLowerCase().startsWith( "_xmpp-client." ) )
+        {
+            return value.substring( "_xmpp-client.".length() );
+        }
+        else
+        {
+            // Not applicable to XMPP. Ignore.
+            Log.debug( "srvName value '{}' of id-on-dnsSRV record is neither _xmpp-server nor _xmpp-client. It is being ignored.", value );
             return null;
         }
     }
 
-    public static String parseOtherNameXmppAddr( ASN1Sequence otherNameSeq )
+    /**
+     * Parse a XmppAddr value as specified in RFC 6120.
+     *
+     * @param xmppAddr The ASN.1 representation of the xmppAddr value (cannot be null).
+     * @return The parsed xmppAddr value.
+     */
+    public static String parseOtherNameXmppAddr( ASN1Primitive xmppAddr )
     {
-        try {
-            final String identity;
-            ASN1Encodable o = otherNameSeq.getObjectAt(1);
-            if (o instanceof DERTaggedObject) {
-                ASN1TaggedObject ato = DERTaggedObject.getInstance(o);
-                Log.debug("... processing DERTaggedObject: " + ato.toString());
-                // TODO: there's bound to be a better way...
-                identity = ato.toString().substring(ato.toString().lastIndexOf(']')+1).trim();
-            } else {
-                DERUTF8String derStr = DERUTF8String.getInstance(o);
-                identity = derStr.getString();
-            }
-            if (identity != null && identity.length() > 0) {
-                // Add the decoded server name to the list of identities
-                return identity;
-            }
-        } catch (IllegalArgumentException ex) {
-            // OF-517: othername formats are extensible. If we don't recognize the format, skip it.
-            Log.debug("Cannot parse id-on-xmppAddr otherName, likely because of unknown record format.", ex);
-        }
-        return null;
+        // RFC 6120 says that this should be a UTF8String. Lets be tolerant and allow all text-based values.
+        return ( (ASN1String) xmppAddr ).getString();
     }
 }

src/test/java/org/jivesoftware/util/CertificateManagerTest.java

@@ -2,7 +2,6 @@ package org.jivesoftware.util;
 
 import org.bouncycastle.asn1.*;
 import org.bouncycastle.asn1.x500.X500Name;
-import org.bouncycastle.asn1.x509.BasicConstraints;
 import org.bouncycastle.asn1.x509.Extension;
 import org.bouncycastle.asn1.x509.GeneralName;
 import org.bouncycastle.asn1.x509.GeneralNames;
@@ -14,7 +13,6 @@ import org.bouncycastle.operator.ContentSigner;
 import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
 import org.junit.BeforeClass;
 import org.junit.Test;
-import sun.security.x509.SubjectAlternativeNameExtension;
 
 import java.math.BigInteger;
 import java.security.KeyPair;
@@ -29,11 +27,14 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 /**
- * Created by guus on 3-3-16.
+ * Unit test to validate the functionality of @{link CertificateManager}.
+ *
+ * @author Guus der Kinderen, guus@goodbytes.nl
  */
 public class CertificateManagerTest
 {
     public static final ASN1ObjectIdentifier XMPP_ADDR_OID = new ASN1ObjectIdentifier( "1.3.6.1.5.5.7.8.5" );
+    public static final ASN1ObjectIdentifier DNS_SRV_OID = new ASN1ObjectIdentifier( "1.3.6.1.5.5.7.8.7" );
 
     private static KeyPairGenerator keyPairGenerator;
     private static KeyPair subjectKeyPair;
@@ -49,7 +50,6 @@ public class CertificateManagerTest
         subjectKeyPair = keyPairGenerator.generateKeyPair();
         issuerKeyPair = keyPairGenerator.generateKeyPair();
         contentSigner = new JcaContentSignerBuilder( "SHA1withRSA" ).build( issuerKeyPair.getPrivate() );
-
     }
 
     /**
@@ -117,11 +117,9 @@ public class CertificateManagerTest
                 subjectKeyPair.getPublic()
         );
 
-        final DERTaggedObject derTaggedDomainName = new DERTaggedObject(0, new DERUTF8String(subjectAltNameXmppAddr) );
-        final DLSequence otherName = new DLSequence(new ASN1Encodable[]{XMPP_ADDR_OID, derTaggedDomainName});
-        final GeneralNames generalNames = new GeneralNames(new GeneralName(GeneralName.otherName, otherName));
-
-        builder.addExtension( Extension.subjectAlternativeName, false, generalNames );
+        final DERSequence otherName = new DERSequence( new ASN1Encodable[] { XMPP_ADDR_OID, new DERUTF8String( subjectAltNameXmppAddr ) });
+        final GeneralNames subjectAltNames = new GeneralNames( new GeneralName(GeneralName.otherName, otherName ) );
+        builder.addExtension( Extension.subjectAlternativeName, true, subjectAltNames );
 
         final X509CertificateHolder certificateHolder = builder.build( contentSigner );
         final X509Certificate cert = new JcaX509CertificateConverter().getCertificate( certificateHolder );
@@ -135,6 +133,51 @@ public class CertificateManagerTest
         assertFalse( serverIdentities.contains( subjectCommonName ) );
     }
 
+
+    /**
+     * {@link CertificateManager#getServerIdentities(X509Certificate)} should return:
+     * <ul>
+     *     <li>the 'DNS SRV' subjectAltName value</li>
+     *     <li>explicitly not the Common Name</li>
+     * </ul>
+     *
+     * when a certificate contains:
+     * <ul>
+     *     <li>a subjectAltName entry of type otherName with an ASN.1 Object Identifier of "id-on-dnsSRV"</li>
+     * </ul>
+     */
+    @Test
+    public void testServerIdentitiesDnsSrv() throws Exception
+    {
+        // Setup fixture.
+        final String subjectCommonName = "MySubjectCommonName";
+        final String subjectAltNameDnsSrv = "MySubjectAltNameXmppAddr";
+
+        final X509v3CertificateBuilder builder = new JcaX509v3CertificateBuilder(
+                new X500Name( "CN=MyIssuer" ),                                          // Issuer
+                BigInteger.valueOf( Math.abs( new SecureRandom().nextInt() ) ),         // Random serial number
+                new Date( System.currentTimeMillis() - ( 1000L * 60 * 60 * 24 * 30 ) ), // Not before 30 days ago
+                new Date( System.currentTimeMillis() + ( 1000L * 60 * 60 * 24 * 99 ) ), // Not after 99 days from now
+                new X500Name( "CN=" + subjectCommonName ),                              // Subject
+                subjectKeyPair.getPublic()
+        );
+
+        final DERSequence otherName = new DERSequence( new ASN1Encodable[] {DNS_SRV_OID, new DERUTF8String( "_xmpp-server."+subjectAltNameDnsSrv ) });
+        final GeneralNames subjectAltNames = new GeneralNames( new GeneralName(GeneralName.otherName, otherName ) );
+        builder.addExtension( Extension.subjectAlternativeName, true, subjectAltNames );
+
+        final X509CertificateHolder certificateHolder = builder.build( contentSigner );
+        final X509Certificate cert = new JcaX509CertificateConverter().getCertificate( certificateHolder );
+
+        // Execute system under test
+        final List<String> serverIdentities = CertificateManager.getServerIdentities( cert );
+
+        // Verify result
+        assertEquals( 1, serverIdentities.size() );
+        assertTrue( serverIdentities.contains( subjectAltNameDnsSrv ));
+        assertFalse( serverIdentities.contains( subjectCommonName ) );
+    }
+
     /**
      * {@link CertificateManager#getServerIdentities(X509Certificate)} should return:
      * <ul>
@@ -210,14 +253,12 @@ public class CertificateManagerTest
                 subjectKeyPair.getPublic()
         );
 
-        final DERTaggedObject derTaggedDomainName = new DERTaggedObject(0, new DERUTF8String(subjectAltNameXmppAddr) );
-        final DLSequence otherName = new DLSequence(new ASN1Encodable[]{XMPP_ADDR_OID, derTaggedDomainName});
-        final GeneralNames generalNames = new GeneralNames( new GeneralName[] {
-                new GeneralName(GeneralName.otherName, otherName),
-                new GeneralName(GeneralName.dNSName, subjectAltNameDNS)
+        final DERSequence otherName = new DERSequence( new ASN1Encodable[] { XMPP_ADDR_OID, new DERUTF8String( subjectAltNameXmppAddr ) });
+        final GeneralNames subjectAltNames = new GeneralNames( new GeneralName[] {
+                new GeneralName( GeneralName.otherName, otherName ),
+                new GeneralName( GeneralName.dNSName, subjectAltNameDNS )
         });
-
-        builder.addExtension( Extension.subjectAlternativeName, false, generalNames );
+        builder.addExtension( Extension.subjectAlternativeName, true, subjectAltNames );
 
         final X509CertificateHolder certificateHolder = builder.build( contentSigner );
         final X509Certificate cert = new JcaX509CertificateConverter().getCertificate( certificateHolder );