feat(auth): support Domain-Wide Delegation for Service Accounts#543
feat(auth): support Domain-Wide Delegation for Service Accounts#543dumko2001 wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 535081a The changes in this PR will be included in the next version bump. 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement for enterprise environments by enabling service accounts to impersonate specific users within a Google Workspace domain. This capability is essential for automating tasks that require user-specific permissions, streamlining operations without the need for individual user credentials. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for Domain-Wide Delegation for Service Accounts by reading the GOOGLE_WORKSPACE_IMPERSONATE_USER environment variable. The implementation is mostly correct, but I've identified a security concern regarding the logging of the impersonated user's email without sanitization. This could lead to terminal escape sequence injection. Additionally, the pull request lacks tests for this new functionality, which is mentioned as a requirement in the PR's checklist.
| let mut builder = yup_oauth2::ServiceAccountAuthenticator::builder(key); | ||
| if let Ok(sub) = std::env::var("GOOGLE_WORKSPACE_IMPERSONATE_USER") { | ||
| if !sub.is_empty() { | ||
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); |
There was a problem hiding this comment.
The sub variable, read from the GOOGLE_WORKSPACE_IMPERSONATE_USER environment variable, is logged without proper sanitization. This can lead to a terminal escape sequence injection vulnerability if the environment variable contains malicious control characters. While the repository's general rule specifically mentions 'error strings', the principle of sanitizing any untrusted input printed to the terminal should be applied to prevent this security risk.
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); | |
| tracing::debug!(impersonate = %sub.chars().flat_map(|c| c.escape_default()).collect::<String>(), "Using Domain-Wide Delegation"); |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for Domain-Wide Delegation, which is a great feature for enterprise automation. However, I've found two significant issues with the current implementation. First, there is a critical bug where tokens for the service account and impersonated users will collide in the cache, leading to incorrect authentication states. Second, there is a high-severity security issue where user-provided data from an environment variable is logged without sanitization, creating a risk of terminal escape sequence injection. Both issues need to be addressed before merging.
| if let Ok(sub) = std::env::var("GOOGLE_WORKSPACE_IMPERSONATE_USER") { | ||
| if !sub.is_empty() { | ||
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); | ||
| builder = builder.subject(sub); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a critical token cache collision issue. The token cache for service accounts is keyed by scopes, but the cache filename is the same whether impersonation is used or not (sa_token_cache.json). This means a token for the service account itself will be overwritten by a token for an impersonated user (and vice-versa) if they use the same scopes.
This will lead to incorrect authentication, causing either permission errors or actions being performed as the wrong identity.
To fix this, you should use a different cache file when impersonation is active. I recommend modifying the sa_cache path to include a hash of the impersonated user's email. This change would be on line 202, which is outside the current diff, but is essential to fix this bug.
For example:
// (on line 202, which is outside the diff)
let impersonate_user = std::env::var("GOOGLE_WORKSPACE_IMPERSONATE_USER").ok().filter(|s| !s.is_empty());
let cache_key_suffix = if let Some(sub) = &impersonate_user {
// Use a hash to create a unique, filename-safe suffix.
// You'll need to add a dependency like `sha256`.
use sha256::digest;
let hash = digest(sub.as_bytes());
format!("_impersonate_{}", &hash[..12])
} else {
String::new()
};
let sa_cache = token_cache_path.with_file_name(format!("sa{}_{tc_filename}", cache_key_suffix));
// ... then later use `impersonate_user` to set the subject
if let Some(sub) = impersonate_user {
// ...
}| let mut builder = yup_oauth2::ServiceAccountAuthenticator::builder(key); | ||
| if let Ok(sub) = std::env::var("GOOGLE_WORKSPACE_IMPERSONATE_USER") { | ||
| if !sub.is_empty() { | ||
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); |
There was a problem hiding this comment.
The sub variable, which is read from the GOOGLE_WORKSPACE_IMPERSONATE_USER environment variable, is logged directly without sanitization. This is a security risk as it can lead to terminal escape sequence injection if the logs are viewed on a terminal. An attacker could use this to hide malicious activity or execute commands on the user's terminal.
Please sanitize the user-provided input before logging it. The project already has a utility for this: crate::error::sanitize_for_terminal.
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); | |
| tracing::debug!(impersonate = %crate::error::sanitize_for_terminal(&sub), "Using Domain-Wide Delegation"); |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for Domain-Wide Delegation for Service Accounts by allowing user impersonation via the GOOGLE_WORKSPACE_IMPERSONATE_USER environment variable. The implementation correctly uses the yup-oauth2 builder pattern to add the subject for impersonation. My review includes one high-severity security recommendation to prevent terminal escape sequence injection by sanitizing the user-provided value in a debug log message.
| let mut builder = yup_oauth2::ServiceAccountAuthenticator::builder(key); | ||
| if let Ok(sub) = std::env::var("GOOGLE_WORKSPACE_IMPERSONATE_USER") { | ||
| if !sub.is_empty() { | ||
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); |
There was a problem hiding this comment.
The user-provided sub value from the GOOGLE_WORKSPACE_IMPERSONATE_USER environment variable is logged directly using Display formatting (%sub). This could allow a malicious user to inject terminal escape sequences into the logs, which can lead to various security issues if the logs are ever displayed on a terminal. To prevent this, you should use Debug formatting (?sub) which will properly escape the string. This aligns with the general rule to sanitize strings printed to the terminal.
| tracing::debug!(impersonate = %sub, "Using Domain-Wide Delegation"); | |
| tracing::debug!(impersonate = ?sub, "Using Domain-Wide Delegation"); |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
2 participants
Description
Enable user impersonation (Domain-Wide Delegation) for Service Account authentication via the
GOOGLE_WORKSPACE_IMPERSONATE_USERenvironment variable. This allows administrators to perform actions on behalf of other users in the Workspace domain, which is a standard requirement for enterprise automation.Fixes #528
Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.