HYPERFLEET-1237 - feat: add authorization header support for api_call#210
HYPERFLEET-1237 - feat: add authorization header support for api_call#210rh-amarin wants to merge 3 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds bearer-token authorization support to Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 622 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: internal/configloader internal/hyperfleetapi | +1 |
Computed by hyperfleet-risk-scorer
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… authorization Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/executor/utils.go (1)
107-127: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize header keys before overriding Authorization (CWE-178).
The override at Line 126 only replaces exact
"Authorization". A user-provided"authorization"key can survive and violate the “authorization block takes precedence” contract.Proposed fix
+import "net/textproto" @@ - for _, h := range apiCall.Headers { + for _, h := range apiCall.Headers { headerValue, headerErr := utils.RenderTemplate(h.Value, execCtx.Params) if headerErr != nil { return nil, url, fmt.Errorf("failed to render header '%s' template: %w", h.Name, headerErr) } - headers[h.Name] = headerValue + headers[textproto.CanonicalMIMEHeaderKey(h.Name)] = headerValue } @@ - headers["Authorization"] = "Bearer " + token + headers[textproto.CanonicalMIMEHeaderKey("Authorization")] = "Bearer " + token }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/executor/utils.go` around lines 107 - 127, The code fails to handle case-insensitive HTTP header names, allowing a user-provided "authorization" or "AUTHORIZATION" header to bypass the authorization override logic (only "Authorization" is checked). To fix this, normalize all header keys to a consistent case (e.g., lowercase or title case) when populating the headers map in the loop where apiCall.Headers are processed, and apply the same normalization when setting the Authorization header at the line where headers["Authorization"] = "Bearer " + token is executed. This ensures that regardless of the case used in user-provided headers, the authorization token will properly override any conflicting header.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/kubernetes.go`:
- Around line 39-44: The code at the os.ReadFile call for mountedTokenPath and
the subsequent return string(data), nil statement does not validate that the
token is non-empty or contains only whitespace before returning it. Add
validation after reading the token data to ensure it is not empty and does not
contain only whitespace characters, returning an appropriate error if validation
fails. Apply the same validation fix at line 81 where another token is read and
returned to ensure consistent token validation across all token reading paths.
In `@internal/auth/provider_test.go`:
- Around line 83-90: The test `TestKubernetesProvider_MountedToken_MissingFile`
and `TestResolveNamespace` depend on external filesystem state (the presence or
absence of mounted ServiceAccount token files), causing them to flake in CI
environments where these files may exist. Make these tests deterministic by
either mocking or controlling the mounted token file path through temporary
directories or environment variable manipulation in the test setup, or
alternatively convert these tests to integration tests by adding the //go:build
integration build tag and moving them to the appropriate test/integration
directory as per the project standards.
In `@internal/auth/provider.go`:
- Around line 47-52: The code does not validate that the rendered token is
non-empty before returning the StaticProvider, which could result in invalid
empty bearer tokens being used. After the utils.RenderTemplate call successfully
renders the token, add validation to check that the rendered value is not empty
or whitespace-only before returning the StaticProvider instance. If validation
fails, return an appropriate error message indicating the rendered token is
empty or invalid, preventing the StaticProvider from being created with an
invalid token.
In `@internal/configloader/types.go`:
- Line 260: The ExpirationSeconds field in the struct definition currently
accepts zero and negative values without validation, deferring failure to
runtime. Add validation logic in the configuration parsing phase (where the YAML
is unmarshaled into this struct) to reject non-positive values for
ExpirationSeconds immediately at the schema boundary. This should be done by
implementing or enhancing validation in the config loader to check that if
ExpirationSeconds is provided, it must be greater than zero, ensuring invalid
configurations fail at parse time rather than at runtime token acquisition.
In `@internal/executor/post_action_executor_test.go`:
- Around line 715-721: The test case "kubernetes type without audience fails
outside cluster" is non-deterministic because it relies on the actual runner
environment, potentially passing in-cluster due to mounted SA files. Either mock
the Kubernetes authentication environment to explicitly simulate an
out-of-cluster scenario with no credentials available, or move this test to the
integration test suite and tag it with //go:build integration to make it clear
it requires specific environment conditions. This will ensure the test behavior
is consistent and deterministic regardless of where it runs.
In `@internal/executor/utils.go`:
- Line 118: The call to auth.NewTokenProvider on line 118 dereferences
execCtx.Config.Clients.Kubernetes without first checking if execCtx or
execCtx.Config are nil, which will cause a panic if either is nil. Add guard
checks before this line to verify that execCtx is not nil and that
execCtx.Config is not nil, and return an appropriate error if either check fails
before attempting to access the nested Kubernetes field.
---
Outside diff comments:
In `@internal/executor/utils.go`:
- Around line 107-127: The code fails to handle case-insensitive HTTP header
names, allowing a user-provided "authorization" or "AUTHORIZATION" header to
bypass the authorization override logic (only "Authorization" is checked). To
fix this, normalize all header keys to a consistent case (e.g., lowercase or
title case) when populating the headers map in the loop where apiCall.Headers
are processed, and apply the same normalization when setting the Authorization
header at the line where headers["Authorization"] = "Bearer " + token is
executed. This ensures that regardless of the case used in user-provided
headers, the authorization token will properly override any conflicting header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 52dadf88-bf6a-4158-b158-b99e5ef120c4
📒 Files selected for processing (9)
docs/adapter-authoring-guide.mdinternal/auth/kubernetes.gointernal/auth/provider.gointernal/auth/provider_test.gointernal/configloader/types.gointernal/configloader/validator.gointernal/executor/post_action_executor_test.gointernal/executor/utils.gointernal/hyperfleetapi/mock.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| data, err := os.ReadFile(mountedTokenPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read mounted ServiceAccount token from %s: %w", mountedTokenPath, err) | ||
| } | ||
| return string(data), nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate Kubernetes-issued/mounted tokens are non-empty before return (CWE-20).
Line 39-44 and Line 81 can return empty/whitespace tokens, which later produce malformed bearer auth.
Suggested fix
import (
"context"
"fmt"
"os"
+ "strings"
@@
func readMountedToken() (string, error) {
data, err := os.ReadFile(mountedTokenPath)
if err != nil {
return "", fmt.Errorf("failed to read mounted ServiceAccount token from %s: %w", mountedTokenPath, err)
}
- return string(data), nil
+ token := strings.TrimSpace(string(data))
+ if token == "" {
+ return "", fmt.Errorf("mounted ServiceAccount token is empty at %s", mountedTokenPath)
+ }
+ return token, nil
}
@@
- return tr.Status.Token, nil
+ token := strings.TrimSpace(tr.Status.Token)
+ if token == "" {
+ return "", fmt.Errorf("token request returned empty token for ServiceAccount %s/%s (audience=%s)",
+ p.namespace, p.serviceAccount, p.audience)
+ }
+ return token, nilAs per path instructions: “Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers).”
Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/kubernetes.go` around lines 39 - 44, The code at the
os.ReadFile call for mountedTokenPath and the subsequent return string(data),
nil statement does not validate that the token is non-empty or contains only
whitespace before returning it. Add validation after reading the token data to
ensure it is not empty and does not contain only whitespace characters,
returning an appropriate error if validation fails. Apply the same validation
fix at line 81 where another token is read and returned to ensure consistent
token validation across all token reading paths.
Source: Path instructions
| // TestKubernetesProvider_MountedToken_MissingFile verifies the error when | ||
| // the mounted SA token file is absent (expected outside a real cluster). | ||
| func TestKubernetesProvider_MountedToken_MissingFile(t *testing.T) { | ||
| p := &KubernetesProvider{audience: ""} // no audience → reads mounted file | ||
| _, err := p.GetToken(context.Background()) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to read mounted ServiceAccount token") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unit tests rely on “outside-cluster” filesystem state and can flake in CI.
TestKubernetesProvider_MountedToken_MissingFile and TestResolveNamespace assume mounted ServiceAccount files are absent. In in-cluster runners, those files exist, so these assertions invert.
Deterministic guard option
import (
"context"
+ "os"
"testing"
@@
func TestKubernetesProvider_MountedToken_MissingFile(t *testing.T) {
+ if _, statErr := os.Stat(mountedTokenPath); statErr == nil {
+ t.Skip("mounted ServiceAccount token exists in this environment")
+ }
p := &KubernetesProvider{audience: ""} // no audience → reads mounted file
_, err := p.GetToken(context.Background())
@@
t.Run("empty namespace reads mounted file — fails outside cluster", func(t *testing.T) {
+ if _, statErr := os.Stat(mountedNamespacePath); statErr == nil {
+ t.Skip("mounted namespace file exists in this environment")
+ }
_, err := resolveNamespace("")As per path instructions, “Integration tests tagged with //go:build integration or in test/integration/.”
Also applies to: 127-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/provider_test.go` around lines 83 - 90, The test
`TestKubernetesProvider_MountedToken_MissingFile` and `TestResolveNamespace`
depend on external filesystem state (the presence or absence of mounted
ServiceAccount token files), causing them to flake in CI environments where
these files may exist. Make these tests deterministic by either mocking or
controlling the mounted token file path through temporary directories or
environment variable manipulation in the test setup, or alternatively convert
these tests to integration tests by adding the //go:build integration build tag
and moving them to the appropriate test/integration directory as per the project
standards.
Source: Path instructions
| rendered, err := utils.RenderTemplate(auth.Token, params) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to render static token template: %w", err) | ||
| } | ||
| return &StaticProvider{token: rendered}, nil | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject empty rendered static tokens before use (CWE-20, CWE-287).
Line 47-52 returns a static provider even when the rendered token is empty/whitespace. That can silently emit Authorization: Bearer .
Suggested fix
import (
"bytes"
"context"
"fmt"
"os"
+ "strings"
@@
case "static":
rendered, err := utils.RenderTemplate(auth.Token, params)
if err != nil {
return nil, fmt.Errorf("failed to render static token template: %w", err)
}
- return &StaticProvider{token: rendered}, nil
+ token := strings.TrimSpace(rendered)
+ if token == "" {
+ return nil, fmt.Errorf("static authorization token rendered empty")
+ }
+ return &StaticProvider{token: token}, nilAs per path instructions: “Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rendered, err := utils.RenderTemplate(auth.Token, params) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to render static token template: %w", err) | |
| } | |
| return &StaticProvider{token: rendered}, nil | |
| rendered, err := utils.RenderTemplate(auth.Token, params) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to render static token template: %w", err) | |
| } | |
| token := strings.TrimSpace(rendered) | |
| if token == "" { | |
| return nil, fmt.Errorf("static authorization token rendered empty") | |
| } | |
| return &StaticProvider{token: token}, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/provider.go` around lines 47 - 52, The code does not validate
that the rendered token is non-empty before returning the StaticProvider, which
could result in invalid empty bearer tokens being used. After the
utils.RenderTemplate call successfully renders the token, add validation to
check that the rendered value is not empty or whitespace-only before returning
the StaticProvider instance. If validation fails, return an appropriate error
message indicating the rendered token is empty or invalid, preventing the
StaticProvider from being created with an invalid token.
Source: Path instructions
| // The generated token is set as "Authorization: Bearer <token>", overriding any Authorization | ||
| // header specified in the headers list. | ||
| type APICallAuth struct { | ||
| ExpirationSeconds *int64 `yaml:"expiration_seconds,omitempty"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce positive expiration_seconds at config-parse time (CWE-20).
Line 260 accepts 0/negative values today, which defers failure to runtime token acquisition. Reject this at the schema boundary.
Suggested fix
- ExpirationSeconds *int64 `yaml:"expiration_seconds,omitempty"`
+ ExpirationSeconds *int64 `yaml:"expiration_seconds,omitempty" validate:"omitempty,gt=0"`As per path instructions: “Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ExpirationSeconds *int64 `yaml:"expiration_seconds,omitempty"` | |
| ExpirationSeconds *int64 `yaml:"expiration_seconds,omitempty" validate:"omitempty,gt=0"` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/configloader/types.go` at line 260, The ExpirationSeconds field in
the struct definition currently accepts zero and negative values without
validation, deferring failure to runtime. Add validation logic in the
configuration parsing phase (where the YAML is unmarshaled into this struct) to
reject non-positive values for ExpirationSeconds immediately at the schema
boundary. This should be done by implementing or enhancing validation in the
config loader to check that if ExpirationSeconds is provided, it must be greater
than zero, ensuring invalid configurations fail at parse time rather than at
runtime token acquisition.
Source: Path instructions
| name: "kubernetes type without audience fails outside cluster", | ||
| authorization: &configloader.APICallAuth{ | ||
| Type: "kubernetes", | ||
| }, | ||
| params: map[string]interface{}{}, | ||
| expectError: true, | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Outside-cluster assumption makes this test non-deterministic.
The case “kubernetes type without audience fails outside cluster” can pass or fail based on runner environment. In-cluster it may succeed using mounted SA files.
Deterministic failure setup
{
- name: "kubernetes type without audience fails outside cluster",
+ name: "kubernetes token request fails with invalid kubeconfig",
authorization: &configloader.APICallAuth{
- Type: "kubernetes",
+ Type: "kubernetes",
+ Audience: "target-service",
+ ServiceAccount: "adapter-sa",
+ Namespace: "hyperfleet",
},
params: map[string]interface{}{},
expectError: true,
},
@@
- execCtx := NewExecutionContext(context.Background(), map[string]interface{}{}, &configloader.Config{})
+ cfg := &configloader.Config{}
+ if tt.authorization != nil && tt.authorization.Type == "kubernetes" {
+ cfg.Clients.Kubernetes.KubeConfigPath = "/definitely/missing/kubeconfig"
+ }
+ execCtx := NewExecutionContext(context.Background(), map[string]interface{}{}, cfg)
execCtx.Params = tt.paramsAs per path instructions, “Integration tests tagged with //go:build integration or in test/integration/.”
Also applies to: 736-737
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/post_action_executor_test.go` around lines 715 - 721, The
test case "kubernetes type without audience fails outside cluster" is
non-deterministic because it relies on the actual runner environment,
potentially passing in-cluster due to mounted SA files. Either mock the
Kubernetes authentication environment to explicitly simulate an out-of-cluster
scenario with no credentials available, or move this test to the integration
test suite and tag it with //go:build integration to make it clear it requires
specific environment conditions. This will ensure the test behavior is
consistent and deterministic regardless of where it runs.
Source: Path instructions
|
|
||
| // Resolve authorization token — always wins over any manually set Authorization header. | ||
| if apiCall.Authorization != nil { | ||
| provider, authErr := auth.NewTokenProvider(apiCall.Authorization, execCtx.Config.Clients.Kubernetes, execCtx.Params) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard auth config dereference to avoid panic (CWE-476).
Line 118 dereferences execCtx.Config.Clients.Kubernetes without checking execCtx/execCtx.Config. If authorization is enabled with a nil config path, this will panic instead of returning an error.
Proposed fix
- provider, authErr := auth.NewTokenProvider(apiCall.Authorization, execCtx.Config.Clients.Kubernetes, execCtx.Params)
+ var k8sCfg configloader.KubernetesConfig
+ if execCtx != nil && execCtx.Config != nil {
+ k8sCfg = execCtx.Config.Clients.Kubernetes
+ }
+ provider, authErr := auth.NewTokenProvider(apiCall.Authorization, k8sCfg, execCtx.Params)As per path instructions, “Flag nil/bounds access without guards.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/utils.go` at line 118, The call to auth.NewTokenProvider on
line 118 dereferences execCtx.Config.Clients.Kubernetes without first checking
if execCtx or execCtx.Config are nil, which will cause a panic if either is nil.
Add guard checks before this line to verify that execCtx is not nil and that
execCtx.Config is not nil, and return an appropriate error if either check fails
before attempting to access the nested Kubernetes field.
Source: Path instructions
|
@rh-amarin: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
authorizationblock to theapi_callconfig (task config level, per-call) that injects anAuthorization: Bearer <token>headertype: static— token is a Go template string rendered against params (e.g.{{ .apiToken }}sourced from env via params config)type: kubernetes— reads the pod's mounted ServiceAccount token by default; calls the Kubernetes TokenRequest API whenaudienceis set (bound, short-lived token for a specific audience)Authorizationheader in theheaders:listWhat changed
internal/configloader/types.goAPICallAuthstruct +Authorizationfield onAPICallinternal/configloader/validator.gostaticrequirestoken;kubernetes+audiencerequiresservice_accountinternal/executor/utils.goExecuteAPICallafter header renderinginternal/auth/provider.goTokenProviderinterface,StaticProvider,NewTokenProviderfactoryinternal/auth/kubernetes.goKubernetesProvider: mounted-file or TokenRequest APIinternal/hyperfleetapi/mock.goRequestOptionfuncs (enables header assertions in tests)Example config
Test plan
go test ./internal/auth/...— 8 new unit tests (static rendering, TokenRequest via fake clientset, namespace resolution, error paths)go test ./internal/executor/... -run TestExecuteAPICall_Authorization— 5 new cases (static, template, override, error, kubernetes outside cluster)make test— all existing tests passmake lint— no new lint issuesRefs: HYPERFLEET-1237
🤖 Generated with Claude Code