Enhance service account access token validation for improved security

[zackcl]

Description:

This PR addresses a security concern in our current access token validation process and implements a more robust solution.

Context:

Our application currently verifies users with ID tokens (for "Sign-in with Google" button logins) and access tokens (generated with service account key JSON files) for API requests. The existing approach had a security weakness: it verified any unexpired access token generated by any service account key JSON file, potentially allowing unauthorized access.

Key changes:

• Added a new environment variable SERVICE_ACCOUNT_ID to store our specific service account's unique identifier.
• Implemented stricter validation of access tokens using the aud (audience) claim.

The AuthService now compares the aud claim from the token info against our stored SERVICE_ACCOUNT_ID. This ensures that only access tokens specifically intended for our service are accepted, significantly enhancing our application's security by rejecting tokens from unrecognized service accounts.

Changes required:

• Update .env file to include SERVICE_ACCOUNT_ID (locally)
• Update deployment configurations to set SERVICE_ACCOUNT_ID in the environment (AWS)

Testing needed:

• Verify that valid service account access tokens are still accepted
• Confirm that tokens with incorrect aud claims are rejected
• Test with expired tokens to ensure they're still properly handled

This change improves our adherence to OAuth 2.0 best practices for token validation and addresses the identified security concern with our previous implementation.

References:

• Relevant PR (merged): Add support for service account access tokens #1829
• Documentation: OAuth2Client - getTokenInfo

The SERVICE_ACCOUNT_ID should be the "Unique ID" shown on the Service account details page under our app:

[shpwe]

This value needs to be added to vault so builds dont fail for each env

[danoswaltCL]

we're still discussing this change @shpwe, it's not something we need to add something in there for yet. @zackcl I included @kcalvo-cli, we should be including SRE review on any infrastructure/config updates, esp right now since it is something being actively worked on.

[zackcl]

@VivekFitkariwala @danoswaltCL I've added the link to the documentation at the bottom. Feel free to review it and provide any suggestions.

[danoswaltCL]

Just so we're straight on what we intend to do, reiterating the usage here:

1. GOOGLE_CLIENT_ID should refer to any valid registered client like the upgrade UI which can pass control to google-auth-services via an individual google user account credential in a pop-up to generate a token. These are for client-to-service calls where we can proxy our token creation to GIS because of the google pop-up login auth mechanism. Each client should have it's own id.

2. GOOGLE_SERVICE_ACCOUNT_ID should refer to any valid registered service like the automated test suite that Chance is creating. These are for backend service - to - backend service calls where the service itself handles creating and passing a token because it can handle grabbing secrets much easier than the frontend. Each service should have it's own id.

I believe we have good current use cases for wanting this overall change for both kinds of ids, as we should easily be able to add as many of these account ids as we want and need. Currently we have just one GOOGLE_CLIENT_ID, but we should probably support a list (for instance we could say both the PPL and CL client_ids are valid, but currently we don't parse a list, we only check the one. You can see in the commented code that this is an anticipated usage).

I'd propose changing SERVICE_ACCOUNT_ID to GOOGLE_SERVICE_ACCOUNT_IDS, which I'd send in as like comma separated list, which is simpler than dealing with a JSON array. And GOOGLE_CLIENT_ID to GOOGLE_CLIENT_IDS, same thing, comma separated list.

[danoswaltCL]

@zackcl presumably you have a use case for something (or some things) you're trying to get to work?

[zackcl]

@danoswaltCL I'm not sure if we have a good use case for supporting multiple Google client IDs or service account IDs at the moment, as our instances are primarily for internal use within the company. If other apps (or developers) need access to our deployed UpGrade instances, we can provide the existing Google Client ID (for authenticating through the "Sign-in with Google" button), or the service account key (JSON file) to generate an access token (for making requests to authorized endpoints). We can also create multiple service accounts (in Google Cloud Console) and have our deployed instances use different service account IDs per environment (QA, Staging, Production) to allow limited access (e.g., we can only share the service account key used for the QA environment with the developers).

[danoswaltCL]

i'm learning about google service accounts here also, I'm no expert. To me it's clear that the best practice is to support multiple ids, and it's trivial to do so. Clients/services are a many-to-one relationship to the backend, so it seems odd to only support one each. we have many use-cases, if we want a sign-in button the tester-app for instance... that should have it's own client id and it's own authorized urls, etc. any automated integration should be a separate service account, etc.

[zackcl]

if we want a sign-in button the tester-app for instance... that should have it's own client id and it's own authorized urls, etc. any automated integration should be a separate service account, etc.

I'm no expert either, but I'm trying to understand the benefits in our specific case. Why should other apps use their own client IDs and service accounts when they can use the ones that already exist for accessing the same resource? It feels to me like we're unnecessarily requiring different keys to open the same door. Could you provide some specific scenarios in our current setup where having multiple IDs would offer tangible benefits?

[zackcl]

Maybe you're considering that we might want to allow different levels of access to our resources depending on the client ID or service account? For example, we could allow the /clearDB endpoint only if the token's service account ID is for the demo app, or only allow specific endpoints to be used by the tester app.

If this is the scenario being considered, then I think it could make our app more protected and controlled, but we might also be over-engineering. I'm good with supporting multiple Google client IDs and service account IDs as you suggested.

[danoswaltCL]

i'm just asking for this support to a list instead of a single value. nothing else needs to change. it is absolutely normal to give different keys to open the same door, that's standard API management. it is not even convenient to reuse a key, so why the pushback? just the other day you had to have me add the tester-app as an authorized url so that you could reuse that client id. we shouldn't have to do that, these are separate concerns.

[zackcl]

As I mentioned earlier, I don't see a clear benefit in managing multiple keys if all keys provide identical access. I think it just adds more complexity in both the backend and for developers. We'll have more keys to manage, and the developers would need to set up and maintain their own Google client ID and service account to access our resource. Having a single key for identical access keeps the system simpler and easier to manage. Also, adding or updating an authorized URL isn't needed that often, and it is still simpler than creating a new client ID with authorized URL, and adding that to UpGrade through AWS.

That said, I'm open to following your suggestion. I just wanted to understand the concrete benefits before we implement this change.

[danoswaltCL]

@zackcl @kcalvo-cli @bcb37 @VivekFitkariwala can we meet separately from the SRE mtg some morning next week to just focus on what our strategy should be with auth in general these days? I think our current setup is more of an artifact of getting the app up-and-running and we have quite different needs now, that will help get this PR moving along.

[danoswaltCL]

talked with Vivek and Zack about this separately, and then in SRE mtg Kenneth said it makes sense to support multiple.

I tested this for google client id, it worked when I supplied put multiple client ids in the env with a comma separated list. NOTE: don't put space between the commas if you use this code.

This worked to simply split the string into an array and pass that in directly to audience. I tested it using both the CL and PPL google client id tokens in my local .env. I can user either token in my environment.ts frontend file, both will login.

try {
      const clientIdList = env.google.clientId.split(',');
      request.logger.info({ message: 'Client ID List', clientIdList });
      // First, try to verify the token as an ID token
      const ticket = await client.verifyIdToken({
        idToken: token,
        audience: clientIdList,
      });
      payload = ticket.getPayload();
      request.logger.info({ message: 'ID Token Validated' });
 }

And for service accounts, we'd check that manually via something like:

try {
    const tokenInfo = await client.getTokenInfo(token);
    const serviceAccountIdList = env.google.serviceAccountId.split(,);
        if (!tokenInfo || serviceAccountIdList.includes(tokenInfo.aud)) {
          throw new Error('Invalid or unauthorized access token');
        }

So the current config will continue to work, and we will be able to add more ids later if we want:
GOOGLE_CLIENT_ID: abc123 as it is currently
GOOGLE_CLIENT_ID: abc123,xyz789 if we register more valid client ids.
GOOGLE_SERVICE_ACCOUNT_ID: same thing.

[zackcl]

@danoswaltCL I've made a commit to support multiple Google client and service account IDs. I also renamed SERVICE_ACCOUNT_ID to GOOGLE_SERVICE_ACCOUNT_ID for consistency. I've tested it using multiple service account IDs, and it worked as expected. Using a single ID also works fine. Please review and approve this PR.

Regarding deployment, I'm uncertain about the necessary updates for Jenkins and AWS. Should we add GOOGLE_SERVICE_ACCOUNT_ID to Vault for Jenkins, and UPGRADE_GOOGLE_SERVICE_ACCOUNT_ID to the AWS Elastic Beanstalk configurations?

[danoswaltCL]

@kcalvo-cli @shpwe where will this get updated in the new setup? Is that something we can do, or will config changes be something we knock on your door for? I can add to the beanstalk configs.

GOOGLE_SERVICE_ACCOUNT_ID: string,

[danoswaltCL]

@kcalvo-cli @shpwe I've added this key/value in the "beanstalk" environment so we can merge this, can this value also be added to the secure Param store?

The key itself can be found on the Google Cloud Console under Service Accounts > test-app > Keys

GOOGLE_SERVICE_ACCOUNT_ID: secure string,

[danoswaltCL]

@zackcl this was added to the configurations https://carnegielearning.atlassian.net/browse/DEVOPS-7188. You should be good to merge it now. I was going to merge but it's my EOD and I didn't want to potentially break something before walking out the door.