fix #3845

[guillaumeblaquiere]

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: Security Critical: Client_secret is shared in OAuth2 flow with authenticated function tool #3845

Problem:
The client_secret was leaked and shared with the client

Solution:
I was unable to solve the issue myself. I vibe coded it with Antigravity.
I checked the code, it looks good to me.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

Manual End-to-End (E2E) Tests:

Manual test is OK

[gemini-code-assist]

Summary of Changes

Hello @guillaumeblaquiere, I'm Gemini Code Assist¹! 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 addresses a critical security concern by implementing a robust system to prevent the leakage of client secrets. It centralizes the secure, in-memory storage of these secrets, redacts them from configuration objects, and only exposes them transiently during necessary authentication flows, significantly enhancing the overall security posture of the system's authentication handling.

Highlights

• Client Secret Redaction and Secure In-Memory Storage: Implemented a new mechanism to prevent client secret leakage by redacting secrets from AuthConfig objects and storing them securely in an in-memory map within the CredentialManager.
• On-Demand Client Secret Restoration: Modified the OAuth2 credential exchange and URI generation processes to temporarily retrieve the client secret from secure storage only when needed for authentication operations, ensuring it is re-redacted immediately after use.
• Credential Key Normalization: Updated the logic for generating credential keys to normalize the client secret field to None, which ensures that the key remains stable and consistent regardless of whether the secret is currently redacted or not.
• Test Suite Enhancements: Refactored mock creation in unit tests for CredentialManager to use a new helper function, create_auth_config_mock, and ensured proper mocking of oauth2 attributes on AuthCredential mocks to align with the new secret handling logic.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[adk-bot]

Response from ADK Triaging Agent

Hello @guillaumeblaquiere, thank you for your contribution!

To help the reviewers better understand and verify this important security fix, could you please provide logs or a screenshot demonstrating that the client_secret is no longer leaked after your changes are applied?

This will help speed up the review process. Thanks!

[gemini-code-assist]

Code Review

This pull request aims to prevent a client secret leak by redacting it and storing it in memory. While the approach is generally correct, the implementation introduces a critical security vulnerability where a secret could be left un-redacted if an error occurs during an OAuth token exchange. Additionally, there's a high-severity issue where sensitive access tokens are leaked to stderr for debugging. I've also included several medium-severity comments to improve code quality, efficiency, and maintainability. It is crucial to address the security flaws before merging.

[guillaumeblaquiere]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a critical security vulnerability by preventing client secrets from being leaked. The approach of redacting the secret and storing it in memory on the server-side is sound. The changes are extensive and include new logic in CredentialManager and AuthHandler, as well as new tests to cover the secret handling.

My review focuses on ensuring the new logic is correct, robust, and maintainable. I've identified a critical bug in the secret re-redaction logic, some code duplication that should be addressed, and issues in the new tests that could lead to flakiness. Please see my detailed comments below.

[guillaumeblaquiere]

/gemini review

[gemini-code-assist]

Code Review

This pull request aims to fix a critical security issue where the client_secret was being leaked. The approach of redacting the secret upon CredentialManager initialization and using a context manager (restore_client_secret) to temporarily restore it for API calls is excellent. The addition of new unit tests to cover this new secret handling logic is also a great improvement. However, I've identified a critical security vulnerability in the new fallback logic within the _exchange_credential method. This logic could lead to using a secret for one client with another client's ID, which must be fixed. My review includes a specific comment with a suggested fix for this issue.

[ryanaiagent]

Hi @guillaumeblaquiere , Thank you for your work on this pull request. We appreciate the effort you've invested. Can you please fix the failing unit tests.

[guillaumeblaquiere]

@ryanaiagent done.

[ryanaiagent]

@guillaumeblaquiere ,Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share.

[ryanaiagent]

Hi @seanzhou1023 , can you please review this.

[ryanaiagent]

Hi @guillaumeblaquiere , we appreciate your patience and support. Can you please fix the failing unit tests and formatting errors.

[guillaumeblaquiere]

@ryanaiagent unit test fixed

[ryanaiagent]

Hi @guillaumeblaquiere , pls fix the formatting error as well.

[guillaumeblaquiere]

@ryanaiagent pyink ok

[ryanaiagent]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a robust client secret management mechanism by redacting client secrets from AuthConfig objects and storing them securely in memory within the CredentialManager. A context manager, restore_client_secret, is implemented to temporarily expose these secrets for operations like token exchange and URI generation, ensuring they are re-redacted afterwards. New unit tests have been added to validate this security enhancement and the stability of credential keys. Additionally, minor import cleanups and updates to test mocks have been performed across several files.

[ryanaiagent]

Hi @guillaumeblaquiere , can you please address the suggestions.

[guillaumeblaquiere]

@ryanaiagent I'm travelling the next 2 days. I will fix this then.
Thank you for your patience.

[guillaumeblaquiere]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a robust mechanism for handling client secrets by redacting them from configuration objects and storing them securely in memory, which is a significant security improvement. The changes are comprehensive, affecting core authentication logic and updating numerous tests to align with the new behavior. My review has identified a critical bug in the credential_manager, a suggestion for simplification in auth_handler, and a couple of issues within the new tests that should be addressed.

[gemini-code-assist]

There is a duplicated license header in this file. It should be removed to maintain code cleanliness.

[guillaumeblaquiere]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a robust mechanism for securely handling client_secret in OAuth2 flows, effectively preventing it from being leaked. The core of the solution involves redacting the secret from credential objects and storing it in an in-memory CredentialManager, only restoring it temporarily within a context manager when required for API calls. This is a significant security enhancement, supported by a comprehensive set of new unit tests that validate the new behavior and ensure key stability. While the implementation is strong, I have identified a potential logic issue regarding credential refreshing and a minor point on code duplication. Overall, this is a valuable contribution to improving the security of the authentication process.

[gemini-code-assist]

Hardcoding was_exchanged to True here alters the credential processing logic. The previous implementation relied on the value returned by the exchanger to determine whether to attempt a refresh. With this change, a refresh will never be attempted after an exchange. If an exchanger is called with a credential that doesn't require an exchange (e.g., it already has a valid access token), was_exchanged would have been False, allowing a refresh to be attempted if necessary. This change prevents that, which could result in using an expiring token that should have been refreshed. It would be safer to use the was_exchanged value from the ExchangeResult.

Suggested change

	with self.restore_client_secret(credential):
	exchanged_credential = await exchanger.exchange(
	credential, self._auth_config.auth_scheme
	)
	return exchanged_credential.credential, True
	with self.restore_client_secret(credential):
	exchange_result = await exchanger.exchange(
	credential, self._auth_config.auth_scheme
	)
	return exchange_result.credential, exchange_result.was_exchanged

[gemini-code-assist]

This local import of ServiceAccountCredentialExchanger is also present in the __init__ method. To avoid duplication, you could import it once at the top of the file. If a top-level import is not feasible due to circular dependencies (which the TODO comment about moving the module seems to hint at), it would be beneficial to add a comment explaining why the duplicated local imports are necessary for future maintainability.