Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/beam_Infrastructure_AuditUnmanagedKeys.yml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_REPOSITORY: ${{ github.repository }}
SMTP_SERVER: smtp.gmail.com
SMTP_PORT: 465
EMAIL_ADDRESS: ${{ secrets.ISSUE_REPORT_SENDER_EMAIL_ADDRESS }}
EMAIL_PASSWORD: ${{ secrets.ISSUE_REPORT_SENDER_EMAIL_PASSWORD }}
EMAIL_RECIPIENT: "dev@beam.apache.org"
run: python account_keys.py --action announce


Expand Down
65 changes: 43 additions & 22 deletions infra/enforcement/account_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,18 +315,17 @@
msg = f"Service account '{service_account}' is not declared in the service account keys file."
compliance_issues.append(msg)
self.logger.warning(msg)
else:
iam_keys = self._get_user_managed_keys_from_iam(service_account)
if iam_keys:
secret_name = f"{self._denormalize_account_email(service_account)}-key"
legal_keys = []
if secret_name in managed_secrets:
legal_keys = self._get_verified_keys_from_secret_manager(secret_name)
unmanaged_keys = set(iam_keys) - set(legal_keys)
for unmanaged_key in unmanaged_keys:
msg = f"SECURITY ALERT: Unmanaged key '{unmanaged_key}' detected on account '{service_account}'. This key was created outside of Beam's service account management system. "
compliance_issues.append(msg)
self.logger.warning(msg)
if iam_keys:
secret_name = f"{self._denormalize_account_email(service_account)}-key"
legal_keys = []
if secret_name in managed_secrets:
legal_keys = self._get_verified_keys_from_secret_manager(secret_name)
unmanaged_keys = set(iam_keys) - set(legal_keys)
for unmanaged_key in unmanaged_keys:
msg = f"SECURITY ALERT: Unmanaged key '{unmanaged_key}' detected on account '{service_account}'. This key was created outside of Beam's service account management system. "
compliance_issues.append(msg)
self.logger.warning(msg)

extracted_secrets = [f"{self._denormalize_account_email(account['account_id'])}-key" for account in file_service_accounts]

Expand Down Expand Up @@ -380,8 +379,9 @@

if general_issues:
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"
body = f"Unauthorized Service Accounts Report\n\n"
body += f"Account keys for project {self.project_id} are not compliant with the defined policies on {self.service_account_keys_file}\n\n"
for issue in general_issues:
body += f"- {issue}\n"

Expand All @@ -405,23 +405,44 @@
"""
if not self.sending_client:
raise ValueError("SendingClient is required for printing announcements")

diff = self.check_compliance()

if not diff:
self.logger.info("No compliance issues found, no announcement will be printed.")
return

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"
for issue in diff:
body += f"- {issue}\n"
unmanaged_keys_issues = [issue for issue in diff if "SECURITY ALERT" in issue]
general_issues = [issue for issue in diff if "SECURITY ALERT" not in issue]

if general_issues:
self.logger.info("Printing general compliance announcement...")
title = f"[SECURITY] Action Required: Unauthorized Service Accounts Detected"
body = f"Unauthorized Service Accounts Report\n\n"
body += f"Account keys for project {self.project_id} are not compliant with the defined policies on {self.service_account_keys_file}\n\n"
for issue in general_issues:
body += f"- {issue}\n"

announcement = f"Dear team,\n\nThis is an automated notification about compliance issues detected in the Account Keys policy for project {self.project_id}.\n\n"
announcement += f"We found {len(general_issues)} compliance issue(s) that need your attention.\n"
announcement += f"\nPlease check the GitHub issue for detailed information and take appropriate action to resolve these compliance violations."

announcement = f"Dear team,\n\nThis is an automated notification about compliance issues detected in the Account Keys policy for project {self.project_id}.\n\n"
announcement += f"We found {len(diff)} compliance issue(s) that need your attention.\n"
announcement += f"\nPlease check the GitHub issue for detailed information and take appropriate action to resolve these compliance violations."
self.sending_client.print_announcement(title, body, recipient, announcement)

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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")

print("\n" + "="*60)
print("SIMULATING GITHUB SECURITY ISSUE CREATION/UPDATE")
print("="*60)
print("Title: [SECURITY] Action Required: Unmanaged Service Account Keys Detected\n")
print(f"Body:\n### Unmanaged Keys Audit Report ({timestamp})")
print(f"The following unauthorized or unmanaged keys were detected in `{self.project_id}`:\n")
for issue in unmanaged_keys_issues:
print(f"- {issue}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (secret)
as clear text.
This expression logs
sensitive data (secret)
as clear text.
This expression logs
sensitive data (secret)
as clear text.
print("\n*Please investigate and revoke these keys if they are not part of the official rotation system.*\n")
print("### History\n<details>\n<summary>Click to expand</summary>\n\n[... Previous reports would be collapsed here ...]\n</details>")
print("="*60 + "\n")

def generate_compliance(self) -> None:
"""
Expand Down
45 changes: 35 additions & 10 deletions infra/enforcement/sending.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,18 +248,36 @@ def create_announcement(self, title: str, body: str, recipient: str, announcemen
"""
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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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, timezone

And simplify this line.

Suggested change
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
  1. Imports should be placed at the top of the file, as per PEP 8 guidelines. (link)

new_report = f"### Compliance Audit Report ({timestamp})\n{body}"

if open_issues:
self.logger.info(f"Issue with title '{title}' already exists: #{open_issues[0].number}")
announcement += f"\n\nRelated GitHub Issue: {open_issues[0].html_url}"
target_issue = open_issues[0]
self.logger.info(f"Issue with title '{title}' already exists: #{target_issue.number}")
announcement += f"\n\nRelated GitHub Issue: {target_issue.html_url}"

old_body = target_issue.body or ""
history_marker = "### History\n<details>\n<summary>Click to expand</summary>\n\n"

if open_issues[0].body != body:
self.logger.info(f"Updating body of issue #{open_issues[0].number}")
self.update_issue_body(open_issues[0].number, body)
if history_marker in old_body:
# If history already exists, append the new report to it
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}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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}"

else:
self.logger.info(f"No changes detected for issue #{open_issues[0].number}")
# First time updating, turn the entire old body into history
combined_history = old_body.strip()

final_body = f"{new_report}\n\n{history_marker}{combined_history}\n</details>"

self.logger.info(f"Appending report and archiving history to existing issue #{target_issue.number}")
self.update_issue_body(target_issue.number, final_body)
self._send_email(title, announcement, recipient)
else:
new_issue = self.create_issue(title, body)
self.logger.info(f"Creating new compliance issue for: {title}")
new_issue = self.create_issue(title, new_report)
announcement += f"\n\nRelated GitHub Issue: {new_issue.html_url}"
self._send_email(title, announcement, recipient)

Expand All @@ -273,6 +291,13 @@ def print_announcement(self, title: str, body: str, recipient: str, announcement
print(f"Recipient: {recipient}")
print(f"Announcement: {announcement}")

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. Imports should be placed at the top of the file, as per PEP 8 guidelines. (link)


print("\n" + "="*60)
print("SIMULATING GITHUB GENERAL ISSUE CREATION/UPDATE")
print("="*60)
print(f"Title: {title}\n")
print(f"Body:\n### Compliance Audit Report ({timestamp})")
print(body)
print("### History\n<details>\n<summary>Click to expand</summary>\n\n[... Previous reports would be collapsed here ...]\n</details>")
print("="*60 + "\n")
Loading