Refactor Cookie generation-&-parsing #152

rtyley · 2024-08-07T16:42:53Z

This is a precursor to PR #150 ("Support accepting multiple public keys"). It helps with that PR because we're going to be changing the cookie processing code to validate against a range of possible acceptable keys, and if a failure occurs in the reading of a cookie, we want to be clear about whether that happens to be due to the particular key we're attempting to validate against, or whether the cookie text itself is malformatted.

New class CookiePayload: an optimal representation of cookie data for checking signature-validity against multiple public keys. The CookiePayload class represents the binary data caught after parsing-and-base64-decoding the cookie text value, ready for signature verification.
New type alias CookieResult: rather than throw exceptions, communicate CookieIntegrityFailures with return values of CookieResult.

bryophyta · 2024-08-09T14:26:36Z

pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookiePayload.scala

+ * 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


this abstraction seems very useful to me, though I wonder if there's a way to make it more obvious from the name of the case class that having a CookiePayload doesn't imply that its signature's been verified?

Maybe UnverifiedCookiePayload? I think bringing in Unverified is a bit tricky though, because CookiePayload transforms both ways and so changing its name also affects what the code looks like when we generate a cookie:

pan-domain-authentication/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala

Line 53 in 41c7baf

CookiePayload.generateForPayloadText(serializeAuthenticatedUser(authUser), prvKey).asCookieText

...given that at that point we're literally signing it with a private key, it wouldn't feel quite right to me to be referring to it as an Unverified CookiePayload...

ah yes I see your point.. probably it would require having two different data types in that case, which seems like it'd be OTT at this point for a fairly theoretical issue 👍

bryophyta · 2024-08-09T14:31:01Z

pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala

+      user = User(firstName, lastName, email, data.get("avatarUrl")),
+      authenticatingSystem = system,
+      authenticatedIn = Set(authedIn.split(",").toSeq :_*),
+      expires = expires.toLong,


The return type of deserializeAuthenticatedUser has been changed to Option[AuthenticatedUser] in this branch. Is the intention that all foreseeable errors should therefore lead to a None, rather than throwing? If so, do we need to change how we're handling the .toLong call, given that it can throw?

Good point! Addressed in f40aa8d.

Whoops - ah, but it fails because we have to compile under Scala 2.12:

[error] /home/runner/work/pan-domain-authentication/pan-domain-authentication/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala:41:48: value toLongOption is not a member of String [error] expires <- data.get("expires").flatMap(_.toLongOption) [error] ^ [error] /home/runner/work/pan-domain-authentication/pan-domain-authentication/pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala:42:56: value toBooleanOption is not a member of String [error] multiFactor <- data.get("multifactor").flatMap(_.toBooleanOption) [error] ^ [error] two errors found

...I'll make something Scala 2.12 compliant...

...I'll make something Scala 2.12 compliant...

Done with 998db1f!

bryophyta · 2024-08-09T14:38:03Z

pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala

-      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


I don't think I follow how this code does what the comment says..

From what I can see it'll either return an AuthenticatedUser, or a SignatureNotValid (or one of the other failure types), but none of these include the key that the user was using.. Am I missing something?

Yeah, it was a note to myself I should have removed - done in 41c7baf!

bryophyta · 2024-08-09T14:41:53Z

...domain-auth-verification/src/test/scala/com/gu/pandomainauth/service/CookiePayloadTest.scala

+import org.scalatest.matchers.should.Matchers
+import TestKeys._
+
+class CookiePayloadTest extends AnyFreeSpec with Matchers with OptionValues {


it looks to me like these cases provide good coverage 👍

@bryophyta

@bryophyta pointed out that this comment was describing something that the code _didn't do_ - it was a note to myself I should have deleted! https://github.com/guardian/pan-domain-authentication/pull/152/files#r1711588388

rtyley · 2024-08-07T21:35:43Z

project/Dependencies.scala

-  val testDependencies = Seq("org.scalatest" %% "scalatest" % "3.2.0" % "test")
+  val testDependencies = Seq("org.scalatest" %% "scalatest" % "3.2.19" % Test)


We need ScalaTest to be updated to at least v3.2.3, because we want to use EitherValues right-biased .value in tests, which was only implemented with scalatest/scalatest#1895. ScalaTest v3.2.19 is currently the latest release.

scalatest/scalatest#1775 (comment)

rtyley · 2024-08-07T21:47:48Z

pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala

-  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)) {


This parsing of cookie-text (to extract the binary data of the payload & signature) has now moved to CookiePayload.parse().

rtyley · 2024-08-09T15:00:11Z

pan-domain-auth-verification/src/main/scala/com/gu/pandomainauth/service/CookieUtils.scala

-      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


Yeah, it was a note to myself I should have removed - done in 41c7baf!

@bryophyta

@bryophyta pointed out that although the `deserializeAuthenticatedUser()` method was now returning `Option[AuthenticatedUser]` (with a `None` return-value indicating conditions that would formerly have thrown an exception), there were still some cases where an exception would be thrown (parsing numbers and booleans): #152 (comment)

Revert this when #149 is fixed!

bryophyta

this looks good to me 👍

This upgrades Panda from v5 to v7, allowing us to use key rotation as introduced with guardian/pan-domain-authentication#150. As login.gutools.co.uk is pretty special user of Panda the upgrade is slightly more involved than other upgrades (eg guardian/atom-workshop#361): * Panda v6: * guardian/pan-domain-authentication#152 `CookieUtils.generateCookieData()` now communicates errors with `CookieResult` values containing `CookieIntegrityFailure`, rather than exceptions. * Panda v7: * guardian/pan-domain-authentication#150 means that code shouldn't directly reference private or public keys anymore (eg do not reference `settings.signingKeyPair`). Instead, use `settings.signingAndVerification` or `publicSettings.verification`. Note also that `publicSettings.publicKey` was previously optional, and `publicSettings.verification` is not.

This upgrades Panda from v3 to v7, allowing us to use key rotation as introduced with guardian/pan-domain-authentication#150. ### Necessary code changes * Panda v5 * guardian/pan-domain-authentication#147 removed the old `PublicKey` & `PrivateKey` classes in our `com.gu.pandomainauth` package, in favour of using the existing `java.security` classes. To create instances of those classes, we can use the `SettingsReader.{privateKeyFor, publicKeyFor}` methods. * Panda v6: * guardian/pan-domain-authentication#152 means the `CookieUtils.generateCookieData()` method now communicates errors with `CookieResult` values containing `CookieIntegrityFailure`, rather than exceptions. * Panda v7: * guardian/pan-domain-authentication#150 means that code shouldn't directly reference private or public keys anymore (eg do not reference `settings.signingKeyPair`). Instead, use `settings.signingAndVerification` or `publicSettings.verification`. Note also that `publicSettings.publicKey` was previously optional, and `publicSettings.verification` is not.

rtyley changed the title ~~Update Cookie generation and parsing code~~ Update Cookie generation & parsing code Aug 7, 2024

rtyley changed the base branch from main to update-settings-loading-and-parsing-code August 7, 2024 16:43

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from c11c67f to 8592c75 Compare August 7, 2024 16:47

rtyley force-pushed the update-settings-loading-and-parsing-code branch from 65ca26c to c081b7e Compare August 7, 2024 16:56

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from 8592c75 to a0f4f81 Compare August 7, 2024 16:58

rtyley mentioned this pull request Aug 7, 2024

Support accepting multiple public keys #150

Merged

rtyley changed the title ~~Update Cookie generation & parsing code~~ Update Cookie generation-&-parsing code Aug 7, 2024

rtyley force-pushed the update-cookie-generation-and-parsing-code branch 3 times, most recently from 350d62d to 4b540ec Compare August 7, 2024 22:16

rtyley changed the title ~~Update Cookie generation-&-parsing code~~ Refactor Cookie generation-&-parsing Aug 8, 2024

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from 4b540ec to 6c4c583 Compare August 8, 2024 08:39

rtyley force-pushed the update-settings-loading-and-parsing-code branch 3 times, most recently from 0630bdd to e1fcd7a Compare August 8, 2024 09:33

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from 6c4c583 to 9f20fc6 Compare August 8, 2024 09:41

rtyley force-pushed the update-settings-loading-and-parsing-code branch from e1fcd7a to d565dd6 Compare August 8, 2024 10:02

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from 9f20fc6 to ab80840 Compare August 8, 2024 10:03

rtyley force-pushed the update-settings-loading-and-parsing-code branch from d565dd6 to 3cd167f Compare August 8, 2024 10:58

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from ab80840 to ec6ca77 Compare August 8, 2024 10:58

rtyley force-pushed the update-settings-loading-and-parsing-code branch from 3cd167f to af01b6b Compare August 8, 2024 11:00

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from ec6ca77 to efcee19 Compare August 8, 2024 11:09

rtyley force-pushed the update-settings-loading-and-parsing-code branch from af01b6b to c483501 Compare August 8, 2024 13:51

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from efcee19 to c06d3fd Compare August 9, 2024 14:03

bryophyta reviewed Aug 9, 2024

View reviewed changes

Update Cookie generation and parsing

a8e3840

rtyley force-pushed the update-cookie-generation-and-parsing-code branch from c06d3fd to a8e3840 Compare August 9, 2024 14:47

Remove misleading comment

41c7baf

@bryophyta pointed out that this comment was describing something that the code _didn't do_ - it was a note to myself I should have deleted! https://github.com/guardian/pan-domain-authentication/pull/152/files#r1711588388

rtyley commented Aug 9, 2024

View reviewed changes

rtyley marked this pull request as ready for review August 9, 2024 15:20

rtyley requested a review from a team as a code owner August 9, 2024 15:20

Scala 2.12 support: Don't use .toLongOption or .toBooleanOption

998db1f

Revert this when #149 is fixed!

bryophyta approved these changes Aug 9, 2024

View reviewed changes

Base automatically changed from update-settings-loading-and-parsing-code to main September 4, 2024 09:22

rtyley merged commit 65e1f65 into main Sep 4, 2024
1 check passed

rtyley deleted the update-cookie-generation-and-parsing-code branch September 4, 2024 09:23

rtyley mentioned this pull request Sep 18, 2024

Upgrade to Panda v7 - support key rotation guardian/login.gutools#81

Merged

rtyley mentioned this pull request Sep 24, 2024

Upgrade Guardian services to Panda v7 - support key rotation #160

Open

7 tasks

rtyley mentioned this pull request Nov 8, 2024

Upgrade to Panda v7 - support key rotation guardian/giant#247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Cookie generation-&-parsing #152

Refactor Cookie generation-&-parsing #152

rtyley commented Aug 7, 2024 •

edited

Loading

bryophyta Aug 9, 2024

rtyley Aug 9, 2024

bryophyta Aug 9, 2024

bryophyta Aug 9, 2024

rtyley Aug 9, 2024

rtyley Aug 9, 2024

rtyley Aug 9, 2024

bryophyta Aug 9, 2024

rtyley Aug 9, 2024

bryophyta Aug 9, 2024 •

edited

Loading

rtyley Aug 7, 2024

rtyley Aug 7, 2024

rtyley Aug 9, 2024

bryophyta left a comment

		val testDependencies = Seq("org.scalatest" %% "scalatest" % "3.2.0" % "test")
		val testDependencies = Seq("org.scalatest" %% "scalatest" % "3.2.19" % Test)

Refactor Cookie generation-&-parsing #152

Refactor Cookie generation-&-parsing #152

Conversation

rtyley commented Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryophyta Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryophyta left a comment

Choose a reason for hiding this comment

rtyley commented Aug 7, 2024 •

edited

Loading

bryophyta Aug 9, 2024 •

edited

Loading