Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Panda v7 - support key rotation #361

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

rtyley
Copy link
Member

@rtyley rtyley commented Aug 8, 2024

This upgrades Panda from v5 to v7, allowing us to use key rotation.

As Atom Workshop is pretty standard user of Panda, the upgrade is pretty simple:

This has been successfully deployed to https://atomworkshop.code.dev-gutools.co.uk/ at 55de061, which is using guardian/pan-domain-authentication@4c87946.

Testing

Using https://atomworkshop.code.dev-gutools.co.uk/ (running Panda with the version from guardian/pan-domain-authentication#151) in the same browser as https://composer.code.dev-gutools.co.uk/ (running standard Panda v4), I've verified that they both accept the same cookie, and don't need to reauth over the top of each-others cookie:

Screen.Recording.2024-08-09.at.17.02.11.mov

@rtyley rtyley force-pushed the try-out-updated-panda-settings-code branch 2 times, most recently from 2e68740 to c27cb00 Compare August 9, 2024 15:35
@rtyley rtyley force-pushed the try-out-updated-panda-settings-code branch 2 times, most recently from d53a040 to df972c9 Compare September 4, 2024 14:47
@rtyley rtyley force-pushed the try-out-updated-panda-settings-code branch from df972c9 to 08de9bd Compare September 12, 2024 09:48
@rtyley
Copy link
Member Author

rtyley commented Sep 12, 2024

Trying out guardian/pan-domain-authentication#150 at guardian/pan-domain-authentication@a6ae5e8 on CODE, everything looks good, except that we seem to be getting way more "Panda settings changed" messages in the logs than we were expecting - there should be just 1 per EC2 instance, but we actually see, for just 1 instance, 56, then 64, then 80, for the 3 successive phases of rotation.

image

rtyley added a commit to guardian/pan-domain-authentication that referenced this pull request Sep 12, 2024
Having `def panDomainSettings: PanDomainAuthSettingsRefresher` in the `AuthActions`
trait as a `def` means that implementors of `AuthActions` are able to make the
mistake of also defining their implementation of that field as a `def`, meaning that
every reference to it can potentially create _another_ instance of the refresher,
as seen here:

guardian/atom-workshop#361 (comment)

Surprisingly, this seems to have been a problem since at least as far back as
#41 in February 2018.

Changing the field type to `val` forces implementers to also use `val` for that
field, effectively making it a singleton, as we want.

Changing the abstract field of a trait to be `val` does open up another danger due
the initialization order of vals - the field could end up being evaluated as
`null` if the trait immediately evaluates the field:

See: https://docs.scala-lang.org/tutorials/FAQ/initialization-order.html

...consequently, I've made all `val` declarations in the `AuthActions` trait (that
evaluate `panDomainSettings` in some way) into `lazy val`s. This hopefully should
fix the problem.
@rtyley
Copy link
Member Author

rtyley commented Sep 12, 2024

Looks like guardian/pan-domain-authentication@6e11a68 has fixed the problem, looking at the logs:

image

rtyley added a commit to guardian/pan-domain-authentication that referenced this pull request Sep 12, 2024
Having `def panDomainSettings: PanDomainAuthSettingsRefresher` in the `AuthActions`
trait as a `def` means that implementors of `AuthActions` are able to make the
mistake of also defining their implementation of that field as a `def`, meaning that
every reference to it can potentially create _another_ instance of the refresher,
as seen here:

guardian/atom-workshop#361 (comment)

Surprisingly, this seems to have been a problem since at least as far back as
#41 in February 2018.

Changing the field type to `val` forces implementers to also use `val` for that
field, effectively making it a singleton, as we want.

Changing the abstract field of a trait to be `val` does open up another danger due
the initialization order of vals - the field could end up being evaluated as
`null` if the trait immediately evaluates the field:

See: https://docs.scala-lang.org/tutorials/FAQ/initialization-order.html

...consequently, I've made all `val` declarations in the `AuthActions` trait (that
evaluate `panDomainSettings` in some way) into `lazy val`s. This hopefully should
fix the problem.
@rtyley rtyley force-pushed the try-out-updated-panda-settings-code branch 2 times, most recently from 035d40d to 74a86ea Compare September 18, 2024 10:03
@rtyley rtyley changed the title Try out updated Panda settings code Upgrade to Panda v7 - support key rotation Sep 18, 2024
@rtyley rtyley force-pushed the try-out-updated-panda-settings-code branch from 74a86ea to e751708 Compare September 18, 2024 10:12
rtyley added a commit to guardian/login.gutools that referenced this pull request Sep 18, 2024
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.
@rtyley rtyley marked this pull request as ready for review September 19, 2024 10:03
@rtyley rtyley requested a review from a team as a code owner September 19, 2024 10:03
This upgrades Panda from v5 to v7, allowing us to use key rotation as
introduced with guardian/pan-domain-authentication#150.

As Atom Workshop is pretty standard user of Panda, the upgrade is pretty simple:

* Panda v6:
  * guardian/pan-domain-authentication#155 requires
    `panDomainSettings` is a `val`, not a `def`
@rtyley rtyley force-pushed the try-out-updated-panda-settings-code branch from e751708 to a7e95f1 Compare September 19, 2024 14:25
override def panDomainSettings: PanDomainAuthSettingsRefresher = new PanDomainAuthSettingsRefresher(
override val panDomainSettings: PanDomainAuthSettingsRefresher = PanDomainAuthSettingsRefresher(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bucketName = "pan-domain-auth-settings",
settingsFileKey = s"${config.pandaDomain}.settings",
s3Client = AWS.S3Client,
S3BucketLoader.forAwsSdkV1(AWS.S3Client, "pan-domain-auth-settings")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR:

...introduced the new S3BucketLoader abstraction, which simplifies constructing a PanDomainAuthSettingsRefresher and means that Panda is no longer tied to AWS SDK v1 - an alternative AWS SDK v2 implementation of S3BucketLoader could be introduced.

rtyley added a commit to guardian/login.gutools that referenced this pull request Sep 19, 2024
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.
@rtyley rtyley merged commit 7bbaf8f into main Sep 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants