Ensure that the contents of OpaqueBytes cannot be modified. (#1871)

* Ensure that contents of OpaqueBytes cannot be modified.
* Update documentation and restore the signature verification check.
* Update the API definition.
* KDoc fixes.
* Update the changelog for v1.1.

.ci/api-current.txt

@@ -2923,7 +2923,7 @@ public static final class net.corda.core.utilities.NonEmptySet$iterator$1 extend
 ##
 public class net.corda.core.utilities.OpaqueBytes extends net.corda.core.utilities.ByteSequence
   public <init>(byte[])
-  @org.jetbrains.annotations.NotNull public byte[] getBytes()
+  @org.jetbrains.annotations.NotNull public final byte[] getBytes()
   public int getOffset()
   public int getSize()
   public static final net.corda.core.utilities.OpaqueBytes$Companion Companion

core/src/main/kotlin/net/corda/core/crypto/SecureHash.kt

@@ -19,39 +19,87 @@ sealed class SecureHash(bytes: ByteArray) : OpaqueBytes(bytes) {
         }
     }
 
+    /**
+     * Convert the hash value to an uppercase hexadecimal [String].
+     */
     override fun toString(): String = bytes.toHexString()
 
+    /**
+     * Returns the first [prefixLen] hexadecimal digits of the [SecureHash] value.
+     * @param prefixLen The number of characters in the prefix.
+     */
     fun prefixChars(prefixLen: Int = 6) = toString().substring(0, prefixLen)
+
+    /**
+     * Append a second hash value to this hash value, and then compute the SHA-256 hash of the result.
+     * @param other The hash to append to this one.
+     */
     fun hashConcat(other: SecureHash) = (this.bytes + other.bytes).sha256()
 
     // Like static methods in Java, except the 'companion' is a singleton that can have state.
     companion object {
+        /**
+         * Converts a SHA-256 hash value represented as a hexadecimal [String] into a [SecureHash].
+         * @param str A sequence of 64 hexadecimal digits that represents a SHA-256 hash value.
+         * @throws IllegalArgumentException The input string does not contain 64 hexadecimal digits, or it contains incorrectly-encoded characters.
+         */
         @JvmStatic
-        fun parse(str: String) = str.toUpperCase().parseAsHex().let {
-            when (it.size) {
-                32 -> SHA256(it)
-                else -> throw IllegalArgumentException("Provided string is ${it.size} bytes not 32 bytes in hex: $str")
+        fun parse(str: String): SHA256 {
+            return str.toUpperCase().parseAsHex().let {
+                when (it.size) {
+                    32 -> SHA256(it)
+                    else -> throw IllegalArgumentException("Provided string is ${it.size} bytes not 32 bytes in hex: $str")
+                }
             }
         }
 
+        /**
+         * Computes the SHA-256 hash value of the [ByteArray].
+         * @param bytes The [ByteArray] to hash.
+         */
         @JvmStatic
         fun sha256(bytes: ByteArray) = SHA256(MessageDigest.getInstance("SHA-256").digest(bytes))
 
+        /**
+         * Computes the SHA-256 hash of the [ByteArray], and then computes the SHA-256 hash of the hash.
+         * @param bytes The [ByteArray] to hash.
+         */
         @JvmStatic
         fun sha256Twice(bytes: ByteArray) = sha256(sha256(bytes).bytes)
 
+        /**
+         * Computes the SHA-256 hash of the [String]'s UTF-8 byte contents.
+         * @param str [String] whose UTF-8 contents will be hashed.
+         */
         @JvmStatic
         fun sha256(str: String) = sha256(str.toByteArray())
 
+        /**
+         * Generates a random SHA-256 value.
+         */
         @JvmStatic
         fun randomSHA256() = sha256(newSecureRandom().generateSeed(32))
 
+        /**
+         * A SHA-256 hash value consisting of 32 0x00 bytes.
+         */
         val zeroHash = SecureHash.SHA256(ByteArray(32, { 0.toByte() }))
+
+        /**
+         * A SHA-256 hash value consisting of 32 0xFF bytes.
+         */
         val allOnesHash = SecureHash.SHA256(ByteArray(32, { 255.toByte() }))
     }
 
     // In future, maybe SHA3, truncated hashes etc.
 }
 
+/**
+ * Compute the SHA-256 hash for the contents of the [ByteArray].
+ */
 fun ByteArray.sha256(): SecureHash.SHA256 = SecureHash.sha256(this)
+
+/**
+ * Compute the SHA-256 hash for the contents of the [OpaqueBytes].
+ */
 fun OpaqueBytes.sha256(): SecureHash.SHA256 = SecureHash.sha256(this.bytes)

core/src/main/kotlin/net/corda/core/transactions/TransactionWithSignatures.kt

@@ -1,8 +1,6 @@
 package net.corda.core.transactions
 
-import net.corda.core.contracts.ContractState
 import net.corda.core.contracts.NamedByHash
-import net.corda.core.contracts.TransactionState
 import net.corda.core.crypto.TransactionSignature
 import net.corda.core.crypto.isFulfilledBy
 import net.corda.core.transactions.SignedTransaction.SignaturesMissingException

core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt

@@ -118,8 +118,11 @@ sealed class ByteSequence : Comparable<ByteSequence> {
  * In an ideal JVM this would be a value type and be completely overhead free. Project Valhalla is adding such
  * functionality to Java, but it won't arrive for a few years yet!
  */
-open class OpaqueBytes(override val bytes: ByteArray) : ByteSequence() {
+open class OpaqueBytes(bytes: ByteArray) : ByteSequence() {
     companion object {
+        /**
+         * Create [OpaqueBytes] from a sequence of [Byte] values.
+         */
         @JvmStatic
         fun of(vararg b: Byte) = OpaqueBytes(byteArrayOf(*b))
     }
@@ -128,13 +131,35 @@ open class OpaqueBytes(override val bytes: ByteArray) : ByteSequence() {
         require(bytes.isNotEmpty())
     }
 
-    override val size: Int get() = bytes.size
-    override val offset: Int get() = 0
+    /**
+     * The bytes are always cloned so that this object becomes immutable. This has been done
+     * to prevent tampering with entities such as [SecureHash] and [PrivacySalt], as well as
+     * preserve the integrity of our hash constants [zeroHash] and [allOnesHash].
+     *
+     * Cloning like this may become a performance issue, depending on whether or not the JIT
+     * compiler is ever able to optimise away the clone. In which case we may need to revisit
+     * this later.
+     */
+    override final val bytes: ByteArray = bytes
+        get() = field.clone()
+    override val size: Int = bytes.size
+    override val offset: Int = 0
 }
 
+/**
+ * Copy [size] bytes from this [ByteArray] starting from [offset] into a new [ByteArray].
+ */
 fun ByteArray.sequence(offset: Int = 0, size: Int = this.size) = ByteSequence.of(this, offset, size)
 
+/**
+ * Converts this [ByteArray] into a [String] of hexadecimal digits.
+ */
 fun ByteArray.toHexString(): String = DatatypeConverter.printHexBinary(this)
+
+/**
+ * Converts this [String] of hexadecimal digits into a [ByteArray].
+ * @throws IllegalArgumentException if the [String] contains incorrectly-encoded characters.
+ */
 fun String.parseAsHex(): ByteArray = DatatypeConverter.parseHexBinary(this)
 
 /**

core/src/test/kotlin/net/corda/core/serialization/TransactionSerializationTests.kt

@@ -12,11 +12,12 @@ import org.junit.Before
 import org.junit.Test
 import java.security.SignatureException
 import java.util.*
+import kotlin.reflect.jvm.javaField
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 
 class TransactionSerializationTests : TestDependencyInjectionBase() {
-    val TEST_CASH_PROGRAM_ID = "net.corda.core.serialization.TransactionSerializationTests\$TestCash"
+    private val TEST_CASH_PROGRAM_ID = "net.corda.core.serialization.TransactionSerializationTests\$TestCash"
 
     class TestCash : Contract {
         override fun verify(tx: LedgerTransaction) {
@@ -65,7 +66,10 @@ class TransactionSerializationTests : TestDependencyInjectionBase() {
         stx.verifyRequiredSignatures()
 
         // Corrupt the data and ensure the signature catches the problem.
-        stx.id.bytes[5] = stx.id.bytes[5].inc()
+        val bytesField = stx.id::bytes.javaField?.apply { setAccessible(true) }
+        val bytes = bytesField?.get(stx.id) as ByteArray
+        bytes[5] = bytes[5].inc()
+
         assertFailsWith(SignatureException::class) {
             stx.verifyRequiredSignatures()
         }

docs/source/changelog.rst

@@ -6,6 +6,9 @@ from the previous milestone release.
 
 UNRELEASED
 ----------
+* ``OpaqueBytes.bytes`` now returns a clone of its underlying ``ByteArray``, and has been redeclared as ``final``.
+  This is a minor change to the public API, but is required to ensure that classes like ``SecureHash`` are immutable.
+
 * ``FlowLogic`` now exposes a series of function called ``receiveAll(...)`` allowing to join ``receive(...)`` instructions.
 
 * The ``Cordformation`` gradle plugin has been split into ``cordformation`` and ``cordapp``. The former builds and