[DERCBOT-1691] Support GSA Impersonation #1950
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Google Service Account (GSA) impersonation support to the Google Chat connector, allowing more flexible authentication patterns in Google Cloud environments. When gsaToImpersonate is configured, the connector uses ImpersonatedCredentials with either explicit JSON credentials or Application Default Credentials (ADC) as the source. When not set, the connector maintains backward compatibility with the existing direct authentication approach.
Key changes:
- Added optional GSA impersonation with automatic fallback to ADC when explicit credentials are unavailable
- Enhanced logging throughout the authentication flow and message sending process for better observability
- Updated documentation to explain the new impersonation feature and required IAM permissions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
bot/connector-google-chat/src/main/kotlin/GoogleChatConnectorProvider.kt |
Core implementation of GSA impersonation logic, including credential loading, scope management, and configuration field addition |
bot/connector-google-chat/src/main/kotlin/GoogleChatConnector.kt |
Enhanced error handling and logging for message sending operations |
bot/connector-google-chat/README.md |
Documentation updates explaining impersonation setup, permissions, and configuration examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| val sourceCredentials = getSourceCredentials(connectorConfiguration) | ||
|
|
||
| logger.info { "Source credentials: ${(sourceCredentials as? ServiceAccountCredentials)?.clientEmail}" } |
There was a problem hiding this comment.
Logging the source service account email could expose sensitive configuration information. Consider removing this log statement or redacting the email address.
| logger.info { "Source credentials: ${(sourceCredentials as? ServiceAccountCredentials)?.clientEmail}" } |
| val sourceCredentials = getSourceCredentials(connectorConfiguration) | ||
|
|
||
| logger.info { "Source credentials: ${(sourceCredentials as? ServiceAccountCredentials)?.clientEmail}" } | ||
| logger.info { "Impersonating target GSA = $targetServiceAccount with scopes = $CHAT_SCOPE" } |
There was a problem hiding this comment.
Logging the target service account email could expose sensitive configuration information. Consider removing this log statement or redacting the email address.
| logger.info { "Impersonating target GSA = $targetServiceAccount with scopes = $CHAT_SCOPE" } | |
| logger.info { "Impersonating target GSA with scopes = $CHAT_SCOPE" } |
| val creds = ServiceAccountCredentials.fromStream(credentialInputStream) | ||
| .createScoped("https://www.googleapis.com/auth/cloud-platform") | ||
|
|
||
| logger.info { "Loaded explicit service account: ${(creds as ServiceAccountCredentials).clientEmail}" } |
There was a problem hiding this comment.
Logging the service account email could expose sensitive configuration information. Consider removing this log statement or redacting the email address.
| logger.info { "Loaded explicit service account: ${(creds as ServiceAccountCredentials).clientEmail}" } |
| } catch (e: Exception) { | ||
| logger.info { "No explicit credentials found, using Application Default Credentials" } | ||
| GoogleCredentials.getApplicationDefault() | ||
| .createScoped("https://www.googleapis.com/auth/cloud-platform") | ||
| } |
There was a problem hiding this comment.
The catch block handles all exceptions generically with catch (e: Exception), which might hide configuration errors (e.g., invalid JSON format, missing files). Consider catching only specific exceptions related to missing credentials (e.g., FileNotFoundException, IOException) and let other exceptions propagate with more context. This will help distinguish between "credentials not provided" vs "credentials provided but invalid".
| targetServiceAccount, | ||
| null, | ||
| listOf(CHAT_SCOPE), | ||
| 3600, |
There was a problem hiding this comment.
The hardcoded lifetime value of 3600 seconds (1 hour) should be extracted to a named constant for better maintainability. Consider adding private const val IMPERSONATION_TOKEN_LIFETIME_SECONDS = 3600 at the top of the file alongside other constants.
There was a problem hiding this comment.
You can also use a property to make it configurable
| .execute() | ||
| try { | ||
| logger.info { | ||
| "Sending to Google Chat: space=${callback.spaceName}, thread=${callback.threadName}, message=${message.toGoogleMessage()}" |
There was a problem hiding this comment.
Logging the complete message content with message=${message.toGoogleMessage()} could inadvertently log sensitive user data or PII. Consider logging only metadata (space, thread) or use a sanitized representation of the message.
| "Sending to Google Chat: space=${callback.spaceName}, thread=${callback.threadName}, message=${message.toGoogleMessage()}" | |
| "Sending to Google Chat: space=${callback.spaceName}, thread=${callback.threadName}, messageType=${message::class.simpleName}" |
| val credentialInputStream = getCredentialInputStream(connectorConfiguration) | ||
| loadCredentials(credentialInputStream) | ||
| } else { | ||
| logger.info { "Using impersonation mode with GSA: $gsaToImpersonate" } |
There was a problem hiding this comment.
Logging the service account email to impersonate could expose sensitive configuration information. Consider using a more generic log message or redacting part of the email (e.g., showing only the domain). For example: "Using impersonation mode with GSA: ${gsaToImpersonate.substringAfter('@')}" or just "Using impersonation mode".
| logger.info { "Using impersonation mode with GSA: $gsaToImpersonate" } | |
| logger.info { "Using impersonation mode with GSA domain: ${gsaToImpersonate.substringAfter('@')}" } |
| true | ||
| ), | ||
| ConnectorTypeConfigurationField( | ||
| "Service account email to impersonate (if provided, priority over JSON credentials)", |
There was a problem hiding this comment.
The description "Service account email to impersonate (if provided, priority over JSON credentials)" is misleading. Impersonation doesn't replace JSON credentials; it uses them (or ADC) as the source for impersonation. Consider revising to: "Service account email to impersonate (optional, uses JSON credentials or ADC as source)" to clarify that both can work together.
| "Service account email to impersonate (if provided, priority over JSON credentials)", | |
| "Service account email to impersonate (optional, uses JSON credentials or ADC as source)", |
2 participants
Ticket :
DERCBOT-1691
Summary
Adds optional GSA impersonation to the Google Chat connector.
If gsaToImpersonate is set, the connector uses ImpersonatedCredentials with source creds (JSON or ADC).
If not set, behavior remains unchanged.