Download public key from S3 with AWS credential by waisingyiu · Pull Request #47 · guardian/pan-domain-node

waisingyiu · 2025-07-03T17:08:38Z

Context

We are looking to address the FSBP high severity security issue about the S3 bucket "pan-domain-auth-settings" not blocking public access.

From S3 access log, we spotted that there were successful, unauthenticated requests to read some objects in this bucket from our applications.

We found that the public settings files there (which were S3 objects with a suffix "settings.public" were configured to allow public access and this client library, pan-domain-node, read these objects via direct HTTP requests without any AWS credential.

Therefore, we have to change the way we access these public setting file before we can turn on "Block all public access" option on the bucket.

What does this change?

This pull request changes the library to read the public key file via AWS SDK S3 client instead of making direct HTTP requests. To achieve this, it makes the following changes:

Add an optional argument, 'localDev', to the main class 'PanDomainAuthentication` to indicate whether the applicant is running locally or in AWS.
Load the AWS credential based on the localDev argument.
Create S3 client object and use it to download the public key from S3 bucket.
Pull in the AWS SDK bundle for S3 and credential providers.
Update some dependencies so that they can work with the AWS SDK.

How to test

I modified the app image-url-signing-service to pull in the local version of this library and started the app locally.

get redirected to login.gutools page for authentication when I open a page on the local image URL signing service.
get back to the image-url-signing-service page successfully after authentication.

changeset-bot · 2025-07-03T17:08:43Z

🦋 Changeset detected

Latest commit: cc5ce32

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@guardian/pan-domain-node	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

SHession

These changes make sense, have they been tested using npm-link on any of the consuming apps?

Looks like a most consumer may be a few version behind so it may be some work to get the up-to-date: https://www.npmjs.com/package/@guardian/pan-domain-node?activeTab=versions

SHession · 2025-07-10T12:55:19Z

src/panda.ts

+    s3Client: S3;

-    constructor(cookieName: string, region: string, bucket: string, keyFile: string, validateUser: ValidateUserFn) {
+    constructor(cookieName: string, region: string, bucket: string, keyFile: string, validateUser: ValidateUserFn, localDev : boolean = false) {


Can we update the README with some detail about this change? What local credentials and level of access will people need for this to work?

Good point, I'll update the README.

At the moment, we grant the GetObject permission on the bucket to any IAM under a (fairly broad) list of AWS accounts (such as composer, cms-fronts, mobile etc). So the execution environment requires credential of any IAM users under these account. I'll add this to the README.

SHession · 2025-07-10T12:59:28Z

src/panda.ts

-        this.publicKey = fetchPublicKey(region, bucket, keyFile);
+        const standardAwsConfig = {
+            region: region,
+            credentials: localDev? fromIni({ profile: LOCAL_PROFILE }): fromNodeProviderChain(),


Presumably any environment running this in AWS will need to be configured with read access to S3 bucket. In some cases will require cross-account permissions to be set up. This would be worth mentioning in the documentation.

src/fetch-public-key.ts

waisingyiu · 2025-07-10T14:06:48Z

These changes make sense, have they been tested using npm-link on any of the consuming apps?

Looks like a most consumer may be a few version behind so it may be some work to get the up-to-date: https://www.npmjs.com/package/@guardian/pan-domain-node?activeTab=versions

yes, I tested the changes locally with these consuming apps: ten_four-quiz-builder and image URL signing. They were on version 0.4 and so, yes, I need to make changes for bumping their pan-domain-node to 1.0.

SHession · 2025-07-10T15:23:01Z

These changes make sense, have they been tested using npm-link on any of the consuming apps?
Looks like a most consumer may be a few version behind so it may be some work to get the up-to-date: https://www.npmjs.com/package/@guardian/pan-domain-node?activeTab=versions

yes, I tested the changes locally with these consuming apps: ten_four-quiz-builder and image URL signing. They were on version 0.4 and so, yes, I need to make changes for bumping their pan-domain-node to 1.0.

This is great! I was planning to do some testing myself, but looks like you're way ahead of me.

SHession · 2025-07-14T07:52:15Z

README.md

+The library requires AWS credential to read the public key files from the S3 bucket `pan-domain-auth-settings` in `Workflow` account.  Please ensure that the execution environment has AWS credential for an IAM profile that has read access to the bucket.
+
+Currently, the bucket has been configured to allow access from any IAM users under a (fairly broad) list of AWS accounts (such as composer, cms-fronts, mobile etc).  Please get in touch with Workflow and Collaboration team if you need to get the read permission on the `pan-domain-auth-settings` bucket for your AWS acount.


It might be worth clarifying your description a bit. Technically we are using a bucket in the Workflow account. However, this package is publicly available and could be utilised by others who are using Panda in a non-guardian environment. Therefore we can't assume that consumers of this library are going to be using the Guardian bucket.

To avoid this assumption, we could remove references to the specific account and bucket and just mention bucket / object access is required. We could also link to the Panda documentation if that would be useful: https://github.com/guardian/pan-domain-authentication.

A very good point. I've updated the readme to make it more general.

I've also added an argument to PanDomainAuthentication for the AWS profile name, which is used when the application is running locally. Previously I assumed that it was Workflow.

SHession · 2025-07-15T09:39:38Z

src/panda.ts

    s3Client: S3;

-    constructor(cookieName: string, region: string, bucket: string, keyFile: string, validateUser: ValidateUserFn, localDev : boolean = false) {
+    constructor(cookieName: string, region: string, bucket: string, keyFile: string, validateUser: ValidateUserFn, localDev : boolean = false, localProfile?: string) {


I hadn't thought about this but it makes sense to change this too!

I was wondering if we should have a single argument to control both. For example localDev being an optional string, which when provided overrides the nodeProviderChain. I'm not quite sure on this, but two arguments which are co-dependent also feels a little clunky.

Maybe the argument should be an optional credentials type for greater flexibility?

A good idea! I've made the change in the last commit.

Looks great! Can we update the README to reflect this too

Sure, I've updated the README.

…DomainAuthentication

SHession

Looks great!

waisingyiu force-pushed the wsy/load-public-settings-using-aws-sdk branch from f598f89 to 99c0a2b Compare July 7, 2025 09:57

Load public key using AWS SDK

b0f394f

waisingyiu force-pushed the wsy/load-public-settings-using-aws-sdk branch from 99c0a2b to b0f394f Compare July 8, 2025 09:10

Add changeset

b762261

waisingyiu changed the title ~~Use aws sdk to load public settings file~~ Download public key from S3 with AWS credential Jul 8, 2025

waisingyiu marked this pull request as ready for review July 8, 2025 09:32

waisingyiu requested a review from a team as a code owner July 8, 2025 09:32

waisingyiu linked an issue Jul 8, 2025 that may be closed by this pull request

Load the public settings file using AWS SDK instead of direct HTTP requests #37

Closed

SHession reviewed Jul 10, 2025

View reviewed changes

waisingyiu added 3 commits July 11, 2025 17:42

Remove redundant argument

f396965

Update Readme

673da6c

Log any errors

2f54d8a

SHession reviewed Jul 14, 2025

View reviewed changes

Not assume any AWS account

d8a5bd2

waisingyiu requested a review from SHession July 14, 2025 14:51

SHession reviewed Jul 15, 2025

View reviewed changes

waisingyiu requested a review from SHession July 15, 2025 09:59

waisingyiu force-pushed the wsy/load-public-settings-using-aws-sdk branch 2 times, most recently from d48e64b to 325284a Compare July 15, 2025 13:13

Change to credentials provider as parameter in the constructor of Pan…

cc5ce32

…DomainAuthentication

waisingyiu force-pushed the wsy/load-public-settings-using-aws-sdk branch from 325284a to cc5ce32 Compare July 15, 2025 13:17

SHession approved these changes Jul 15, 2025

View reviewed changes

waisingyiu merged commit b2fa4ee into main Jul 15, 2025
1 check passed

LAKSHMIRPILLAI mentioned this pull request Jul 22, 2025

Bump pan-domain-node to v1.2 guardian/editorial-tools-user-telemetry-service#64

Merged

This was referenced Jul 22, 2025

Bump pan domain node to v1.2 guardian/image-url-signing-service#225

Merged

Upgrade pan-domain-node to 1.2.0 guardian/pinboard#370

Merged

Bump pan-domain-node to 1.2.0 guardian/gudocs2#20

Merged

LAKSHMIRPILLAI mentioned this pull request Jul 30, 2025

Bump pan-domain-node to v1.2.0 guardian/editorial-collaboration#23

Merged

		The library requires AWS credential to read the public key files from the S3 bucket `pan-domain-auth-settings` in `Workflow` account. Please ensure that the execution environment has AWS credential for an IAM profile that has read access to the bucket.

		Currently, the bucket has been configured to allow access from any IAM users under a (fairly broad) list of AWS accounts (such as composer, cms-fronts, mobile etc). Please get in touch with Workflow and Collaboration team if you need to get the read permission on the `pan-domain-auth-settings` bucket for your AWS acount.

Conversation

waisingyiu commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

What does this change?

How to test

Uh oh!

changeset-bot bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

SHession left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

waisingyiu commented Jul 10, 2025

Uh oh!

SHession commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SHession Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SHession left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

waisingyiu commented Jul 3, 2025 •

edited

Loading

changeset-bot bot commented Jul 3, 2025 •

edited

Loading

SHession Jul 15, 2025 •

edited

Loading