CORDA-1715 Ordering the X500 name for the CRL extension of the TLS certificates (#3515)

* CORDA-1715 Ordering the X500 name for the CRL extension of the TLS certificate

* Addressing review comments

* Addressing review comments - round 2

* Throwing an exception on incorrect TLS CRL issuer configuration

* Changes after the redesign decisions

* Small refactoring

core/src/main/kotlin/net/corda/core/identity/CordaX500Name.kt

@@ -3,13 +3,10 @@ package net.corda.core.identity
 import com.google.common.collect.ImmutableSet
 import net.corda.core.KeepForDJVM
 import net.corda.core.internal.LegalNameValidator
+import net.corda.core.internal.toAttributesMap
 import net.corda.core.internal.unspecifiedCountry
-import net.corda.core.internal.x500Name
+import net.corda.core.internal.toX500Name
 import net.corda.core.serialization.CordaSerializable
-import org.bouncycastle.asn1.ASN1Encodable
-import org.bouncycastle.asn1.ASN1ObjectIdentifier
-import org.bouncycastle.asn1.x500.AttributeTypeAndValue
-import org.bouncycastle.asn1.x500.X500Name
 import org.bouncycastle.asn1.x500.style.BCStyle
 import java.util.*
 import javax.security.auth.x500.X500Principal
@@ -86,29 +83,13 @@ data class CordaX500Name(val commonName: String?,
 
         @JvmStatic
         fun build(principal: X500Principal): CordaX500Name {
-            val x500Name = X500Name.getInstance(principal.encoded)
+            val attrsMap = principal.toAttributesMap(supportedAttributes)
-            val attrsMap: Map<ASN1ObjectIdentifier, ASN1Encodable> = x500Name.rdNs
-                    .flatMap { it.typesAndValues.asList() }
-                    .groupBy(AttributeTypeAndValue::getType, AttributeTypeAndValue::getValue)
-                    .mapValues {
-                        require(it.value.size == 1) { "Duplicate attribute ${it.key}" }
-                        it.value[0]
-                    }
-
-            // Supported attribute checks.
-            (attrsMap.keys - supportedAttributes).let { unsupported ->
-                require(unsupported.isEmpty()) {
-                    "The following attribute${if (unsupported.size > 1) "s are" else " is"} not supported in Corda: " +
-                            unsupported.map { BCStyle.INSTANCE.oidToDisplayName(it) }
-                }
-            }
-
             val CN = attrsMap[BCStyle.CN]?.toString()
             val OU = attrsMap[BCStyle.OU]?.toString()
-            val O = attrsMap[BCStyle.O]?.toString() ?: throw IllegalArgumentException("Corda X.500 names must include an O attribute")
+            val O = requireNotNull(attrsMap[BCStyle.O]?.toString()) { "Corda X.500 names must include an O attribute" }
-            val L = attrsMap[BCStyle.L]?.toString() ?: throw IllegalArgumentException("Corda X.500 names must include an L attribute")
+            val L = requireNotNull(attrsMap[BCStyle.L]?.toString()) { "Corda X.500 names must include an L attribute" }
             val ST = attrsMap[BCStyle.ST]?.toString()
-            val C = attrsMap[BCStyle.C]?.toString() ?: throw IllegalArgumentException("Corda X.500 names must include an C attribute")
+            val C = requireNotNull(attrsMap[BCStyle.C]?.toString()) { "Corda X.500 names must include an C attribute" }
             return CordaX500Name(CN, OU, O, L, ST, C)
         }
 
@@ -122,7 +103,7 @@ data class CordaX500Name(val commonName: String?,
     /** Return the [X500Principal] equivalent of this name. */
     val x500Principal: X500Principal
         get() {
-            return _x500Principal ?: X500Principal(this.x500Name.encoded).also { _x500Principal = it }
+            return _x500Principal ?: X500Principal(this.toX500Name().encoded).also { _x500Principal = it }
         }
 
     override fun toString(): String = x500Principal.toString()

core/src/main/kotlin/net/corda/core/internal/InternalUtils.kt

@@ -11,7 +11,6 @@ import net.corda.core.cordapp.CordappConfig
 import net.corda.core.cordapp.CordappContext
 import net.corda.core.crypto.*
 import net.corda.core.flows.FlowLogic
-import net.corda.core.identity.CordaX500Name
 import net.corda.core.node.ServicesForResolution
 import net.corda.core.serialization.*
 import net.corda.core.transactions.LedgerTransaction
@@ -20,9 +19,6 @@ import net.corda.core.transactions.TransactionBuilder
 import net.corda.core.transactions.WireTransaction
 import net.corda.core.utilities.OpaqueBytes
 import net.corda.core.utilities.UntrustworthyData
-import org.bouncycastle.asn1.x500.X500Name
-import org.bouncycastle.asn1.x500.X500NameBuilder
-import org.bouncycastle.asn1.x500.style.BCStyle
 import org.slf4j.Logger
 import org.slf4j.MDC
 import rx.Observable
@@ -477,27 +473,6 @@ $trustAnchor""", e, this, e.index)
     }
 }
 
-/**
- * Return the underlying X.500 name from this Corda-safe X.500 name. These are guaranteed to have a consistent
- * ordering, such that their `toString()` function returns the same value every time for the same [CordaX500Name].
- */
-val CordaX500Name.x500Name: X500Name
-    get() {
-        return X500NameBuilder(BCStyle.INSTANCE).apply {
-            addRDN(BCStyle.C, country)
-            state?.let { addRDN(BCStyle.ST, it) }
-            addRDN(BCStyle.L, locality)
-            addRDN(BCStyle.O, organisation)
-            organisationUnit?.let { addRDN(BCStyle.OU, it) }
-            commonName?.let { addRDN(BCStyle.CN, it) }
-        }.build()
-    }
-
-@Suppress("unused")
-@VisibleForTesting
-val CordaX500Name.Companion.unspecifiedCountry
-    get() = "ZZ"
-
 inline fun <T : Any> T.signWithCert(signer: (SerializedBytes<T>) -> DigitalSignatureWithCert): SignedDataWithCert<T> {
     val serialised = serialize()
     return SignedDataWithCert(serialised, signer(serialised))

core/src/main/kotlin/net/corda/core/internal/X500Utils.kt

@@ -0,0 +1,73 @@
+@file:KeepForDJVM
+
+package net.corda.core.internal
+
+import net.corda.core.KeepForDJVM
+import net.corda.core.identity.CordaX500Name
+import org.bouncycastle.asn1.ASN1Encodable
+import org.bouncycastle.asn1.ASN1ObjectIdentifier
+import org.bouncycastle.asn1.x500.AttributeTypeAndValue
+import org.bouncycastle.asn1.x500.X500Name
+import org.bouncycastle.asn1.x500.X500NameBuilder
+import org.bouncycastle.asn1.x500.style.BCStyle
+import javax.security.auth.x500.X500Principal
+
+/**
+ * Return the underlying X.500 name from this Corda-safe X.500 name. These are guaranteed to have a consistent
+ * ordering, such that their `toString()` function returns the same value every time for the same [CordaX500Name].
+ */
+fun CordaX500Name.toX500Name(): X500Name {
+    return X500NameBuilder(BCStyle.INSTANCE).apply {
+        addRDN(BCStyle.C, country)
+        state?.let { addRDN(BCStyle.ST, it) }
+        addRDN(BCStyle.L, locality)
+        addRDN(BCStyle.O, organisation)
+        organisationUnit?.let { addRDN(BCStyle.OU, it) }
+        commonName?.let { addRDN(BCStyle.CN, it) }
+    }.build()
+}
+
+/**
+ * Converts the X500Principal instance to the X500Name object.
+ */
+fun X500Principal.toX500Name(): X500Name = X500Name.getInstance(this.encoded)
+
+/**
+ * Transforms the X500Principal to the attributes map.
+ *
+ * @param supportedAttributes list of supported attributes. If empty, it accepts all the attributes.
+ *
+ * @return attributes map for this principal
+ * @throws IllegalArgumentException if this principal consists of duplicated attributes or the attribute is not supported.
+ *
+ */
+fun X500Principal.toAttributesMap(supportedAttributes: Set<ASN1ObjectIdentifier> = emptySet()): Map<ASN1ObjectIdentifier, ASN1Encodable> {
+    val x500Name = this.toX500Name()
+    val attrsMap: Map<ASN1ObjectIdentifier, ASN1Encodable> = x500Name.rdNs
+            .flatMap { it.typesAndValues.asList() }
+            .groupBy(AttributeTypeAndValue::getType, AttributeTypeAndValue::getValue)
+            .mapValues {
+                require(it.value.size == 1) { "Duplicate attribute ${it.key}" }
+                it.value[0]
+            }
+    if (supportedAttributes.isNotEmpty()) {
+        (attrsMap.keys - supportedAttributes).let { unsupported ->
+            require(unsupported.isEmpty()) {
+                "The following attribute${if (unsupported.size > 1) "s are" else " is"} not supported in Corda: " +
+                        unsupported.map { BCStyle.INSTANCE.oidToDisplayName(it) }
+            }
+        }
+    }
+    return attrsMap
+}
+
+/**
+ * Checks equality between the two X500Principal instances ignoring the ordering of the X500Name parts.
+ */
+fun X500Principal.isEquivalentTo(other: X500Principal): Boolean {
+    return toAttributesMap() == other.toAttributesMap()
+}
+
+@VisibleForTesting
+val CordaX500Name.Companion.unspecifiedCountry
+    get() = "ZZ"

core/src/test/kotlin/net/corda/core/identity/X500UtilsTest.kt

@@ -0,0 +1,26 @@
+package net.corda.core.identity
+
+import net.corda.core.internal.isEquivalentTo
+import org.junit.Test
+import javax.security.auth.x500.X500Principal
+import kotlin.test.assertTrue
+
+class X500UtilsTest {
+
+    @Test
+    fun `X500Principal equalX500NameParts matches regardless the order`() {
+        // given
+        val orderingA = "O=Bank A, OU=Organisation Unit, L=New York, C=US"
+        val orderingB = "OU=Organisation Unit, O=Bank A, L=New York, C=US"
+        val orderingC = "L=New York, O=Bank A, C=US, OU=Organisation Unit"
+
+        // when
+        val principalA = X500Principal(orderingA)
+        val principalB = X500Principal(orderingB)
+        val principalC = X500Principal(orderingC)
+
+        // then
+        assertTrue { principalA.isEquivalentTo(principalB) }
+        assertTrue { principalB.isEquivalentTo(principalC) }
+    }
+}

node-api/src/main/kotlin/net/corda/nodeapi/internal/KeyStoreConfigHelpers.kt

@@ -4,7 +4,7 @@ import net.corda.core.crypto.Crypto
 import net.corda.core.crypto.Crypto.generateKeyPair
 import net.corda.core.identity.CordaX500Name
 import net.corda.core.identity.PartyAndCertificate
-import net.corda.core.internal.x500Name
+import net.corda.core.internal.toX500Name
 import net.corda.nodeapi.internal.config.SSLConfiguration
 import net.corda.nodeapi.internal.crypto.*
 import org.bouncycastle.asn1.x509.GeneralName
@@ -83,7 +83,7 @@ fun createDevNetworkMapCa(rootCa: CertificateAndKeyPair = DEV_ROOT_CA): Certific
 fun createDevNodeCa(intermediateCa: CertificateAndKeyPair,
                     legalName: CordaX500Name,
                     nodeKeyPair: KeyPair = generateKeyPair(X509Utilities.DEFAULT_TLS_SIGNATURE_SCHEME)): CertificateAndKeyPair {
-    val nameConstraints = NameConstraints(arrayOf(GeneralSubtree(GeneralName(GeneralName.directoryName, legalName.x500Name))), arrayOf())
+    val nameConstraints = NameConstraints(arrayOf(GeneralSubtree(GeneralName(GeneralName.directoryName, legalName.toX500Name()))), arrayOf())
     val cert = X509Utilities.createCertificate(
             CertificateType.NODE_CA,
             intermediateCa.certificate,

node/src/integration-test/kotlin/net/corda/services/messaging/MQSecurityAsNodeTest.kt

@@ -5,7 +5,7 @@ import net.corda.core.identity.CordaX500Name
 import net.corda.core.internal.copyTo
 import net.corda.core.internal.createDirectories
 import net.corda.core.internal.exists
-import net.corda.core.internal.x500Name
+import net.corda.core.internal.toX500Name
 import net.corda.nodeapi.RPCApi
 import net.corda.nodeapi.internal.ArtemisMessagingComponent.Companion.NODE_P2P_USER
 import net.corda.nodeapi.internal.ArtemisMessagingComponent.Companion.PEER_USER
@@ -99,7 +99,7 @@ class MQSecurityAsNodeTest : P2PMQSecurityTest() {
 
                 val clientKeyPair = Crypto.generateKeyPair(X509Utilities.DEFAULT_TLS_SIGNATURE_SCHEME)
                 // Set name constrain to the legal name.
-                val nameConstraints = NameConstraints(arrayOf(GeneralSubtree(GeneralName(GeneralName.directoryName, legalName.x500Name))), arrayOf())
+                val nameConstraints = NameConstraints(arrayOf(GeneralSubtree(GeneralName(GeneralName.directoryName, legalName.toX500Name()))), arrayOf())
                 val clientCACert = X509Utilities.createCertificate(
                         CertificateType.INTERMEDIATE_CA,
                         DEV_INTERMEDIATE_CA.certificate,

node/src/main/kotlin/net/corda/node/utilities/registration/NetworkRegistrationHelper.kt

@@ -25,6 +25,7 @@ import java.security.PublicKey
 import java.security.cert.X509Certificate
 import java.time.Duration
 import javax.naming.ServiceUnavailableException
+import javax.security.auth.x500.X500Principal
 
 /**
  * Helper for managing the node registration process, which checks for any existing certificates and requests them if
@@ -43,13 +44,14 @@ open class NetworkRegistrationHelper(private val config: SSLConfiguration,
 
     companion object {
         const val SELF_SIGNED_PRIVATE_KEY = "Self Signed Private Key"
+        val logger = contextLogger()
     }
 
     private val requestIdStore = config.certificatesDirectory / "certificate-request-id.txt"
     // TODO: Use different password for private key.
     private val privateKeyPassword = config.keyStorePassword
     private val rootTrustStore: X509KeyStore
-    private val rootCert: X509Certificate
+    protected val rootCert: X509Certificate
 
     init {
         require(networkRootTrustStorePath.exists()) {
@@ -78,6 +80,13 @@ open class NetworkRegistrationHelper(private val config: SSLConfiguration,
             println("Certificate already exists, Corda node will now terminate...")
             return
         }
+        val tlsCrlIssuerCert = validateAndGetTlsCrlIssuerCert()
+        if (tlsCrlIssuerCert == null && isTlsCrlIssuerCertRequired()) {
+            System.err.println("""tlsCrlIssuerCert config does not match the root certificate issuer and nor is there any other certificate in the trust store with a matching issuer.
+                | Please make sure the config is correct or that the correct certificate for the CRL issuer is added to the node's trust store.
+                | The node will now terminate.""".trimMargin())
+            throw IllegalArgumentException("TLS CRL issuer certificate not found in the trust store.")
+        }
 
         val keyPair = nodeKeyStore.loadOrCreateKeyPair(SELF_SIGNED_PRIVATE_KEY)
 
@@ -94,7 +103,7 @@ open class NetworkRegistrationHelper(private val config: SSLConfiguration,
         }
         validateCertificates(keyPair.public, certificates)
         storePrivateKeyWithCertificates(nodeKeyStore, keyPair, certificates, keyAlias)
-        onSuccess(keyPair, certificates)
+        onSuccess(keyPair, certificates, tlsCrlIssuerCert?.let { it.subjectX500Principal.toX500Name() })
         // All done, clean up temp files.
         requestIdStore.deleteIfExists()
     }
@@ -216,7 +225,11 @@ open class NetworkRegistrationHelper(private val config: SSLConfiguration,
         }
     }
 
-    protected open fun onSuccess(nodeCAKeyPair: KeyPair, certificates: List<X509Certificate>) {}
+    protected open fun onSuccess(nodeCAKeyPair: KeyPair, certificates: List<X509Certificate>, tlsCrlCertificateIssuer: X500Name?) {}
+
+    protected open fun validateAndGetTlsCrlIssuerCert(): X509Certificate? = null
+
+    protected open fun isTlsCrlIssuerCertRequired(): Boolean = false
 }
 
 class UnableToRegisterNodeWithDoormanException : IOException()
@@ -236,12 +249,12 @@ class NodeRegistrationHelper(private val config: NodeConfiguration, certService:
         val logger = contextLogger()
     }
 
-    override fun onSuccess(nodeCAKeyPair: KeyPair, certificates: List<X509Certificate>) {
+    override fun onSuccess(nodeCAKeyPair: KeyPair, certificates: List<X509Certificate>, tlsCrlCertificateIssuer: X500Name?) {
-        createSSLKeystore(nodeCAKeyPair, certificates)
+        createSSLKeystore(nodeCAKeyPair, certificates, tlsCrlCertificateIssuer)
         createTruststore(certificates.last())
     }
 
-    private fun createSSLKeystore(nodeCAKeyPair: KeyPair, certificates: List<X509Certificate>) {
+    private fun createSSLKeystore(nodeCAKeyPair: KeyPair, certificates: List<X509Certificate>, tlsCertCrlIssuer: X500Name?) {
         config.loadSslKeyStore(createNew = true).update {
             println("Generating SSL certificate for node messaging service.")
             val sslKeyPair = Crypto.generateKeyPair(X509Utilities.DEFAULT_TLS_SIGNATURE_SCHEME)
@@ -252,7 +265,7 @@ class NodeRegistrationHelper(private val config: NodeConfiguration, certService:
                     config.myLegalName.x500Principal,
                     sslKeyPair.public,
                     crlDistPoint = config.tlsCertCrlDistPoint?.toString(),
-                    crlIssuer = if (config.tlsCertCrlIssuer != null) X500Name(config.tlsCertCrlIssuer) else null)
+                    crlIssuer = tlsCertCrlIssuer)
             logger.info("Generated TLS certificate: $sslCert")
             setPrivateKey(CORDA_CLIENT_TLS, sslKeyPair.private, listOf(sslCert) + certificates)
         }
@@ -268,6 +281,37 @@ class NodeRegistrationHelper(private val config: NodeConfiguration, certService:
         }
         println("Node trust store stored in ${config.trustStoreFile}.")
     }
+
+    override fun validateAndGetTlsCrlIssuerCert(): X509Certificate? {
+        config.tlsCertCrlIssuer ?: return null
+        val tlsCertCrlIssuerPrincipal = X500Principal(config.tlsCertCrlIssuer)
+        if (principalMatchesCertificatePrincipal(tlsCertCrlIssuerPrincipal, rootCert)) {
+            return rootCert
+        }
+        return if (config.trustStoreFile.exists()) {
+            findMatchingCertificate(tlsCertCrlIssuerPrincipal, config.loadTrustStore())
+        } else {
+            null
+        }
+    }
+
+    override fun isTlsCrlIssuerCertRequired(): Boolean {
+        return !config.tlsCertCrlIssuer.isNullOrEmpty()
+    }
+
+    private fun findMatchingCertificate(principal: X500Principal, trustStore: X509KeyStore): X509Certificate? {
+        trustStore.aliases().forEach {
+            val certificate = trustStore.getCertificate(it)
+            if (principalMatchesCertificatePrincipal(principal, certificate)) {
+                return certificate
+            }
+        }
+        return null
+    }
+
+    private fun principalMatchesCertificatePrincipal(principal: X500Principal, certificate: X509Certificate): Boolean {
+        return certificate.subjectX500Principal.isEquivalentTo(principal)
+    }
 }
 
 private class FixedPeriodLimitedRetrialStrategy(times: Int, private val period: Duration) : (Duration?) -> Duration? {

node/src/test/kotlin/net/corda/node/utilities/registration/NetworkRegistrationHelperTest.kt

@@ -12,10 +12,9 @@ import net.corda.core.identity.CordaX500Name
 import net.corda.core.internal.CertRole
 import net.corda.core.internal.createDirectories
 import net.corda.core.internal.div
-import net.corda.core.internal.x500Name
+import net.corda.core.internal.toX500Name
 import net.corda.core.utilities.seconds
 import net.corda.node.NodeRegistrationOption
-import net.corda.node.VersionInfo
 import net.corda.node.services.config.NodeConfiguration
 import net.corda.nodeapi.internal.DevIdentityGenerator
 import net.corda.nodeapi.internal.crypto.CertificateAndKeyPair
@@ -61,6 +60,7 @@ class NetworkRegistrationHelperTest {
             doReturn("").whenever(it).emailAddress
             doReturn(null).whenever(it).tlsCertCrlDistPoint
             doReturn(null).whenever(it).tlsCertCrlIssuer
+            doReturn(true).whenever(it).crlCheckSoftFail
         }
     }
 
@@ -182,7 +182,7 @@ class NetworkRegistrationHelperTest {
                                      rootAndIntermediateCA: Pair<CertificateAndKeyPair, CertificateAndKeyPair> = createDevIntermediateCaCertPath()): List<X509Certificate> {
         val (rootCa, intermediateCa) = rootAndIntermediateCA
         val nameConstraints = if (type == CertificateType.NODE_CA) {
-            NameConstraints(arrayOf(GeneralSubtree(GeneralName(GeneralName.directoryName, legalName.x500Name))), arrayOf())
+            NameConstraints(arrayOf(GeneralSubtree(GeneralName(GeneralName.directoryName, legalName.toX500Name()))), arrayOf())
         } else {
             null
         }