Correct certificate path validation in identity service

* Unify certificate path validation in identity service
* Ensure we won't accept trust root or intermediaries as owner of a transaction certificate
* Fix subject key parsing

node/src/main/kotlin/net/corda/node/services/identity/InMemoryIdentityService.kt

@@ -1,7 +1,6 @@
 package net.corda.node.services.identity
 
 import net.corda.core.contracts.PartyAndReference
-import net.corda.core.contracts.requireThat
 import net.corda.core.crypto.cert
 import net.corda.core.crypto.subject
 import net.corda.core.crypto.toStringShort
@@ -21,6 +20,7 @@ import java.security.cert.*
 import java.util.*
 import java.util.concurrent.ConcurrentHashMap
 import javax.annotation.concurrent.ThreadSafe
+import javax.security.auth.x500.X500Principal
 import kotlin.collections.ArrayList
 
 /**
@@ -56,17 +56,7 @@ class InMemoryIdentityService(identities: Iterable<PartyAndCertificate>,
     override fun registerIdentity(party: PartyAndCertificate) {
         require(party.certPath.certificates.isNotEmpty()) { "Certificate path must contain at least one certificate" }
         // Validate the chain first, before we do anything clever with it
-        val validatorParameters = if (trustAnchor != null) {
-            PKIXParameters(setOf(trustAnchor))
-        } else {
-            // TODO: We should always require a full chain back to a trust anchor, but until we have a network
-            // trust anchor everywhere, this will have to do.
-            PKIXParameters(setOf(TrustAnchor(party.certificate.cert, null)))
-        }
-        validatorParameters.isRevocationEnabled = false
-        // TODO: val result = validator.validate(party.certPath, validatorParameters) as PKIXCertPathValidatorResult
-        // require(trustAnchor == null || result.trustAnchor == trustAnchor)
-        // require(result.publicKey == party.owningKey) { "Certificate path validation must end at transaction key ${anonymousParty.owningKey.toStringShort()}, found ${result.publicKey.toStringShort()}" }
+        if (trustRoot != null) validateCertificatePath(party.party, party.certPath)
 
         log.trace { "Registering identity $party" }
         require(Arrays.equals(party.certificate.subjectPublicKeyInfo.encoded, party.owningKey.encoded)) { "Party certificate must end with party's public key" }
@@ -117,12 +107,11 @@ class InMemoryIdentityService(identities: Iterable<PartyAndCertificate>,
     @Throws(IdentityService.UnknownAnonymousPartyException::class)
     override fun assertOwnership(party: Party, anonymousParty: AnonymousParty) {
         val path = partyToPath[anonymousParty] ?: throw IdentityService.UnknownAnonymousPartyException("Unknown anonymous party ${anonymousParty.owningKey.toStringShort()}")
-        val root: X509Certificate = path.certificates
-                .filterIsInstance<X509Certificate>()
-                .lastOrNull { it.publicKey == party.owningKey } ?: throw IllegalArgumentException("Certificate path must include a certificate for the party public key.")
-        // Verify there's a previous certificate in the path, which matches
-        val target = path.certificates.first() as X509Certificate
-        require(target.publicKey == anonymousParty.owningKey) { "Certificate path starts with a certificate for the anonymous party" }
+        require(path.certificates.size > 1) { "Certificate path must contain at least two certificates" }
+        val actual = path.certificates[1]
+        require(actual is X509Certificate && actual.publicKey == party.owningKey) { "Next certificate in the path must match the party key ${party.owningKey.toStringShort()}." }
+        val target = path.certificates.first()
+        require(target is X509Certificate && target.publicKey == anonymousParty.owningKey) { "Certificate path starts with a certificate for the anonymous party" }
     }
 
     override fun pathForAnonymous(anonymousParty: AnonymousParty): CertPath? = partyToPath[anonymousParty]
@@ -132,19 +121,8 @@ class InMemoryIdentityService(identities: Iterable<PartyAndCertificate>,
         val fullParty = certificateFromParty(party) ?: throw IllegalArgumentException("Unknown identity ${party.name}")
         require(path.certificates.isNotEmpty()) { "Certificate path must contain at least one certificate" }
         // Validate the chain first, before we do anything clever with it
-        val validator = CertPathValidator.getInstance("PKIX")
-        val validatorParameters = if (trustAnchor != null) {
-            PKIXParameters(setOf(trustAnchor))
-        } else {
-            // TODO: We should always require a full chain back to a trust anchor, but until we have a network
-            // trust anchor everywhere, this will have to do.
-            PKIXParameters(setOf(TrustAnchor(fullParty.certificate.cert, null)))
-        }
-        validatorParameters.isRevocationEnabled = false
-        val result = validator.validate(path, validatorParameters) as PKIXCertPathValidatorResult
+        if (trustRoot != null) validateCertificatePath(anonymousParty, path)
         val subjectCertificate = path.certificates.first()
-        require(trustAnchor == null || result.trustAnchor == trustAnchor)
-        require(result.publicKey == anonymousParty.owningKey) { "Certificate path validation must end at transaction key ${anonymousParty.owningKey.toStringShort()}, found ${result.publicKey.toStringShort()}" }
         require(subjectCertificate is X509Certificate && subjectCertificate.subject == fullParty.name) { "Subject of the transaction certificate must match the well known identity" }
 
         log.trace { "Registering identity $fullParty" }
@@ -153,4 +131,17 @@ class InMemoryIdentityService(identities: Iterable<PartyAndCertificate>,
         keyToParties[anonymousParty.owningKey] = fullParty
         principalToParties[fullParty.name] = fullParty
     }
+
+    /**
+     * Verify that the given certificate path is valid and leads to the owning key of the party.
+     */
+    private fun validateCertificatePath(party: AbstractParty, path: CertPath): PKIXCertPathValidatorResult {
+        val validatorParameters = PKIXParameters(setOf(trustAnchor))
+        val validator = CertPathValidator.getInstance("PKIX")
+        validatorParameters.isRevocationEnabled = false
+        val result = validator.validate(path, validatorParameters) as PKIXCertPathValidatorResult
+        require(result.trustAnchor == null || result.trustAnchor == trustAnchor)
+        require(result.publicKey == party.owningKey) { "Certificate path validation must end at owning key ${party.owningKey.toStringShort()}, found ${result.publicKey.toStringShort()}" }
+        return result
+    }
 }

node/src/test/kotlin/net/corda/node/services/network/InMemoryIdentityServiceTests.kt

@@ -129,6 +129,11 @@ class InMemoryIdentityServiceTests {
         assertFailsWith<IllegalArgumentException> {
             service.assertOwnership(bob.party, aliceTxIdentity.identity)
         }
+
+        assertFailsWith<IllegalArgumentException> {
+            val owningKey = Crypto.decodePublicKey(trustRoot.certificate.subjectPublicKeyInfo.encoded)
+            service.assertOwnership(Party(trustRoot.certificate.subject, owningKey), aliceTxIdentity.identity)
+        }
     }
 
     private fun createParty(x500Name: X500Name, ca: CertificateAndKeyPair): Pair<PartyAndCertificate, TxKeyFlow.AnonymousIdentity> {