Add AWS OIDC outgoing auth plugin #575
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| metaURL := fmt.Sprintf("%s/latest/meta-data/iam/security-credentials/oidc?audience=%s", MetadataHost, url.QueryEscape(aud)) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, metaURL, nil) | ||
| if err != nil { | ||
| return "", time.Time{}, err |
There was a problem hiding this comment.
The plugin calls http://169.254.169.254/latest/meta-data/iam/security-credentials/oidc?audience=..., but the IMDS iam/security-credentials API expects an attached IAM role name as a path segment and returns STS credential JSON—there is no documented audience query or oidc resource. On real EC2 hosts this GET will return 404 (or an STS credential payload instead of a JWT), so AddAuth will consistently fail to attach any token. Please point this request at a valid AWS OIDC token source or adjust parsing to the actual IMDS response.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type roleCredentials struct { | ||
| Expiration string `json:"Expiration"` | ||
| Token string `json:"Token"` | ||
| } |
There was a problem hiding this comment.
roleCredentials only captures the metadata response’s Token and Expiration, and AddAuth later sends that token directly in a header. IMDS role credentials also include AccessKeyId and SecretAccessKey, which are required to produce the SigV4 signature and to accompany the session token as X-Amz-Security-Token. Because the plugin discards the signing keys and treats the session token as a bearer value, any request to AWS APIs using this auth type will be rejected with 401/403—it never produces a valid AWS credential.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if service == "" { | ||
| service = parts[0] | ||
| } | ||
| if region == "" { | ||
| region = parts[1] |
There was a problem hiding this comment.
The auto-discovery block treats any hostname with four or more labels ending in amazonaws as service.region.amazonaws.com and blindly assigns service = parts[0] and region = parts[1]. For common S3 virtual-hosted URLs like mybucket.s3.us-west-2.amazonaws.com, this resolves to service mybucket and region s3, producing an incorrect SigV4 credential scope and signatures that AWS will reject unless callers override both params manually. The detection should only fire when the host actually matches the service.region.amazonaws.com pattern (or strip leading resource labels) instead of silently deriving invalid values for multi-label hosts.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uri := u.EscapedPath() | ||
| if uri == "" { | ||
| uri = "/" | ||
| } | ||
| return path.Clean(uri) |
There was a problem hiding this comment.
In canonicalURI we run the request path through path.Clean, which strips trailing slashes and collapses duplicate separators before signing. AWS SigV4 expects the canonical URI to reflect the actual request path (apart from dot-segment removal), including trailing or repeated slashes; otherwise the server computes a different canonical request and rejects the signature. Any outbound call with a path ending in / or containing doubled slashes (e.g., S3 object keys that include a trailing slash) will now fail with SignatureDoesNotMatch even though the request itself is valid.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Use the right-most service and region portions to support hosts with | ||
| // additional labels (e.g., bucket.s3.us-west-2.amazonaws.com). | ||
| serviceIdx := len(parts) - 4 | ||
| regionIdx := len(parts) - 3 | ||
| if serviceIdx >= 0 && regionIdx >= 0 { |
There was a problem hiding this comment.
The service/region auto‑detection assumes the service label is always four components from the end (serviceIdx := len(parts) - 4), so a hostname like s3.dualstack.us-east-1.amazonaws.com is interpreted as service dualstack instead of s3. With region/service left blank (the documented default), SigV4 signs requests against the wrong service and AWS responds with SignatureDoesNotMatch for any dualstack or similarly prefixed endpoints unless callers manually override the service parameter.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uri := u.EscapedPath() | ||
| if uri == "" { | ||
| uri = "/" | ||
| } | ||
| if !strings.HasPrefix(uri, "/") { |
There was a problem hiding this comment.
canonicalURI currently returns the raw EscapedPath without removing ./.. segments, so a request like /foo/../bar is signed over the literal path. SigV4 canonicalization requires RFC3986 normalization (including collapsing dot-segments) before computing the canonical request; AWS will canonicalize to /bar, yielding a different string-to-sign and signature failure for any URL containing relative segments. Consider cleaning the path before hashing so such requests can be authenticated.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // HTTPClient is used for all metadata requests. | ||
| var HTTPClient = &http.Client{Timeout: 5 * time.Second} |
There was a problem hiding this comment.
The metadata client is instantiated with the default transport, which honors HTTP(S)_PROXY (lines 36-37). In environments where those env vars are set—a common case on EC2—the IMDS calls will be routed through the proxy, leaking the session token/credentials and often failing to reach 169.254.169.254 at all. AWS SDKs explicitly disable proxies for IMDS requests; the plugin should do the same (e.g., set a transport with Proxy: nil) to avoid credential exposure and spurious failures.
Useful? React with 👍 / 👎.
| host = strings.Split(host, ":")[0] // strip port if present | ||
| parts := strings.Split(host, ".") | ||
| if len(parts) >= 4 && parts[len(parts)-2] == "amazonaws" { | ||
| // Use the right-most service and region portions to support hosts with |
There was a problem hiding this comment.
The hostname parser only infers region/service when the penultimate label is exactly amazonaws (lines 226-229). Standard AWS China endpoints such as s3.cn-north-1.amazonaws.com.cn have com as the penultimate label, so autodiscovery fails and AddAuth returns the "requires region and service" error even though the hostname follows the documented AWS pattern. Broader domain matching (e.g., handling amazonaws.com.cn and other partitions) is needed to make region/service inference work outside the default AWS partition.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| canonicalURI, | ||
| canonicalQuery, | ||
| canonicalHeaders, | ||
| "", | ||
| signedHeaders, |
There was a problem hiding this comment.
The canonical request is built by joining the headers with an explicit empty string even though canonicalizeHeaders already terminates canonicalHeaders with a newline. That yields two blank lines between the canonical headers and SignedHeaders, so the hash used in Signature= does not match what AWS computes (SigV4 expects exactly one blank line). Any request signed by this plugin will be rejected with a signature-mismatch error until the extra newline is removed.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| r.Body = io.NopCloser(bytes.NewReader(body)) |
There was a problem hiding this comment.
When hashing the payload in readBody, the code reads the current r.Body, replaces it with a new buffer, but never closes the original io.ReadCloser. For requests whose bodies come from files or streaming readers, this leaves the underlying resource open because http.Client will only close the new NopCloser, leading to file-descriptor leaks or hung streams whenever the plugin signs such requests. Close the original body before replacing it.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(parts) >= 4 && parts[len(parts)-2] == "amazonaws" { | ||
| // Use the right-most service and region portions to support hosts with | ||
| // additional labels (e.g., bucket.s3.us-west-2.amazonaws.com). | ||
| serviceIdx := len(parts) - 4 | ||
| regionIdx := len(parts) - 3 |
There was a problem hiding this comment.
Region/service autodetection only runs when the penultimate host label equals amazonaws. Hosts in the China partition follow the standard pattern service.region.amazonaws.com.cn, so parts[len(parts)-2] is com and this branch is skipped. When users omit explicit params (as suggested in the docs) for endpoints like s3.cn-north-1.amazonaws.com.cn, AddAuth returns an error that region and service are required and the request is never signed. Consider handling the .amazonaws.com.cn suffix so autodiscovery works for CN endpoints too.
Useful? React with 👍 / 👎.
Summary
Testing
Codex Task