From 4b540ec8171295c3a2e4a746db77ed5afe05ad24 Mon Sep 17 00:00:00 2001 From: Roberto Tyley <52038+rtyley@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:31:52 +0100 Subject: [PATCH] Update Cookie generation and parsing --- .../com/gu/pandomainauth/action/Actions.scala | 4 +- .../com/gu/pandomainauth/PanDomain.scala | 8 +- .../model/AuthenticationStatus.scala | 4 +- .../pandomainauth/service/CookiePayload.scala | 56 ++++++++++++++ .../pandomainauth/service/CookieUtils.scala | 74 +++++++++---------- .../service/CookiePayloadTest.scala | 37 ++++++++++ .../service/CookieUtilsTest.scala | 37 +++++----- project/Dependencies.scala | 2 +- 8 files changed, 155 insertions(+), 67 deletions(-) create mode 100644 pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookiePayload.scala create mode 100644 pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookiePayloadTest.scala diff --git a/pan-domain-auth-play/src/main/scala/com/gu/pandomainauth/action/Actions.scala b/pan-domain-auth-play/src/main/scala/com/gu/pandomainauth/action/Actions.scala index 403b75c..1dfb01a 100644 --- a/pan-domain-auth-play/src/main/scala/com/gu/pandomainauth/action/Actions.scala +++ b/pan-domain-auth-play/src/main/scala/com/gu/pandomainauth/action/Actions.scala @@ -197,8 +197,8 @@ trait AuthActions { flushCookie(showUnauthedMessage("logged out")) } - def readAuthenticatedUser(request: RequestHeader): Option[AuthenticatedUser] = readCookie(request) map { cookie => - CookieUtils.parseCookieData(cookie.cookie.value, settings.signingKeyPair.publicKey) + def readAuthenticatedUser(request: RequestHeader): Option[AuthenticatedUser] = readCookie(request) flatMap { cookie => + CookieUtils.parseCookieData(cookie.cookie.value, settings.signingKeyPair.publicKey).toOption } def readCookie(request: RequestHeader): Option[PandomainCookie] = { diff --git a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/PanDomain.scala b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/PanDomain.scala index 23e758a..7dd8ba1 100644 --- a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/PanDomain.scala +++ b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/PanDomain.scala @@ -12,13 +12,9 @@ object PanDomain { */ def authStatus(cookieData: String, publicKey: PublicKey, validateUser: AuthenticatedUser => Boolean, apiGracePeriod: Long, system: String, cacheValidation: Boolean, forceExpiry: Boolean): AuthenticationStatus = { - try { - val authedUser = CookieUtils.parseCookieData(cookieData, publicKey) + CookieUtils.parseCookieData(cookieData, publicKey).fold(InvalidCookie, { authedUser => checkStatus(authedUser, validateUser, apiGracePeriod, system, cacheValidation, forceExpiry) - } catch { - case e: Exception => - InvalidCookie(e) - } + }) } private def checkStatus(authedUser: AuthenticatedUser, validateUser: AuthenticatedUser => Boolean, diff --git a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/model/AuthenticationStatus.scala b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/model/AuthenticationStatus.scala index 84daadc..13150b4 100644 --- a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/model/AuthenticationStatus.scala +++ b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/model/AuthenticationStatus.scala @@ -1,9 +1,11 @@ package com.gu.pandomainauth.model +import com.gu.pandomainauth.service.CookieUtils.CookieIntegrityFailure + sealed trait AuthenticationStatus case class Expired(authedUser: AuthenticatedUser) extends AuthenticationStatus case class GracePeriod(authedUser: AuthenticatedUser) extends AuthenticationStatus case class Authenticated(authedUser: AuthenticatedUser) extends AuthenticationStatus case class NotAuthorized(authedUser: AuthenticatedUser) extends AuthenticationStatus -case class InvalidCookie(exception: Exception) extends AuthenticationStatus +case class InvalidCookie(e: CookieIntegrityFailure) extends AuthenticationStatus case object NotAuthenticated extends AuthenticationStatus diff --git a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookiePayload.scala b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookiePayload.scala new file mode 100644 index 0000000..1539f79 --- /dev/null +++ b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookiePayload.scala @@ -0,0 +1,56 @@ +package com.gu.pandomainauth.service + +import com.gu.pandomainauth.service.CookiePayload.encodeBase64 +import org.apache.commons.codec.binary.Base64 +import org.apache.commons.codec.binary.Base64.isBase64 + +import java.nio.charset.StandardCharsets.UTF_8 +import java.security.{PrivateKey, PublicKey} +import scala.util.matching.Regex + +/** + * A representation of the underlying binary data (both payload & signature) in a Panda cookie. + * + * If an instance has been parsed from a cookie's text value, the existence of the instance + * *does not* imply that the signature has been verified. It only means that the cookie text was + * correctly formatted (two Base64 strings separated by '.'). + * + * `CookiePayload` is designed to be the optimal representation of cookie data for checking + * signature-validity against *multiple* possible accepted public keys. It's a bridge between + * these two contexts: + * + * * cookie text: the raw cookie value - two Base64-encoded strings (payload & signature), separated by '.' + * * payload text: in Panda, a string representation of `AuthenticatedUser` + * + * To make those transformations, you need either a public or private key: + * + * * payload text -> cookie text: uses a *private* key to generate the signature + * * cookie text -> payload text: uses a *public* key to verify the signature + */ +case class CookiePayload(payloadBytes: Array[Byte], sig: Array[Byte]) { + def payloadTextVerifiedSignedWith(publicKey: PublicKey): Option[String] = + if (Crypto.verifySignature(payloadBytes, sig, publicKey)) Some(new String(payloadBytes, UTF_8)) else None + + lazy val asCookieText: String = s"${encodeBase64(payloadBytes)}.${encodeBase64(sig)}" +} + +object CookiePayload { + private val CookieRegEx: Regex = "^([\\w\\W]*)\\.([\\w\\W]*)$".r + + private def encodeBase64(data: Array[Byte]): String = new String(Base64.encodeBase64(data), UTF_8) + private def decodeBase64(text: String): Array[Byte] = Base64.decodeBase64(text.getBytes(UTF_8)) + + /** + * @return `None` if the cookie text is incorrectly formatted (ie not "abc.xyz", with a '.' separator) + */ + def parse(cookieText: String): Option[CookiePayload] = cookieText match { + case CookieRegEx(data, sig) if isBase64(data) && isBase64(sig) => + Some(CookiePayload(decodeBase64(data), decodeBase64(sig))) + case _ => None + } + + def generateForPayloadText(payloadText: String, privateKey: PrivateKey): CookiePayload = { + val data = payloadText.getBytes(UTF_8) + CookiePayload(data, Crypto.signData(data, privateKey)) + } +} diff --git a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala index fe9b9f6..cb9da61 100644 --- a/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala +++ b/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala @@ -1,10 +1,19 @@ package com.gu.pandomainauth.service -import java.security.{PrivateKey, PublicKey, SignatureException} -import com.gu.pandomainauth.model.{AuthenticatedUser, CookieParseException, CookieSignatureInvalidException, User} -import org.apache.commons.codec.binary.Base64 +import com.gu.pandomainauth.model.{AuthenticatedUser, User} +import com.gu.pandomainauth.service.CookieUtils.CookieIntegrityFailure.{MalformedCookieText, MissingUserData, SignatureNotValid} + +import java.security.{PrivateKey, PublicKey} object CookieUtils { + sealed trait CookieIntegrityFailure + object CookieIntegrityFailure { + case object MalformedCookieText extends CookieIntegrityFailure + case object SignatureNotValid extends CookieIntegrityFailure + case object MissingUserData extends CookieIntegrityFailure + } + + type CookieResult[A] = Either[CookieIntegrityFailure, A] private[service] def serializeAuthenticatedUser(authUser: AuthenticatedUser): String = s"firstName=${authUser.user.firstName}" + @@ -16,48 +25,39 @@ object CookieUtils { s"&expires=${authUser.expires}" + s"&multifactor=${authUser.multiFactor}" - private[service] def deserializeAuthenticatedUser(serializedForm: String): AuthenticatedUser = { + private[service] def deserializeAuthenticatedUser(serializedForm: String): Option[AuthenticatedUser] = { val data = serializedForm .split("&") .map(_.split("=", 2)) .map{p => p(0) -> p(1)} .toMap - AuthenticatedUser( - user = User(data("firstName"), data("lastName"), data("email"), data.get("avatarUrl")), - authenticatingSystem = data("system"), - authenticatedIn = Set(data("authedIn").split(",").toSeq :_*), - expires = data("expires").toLong, - multiFactor = data("multifactor").toBoolean + for { + firstName <- data.get("firstName") + lastName <- data.get("lastName") + email <- data.get("email") + system <- data.get("system") + authedIn <- data.get("authedIn") + expires <- data.get("expires") + multifactor <- data.get("multifactor") + } yield AuthenticatedUser( + user = User(firstName, lastName, email, data.get("avatarUrl")), + authenticatingSystem = system, + authenticatedIn = Set(authedIn.split(",").toSeq :_*), + expires = expires.toLong, + multiFactor = multifactor.toBoolean ) } - def generateCookieData(authUser: AuthenticatedUser, prvKey: PrivateKey): String = { - val data = serializeAuthenticatedUser(authUser) - val encodedData = new String(Base64.encodeBase64(data.getBytes("UTF-8"))) - val signature = Crypto.signData(data.getBytes("UTF-8"), prvKey) - val encodedSignature = new String(Base64.encodeBase64(signature)) + def generateCookieData(authUser: AuthenticatedUser, prvKey: PrivateKey): String = + CookiePayload.generateForPayloadText(serializeAuthenticatedUser(authUser), prvKey).asCookieText - s"$encodedData.$encodedSignature" - } - - lazy val CookieRegEx = "^^([\\w\\W]*)\\.([\\w\\W]*)$".r - - def parseCookieData(cookieString: String, pubKey: PublicKey): AuthenticatedUser = { - - cookieString match { - case CookieRegEx(data, sig) => - try { - if (Crypto.verifySignature(Base64.decodeBase64(data.getBytes("UTF-8")), Base64.decodeBase64(sig.getBytes("UTF-8")), pubKey)) { - deserializeAuthenticatedUser(new String(Base64.decodeBase64(data.getBytes("UTF-8")))) - } else { - throw new CookieSignatureInvalidException - } - } catch { - case e: SignatureException => - throw new CookieSignatureInvalidException - } - case _ => throw new CookieParseException - } - } + // We would quite like to know, if a user is using an old (but accepted) key, *who* that user is- or to put it another + // way, give me the authenticated user, and tell me which key they're using + def parseCookieData(cookieString: String, publicKey: PublicKey): CookieResult[AuthenticatedUser] = for { + cookiePayload <- CookiePayload.parse(cookieString).toRight(MalformedCookieText) + cookiePayloadText <- cookiePayload.payloadTextVerifiedSignedWith(publicKey).toRight(SignatureNotValid) + authUser <- deserializeAuthenticatedUser(cookiePayloadText).toRight(MissingUserData) + } yield authUser } + diff --git a/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookiePayloadTest.scala b/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookiePayloadTest.scala new file mode 100644 index 0000000..377fcec --- /dev/null +++ b/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookiePayloadTest.scala @@ -0,0 +1,37 @@ +package com.gu.pandomainauth.service + +import org.scalatest.OptionValues +import org.scalatest.freespec.AnyFreeSpec +import org.scalatest.matchers.should.Matchers +import TestKeys._ + +class CookiePayloadTest extends AnyFreeSpec with Matchers with OptionValues { + + "CookiePayload" - { + "round-trip from payload text to CookiePayload if matching public-private keys are used" in { + val payloadText = "Boom" + val payload = CookiePayload.generateForPayloadText(payloadText, testPrivateKey.key) + payload.payloadTextVerifiedSignedWith(testPublicKey.key).value shouldBe payloadText + } + + "not return payload text if a rogue key was used" in { + val payload = CookiePayload.generateForPayloadText("Boom", testINCORRECTPrivateKey.key) + payload.payloadTextVerifiedSignedWith(testPublicKey.key) shouldBe None + } + + "reject cookie text that does not contain a ." in { + CookiePayload.parse("AQIDBAUG") shouldBe None + } + + "reject cookie text that does not contain valid BASE64 text" in { + CookiePayload.parse("AQ!D.BAUG") shouldBe None + CookiePayload.parse("AQID.BA!G") shouldBe None + } + + "round-trip from correctly-formatted cookie-text to CookiePayload instance and back again" in { + val correctlyFormattedCookieText = "AQID.BAUG" + val cookiePayload = CookiePayload.parse(correctlyFormattedCookieText).value + cookiePayload.asCookieText shouldBe correctlyFormattedCookieText + } + } +} \ No newline at end of file diff --git a/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookieUtilsTest.scala b/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookieUtilsTest.scala index b2c3dbd..346da2e 100644 --- a/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookieUtilsTest.scala +++ b/pan-domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookieUtilsTest.scala @@ -1,19 +1,22 @@ package com.gu.pandomainauth.service -import java.util.Date - -import com.gu.pandomainauth.model.{AuthenticatedUser, CookieParseException, CookieSignatureInvalidException, User} +import com.gu.pandomainauth.model.{AuthenticatedUser, User} +import com.gu.pandomainauth.service.CookieUtils.CookieIntegrityFailure.{MalformedCookieText, SignatureNotValid} +import com.gu.pandomainauth.service.CookieUtils.{deserializeAuthenticatedUser, parseCookieData, serializeAuthenticatedUser} import org.scalatest.freespec.AnyFreeSpec import org.scalatest.matchers.should.Matchers +import org.scalatest.{EitherValues, OptionValues} + +import java.util.Date -class CookieUtilsTest extends AnyFreeSpec with Matchers { +class CookieUtilsTest extends AnyFreeSpec with Matchers with EitherValues with OptionValues { import TestKeys._ val authUser = AuthenticatedUser(User("test", "üsér", "test.user@example.com", None), "testsuite", Set("testsuite", "another"), new Date().getTime + 86400, multiFactor = true) "generateCookieData" - { - "generates a a base64-encoded 'data.signature' cookie value" in { + "generates a base64-encoded 'data.signature' cookie value" in { CookieUtils.generateCookieData(authUser, testPrivateKey.key) should fullyMatch regex "[\\w+/]+=*\\.[\\w+/]+=*".r } } @@ -21,31 +24,25 @@ class CookieUtilsTest extends AnyFreeSpec with Matchers { "parseCookieData" - { "can extract an authenticatedUser from real cookie data" in { val cookieData = CookieUtils.generateCookieData(authUser, testPrivateKey.key) - CookieUtils.parseCookieData(cookieData, testPublicKey.key) should equal(authUser) + + parseCookieData(cookieData, testPublicKey.key).value should equal(authUser) } - "fails to extract invalid data with a CookieSignatureInvalidException" in { - val cookieData = CookieUtils.generateCookieData(authUser, testINCORRECTPrivateKey.key) - intercept[CookieSignatureInvalidException] { - CookieUtils.parseCookieData("data.invalidSignature", testPublicKey.key) - } + "fails to extract invalid data with a SignatureNotValid" in { + parseCookieData("data.invalidSignature", testPublicKey.key).left.value shouldBe SignatureNotValid } - "fails to extract incorrectly signed data with a CookieSignatureInvalidException" - { + "fails to extract incorrectly signed data with a CookieSignatureInvalidException" in { val cookieData = CookieUtils.generateCookieData(authUser, testINCORRECTPrivateKey.key) - intercept[CookieSignatureInvalidException] { - CookieUtils.parseCookieData(cookieData, testPublicKey.key) - } + parseCookieData(cookieData, testPublicKey.key).left.value should equal(SignatureNotValid) } - "fails to extract completely incorrect cookie data with a CookieParseException" - { - intercept[CookieParseException] { - CookieUtils.parseCookieData("Completely incorrect cookie data", testPublicKey.key) - } + "fails to extract completely incorrect cookie data with a CookieParseException" in { + parseCookieData("Completely incorrect cookie data", testPublicKey.key).left.value shouldBe MalformedCookieText } } "serialize/deserialize preserves data" in { - CookieUtils.deserializeAuthenticatedUser(CookieUtils.serializeAuthenticatedUser(authUser)) should equal(authUser) + deserializeAuthenticatedUser(serializeAuthenticatedUser(authUser)).value shouldEqual authUser } } diff --git a/project/Dependencies.scala b/project/Dependencies.scala index d020060..85565c5 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -43,7 +43,7 @@ object Dependencies { "commons-codec" % "commons-codec" % "1.14" ) - val testDependencies = Seq("org.scalatest" %% "scalatest" % "3.2.0" % "test") + val testDependencies = Seq("org.scalatest" %% "scalatest" % "3.2.19" % Test) val loggingDependencies = Seq("org.slf4j" % "slf4j-api" % "1.7.30")