feat(remote-tasks): gate remote annotation URLs#2745
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2745 +/- ##
==========================================
- Coverage 59.67% 59.46% -0.21%
==========================================
Files 210 211 +1
Lines 21007 21404 +397
==========================================
+ Hits 12536 12728 +192
- Misses 7685 7853 +168
- Partials 786 823 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 introduces security controls for fetching remote tasks and pipelines in Pipelines-as-Code. It adds configuration options for a host allowlist, blocked CIDRs, and a maximum response size limit, along with documentation and comprehensive tests. The review feedback highlights two critical security vulnerabilities in the remote resource client implementation: a potential SSRF bypass if the HTTP client uses a custom wrapped transport, and another bypass for HTTPS requests if DialTLSContext or DialTLS is configured on the transport. Actionable suggestions are provided to address both issues by falling back to a default transport and clearing TLS dialers to guarantee security enforcement.
chmouel
force-pushed
the
fix-remote-annotation-url-policy
branch
5 times, most recently
from
44f707e to
2ce187b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces security controls for fetching remote tasks and pipelines via annotations in Pipelines-as-Code. It adds new configuration options to allowlist hosts, block specific CIDRs, and limit response sizes, alongside implementing validation logic to prevent unsafe fetches (such as loopback, private IPs, and unauthorized redirects). Documentation and tests have been updated accordingly. Feedback on the changes highlights that if a custom HTTP client's transport is not of type *http.Transport, it is silently replaced with http.DefaultTransport, which could break custom transport configurations in enterprise environments; logging a warning when this fallback occurs is recommended.
| remote-tasks-url-blocked-cidrs: "" | ||
|
|
||
| # Maximum response size, in bytes, for remote annotation fetches. | ||
| remote-tasks-url-max-response-size: "1048576" |
There was a problem hiding this comment.
I don't like exposing low level stuff as configuration but if for whatever reason someone has a task/prun bigger than 1mb to fetch then we let them fetch it
There was a problem hiding this comment.
Pull request overview
This PR adds SSRF-focused safety controls for fetching remote Tekton resources referenced via Pipelines-as-Code annotations, by introducing an operator-configurable allowlist, additional IP-range blocking, and maximum response-size enforcement (including for provider-backed fetch paths).
Changes:
- Introduces a guarded remote resource fetcher (
GetRemoteResourceURL) with scheme/allowlist enforcement, DNS/IP-range blocks, redirect checks, and response-size limits. - Adds new
pac-configsettings (remote-tasks-url-allowlist,remote-tasks-url-blocked-cidrs,remote-tasks-url-max-response-size) with validation, docs, and default configmap entries. - Updates unit/integration tests and fixtures to use HTTPS defaults and to cover allowlist/size-limit behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/gitea_test.go |
Adds an integration test asserting allowlist enforcement surfaces as a failure comment. |
pkg/test/provider/testwebvcs.go |
Extends the test provider to return provider-fetched remote task content. |
pkg/test/http/http.go |
Adds a transport-backed test HTTP client to exercise real http.Transport behavior and dial controls. |
pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml |
Switches remote annotation URLs to HTTPS in testdata. |
pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-pipeline-annotation.yaml |
Switches remote annotation URLs to HTTPS in testdata. |
pkg/resolve/testdata/skip-fetching-multiple-pipelines-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml |
Switches remote annotation URLs to HTTPS in testdata. |
pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipelinerun.yaml |
Switches remote annotation URLs to HTTPS in testdata. |
pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipeline.yaml |
Switches remote annotation URLs to HTTPS in testdata. |
pkg/resolve/testdata/remote-pipeline-with-relative-tasks.yaml |
Switches remote pipeline URL to HTTPS in testdata. |
pkg/resolve/testdata/remote-pipeline-with-relative-tasks-same-pipeline.yaml |
Switches remote pipeline URL to HTTPS in testdata. |
pkg/resolve/testdata/remote-pipeline-with-relative-tasks-1.yaml |
Switches remote pipeline URL to HTTPS in testdata. |
pkg/resolve/remote_test.go |
Updates remote resolution tests to use HTTPS and the new transport-backed HTTP test client. |
pkg/params/settings/config.go |
Adds new settings fields and validators for allowlist/CIDRs/max-size. |
pkg/params/settings/config_test.go |
Adds coverage for the new settings validation behavior. |
pkg/params/clients/remote_resource.go |
Implements remote URL policy validation and safe fetching (redirect validation, DNS/IP checks, size limits). |
pkg/params/clients/clients.go |
Adds a default max response size constant for remote resources. |
pkg/params/clients/clients_test.go |
Adds tests for allowlist behavior, IP blocking, redirects, and transport guarding. |
pkg/matcher/annotation_tasks_install.go |
Routes remote annotation fetches through the new guarded remote resource client and enforces provider-path size limits. |
pkg/matcher/annotation_tasks_install_test.go |
Expands tests for allowlist enforcement, response-size checks, and transport-backed URL fetching. |
docs/content/docs/guides/pipeline-resolution/_index.md |
Updates docs to describe “Remote URL” behavior and http-vs-https allowlisting. |
docs/content/docs/guides/creating-pipelines/_index.md |
Updates docs links/wording to match the new “Remote URL” section. |
docs/content/docs/api/configmap.md |
Documents the new configmap keys and provides examples/defaults. |
config/302-pac-configmap.yaml |
Adds the new remote URL policy configuration entries with inline comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chmouel
changed the title
Check remote task and pipeline URLs before fetching them. HTTPS URLs continue to work by default, and operators can restrict them with the remote task URL allowlist. HTTP URLs stay blocked unless an operator explicitly allows that host with an http:// allowlist entry. Apply the same checks before same-provider fetches, and keep large provider responses from being parsed as pipeline resources. Jira: https://redhat.atlassian.net/browse/SRVKP-12181 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
chmouel
force-pushed
the
fix-remote-annotation-url-policy
branch
from
2ce187b to
ab12ebd
Compare
| if strings.HasPrefix(loweredURI, "http://") || strings.HasPrefix(loweredURI, "https://") { | ||
| if err := clients.ValidateRemoteResourceProviderURL(uri, fetchOptions); err != nil { | ||
| return "", err | ||
| } | ||
| if fetchedFromURIFromProvider, task, err := rt.ProviderInterface.GetTaskURI(ctx, rt.Event, uri); fetchedFromURIFromProvider { | ||
| rt.Logger.Debugf("getRemote: fetched %s via provider hook for uri=%s", kind, uri) | ||
| if err != nil { | ||
| return task, err | ||
| } | ||
| if err := clients.CheckRemoteResourceResponseSize(int64(len(task)), fetchOptions); err != nil { | ||
| return "", err | ||
| } | ||
| return task, nil | ||
| } | ||
| rt.Logger.Debugf("getRemote: fetching %s from http(s) url", kind) | ||
| data, err := rt.Run.Clients.GetURL(ctx, uri) | ||
| data, err := rt.Run.Clients.GetRemoteResourceURL(ctx, uri, fetchOptions) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| rt.Logger.Infof("successfully fetched %s from remote HTTPS URL", uri) | ||
| rt.Logger.Infof("successfully fetched %s from remote URL", uri) | ||
| return string(data), nil | ||
| } | ||
| if fetchedFromURIFromProvider, task, err := rt.ProviderInterface.GetTaskURI(ctx, rt.Event, uri); fetchedFromURIFromProvider { | ||
| rt.Logger.Debugf("getRemote: fetched %s via provider hook for uri=%s", kind, uri) | ||
| if err != nil { | ||
| return task, err | ||
| } | ||
| if err := clients.CheckRemoteResourceResponseSize(int64(len(task)), fetchOptions); err != nil { | ||
| return "", err | ||
| } | ||
| return task, nil | ||
| } |
There was a problem hiding this comment.
Why not create the policy as part of remoteResourceFetchOptions?
It is a small overhead but the same newRemoteResourcePolicy call is being made multiple times by the clients.Func methods.
| if err != nil { | ||
| return task, err | ||
| } |
There was a problem hiding this comment.
| if err != nil { | |
| return task, err | |
| } |
There was a problem hiding this comment.
if fetchedFromURIFromProvider is true, then there is no chance of err being non-nil
| return "", err | ||
| } | ||
| return task, nil | ||
| } |
There was a problem hiding this comment.
| } | |
| } else if err != nil { | |
| return task, err | |
| } |
| rt.Logger.Infof("successfully fetched %s from remote URL", uri) | ||
| return string(data), nil | ||
| } | ||
| if fetchedFromURIFromProvider, task, err := rt.ProviderInterface.GetTaskURI(ctx, rt.Event, uri); fetchedFromURIFromProvider { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces security controls for fetching remote tasks and pipelines in Pipelines-as-Code. It adds configuration options to allowlist specific hosts, block specific CIDRs, and limit the maximum response size for remote annotation fetches. It also implements safe URL validation, including blocking loopback, private, and multicast IP ranges by default, and enforces these policies during HTTP requests and redirects. There are no review comments, so no feedback is provided.
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.
| safeTransport := baseTransport.Clone() | ||
| originalDialContext := safeTransport.DialContext | ||
| if originalDialContext == nil { | ||
| originalDialContext = (&net.Dialer{Timeout: ConnectMaxWaitTime}).DialContext | ||
| } |
| httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { | ||
| if len(via) >= 10 { | ||
| return fmt.Errorf("stopped after 10 redirects") | ||
| } | ||
| if err := policy.validateURL(req.URL); err != nil { | ||
| return fmt.Errorf("blocked redirect target: %w", err) | ||
| } | ||
| return nil | ||
| } |
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return fmt.Errorf("host %q must use http:// or https:// when including a URL scheme", host) | ||
| } | ||
| if parsedURL.User != nil || parsedURL.RawQuery != "" || parsedURL.Fragment != "" || (parsedURL.Path != "" && parsedURL.Path != "/") { | ||
| return fmt.Errorf("host %q must not include a URL path, query, fragment, or userinfo", host) | ||
| } | ||
| host = parsedURL.Host | ||
| } | ||
| if strings.Contains(host, "*") && !strings.HasPrefix(host, "*.") { | ||
| return fmt.Errorf("host %q has an invalid wildcard", host) | ||
| } | ||
| trimmedHost := strings.TrimPrefix(host, "*.") | ||
| if trimmedHost == "" || strings.ContainsAny(trimmedHost, "/?#") { | ||
| return fmt.Errorf("host %q is invalid", host) | ||
| } |
|
holding on this until i get time to address the comments |
📝 Description of the Change
This PR implements URL allowlisting for remote task and pipeline annotations to prevent SSRF (Server-Side Request Forgery) vulnerabilities.
Problem: Pipelines-as-Code previously fetched remote task and pipeline URLs referenced in annotations without any restrictions, creating a potential SSRF attack surface.
Solution: This change introduces an operator-configurable URL allowlist that:
remote-tasks-url-allowlistConfigMap entryThe allowlist is configured via the
remote-tasks-url-allowlistfield in thepac-configConfigMap, supporting both hostname patterns and full URL patterns with http/https schemes.🔗 Linked GitHub Issue
Fixes https://redhat.atlassian.net/browse/SRVKP-12181
🧪 Testing Strategy
Test coverage:
annotation_tasks_install_test.goandclients_test.gocovering allowlist policy enforcementremote-tasks-url-allowlistin the ConfigMap and observing enforcement behavior🤖 AI Assistance
Details: Claude AI was used to generate the majority of the code for this PR, including:
remote_resource.gomoduleAll generated code has been reviewed and verified to meet project standards. The implementation has been validated with
make testandmake lintlocally.✅ Submitter Checklist
feat:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix any issues. For an efficient workflow, I have considered installing pre-commit and runningpre-commit installto automate these checks.