[#131] Verify signed challenges (partially complete) (#324)

Co-authored-by: Jem Mawson <[email protected]>

src/main/scala/stellar/sdk/auth/AuthChallenger.scala

@@ -1,6 +1,7 @@
 package stellar.sdk.auth
 
 import java.security.SecureRandom
+import java.time.Clock
 
 import stellar.sdk.model.op.WriteDataOperation
 import stellar.sdk.model._
@@ -10,8 +11,8 @@ import scala.concurrent.duration._
 
 class AuthChallenger(
   serverKey: KeyPair,
-  implicit val network: Network
-) {
+  clock: Clock = Clock.systemUTC()
+)(implicit network: Network) {
 
   def challenge(
     accountId: AccountId,
@@ -27,7 +28,7 @@ class AuthChallenger(
           sourceAccount = Some(accountId.publicKey)
         )
       ),
-      timeBounds = TimeBounds.timeout(timeout),
+      timeBounds = TimeBounds.timeout(timeout, clock),
       maxFee = NativeAmount(100)
     ).sign(serverKey), network.passphrase
   )

src/main/scala/stellar/sdk/auth/Challenge.scala

@@ -1,12 +1,17 @@
 package stellar.sdk.auth
 
+import java.time.Clock
+
+import okio.ByteString
 import org.json4s.JsonDSL._
 import org.json4s.native.JsonMethods._
 import org.json4s.{DefaultFormats, Formats}
-import stellar.sdk.PublicNetwork
+import stellar.sdk.{Network, PublicNetwork, Signature}
 import stellar.sdk.model.SignedTransaction
 import stellar.sdk.util.DoNothingNetwork
 
+import scala.util.Try
+
 /**
  * An authentication challenge as specified in SEP-0010
  * @param signedTransaction a specially formed transaction that forms the basis of the challenge.
@@ -16,8 +21,31 @@ case class Challenge(
   signedTransaction: SignedTransaction,
   networkPassphrase: String
 ) {
+
+  /**
+   * Verifies that the provided signed transaction is the same as the challenge and has been signed by the
+   * challenged account.
+   *
+   * @param answer  the transaction that may have been signed by the challenged account.
+   * @param clock   the clock to used to detect timebound expiry.
+   * @param network the network that the transaction is signed for.
+   */
+  def verify(answer: SignedTransaction, clock: Clock = Clock.systemUTC())(implicit network: Network): Boolean =
+    byteStrings(answer.signatures).containsSlice(byteStrings(signedTransaction.signatures)) &&
+      transaction.timeBounds.includes(clock.instant()) &&
+      answer.verify(transaction.operations.head.sourceAccount.get)
+
+  private def byteStrings(signatures: Seq[Signature]): Seq[ByteString] =
+    signatures.map(_.data).map(new ByteString(_))
+
+  /**
+   * The inner, raw transaction.
+   */
   def transaction = signedTransaction.transaction
 
+  /**
+   * Encode this challenge as JSON.
+   */
   def toJson: String = {
     compact(render(
       ("transaction" -> signedTransaction.encodeXDR) ~
@@ -29,6 +57,10 @@ case class Challenge(
 object Challenge {
 
   implicit val formats: Formats = DefaultFormats
+
+  /**
+   * Decode a Challenge from JSON.
+   */
   def apply(json: String): Challenge = {
     val o = parse(json)
     implicit val network = (o \ "network_passphrase").extractOpt[String]

src/main/scala/stellar/sdk/model/TimeBounds.scala

@@ -1,6 +1,6 @@
 package stellar.sdk.model
 
-import java.time.Instant
+import java.time.{Clock, Instant}
 import java.time.temporal.{ChronoField, TemporalAdjuster, TemporalField}
 
 import cats.data.State
@@ -9,8 +9,14 @@ import stellar.sdk.model.xdr.{Decode, Encodable}
 import scala.concurrent.duration.Duration
 
 case class TimeBounds(start: Instant, end: Instant) extends Encodable {
-  require(start.isBefore(end) || (start == end && start.getEpochSecond == 0),
-    s"Range start is not before the end [start=$start][end=$end]")
+  private val isUnbounded: Boolean = start == end && start.getEpochSecond == 0
+  require(start.isBefore(end) || isUnbounded, s"Range start is not before the end [start=$start][end=$end]")
+
+  /**
+   * Whether the given instant is within these bounds, inclusive.
+   */
+  def includes(instant: Instant): Boolean =
+    isUnbounded || !(start.isAfter(instant) || end.isBefore(instant))
 
   def encode: LazyList[Byte] = {
     import stellar.sdk.model.xdr.Encode._
@@ -25,11 +31,10 @@ object TimeBounds extends Decode {
     end <- instant
   } yield TimeBounds(start, end)
 
-  val Unbounded = TimeBounds(Instant.ofEpochSecond(0), Instant.ofEpochSecond(0))
+  val Unbounded: TimeBounds = TimeBounds(Instant.ofEpochSecond(0), Instant.ofEpochSecond(0))
 
-  def timeout(duration: Duration) = {
-    val now = Instant.now().`with`(ChronoField.NANO_OF_SECOND, 0)
+  def timeout(duration: Duration, clock: Clock = Clock.systemUTC()): TimeBounds = {
+    val now = clock.instant().`with`(ChronoField.NANO_OF_SECOND, 0)
     TimeBounds(now.minusSeconds(5), now.plusMillis(duration.toMillis))
   }
-
 }

src/test/scala/stellar/sdk/auth/ChallengeSpec.scala

@@ -1,9 +1,11 @@
 package stellar.sdk.auth
 
 import java.time.Instant
+import java.util.concurrent.TimeUnit
 
 import org.specs2.mutable.Specification
 import stellar.sdk.model.op.{Operation, WriteDataOperation}
+import stellar.sdk.util.FakeClock
 import stellar.sdk.{ArbitraryInput, DomainMatchers, KeyPair, PublicNetwork}
 
 import scala.concurrent.duration._
@@ -14,40 +16,41 @@ import scala.concurrent.duration._
  */
 class ChallengeSpec extends Specification with DomainMatchers with ArbitraryInput {
 
-  val serverKey = KeyPair.random
-  val subject = new AuthChallenger(serverKey, PublicNetwork)
+  private val serverKey = KeyPair.random
+  private def fixtures: (AuthChallenger, FakeClock, KeyPair) = {
+    val fakeClock = FakeClock()
+    (new AuthChallenger(serverKey, fakeClock)(PublicNetwork), fakeClock, KeyPair.random)
+  }
 
   "a generated authentication challenge" should {
     "have the source account set the the server's signing account" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.transaction.source.id mustEqual serverKey.toAccountId
     }
 
     "have a sequence number of zero" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.transaction.source.sequenceNumber mustEqual 0
     }
 
     "default to a timeout of the recommended 15 minutes" >> {
-      val clientKey = KeyPair.random
+      val (subject, clock, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.transaction.timeBounds.end must beLike { when: Instant =>
-        when.toEpochMilli must beCloseTo(Instant.now().plusSeconds(900).toEpochMilli, 5_000)
+        when.toEpochMilli mustEqual clock.instant().plusSeconds(900).toEpochMilli
       }
     }
 
     "include the actual timeout if one is specified" >> {
-      val clientKey = KeyPair.random
+      val (subject, clock, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com", timeout = 3.minutes)
-      challenge.transaction.timeBounds.end must beLike { when: Instant =>
-        when.toEpochMilli must beCloseTo(Instant.now().plusSeconds(180).toEpochMilli, 2_000)
-      }
+      challenge.transaction.timeBounds.end mustEqual clock.instant().plusSeconds(180)
     }
 
     "have a single manage data operation from the challenged account" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.transaction.operations.size mustEqual 1
       challenge.transaction.operations.head must beLike[Operation] { case op: WriteDataOperation =>
@@ -56,41 +59,73 @@ class ChallengeSpec extends Specification with DomainMatchers with ArbitraryInpu
     }
 
     "be signed by the provided server key" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.signedTransaction.signatures.size mustEqual 1
       challenge.signedTransaction.verify(serverKey) must beTrue
     }
 
     "json serialise and deserialise" >> prop { clientKey: KeyPair =>
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       Challenge(challenge.toJson) must beEquivalentTo(challenge)
     }
   }
 
   "the operation in the generated authentication challenge" should {
     "have the key '<home domain> auth'" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.transaction.operations.head must beLike[Operation] { case op: WriteDataOperation =>
         op.name mustEqual "test.com auth"
       }
     }
 
     "disallow home domains greater than 59 characters" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       subject.challenge(
         clientKey.toAccountId,
         homeDomain = "." * 60
       ) should throwAn[IllegalArgumentException]
     }
 
     "have a cryptographic random 48 byte value" >> {
-      val clientKey = KeyPair.random
+      val (subject, _, clientKey) = fixtures
       val challenge = subject.challenge(clientKey.toAccountId, "test.com")
       challenge.transaction.operations.head must beLike[Operation] { case op: WriteDataOperation =>
         op.value must haveSize(48)
       }
     }
   }
+
+  "verifying a challenge response" should {
+    "succeed using signed transaction when the challenge is correctly signed" >> {
+      val (subject, clock, clientKey) = fixtures
+      val challenge = subject.challenge(clientKey.toAccountId, "test.com")
+      val answer = challenge.signedTransaction.sign(clientKey)
+      challenge.verify(answer, clock) must beTrue
+    }
+
+    "fail if the source account does not match that of the challenge" >> {
+      val (subject, clock, clientKey) = fixtures
+      val challenge = subject.challenge(clientKey.toAccountId, "test.com")
+      val answer = challenge.copy(signedTransaction = challenge.signedTransaction.sign(KeyPair.random))
+      challenge.verify(answer.signedTransaction, clock) must beFalse
+    }
+
+    "fail if the signed transaction does not contain the signatures of the challenge" >> {
+      val (subject, clock, clientKey) = fixtures
+      val challenge = subject.challenge(clientKey.toAccountId, "test.com")
+      val answer = challenge.signedTransaction.copy(signatures = Nil).sign(clientKey)
+      challenge.verify(answer, clock) must beFalse
+    }
+
+    "fail if the timebounds are expired" >> {
+      val (subject, clock, clientKey) = fixtures
+      val challenge = subject.challenge(clientKey.toAccountId, "test.com")
+      val answer = challenge.signedTransaction.sign(clientKey)
+      clock.advance(java.time.Duration.ofMinutes(16))
+      challenge.verify(answer, clock) must beFalse
+    }
+  }
 }

src/test/scala/stellar/sdk/model/TimeBoundsSpec.scala

@@ -38,5 +38,32 @@ class TimeBoundsSpec extends Specification with ArbitraryInput with DomainMatche
     "serde to/from xdr" >> prop { tb: TimeBounds =>
       tb must serdeUsing(TimeBounds.decode)
     }
+
+    "exclude any instant before the start" >> prop { tb: TimeBounds =>
+      tb.includes(tb.start.minusNanos(1)) must beFalse
+    }
+
+    "exclude any instant after the end" >> prop { tb: TimeBounds =>
+      tb.includes(tb.end.plusNanos(1)) must beFalse
+    }
+
+    "include the instant at the start" >> prop { tb: TimeBounds =>
+      tb.includes(tb.start) must beTrue
+    }
+
+    "include the instant at the end" >> prop { tb: TimeBounds =>
+      tb.includes(tb.end) must beTrue
+    }
+
+    "include the instants within the bounds" >> prop { tb: TimeBounds =>
+      val midpoint = Instant.ofEpochMilli((tb.end.toEpochMilli - tb.start.toEpochMilli) / 2 + tb.start.toEpochMilli)
+      tb.includes(midpoint) must beTrue
+    }
+  }
+
+  "unbounded time bounds" should {
+    "always include any instant" >> prop { instant: Instant =>
+      TimeBounds.Unbounded.includes(instant) must beTrue
+    }
   }
 }

src/test/scala/stellar/sdk/util/FakeClock.scala

@@ -0,0 +1,18 @@
+package stellar.sdk.util
+
+import java.time.{Clock, Duration, Instant, ZoneId, ZonedDateTime}
+
+case class FakeClock(
+  zoneId: ZoneId = ZoneId.of("UTC")
+) extends Clock {
+
+  private var fixedInstant: Instant = ZonedDateTime.of(2020, 8, 15, 0, 0, 0, 0, zoneId).toInstant
+
+  override def getZone: ZoneId = zoneId
+
+  override def withZone(zoneId: ZoneId): Clock = this.copy(zoneId = zoneId)
+
+  override def instant(): Instant = fixedInstant
+
+  def advance(duration: Duration): Unit = fixedInstant = fixedInstant.plus(duration)
+}