-
Notifications
You must be signed in to change notification settings - Fork 260
fix: auto-detect host SA from metadata + accept default Compute SA #1482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,11 @@ const ( | |
| ) | ||
|
|
||
| // isValidServiceAccountEmail validates that an email address looks like a | ||
| // GCP service account email: <name>@<project>.iam.gserviceaccount.com. | ||
| // GCP service account email. Accepted formats: | ||
| // | ||
| // - Custom IAM SA: <name>@<project>.iam.gserviceaccount.com | ||
| // - Default Compute SA: <number>-compute@developer.gserviceaccount.com | ||
| // - App Engine default: <project-id>@appspot.gserviceaccount.com | ||
| func isValidServiceAccountEmail(email string) bool { | ||
| at := strings.IndexByte(email, '@') | ||
| if at <= 0 { | ||
|
|
@@ -51,14 +55,20 @@ func isValidServiceAccountEmail(email string) bool { | |
| return false | ||
| } | ||
|
|
||
| suffix := ".iam.gserviceaccount.com" | ||
| if !strings.HasSuffix(domain, suffix) { | ||
| switch { | ||
| case strings.HasSuffix(domain, ".iam.gserviceaccount.com"): | ||
| // Custom IAM SA: project ID portion must be non-empty. | ||
| projectID := domain[:len(domain)-len(".iam.gserviceaccount.com")] | ||
| return len(projectID) > 0 | ||
| case domain == "developer.gserviceaccount.com": | ||
| // Default Compute Engine SA (e.g. <project-number>-compute@developer.gserviceaccount.com). | ||
| return true | ||
| case domain == "appspot.gserviceaccount.com": | ||
| // App Engine default SA (e.g. <project-id>@appspot.gserviceaccount.com). | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
|
Comment on lines
+58
to
71
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While adding support for Consider also accepting switch {
case strings.HasSuffix(domain, ".iam.gserviceaccount.com"):
// Custom IAM SA: project ID portion must be non-empty.
projectID := domain[:len(domain)-len(".iam.gserviceaccount.com")]
return len(projectID) > 0
case domain == "developer.gserviceaccount.com":
// Default Compute Engine SA (e.g. <project-number>-compute@developer.gserviceaccount.com).
return true
case domain == "appspot.gserviceaccount.com":
// App Engine default SA (e.g. <project-id>@appspot.gserviceaccount.com).
return true
default:
return false
} |
||
|
|
||
| // Project ID portion must be non-empty. | ||
| projectID := domain[:len(domain)-len(suffix)] | ||
| return len(projectID) > 0 | ||
| } | ||
|
|
||
| // authorizePassthroughIdentity gates passthrough mode for a caller against a | ||
|
|
@@ -146,9 +156,19 @@ func (s *Server) authorizePassthroughIdentity( | |
| // Synthesize a transient store.GCPServiceAccount target for the broker | ||
| // host SA. This is not persisted — it exists only as the target shape | ||
| // required by the frozen checker interface. | ||
| // | ||
| // For default Compute Engine SAs (@developer.gserviceaccount.com) and | ||
| // App Engine default SAs (@appspot.gserviceaccount.com), the project ID | ||
| // cannot be reliably extracted from the email (Compute SA emails | ||
| // contain the project NUMBER, not the project ID). The auto-detect | ||
| // path in registerGlobalProjectAndBroker sets GCPHostProjectID from | ||
| // the metadata server, which returns the correct project ID. | ||
| hostProjectID := broker.GCPHostProjectID | ||
| if hostProjectID == "" { | ||
| // Derive from the email when the operator did not set it explicitly. | ||
| // This only works for custom IAM SAs (<name>@<project>.iam.gserviceaccount.com). | ||
| // Default Compute SAs and App Engine SAs require GCPHostProjectID to be | ||
| // set explicitly (via auto-detection or manual configuration). | ||
| hostProjectID = projectIDFromServiceAccountEmail(broker.GCPHostServiceAccountEmail) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For
@developer.gserviceaccount.comservice accounts, the project ID cannot be derived from the email address because the email contains the project number (e.g.,721899303052-compute), not the project ID (alphanumeric string), and the domain does not encode the project ID.If
GCPHostProjectIDis left empty, the fallbackprojectIDFromServiceAccountEmail(called on line 158) will fail to extract a valid project ID, leading to failed GCP IAM checks.Consider adding a validation check in the broker update/registration path to ensure
GCPHostProjectIDis explicitly provided when a@developer.gserviceaccount.comservice account is used, or updateprojectIDFromServiceAccountEmailto explicitly handle or fail for this domain.