[GSoC-273] Fixing the github action Unmanaged Service Account Keys - #39177
[GSoC-273] Fixing the github action Unmanaged Service Account Keys#39177HansMarcus01 wants to merge 13 commits into
Conversation
…rt for service accounts that are not yet found in keys.yaml
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 enhances the 'Unmanaged Service Account Keys' GitHub action by improving how compliance issues are reported and tracked. It introduces a more robust system for logging unauthorized service accounts and compliance violations, ensuring that security alerts are distinct from general issues and that historical audit data is preserved within GitHub issues instead of being lost during updates. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
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 the 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 counterproductive. 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 refactors the compliance enforcement logic for service account keys. It separates general compliance issues from unmanaged key security alerts, and updates the announcement system to append new reports to existing GitHub issues while archiving older reports into a collapsed history section. The review feedback highlights a potential runtime NameError in account_keys.py due to a missing datetime import, points out robustness issues in sending.py when parsing and replacing HTML tags in the issue body, and recommends replacing non-idiomatic inline import statements with standard imports at the top of the file.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self.sending_client.print_announcement(title, body, recipient, announcement) | ||
| if unmanaged_keys_issues: | ||
| self.logger.info("Printing security dashboard update for unmanaged keys...") | ||
| timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
There was a problem hiding this comment.
Potential Runtime Error: Ensure that datetime is imported at the top of the file. If it is not imported, this line will raise a NameError at runtime.
If datetime is not imported, please add from datetime import datetime, timezone at the top of the file and update this line to use them directly.
| timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") | |
| timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
| headed = old_body.split(history_marker) | ||
| last_report = headed[0].strip() | ||
| old_history = headed[1].replace("</details>", "").strip() | ||
| combined_history = f"{last_report}\n\n---\n\n{old_history}" |
There was a problem hiding this comment.
Correctness/Robustness: Using .replace("</details>", "") is risky because it will remove all occurrences of </details> within the history, which will corrupt the Markdown rendering if there are other collapsed sections or nested HTML details blocks. Additionally, split(history_marker) should limit the split to 1 to prevent issues if the marker appears elsewhere.
Instead, split with maxsplit=1 and safely strip only the trailing </details> tag.
| headed = old_body.split(history_marker) | |
| last_report = headed[0].strip() | |
| old_history = headed[1].replace("</details>", "").strip() | |
| combined_history = f"{last_report}\n\n---\n\n{old_history}" | |
| headed = old_body.split(history_marker, 1) | |
| last_report = headed[0].strip() | |
| old_history = headed[1].rstrip() | |
| if old_history.endswith("</details>"): | |
| old_history = old_history[:-10].rstrip() | |
| combined_history = f"{last_report}\n\n---\n\n{old_history}" |
| open_issues = self._get_open_issues(title) | ||
| open_issues.sort(key=lambda x: x.updated_at, reverse=True) | ||
|
|
||
| timestamp = __import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
There was a problem hiding this comment.
Style/Maintainability: Using inline __import__("datetime") is non-idiomatic and violates PEP 8 guidelines regarding imports being at the top of the file.
Please import datetime and timezone at the top of the file:
from datetime import datetime, timezoneAnd simplify this line.
| timestamp = __import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") | |
| timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
References
- Imports should be placed at the top of the file, as per PEP 8 guidelines. (link)
| print("\nSimulating GitHub issue creation...") | ||
| print(f"Title: {title}") | ||
| print(f"Body: {body}") | ||
| timestamp = __import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
There was a problem hiding this comment.
Style/Maintainability: Using inline __import__("datetime") is non-idiomatic and violates PEP 8 guidelines.
Please import datetime and timezone at the top of the file and simplify this line.
| timestamp = __import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") | |
| timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
References
- Imports should be placed at the top of the file, as per PEP 8 guidelines. (link)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
…their roles within the GCP environment, eliminates redundant code, and updates the documentation.
…sting the correct version of hashicorp/terraform and avoiding the permissions error
|
@Amar3tto PTAL |
…solve the mail server connection error.
|
Note: The "Clear-text logging of sensitive information" warning flagged on this file is a false positive.CodeQL's heuristics flagged the variables secret, secret_name, and managed_secrets. However, these variables exclusively store the GCP Secret Manager resource identifiers (names/IDs), not the actual cryptographic payloads or sensitive data. The actual secret payloads are securely handled in a separate method (_get_verified_keys_from_secret_manager) and are never passed to the logger. Logging resource names is standard practice and strictly necessary for infrastructure auditing and debugging. Since no sensitive data is being exposed in plain text, I suggest we dismiss or ignore this specific alert. |
| @@ -82,4 +83,4 @@ jobs: | |||
| EMAIL_ADDRESS: ${{ secrets.ISSUE_REPORT_SENDER_EMAIL_ADDRESS }} | |||
| EMAIL_PASSWORD: ${{ secrets.ISSUE_REPORT_SENDER_EMAIL_PASSWORD }} | |||
| EMAIL_RECIPIENT: "dev@beam.apache.org" | |||
There was a problem hiding this comment.
if we do not need the email recipient, please create an issue to track removing this (no need to fix in this pull request)
|
Assigning reviewers: R: @Abacn for label build. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
…suring that unmanaged keys are reported for both authorized and unauthorized service accounts.
…01/beam into Fix-AuditUnmanagedAccountKeys
…AM_USER]' and '[IAC_DRIFT_SA_KEY]' tags in the enforcement scripts. This change allows for better filtering of notifications, reducing alert fatigue, and clearly distinguishing between routine infrastructure drift and actual critical security vulnerabilities.
HansMarcus01
changed the title
| self.logger.info(f"Found {len(general_issues)} general compliance issues. Triggering announcement...") | ||
| title = f"Account Keys Compliance Issue Detected" | ||
| body = f"Account keys for project {self.project_id} are not compliant with the defined policies on {self.service_account_keys_file}\n\n" | ||
| title = f"[SECURITY] Action Required: Unauthorized Service Accounts Detected" |
There was a problem hiding this comment.
change the title too to include IAC_DRIFT_SA_KEY - also, create a constant at the top of the file for IAC_DRIFT_SA_KEY. Even better if we have a central place for all the constants.
|
|
||
| if current_user and not existing_user: | ||
| differences.append(f"User {email} not found in existing policy.") | ||
| differences.append(f"IAC_DRIFT_IAM_USER: Unauthorized user '{email}' detected in GCP but not found in existing policy.") |
There was a problem hiding this comment.
create a constant for IAC_DRIFT_IAM_USER
| for issue in diff: | ||
| body += f"- {issue}\n" | ||
| iam_drift_issues = [issue for issue in diff if "IAC_DRIFT_IAM_USER" in issue] | ||
| general_issues = [issue for issue in diff if "IAC_DRIFT_IAM_USER" not in issue] |
There was a problem hiding this comment.
is it okay if general_issues only filters out IAC_DRIFT_IAM_USER but not IAC_DRIFT_SA_KEY
| for issue in diff: | ||
| body += f"- {issue}\n" | ||
| iam_drift_issues = [issue for issue in diff if "IAC_DRIFT_IAM_USER" in issue] | ||
| general_issues = [issue for issue in diff if "IAC_DRIFT_IAM_USER" not in issue] |
There was a problem hiding this comment.
use a coinstant here please and make sure filtering is correct (do we only filter out IAC_DRIFT_IAM_USER ? or should we filter out other IAC_DRIFT constants?
…the reported problem.
4 participants
This pull request fixes a runtime error in the GitHub action "Unmanaged Service Account Keys." It adds a new report for unauthorized service accounts and prompts users to comply with compliance policies. Additionally, it enables the action that reports permission issues for users working on the Apache Beam project. Handling the hashicorp/terraform procedure is included to address the permissions issue it had.
Currently, the keys.yaml file is empty, indicating that the service accounts were created manually, thus preventing secure management.
Automation and Workflow Integration:
account_keys.py) This file corrects the generation of the report to include the case where both the service accounts and their keys are created from the project policies..github/workflows/beam_Infrastructure_UsersPermissions.yml) the current version used was updated to avoid the permissions problem with the Terraform procedure.github/workflows/beam_Infrastructure_PolicyEnforcer.yml, iam.py) The generation of the report for user policy violations was updated to include a history and allow generating the issue that notifies such problems.Output expected:
Expected output from iam.py
Title: [SECURITY] Action Required: Unauthorized IAM Users Detected
Body:
Compliance Audit Report (2026-08-18 02:33:07 UTC)
Critical security violations detected in IAM policies for project apache-beam-testing:
History
Click to expand
[... Previous reports would be collapsed here ...]
Expected output form account_keys.py
Title: [SECURITY] Action Required: Unmanaged Service Account Keys Detected
Body:
Unmanaged Keys Audit Report (2026-08-18 02:35:41 UTC)
The following unauthorized or unmanaged keys were detected in
apache-beam-testing:Please investigate and revoke these keys if they are not part of the official rotation system.
History
Click to expand
[... Previous reports would be collapsed here ...]