CORDA-1264: Mask internal errors if devMode is false (#3942)

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

@@ -21,13 +21,13 @@ import net.corda.core.CordaRuntimeException
  *     the exception is handled. This ID is propagated to counterparty flows, even when the [FlowException] is
  *     downgraded to an [UnexpectedFlowEndException]. This is so the error conditions may be correlated later on.
  */
-open class FlowException(message: String?, cause: Throwable?) :
+open class FlowException(message: String?, cause: Throwable?, var originalErrorId: Long? = null) :
         CordaException(message, cause), IdentifiableException {
+    constructor(message: String?, cause: Throwable?) : this(message, cause, null)
     constructor(message: String?) : this(message, null)
     constructor(cause: Throwable?) : this(cause?.toString(), cause)
     constructor() : this(null, null)
 
-    var originalErrorId: Long? = null
     override fun getErrorId(): Long? = originalErrorId
 }
 // DOCEND 1

docs/source/changelog.rst

@@ -105,6 +105,9 @@ Unreleased
 * ``WireTransaction.Companion.createComponentGroups`` has been marked as ``@CordaInternal``. It was never intended to be
   public and was already internal for Kotlin code.
 
+* RPC server will now mask internal errors to RPC clients if not in devMode. ``Throwable``s implementing ``ClientRelevantError``
+  will continue to be propagated to clients.
+
 * RPC Framework moved from Kryo to the Corda AMQP implementation [Corda-847]. This completes the removal
   of ``Kryo`` from general use within Corda, remaining only for use in flow checkpointing.
 

docs/source/clientrpc.rst

@@ -338,9 +338,15 @@ Error handling
 --------------
 If something goes wrong with the RPC infrastructure itself, an ``RPCException`` is thrown. If you call a method that
 requires a higher version of the protocol than the server supports, ``UnsupportedOperationException`` is thrown.
-Otherwise, if the server implementation throws an exception, that exception is serialised and rethrown on the client
+Otherwise the behaviour depends on the ``devMode`` node configuration option.
+
+In ``devMode``, if the server implementation throws an exception, that exception is serialised and rethrown on the client
 side as if it was thrown from inside the called RPC method. These exceptions can be caught as normal.
 
+When not in ``devMode``, the server will mask exceptions not meant for clients and return an ``InternalNodeException`` instead.
+This does not expose internal information to clients, strengthening privacy and security. CorDapps can have exceptions implement
+``ClientRelevantError`` to allow them to reach RPC clients.
+
 Connection management
 ---------------------
 It is possible to not be able to connect to the server on the first attempt. In that case, the ``CordaRPCClient.start()``

node/src/integration-test/kotlin/net/corda/node/BootTests.kt

@@ -12,10 +12,14 @@ import net.corda.core.messaging.startFlow
 import net.corda.core.utilities.getOrThrow
 import net.corda.node.internal.NodeStartup
 import net.corda.node.services.Permissions.Companion.startFlow
+import net.corda.nodeapi.exceptions.InternalNodeException
 import net.corda.testing.core.ALICE_NAME
 import net.corda.testing.driver.DriverParameters
+import net.corda.testing.driver.NodeHandle
+import net.corda.testing.driver.NodeParameters
 import net.corda.testing.driver.driver
 import net.corda.testing.node.User
+import net.corda.testing.node.internal.startNode
 import org.assertj.core.api.Assertions.assertThatThrownBy
 import org.junit.Test
 import java.io.*
@@ -24,12 +28,21 @@ import kotlin.test.assertEquals
 class BootTests {
     @Test
     fun `java deserialization is disabled`() {
-        driver(DriverParameters(notarySpecs = emptyList())) {
         val user = User("u", "p", setOf(startFlow<ObjectInputStreamFlow>()))
-            val future = CordaRPCClient(startNode(rpcUsers = listOf(user)).getOrThrow().rpcAddress).
+        val params = NodeParameters(rpcUsers = listOf(user))
-                    start(user.username, user.password).proxy.startFlow(::ObjectInputStreamFlow).returnValue
+
-            assertThatThrownBy { future.getOrThrow() }
+        fun NodeHandle.attemptJavaDeserialization() {
-                    .isInstanceOf(CordaRuntimeException::class.java)
+            CordaRPCClient(rpcAddress).use(user.username, user.password) { connection ->
+                connection.proxy
+                rpc.startFlow(::ObjectInputStreamFlow).returnValue.getOrThrow()
+            }
+        }
+        driver {
+            val devModeNode = startNode(params).getOrThrow()
+            val node = startNode(ALICE_NAME, devMode = false, parameters = params).getOrThrow()
+
+            assertThatThrownBy { devModeNode.attemptJavaDeserialization() }.isInstanceOf(CordaRuntimeException::class.java)
+            assertThatThrownBy { node.attemptJavaDeserialization() }.isInstanceOf(InternalNodeException::class.java)
         }
     }
 

node/src/integration-test/kotlin/net/corda/node/services/network/NetworkMapTest.kt

@@ -1,8 +1,6 @@
 package net.corda.node.services.network
 
-import net.corda.core.concurrent.CordaFuture
 import net.corda.core.crypto.random63BitValue
-import net.corda.core.identity.CordaX500Name
 import net.corda.core.internal.*
 import net.corda.core.internal.concurrent.transpose
 import net.corda.core.messaging.ParametersUpdateInfo
@@ -10,7 +8,6 @@ import net.corda.core.node.NodeInfo
 import net.corda.core.serialization.serialize
 import net.corda.core.utilities.getOrThrow
 import net.corda.core.utilities.seconds
-import net.corda.node.services.config.configureDevKeyAndTrustStores
 import net.corda.nodeapi.internal.NODE_INFO_DIRECTORY
 import net.corda.nodeapi.internal.network.NETWORK_PARAMS_FILE_NAME
 import net.corda.nodeapi.internal.network.NETWORK_PARAMS_UPDATE_FILE_NAME
@@ -20,7 +17,6 @@ import net.corda.testing.core.*
 import net.corda.testing.driver.NodeHandle
 import net.corda.testing.driver.PortAllocation
 import net.corda.testing.driver.internal.NodeHandleInternal
-import net.corda.testing.internal.stubs.CertificateStoreStubs
 import net.corda.testing.node.internal.*
 import net.corda.testing.node.internal.network.NetworkMapServer
 import org.assertj.core.api.Assertions.assertThat
@@ -242,16 +238,3 @@ class NetworkMapTest(var initFunc: (URL, NetworkMapServer) -> CompatibilityZoneP
         assertThat(rpc.networkMapSnapshot()).containsOnly(*nodes)
     }
 }
-
-private fun DriverDSLImpl.startNode(providedName: CordaX500Name, devMode: Boolean): CordaFuture<NodeHandle> {
-    var customOverrides = emptyMap<String, String>()
-    if (!devMode) {
-        val nodeDir = baseDirectory(providedName)
-        val certificatesDirectory = nodeDir / "certificates"
-        val signingCertStore = CertificateStoreStubs.Signing.withCertificatesDirectory(certificatesDirectory)
-        val p2pSslConfig = CertificateStoreStubs.P2P.withCertificatesDirectory(certificatesDirectory)
-        p2pSslConfig.configureDevKeyAndTrustStores(providedName, signingCertStore, certificatesDirectory)
-        customOverrides = mapOf("devMode" to "false")
-    }
-    return startNode(providedName = providedName, customOverrides = customOverrides)
-}

node/src/integration-test/kotlin/net/corda/node/services/rpc/RpcExceptionHandlingTest.kt

@@ -9,13 +9,13 @@ import net.corda.core.messaging.startFlow
 import net.corda.core.utilities.getOrThrow
 import net.corda.core.utilities.unwrap
 import net.corda.node.services.Permissions
+import net.corda.nodeapi.exceptions.InternalNodeException
 import net.corda.testing.core.ALICE_NAME
 import net.corda.testing.core.BOB_NAME
 import net.corda.testing.core.singleIdentity
-import net.corda.testing.driver.DriverParameters
+import net.corda.testing.driver.*
-import net.corda.testing.driver.NodeParameters
-import net.corda.testing.driver.driver
 import net.corda.testing.node.User
+import net.corda.testing.node.internal.startNode
 import org.assertj.core.api.Assertions.assertThatThrownBy
 import org.assertj.core.api.AssertionsForInterfaceTypes.assertThat
 import org.hibernate.exception.GenericJDBCException
@@ -23,75 +23,95 @@ import org.junit.Test
 import java.sql.SQLException
 
 class RpcExceptionHandlingTest {
-
     private val user = User("mark", "dadada", setOf(Permissions.all()))
     private val users = listOf(user)
 
     @Test
-    fun `rpc client handles exceptions thrown on node side`() {
+    fun `rpc client receive client-relevant exceptions regardless of devMode`() {
-        driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList())) {
+        val params = NodeParameters(rpcUsers = users)
-
-            val node = startNode(NodeParameters(rpcUsers = users)).getOrThrow()
-
-            assertThatThrownBy { node.rpc.startFlow(::Flow).returnValue.getOrThrow() }.isInstanceOfSatisfying(CordaRuntimeException::class.java) { exception ->
-
-                assertThat(exception).hasNoCause()
-                assertThat(exception.stackTrace).isEmpty()
-            }
-        }
-    }
-
-    @Test
-    fun `rpc client handles client-relevant exceptions thrown on node side`() {
-        driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList())) {
-
-            val node = startNode(NodeParameters(rpcUsers = users)).getOrThrow()
         val clientRelevantMessage = "This is for the players!"
 
-            assertThatThrownBy { node.rpc.startFlow(::ClientRelevantErrorFlow, clientRelevantMessage).returnValue.getOrThrow() }.isInstanceOfSatisfying(CordaRuntimeException::class.java) { exception ->
+        fun NodeHandle.throwExceptionFromFlow() {
+            rpc.startFlow(::ClientRelevantErrorFlow, clientRelevantMessage).returnValue.getOrThrow()
+        }
+
+        fun assertThatThrownExceptionIsReceivedUnwrapped(node: NodeHandle) {
+            assertThatThrownBy { node.throwExceptionFromFlow() }.isInstanceOfSatisfying(ClientRelevantException::class.java) { exception ->
 
                 assertThat(exception).hasNoCause()
                 assertThat(exception.stackTrace).isEmpty()
                 assertThat(exception.message).isEqualTo(clientRelevantMessage)
             }
         }
+
+        driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList())) {
+            val devModeNode = startNode(params, BOB_NAME).getOrThrow()
+            val node = startNode(ALICE_NAME, devMode = false, parameters = params).getOrThrow()
+
+            assertThatThrownExceptionIsReceivedUnwrapped(devModeNode)
+            assertThatThrownExceptionIsReceivedUnwrapped(node)
+        }
     }
 
     @Test
-    fun `FlowException is received by the RPC client`() {
+    fun `FlowException is received by the RPC client only if in devMode`() {
+        val params = NodeParameters(rpcUsers = users)
+        val expectedMessage = "Flow error!"
+        val expectedErrorId = 123L
+
+        fun NodeHandle.throwExceptionFromFlow() {
+            rpc.startFlow(::FlowExceptionFlow, expectedMessage, expectedErrorId).returnValue.getOrThrow()
+        }
+
         driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList())) {
-            val node = startNode(NodeParameters(rpcUsers = users)).getOrThrow()
+            val devModeNode = startNode(params, BOB_NAME).getOrThrow()
-            val exceptionMessage = "Flow error!"
+            val node = startNode(ALICE_NAME, devMode = false, parameters = params).getOrThrow()
-            assertThatThrownBy { node.rpc.startFlow(::FlowExceptionFlow, exceptionMessage).returnValue.getOrThrow() }
+
-                    .isInstanceOfSatisfying(FlowException::class.java) { exception ->
+            assertThatThrownBy { devModeNode.throwExceptionFromFlow() }.isInstanceOfSatisfying(FlowException::class.java) { exception ->
+
                 assertThat(exception).hasNoCause()
                 assertThat(exception.stackTrace).isEmpty()
-                        assertThat(exception.message).isEqualTo(exceptionMessage)
+                assertThat(exception.message).isEqualTo(expectedMessage)
+                assertThat(exception.errorId).isEqualTo(expectedErrorId)
+            }
+            assertThatThrownBy { node.throwExceptionFromFlow() }.isInstanceOfSatisfying(InternalNodeException::class.java) { exception ->
+
+                assertThat(exception).hasNoCause()
+                assertThat(exception.stackTrace).isEmpty()
+                assertThat(exception.message).isEqualTo(InternalNodeException.message)
+                assertThat(exception.errorId).isEqualTo(expectedErrorId)
             }
         }
     }
 
     @Test
     fun `rpc client handles exceptions thrown on counter-party side`() {
+        val params = NodeParameters(rpcUsers = users)
+
+        fun DriverDSL.scenario(devMode: Boolean) {
+
+            val nodeA = startNode(ALICE_NAME, devMode, params).getOrThrow()
+            val nodeB = startNode(BOB_NAME, devMode, params).getOrThrow()
+
+            nodeA.rpc.startFlow(::InitFlow, nodeB.nodeInfo.singleIdentity()).returnValue.getOrThrow()
+        }
+
         driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList())) {
 
-            val nodeA = startNode(NodeParameters(providedName = ALICE_NAME, rpcUsers = users)).getOrThrow()
+            assertThatThrownBy { scenario(true) }.isInstanceOfSatisfying(CordaRuntimeException::class.java) { exception ->
-            val nodeB = startNode(NodeParameters(providedName = BOB_NAME, rpcUsers = users)).getOrThrow()
-
-            assertThatThrownBy { nodeA.rpc.startFlow(::InitFlow, nodeB.nodeInfo.singleIdentity()).returnValue.getOrThrow() }.isInstanceOfSatisfying(CordaRuntimeException::class.java) { exception ->
 
                 assertThat(exception).hasNoCause()
                 assertThat(exception.stackTrace).isEmpty()
             }
         }
-    }
+        driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList())) {
-}
+            assertThatThrownBy { scenario(false) }.isInstanceOfSatisfying(InternalNodeException::class.java) { exception ->
 
-@StartableByRPC
+                assertThat(exception).hasNoCause()
-class Flow : FlowLogic<String>() {
+                assertThat(exception.stackTrace).isEmpty()
-    @Suspendable
+                assertThat(exception.message).isEqualTo(InternalNodeException.message)
-    override fun call(): String {
+            }
-        throw GenericJDBCException("Something went wrong!", SQLException("Oops!"))
+        }
     }
 }
 
@@ -121,7 +141,11 @@ class ClientRelevantErrorFlow(private val message: String) : FlowLogic<String>()
 }
 
 @StartableByRPC
-class FlowExceptionFlow(private val message: String) : FlowLogic<String>() {
+class FlowExceptionFlow(private val message: String, private val errorId: Long? = null) : FlowLogic<String>() {
     @Suspendable
-    override fun call(): String = throw FlowException(message)
+    override fun call(): String {
+        val exception = FlowException(message)
+        errorId?.let { exception.originalErrorId = it }
+        throw exception
+    }
 }

node/src/main/kotlin/net/corda/node/internal/AbstractNode.kt

@@ -38,6 +38,7 @@ import net.corda.node.internal.cordapp.CordappConfigFileProvider
 import net.corda.node.internal.cordapp.CordappProviderImpl
 import net.corda.node.internal.cordapp.CordappProviderInternal
 import net.corda.node.internal.rpc.proxies.AuthenticatedRpcOpsProxy
+import net.corda.node.internal.rpc.proxies.ExceptionMaskingRpcOpsProxy
 import net.corda.node.internal.rpc.proxies.ExceptionSerialisingRpcOpsProxy
 import net.corda.node.services.ContractUpgradeHandler
 import net.corda.node.services.FinalityHandler
@@ -238,8 +239,13 @@ abstract class AbstractNode<S>(val configuration: NodeConfiguration,
     /** The implementation of the [CordaRPCOps] interface used by this node. */
     open fun makeRPCOps(): CordaRPCOps {
         val ops: CordaRPCOps = CordaRPCOpsImpl(services, smm, flowStarter) { shutdownExecutor.submit { stop() } }
+        val proxies = mutableListOf<(CordaRPCOps) -> CordaRPCOps>()
         // Mind that order is relevant here.
-        val proxies = listOf<(CordaRPCOps) -> CordaRPCOps>(::AuthenticatedRpcOpsProxy, { ExceptionSerialisingRpcOpsProxy(it, true) })
+        proxies += ::AuthenticatedRpcOpsProxy
+        if (!configuration.devMode) {
+            proxies += { it -> ExceptionMaskingRpcOpsProxy(it, true) }
+        }
+        proxies += { it -> ExceptionSerialisingRpcOpsProxy(it, configuration.devMode) }
         return proxies.fold(ops) { delegate, decorate -> decorate(delegate) }
     }
 

node/src/main/kotlin/net/corda/node/internal/rpc/proxies/ExceptionMaskingRpcOpsProxy.kt

@@ -0,0 +1,125 @@
+package net.corda.node.internal.rpc.proxies
+
+import net.corda.core.ClientRelevantError
+import net.corda.core.CordaException
+import net.corda.core.CordaRuntimeException
+import net.corda.core.concurrent.CordaFuture
+import net.corda.core.contracts.TransactionVerificationException
+import net.corda.core.doOnError
+import net.corda.core.flows.IdentifiableException
+import net.corda.core.internal.concurrent.doOnError
+import net.corda.core.internal.concurrent.mapError
+import net.corda.core.internal.declaredField
+import net.corda.core.mapErrors
+import net.corda.core.messaging.CordaRPCOps
+import net.corda.core.messaging.DataFeed
+import net.corda.core.messaging.FlowHandle
+import net.corda.core.messaging.FlowHandleImpl
+import net.corda.core.messaging.FlowProgressHandle
+import net.corda.core.messaging.FlowProgressHandleImpl
+import net.corda.core.utilities.loggerFor
+import net.corda.node.internal.InvocationHandlerTemplate
+import net.corda.nodeapi.exceptions.InternalNodeException
+import rx.Observable
+import java.lang.reflect.Method
+import java.lang.reflect.Proxy.newProxyInstance
+import kotlin.reflect.KClass
+
+internal class ExceptionMaskingRpcOpsProxy(private val delegate: CordaRPCOps, doLog: Boolean) : CordaRPCOps by proxy(delegate, doLog) {
+    private companion object {
+        private val logger = loggerFor<ExceptionMaskingRpcOpsProxy>()
+
+        private val whitelist = setOf(
+                ClientRelevantError::class,
+                TransactionVerificationException::class
+        )
+
+        private fun proxy(delegate: CordaRPCOps, doLog: Boolean): CordaRPCOps {
+            val handler = ErrorObfuscatingInvocationHandler(delegate, whitelist, doLog)
+            return newProxyInstance(delegate::class.java.classLoader, arrayOf(CordaRPCOps::class.java), handler) as CordaRPCOps
+        }
+    }
+
+    private class ErrorObfuscatingInvocationHandler(override val delegate: CordaRPCOps, private val whitelist: Set<KClass<*>>, private val doLog: Boolean) : InvocationHandlerTemplate {
+        override fun invoke(proxy: Any, method: Method, arguments: Array<out Any?>?): Any? {
+            try {
+                val result = super.invoke(proxy, method, arguments)
+                return result?.let { obfuscateResult(it) }
+            } catch (exception: Exception) {
+                // In this special case logging and re-throwing is the right approach.
+                log(exception)
+                throw obfuscate(exception)
+            }
+        }
+
+        private fun <RESULT : Any> obfuscateResult(result: RESULT): Any {
+            return when (result) {
+                is CordaFuture<*> -> wrapFuture(result)
+                is DataFeed<*, *> -> wrapFeed(result)
+                is FlowProgressHandle<*> -> wrapFlowProgressHandle(result)
+                is FlowHandle<*> -> wrapFlowHandle(result)
+                is Observable<*> -> wrapObservable(result)
+                else -> result
+            }
+        }
+
+        private fun wrapFlowProgressHandle(handle: FlowProgressHandle<*>): FlowProgressHandle<*> {
+            val returnValue = wrapFuture(handle.returnValue)
+            val progress = wrapObservable(handle.progress)
+            val stepsTreeIndexFeed = handle.stepsTreeIndexFeed?.let { wrapFeed(it) }
+            val stepsTreeFeed = handle.stepsTreeFeed?.let { wrapFeed(it) }
+
+            return FlowProgressHandleImpl(handle.id, returnValue, progress, stepsTreeIndexFeed, stepsTreeFeed)
+        }
+
+        private fun wrapFlowHandle(handle: FlowHandle<*>): FlowHandle<*> {
+            return FlowHandleImpl(handle.id, wrapFuture(handle.returnValue))
+        }
+
+        private fun <ELEMENT> wrapObservable(observable: Observable<ELEMENT>): Observable<ELEMENT> {
+            return observable.doOnError(::log).mapErrors(::obfuscate)
+        }
+
+        private fun <SNAPSHOT, ELEMENT> wrapFeed(feed: DataFeed<SNAPSHOT, ELEMENT>): DataFeed<SNAPSHOT, ELEMENT> {
+            return feed.doOnError(::log).mapErrors(::obfuscate)
+        }
+
+        private fun <RESULT> wrapFuture(future: CordaFuture<RESULT>): CordaFuture<RESULT> {
+            return future.doOnError(::log).mapError(::obfuscate)
+        }
+
+        private fun log(error: Throwable) {
+            if (doLog) {
+                logger.error("Error during RPC invocation", error)
+            }
+        }
+
+        private fun obfuscate(error: Throwable): Throwable {
+            val exposed = if (error.isWhitelisted()) error else InternalNodeException((error as? IdentifiableException)?.errorId)
+            removeDetails(exposed)
+            return exposed
+        }
+
+        private fun removeDetails(error: Throwable) {
+            error.stackTrace = arrayOf<StackTraceElement>()
+            error.declaredField<Any?>("cause").value = null
+            error.declaredField<Any?>("suppressedExceptions").value = null
+            when (error) {
+                is CordaException -> error.setCause(null)
+                is CordaRuntimeException -> error.setCause(null)
+            }
+        }
+
+        private fun Throwable.isWhitelisted(): Boolean {
+            return whitelist.any { it.isInstance(this) }
+        }
+
+        override fun toString(): String {
+            return "ErrorObfuscatingInvocationHandler(whitelist=$whitelist)"
+        }
+    }
+
+    override fun toString(): String {
+        return "ExceptionMaskingRpcOpsProxy"
+    }
+}

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt

@@ -47,6 +47,7 @@ import net.corda.testing.driver.internal.InProcessImpl
 import net.corda.testing.driver.internal.NodeHandleInternal
 import net.corda.testing.driver.internal.OutOfProcessImpl
 import net.corda.testing.internal.setGlobalSerialization
+import net.corda.testing.internal.stubs.CertificateStoreStubs
 import net.corda.testing.node.ClusterSpec
 import net.corda.testing.node.NotarySpec
 import net.corda.testing.node.User
@@ -1123,3 +1124,16 @@ private fun Config.toNodeOnly(): Config {
 
 internal fun DriverParameters.cordappsForAllNodes(): Set<TestCorDapp> = cordappsForAllNodes
         ?: cordappsInCurrentAndAdditionalPackages(extraCordappPackagesToScan)
+
+fun DriverDSL.startNode(providedName: CordaX500Name, devMode: Boolean, parameters: NodeParameters = NodeParameters()): CordaFuture<NodeHandle> {
+    var customOverrides = emptyMap<String, String>()
+    if (!devMode) {
+        val nodeDir = baseDirectory(providedName)
+        val certificatesDirectory = nodeDir / "certificates"
+        val signingCertStore = CertificateStoreStubs.Signing.withCertificatesDirectory(certificatesDirectory)
+        val p2pSslConfig = CertificateStoreStubs.P2P.withCertificatesDirectory(certificatesDirectory)
+        p2pSslConfig.configureDevKeyAndTrustStores(providedName, signingCertStore, certificatesDirectory)
+        customOverrides = mapOf("devMode" to "false")
+    }
+    return startNode(parameters, providedName = providedName, customOverrides = customOverrides)
+}