feat!: check for trust boundaries before sending requests google apis. #1966
Conversation
src/auth/computeclient.ts
Outdated
| if (!this.trustBoundaryEnabled) { | ||
| return null; | ||
| } | ||
| //todo pjiyer, add Error handling |
There was a problem hiding this comment.
// TODO(pjiyer): Implement error handling logic for lookup endpoint.
src/auth/trustboundary.ts
Outdated
| requestOptions.headers = new Headers(); | ||
| requestOptions.headers.set('authorization', authHeader); | ||
|
|
||
| console.log(`TrustBoundary: Fetching data from ${url}`); |
There was a problem hiding this comment.
Don't forget to cleanup all these console.log comments
src/auth/trustboundary.ts
Outdated
| } | ||
| } | ||
|
|
||
| // --- Internal Helper Function --- |
src/auth/trustboundary.ts
Outdated
| requestOptions, | ||
| ); | ||
| console.log('Response from lookup endpoint ' + response); | ||
| // const response: GaxiosResponse<AllowedLocationsResponse> = await authenticatedClient.request<AllowedLocationsResponse>(requestOptions); |
src/auth/trustboundary.ts
Outdated
| } | ||
| } | ||
|
|
||
| // --- Exported Lookup Function --- |
There was a problem hiding this comment.
| // --- Exported Lookup Function --- |
src/auth/authclient.ts
Outdated
| if ( | ||
| !headers.has('x-goog-allowed-locations') && // don't override a value the user sets. | ||
| this.trustBoundary && | ||
| this.trustBoundary.encodedLocations | ||
| ) { | ||
| headers.set( | ||
| 'x-goog-allowed-locations', | ||
| this.trustBoundary.encodedLocations, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Prefer comments inline, and maybe splitting this up for readability
src/auth/trustboundary.ts
Outdated
| const ServiceAccountAllowedLocationsEndpoint = | ||
| 'https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s/allowedLocations'; | ||
|
|
||
| // --- Interfaces --- |
There was a problem hiding this comment.
| // --- Interfaces --- |
src/auth/trustboundary.ts
Outdated
| * Workload Identity Pool -> project_id, pool_id | ||
| * Workforce Pool -> pool_id | ||
| */ | ||
| export interface TrustBoundaryDescriptor { |
There was a problem hiding this comment.
I don't think this is the right approach (mixing all of these together)
src/auth/trustboundary.ts
Outdated
| */ | ||
| async function _fetchTrustBoundaryData( | ||
| authenticatedClient: AuthClient, | ||
| authHeader: string, |
There was a problem hiding this comment.
Why does this need to be passed? can't we get it from the auth client?
There was a problem hiding this comment.
Since the lookup endpoint is called in the refreshTokenNoCache(), and in the refreshToken function, the new authClient credentials aren't set.
They are set in the getRequestMetadataAsync() in the oauth2client. We cannot make the trust boundary calls in the oauth2client as it doesn't exclusively pertain to service accounts.
That's why I chose to pass the authHeader exclusively.
Another option is to set the access_token of the client credentials in the refreshTokenNoCache function. That would allow us to get the header directly from the authClient without the header param.
I'd welcome suggestions to circumvent this.
src/auth/trustboundary.ts
Outdated
| * @param cachedData Optional previously fetched data to check for no-op or use as fallback on error. | ||
| * @returns A Promise resolving to TrustBoundaryData or null if fetching fails and no cache is available. | ||
| */ | ||
| export async function lookupServiceAccountTrustBoundary( |
There was a problem hiding this comment.
The difference in logic for retrieving the trust boundary based on credential type is just the lookup endpoint URL. So there should be one lookup method. Different auth clients can provide the lookup endpoint.
|
The current approach has each client duplicating trust boundary logic. Can we centralize and simplify this? |
f0454e7 to
6d8fd9d
Compare
src/auth/authclient.ts
Outdated
| @@ -317,11 +325,22 @@ export abstract class AuthClient | ||
| ) { | ||
| headers.set('x-goog-user-project', this.quotaProjectId); | ||
| } | ||
| if ( | ||
| !headers.has('x-goog-allowed-locations') && |
There was a problem hiding this comment.
I think we need to send the most up to date TB data. The !headers.has('x-goog-allowed-locations') check will skip setting and updating the value it if was previously set.
src/auth/authclient.ts
Outdated
| if ( | ||
| !headers.has('x-goog-allowed-locations') && | ||
| this.trustBoundary && | ||
| this.trustBoundary.encodedLocations |
There was a problem hiding this comment.
Checking that encodedLocations is not an empty string is not enough.
We only want to set the header if the value is not no-op (0x0)
src/auth/authclient.ts
Outdated
| @@ -343,6 +363,10 @@ export abstract class AuthClient | ||
| target.set('authorization', authorizationHeader); | ||
| } | ||
|
|
||
| if (xGoogAllowedLocs) { |
There was a problem hiding this comment.
The name of the method still implies that it only sets x-goog-user-project and authorization headers. I'd recommend either creating a separate method to set x-goog-allowed-locations, or updating the method name.
This change includes trust boundaries for Service Accounts in the google auth nodejs library.
Impact
This will allow clients who use the google auth library to also enable trust boundaries ensuring service accounts can access only those resources which are within the trust boundary. We use this design doc as reference for the code changes.
Testing
Testing will be added in post initial design review.