CORDA-3124: Ensure transaction is fully verified before sending to notary (#5516)

NotaryFlow.Client flow assumes the passed transaction is verified, which is
the case when it's invoked as a sub-flow in FinalityFlow. However,
NotaryFlow.Client can also be called directly from any custom flow, so
we need to ensure it performs transaction verification to avoid accidentally
sending an invalid transaction to a non-validating notary.

core/src/main/kotlin/net/corda/core/flows/FinalityFlow.kt

@@ -216,7 +216,7 @@ class FinalityFlow private constructor(val transaction: SignedTransaction,
     private fun notariseAndRecord(): SignedTransaction {
         val notarised = if (needsNotarySignature(transaction)) {
             progressTracker.currentStep = NOTARISING
-            val notarySignatures = subFlow(NotaryFlow.Client(transaction))
+            val notarySignatures = subFlow(NotaryFlow.Client(transaction, skipVerification = true))
             transaction + notarySignatures
         } else {
             logger.info("No need to notarise this transaction.")
@@ -249,6 +249,7 @@ class FinalityFlow private constructor(val transaction: SignedTransaction,
         val notary = transaction.tx.notary
         // The notary signature(s) are allowed to be missing but no others.
         if (notary != null) transaction.verifySignaturesExcept(notary.owningKey) else transaction.verifyRequiredSignatures()
+        // TODO= [CORDA-3267] Remove duplicate signature verification
         val ltx = transaction.toLedgerTransaction(serviceHub, false)
         ltx.verify()
         return ltx

core/src/main/kotlin/net/corda/core/flows/NotaryFlow.kt

@@ -8,9 +8,12 @@ import net.corda.core.contracts.TimeWindow
 import net.corda.core.crypto.SecureHash
 import net.corda.core.crypto.TransactionSignature
 import net.corda.core.identity.Party
-import net.corda.core.internal.*
+import net.corda.core.internal.BackpressureAwareTimedFlow
+import net.corda.core.internal.FetchDataFlow
+import net.corda.core.internal.NetworkParametersStorage
 import net.corda.core.internal.notary.generateSignature
 import net.corda.core.internal.notary.validateSignatures
+import net.corda.core.internal.pushToLoggingContext
 import net.corda.core.transactions.*
 import net.corda.core.utilities.ProgressTracker
 import net.corda.core.utilities.UntrustworthyData
@@ -35,9 +38,16 @@ class NotaryFlow {
     @InitiatingFlow
     open class Client(
             private val stx: SignedTransaction,
-            override val progressTracker: ProgressTracker
+            override val progressTracker: ProgressTracker,
+            /**
+             * Set to *true* if the [stx] has already been verified for signature and contract validity,
+             * to prevent re-verification.
+             */
+            private val skipVerification: Boolean = false
     ) : BackpressureAwareTimedFlow<List<TransactionSignature>>() {
-        constructor(stx: SignedTransaction) : this(stx, tracker())
+        @JvmOverloads
+        constructor(stx: SignedTransaction, skipVerification: Boolean = false) : this(stx, tracker(), skipVerification)
+        constructor(stx: SignedTransaction, progressTracker: ProgressTracker): this(stx, progressTracker, false)
 
         companion object {
             object REQUESTING : ProgressTracker.Step("Requesting signature by Notary service")
@@ -70,16 +80,21 @@ class NotaryFlow {
          * Checks that the transaction specifies a valid notary, and verifies that it contains all required signatures
          * apart from the notary's.
          */
-        // TODO: [CORDA-2274] Perform full transaction verification once verification caching is enabled.
         protected fun checkTransaction(): Party {
             val notaryParty = stx.notary ?: throw IllegalStateException("Transaction does not specify a Notary")
             check(serviceHub.networkMapCache.isNotary(notaryParty)) { "$notaryParty is not a notary on the network" }
             check(serviceHub.loadStates(stx.inputs.toSet() + stx.references.toSet()).all { it.state.notary == notaryParty }) {
                 "Input states and reference input states must have the same Notary"
             }
-            stx.resolveTransactionWithSignatures(serviceHub).verifySignaturesExcept(notaryParty.owningKey)
+
+            if (!skipVerification) {
+                // TODO= [CORDA-3267] Remove duplicate signature verification
+                stx.resolveTransactionWithSignatures(serviceHub).verifySignaturesExcept(notaryParty.owningKey)
+                stx.verify(serviceHub, false)
+            }
             return notaryParty
         }
+
         /** Notarises the transaction with the [notaryParty], obtains the notary's signature(s). */
         @Throws(NotaryException::class)
         @Suspendable
@@ -107,7 +122,6 @@ class NotaryFlow {
                 val historicNotary = (serviceHub.networkParametersService as NetworkParametersStorage).getHistoricNotary(notaryParty)
                         ?: throw IllegalStateException("The notary party $notaryParty specified by transaction ${stx.id}, is not recognised as a current or historic notary.")
                 historicNotary.validating
-
             } else serviceHub.networkMapCache.isValidatingNotary(notaryParty)
         }
 

docs/source/changelog.rst

@@ -6,6 +6,7 @@ release, see :doc:`app-upgrade-notes`.
 
 Unreleased
 ----------
+
 * Moved and renamed the testing web server to the ``testing`` subproject. Also renamed the published artifact to ``corda-testserver.jar``.
 
 * Support for Java 11 (compatibility mode). Please read https://github.com/corda/corda/pull/5356.
@@ -33,6 +34,10 @@ Unreleased
 
 * Introduced a new low level flow diagnostics tool: checkpoint agent (that can be used standalone or in conjunction with the ``checkpoints dump`` shell command).
   See :doc:`checkpoint-tooling` for more information.
+  
+* ``NotaryFlow.Client`` now performs transaction verification by default to prevent accidentally sending an invalid transaction to a
+  non-validating notary. The behaviour can be controlled by passing a constructor parameter flag ``skipVerification``.
+  Note: this only affects flows that invoke ``NotaryFlow.Client`` directly – no behavioural change if using ``FinalityFlow``.
 
 * The MockNet now supports setting a custom Notary class name, as was already supported by normal node config. See :doc:`tutorial-custom-notary`.
 

node/src/test/kotlin/net/corda/node/services/transactions/NotaryWhitelistTests.kt

@@ -167,7 +167,7 @@ class NotaryWhitelistTests(
     }
 
     private fun runNotaryClient(stx: SignedTransaction): CordaFuture<List<TransactionSignature>> {
-        val flow = NotaryFlow.Client(stx)
+        val flow = NotaryFlow.Client(stx, skipVerification = true)
         val future = aliceNode.services.startFlow(flow).resultFuture
         mockNet.runNetwork()
         return future

node/src/test/kotlin/net/corda/node/services/transactions/ValidatingNotaryServiceTests.kt

@@ -91,13 +91,12 @@ class ValidatingNotaryServiceTests {
             aliceNode.services.signInitialTransaction(tx)
         }
 
-        // Expecting SignaturesMissingException instead of NotaryException, since the exception should originate from
-        // the client flow.
-        val ex = assertFailsWith<SignedTransaction.SignaturesMissingException> {
+        val ex = assertFailsWith<NotaryException> {
             val future = runNotaryClient(stx)
             future.getOrThrow()
         }
-        val missingKeys = ex.missing
+        val exception = (ex.error as NotaryError.TransactionInvalid).cause as SignedTransaction.SignaturesMissingException
+        val missingKeys = exception.missing
         assertEquals(setOf(expectedMissingKey), missingKeys)
     }
 
@@ -349,7 +348,7 @@ class ValidatingNotaryServiceTests {
     }
 
     private fun runNotaryClient(stx: SignedTransaction): CordaFuture<List<TransactionSignature>> {
-        val flow = NotaryFlow.Client(stx)
+        val flow = NotaryFlow.Client(stx, skipVerification = true)
         val future = aliceNode.services.startFlow(flow).resultFuture
         mockNet.runNetwork()
         return future