Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vinokurig 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 |
… providers Restrict URL fetching to http/https schemes only, rejecting file://, ftp://, jar:, and other schemes that could be exploited to read local files or access cloud metadata endpoints. Also fix incorrect null-check in ScmService that validated `repository` twice instead of `filePath`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Docker image build succeeded: quay.io/eclipse/che-server:pr-1048 kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/cheServer/deployment", "value": {containers: [{image: "quay.io/eclipse/che-server:pr-1048", name: che}]}}]" |
|
/retest |
…forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Docker image build succeeded: quay.io/eclipse/che-server:pr-1048 kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/cheServer/deployment", "value": {containers: [{image: "quay.io/eclipse/che-server:pr-1048", name: che}]}}]" |
1 similar comment
|
Docker image build succeeded: quay.io/eclipse/che-server:pr-1048 kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/cheServer/deployment", "value": {containers: [{image: "quay.io/eclipse/che-server:pr-1048", name: che}]}}]" |
|
/che-ai-assistant help |
|
Hi! I'm che-ai-assistant — I help with your pull requests. I check for new comments every 10m0s, so there may be a short delay before I respond. Available commands:
|
|
/che-ai-assistant ok-pr-review Task completed. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Review summary
The scheme restriction is solid work. It is placed at the shared formatUrl chokepoint so all six provider subclasses inherit it, URLFetcher re-checks as deliberate defence in depth, and the rejection tests use expectedExceptionsMessageRegExp rather than bare exception types. shouldStillResolveRelativePaths / shouldStillResolveRelativePathsWithDotSlash are good regression guards, and checkFileSchemeIsRejectedSafely correctly covers the fetchSafely wrapper separately.
The SSRF objective in the title is not met yet, though. The host check added in c3a741e4 is bypassable via HTTP redirects, and the repo's own scanner agrees: CodeQL alert #100 (java/ssrf, critical) is still state: open on refs/pull/1048/head at URLFetcher.java:142, and the CodeQL check run on the head SHA is red. Details inline.
Blocking
- Redirects bypass
validateUrlTarget(URLFetcher.java:142). Verified by execution. Chained with theBearer <PAT>header attached atAuthorizingFileContentProvider:97, this leaks the user's SCM token to an attacker-chosen host, not just an internal response body. fc00::/7is not blocked (URLFetcher.java:207).isSiteLocalAddress()only matches the deprecatedfec0::/10.fd00::/8is the default Service/Pod CIDR on IPv6 and dual-stack clusters.
Please resolve or document before merge
- Blocking site-local addresses breaks self-hosted SCM on RFC1918 with no allowlist, no flag, and an empty Release Notes section (URLFetcher.java:209).
InetAddress.getByNamesits outside the connect/read timeout budget and is a thread-exhaustion path under DNS degradation (URLFetcher.java:199).validateUrlTargethas no test coverage at all.
Two cross-cutting items that did not fit on a line
- No port restriction.
http://public-host:6379/or:11211is still permitted. Given the threat model includes metadata endpoints, restricting to 80/443 or the configured provider ports is cheap. - Rejection messages are mangled before users see them.
AuthorizingFileContentProvider:141callsgetDevfileConnectionErrorMessage(exception.getMessage()), and that helper isString.format("Could not reach devfile at %s", location)- the parameter is a location, not a message. Afile:URL currently producesCould not reach devfile at Only http and https URLs are allowed, got: file in URL file:///etc/passwd. Pre-existing, but this PR is what routes the new wording through it. Also worth redacting user-supplied URLs in these messages:URLFileContentProvider.getCredentialsAuthorization:83-85meanshttps://user:token@host/...is a supported input shape, and it currently lands verbatim in messages that reach API responses.
One process note
11 required ci/prow/v19-* checks are failing, but unrelated PR #1047 fails a comparable set including v19-che-smoke-test, so I am not attributing those to this PR - worth a /retest to confirm. The CodeQL check is a different matter: it is this PR's own success criterion, so I would not merge on a green build alone while that one is red and the alert it points at is open on the head ref.
The direction is right and the remaining work is bounded: redirects off (or re-validated), resolve-once-and-connect over getAllByName, an explicit CIDR table, an allowlist property, and about a dozen test cases.
Reviewed with ok-pr-review (summary / review / deep-review / impact).
| "Only http and https URLs are allowed, got: " + scheme + " in URL " + url); | ||
| } | ||
| validateUrlTarget(parsedUrl, url); | ||
| URLConnection connection = parsedUrl.openConnection(); |
There was a problem hiding this comment.
validateUrlTarget only checks the URL the caller passed in. HttpURLConnection.getFollowRedirects() defaults to true on JDK 17 and nothing here turns it off, so a 302 to 169.254.169.254 or to an in-cluster Service is followed and its body returned - never re-validated.
I reproduced this against the exact validateUrlTarget from this PR:
body read = "INTERNAL-METADATA-TOKEN"
final URL = http://127.0.0.1:45289/secret
=> redirect target was NEVER passed through validateUrlTarget.
This is very likely why CodeQL alert #100 is still open on exactly this line.
It also escalates past blind SSRF: AuthorizingFileContentProvider:97 attaches Authorization: Bearer <user's SCM PAT>, and HttpURLConnection keeps headers set via setRequestProperty across a same-scheme redirect to a different host. The attacker gets the token, not just the response.
The repo already does this elsewhere (IoUtil.java:258, OpenShiftOAuthAuthenticator.java:73):
| URLConnection connection = parsedUrl.openConnection(); | |
| URLConnection connection = parsedUrl.openConnection(); | |
| if (connection instanceof HttpURLConnection) { | |
| ((HttpURLConnection) connection).setInstanceFollowRedirects(false); | |
| } |
If redirects need to keep working, re-run the policy on each hop and drop Authorization on any cross-host hop.
| if (address.isAnyLocalAddress() | ||
| || address.isLoopbackAddress() | ||
| || address.isLinkLocalAddress() | ||
| || address.isSiteLocalAddress() |
There was a problem hiding this comment.
Inet6Address.isSiteLocalAddress() only matches the deprecated fec0::/10, not fc00::/7. fd00::/8 is the conventional ULA prefix and the default Service/Pod CIDR on IPv6 and dual-stack Kubernetes clusters, so this is the IPv6 equivalent of leaving 10.0.0.0/8 open.
Verified on JDK 17:
fd00::1 -> Inet6Address fd00:0:0:0:0:0:0:1 *** ALLOWED ***
fc00::1 -> Inet6Address fc00:0:0:0:0:0:0:1 *** ALLOWED ***
fec0::1 -> Inet6Address fec0:0:0:0:0:0:0:1 BLOCKED
Also unblocked: 100.64.0.0/10 (CGNAT) and 0.0.0.0/8 other than 0.0.0.0 itself. An explicit CIDR table would be more robust than this predicate chain, and it makes each future range a data change rather than an edit to a boolean expression.
| || address.isLinkLocalAddress() | ||
| || address.isSiteLocalAddress() | ||
| || address.isMulticastAddress()) { | ||
| throw new IOException("URL host is not allowed: " + host); |
There was a problem hiding this comment.
Che explicitly supports self-hosted SCM (che.integration.bitbucket.server_endpoints in che.properties:205, GitLab Self-Managed, Gitea, Azure DevOps Server), and hosting those on RFC1918 space is the normal on-prem topology. After this change every devfile fetch against such a host fails with URL host is not allowed: <host>.
There is currently no allowlist property, no feature flag, no mention in the PR description, and the Release Notes section is empty. Could we add a config escape hatch, alongside the injectable limit this class already has?
# Comma-separated hosts or CIDRs the server may fetch devfiles from even if
# they resolve to private addresses. Empty by default.
che.factory.url_fetcher_allowed_hosts=At minimum this needs a release note and a documented workaround before it ships.
Separately, this throw is silent. LOG is declared at :51 and used at :170. Without a log line, an admin whose internal GitLab stops working after upgrade has nothing distinguishing "blocked by the new egress policy" from "SCM host is down" - and fetchSafely:74-81 turns it into a bare null, while RawDevfileUrlFactoryParameterResolver.containsYaml:76-86 discards it entirely on the raw-devfile-URL path.
LOG.warn("Rejected outbound fetch to {}: host is not permitted", host);|
|
||
| final InetAddress address; | ||
| try { | ||
| address = InetAddress.getByName(host); |
There was a problem hiding this comment.
Three separate problems land on this one line, and one change fixes all of them:
- TOCTOU. The host is resolved here and resolved again by
openConnection()at:142. A short-TTL record can return a public IP first and an internal one second. - First address only.
getByNamereturns one address. A host publishing both a public and an internal A record passes validation and can still connect to the internal one. - Unbounded latency.
InetAddress.getByNamehonours neithersetConnectTimeoutnorsetReadTimeout(both applied afterwards, at:143-144) and has no timeout of its own. Worst-casefetchlatency went fromconnect + readtoresolver + connect + read, with the first term outside anything che-server controls. With Kubernetesndots:5search expansion, and devfile resolution fanning out over several files per factory, degraded cluster DNS now stalls Tomcat request threads for a multiple of what it did before.
Resolving once via getAllByName, rejecting if any returned address is disallowed, and then connecting to the validated InetAddress (preserving the Host header) closes all three.
| private void validateUrlTarget(URL parsedUrl, String originalUrl) throws IOException { | ||
| final String host; | ||
| try { | ||
| host = new URI(parsedUrl.toString()).getHost(); |
There was a problem hiding this comment.
parsedUrl was already constructed successfully by new URL(...) at :135, so new URI(parsedUrl.toString()) cannot throw and the URISyntaxException handler at :189-191 is unreachable. parsedUrl.getHost() is right there.
| host = new URI(parsedUrl.toString()).getHost(); | |
| final String host = parsedUrl.getHost(); |
The surrounding try/catch can go with it.
| URLFetcher fetcher = | ||
| new TimeoutCheckURLFetcher( | ||
| timeout -> assertEquals(timeout.intValue(), CONNECTION_READ_TIMEOUT)); | ||
| fetcher.fetch("http://example.com/devfile.yaml"); |
There was a problem hiding this comment.
TimeoutCheckURLFetcher overrides only fetch(URLConnection), so the only assertion that runs checks the connect/read timeout - the return value is discarded. The body is a copy of the pre-existing testDefaultFetchTimeoutIsSet (:158-165) with the URL swapped, so checkHttpSchemeIsAllowed and checkHttpsSchemeIsAllowed pass iff no exception is thrown.
Because that override does not cover fetch(String, int, String), validateUrlTarget runs in full and four tests now hit the real resolver - these two on example.com, plus testDefaultFetchTimeoutIsSet and testFetchTimeoutIsSet (pre-existing, newly broken) on eclipse.org. They fail in an air-gapped build with Unable to resolve URL host ..., and they also fail behind a corporate DNS that sinkholes unknown names, since the sinkhole IP is site-local and gets rejected.
An overridable resolve(String host) seam would fix the offline problem, and asserting on the returned content would give these two tests a reason to exist beyond the one that was already there.
| URLFetcher fetcher = | ||
| new TimeoutCheckURLFetcher( | ||
| timeout -> assertEquals(timeout.intValue(), CONNECTION_READ_TIMEOUT)); | ||
| fetcher.fetch("https://example.com/devfile.yaml"); |
There was a problem hiding this comment.
Every new test targets scheme rejection; the host blocklist - the most security-sensitive code in the PR - has zero coverage. Nothing asserts 127.0.0.1, 169.254.169.254, 10.0.0.1 or [::1] is refused, which is also why the fc00::/7 gap went unnoticed.
A @DataProvider table would cover it in a few lines:
@DataProvider
public Object[][] blockedHosts() {
return new Object[][] {
{"http://127.0.0.1/x"}, {"http://[::1]/x"}, {"http://169.254.169.254/x"},
{"http://10.0.0.1/x"}, {"http://192.168.1.1/x"}, {"http://172.16.0.1/x"},
{"http://[fd00::1]/x"}, {"http://0.0.0.0/x"},
};
}Worth adding an HttpServer-based case too: a 302 to a blocked host must be refused, and no Authorization header may reach the redirect target. I used exactly that fixture to reproduce the redirect finding, so it is a few lines of work.
| Assert.assertNotNull(urlJson); | ||
|
|
||
| String content = urlFetcher.fetchSafely(urlJson.toString()); | ||
| public void checkGetContent() throws IOException { |
There was a problem hiding this comment.
Moving checkGetContent and checkPartialContent onto a mocked URLConnection makes sense - the old versions used file: classpath URLs that the new check rejects. The side effect is that no test now drives fetch(String) end-to-end through openConnection, and that is precisely the path the redirect bypass lives on.
A com.sun.net.httpserver.HttpServer fixture on an ephemeral port would restore that coverage and serve the validateUrlTarget tests at the same time.
| assertEquals(provider.formatUrl(".gitconfig"), ".gitconfig"); | ||
| } | ||
|
|
||
| @Test( |
There was a problem hiding this comment.
These @Test methods differ only in the URL string, down to a duplicated 3-line annotation block, and the same shape repeats in URLFileContentProviderTest. BitbucketServerAuthorizingFileContentProviderTest:108 - a file this PR edits - already uses @DataProvider, as do AzureDevOpsURLParserTest, AzureDevOpsURLTest and BitbucketServerApiClientProviderTest.
Collapsing to one data-driven test would also make adding data: and netdoc: cases free. Worth knowing: gopher:// currently fails with MalformedURLException ("unknown protocol") rather than this message - still rejected, but by JDK handler registration rather than by the check.
Using expectedExceptionsMessageRegExp instead of a bare exception type is the right call - it distinguishes "rejected for the right reason" from "blew up somewhere".
| throws ApiException { | ||
| requireNonNull(repository, "Repository"); | ||
| requireNonNull(repository, "File"); | ||
| requireNonNull(filePath, "File"); |
There was a problem hiding this comment.
Nice catch - easy one to read straight past.
Since requireNonNull here is the private helper on :66 that throws BadRequestException, this also changes GET /scm/resolve?repository=X with no file from a 500 to a 400. Correct, and almost certainly what any client wanted, but it is an observable contract change with no test in ScmServiceTest and no release note.
Two small asks: a regression test for the 400, and - since this is independent of the security work - consider splitting it out. Single-purpose security PRs are much easier to backport to maintenance branches.
A devfile can reference an absolute URL on an arbitrary host. Both file content providers attached the caller's credentials to such requests regardless of the destination, disclosing the user's personal access token, or the Basic credentials taken from the devfile URL, to any host a devfile chose to reference. Credentials are now only sent to hosts belonging to the SCM provider they were issued for. The allowlist is derived from the provider URL, the host name and the raw file location, as the latter is not necessarily the same host (github.com vs raw.githubusercontent.com, or the raw. subdomain of a GitHub Enterprise server). Requests to any other host fall back to an anonymous fetch, so devfiles referencing public content elsewhere keep working. Bitbucket Cloud additionally trusts api.bitbucket.org, which its API client legitimately authenticates against, and applies the same check to the fetchContent override that passes the token to that client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The URL a caller hands to POST /factory/resolver ends up being requested by the server. The existing check ran once, on the URL as given, and HttpURLConnection then followed redirects on its own - so a 302 to 169.254.169.254 or to a neighbouring pod was never checked. The check also missed several address ranges that are not the public internet. Add UrlTargetValidator to che-core-commons-lang as the one place that decides whether a target may be requested: http/https only, host required, and every address the host resolves to must be publicly routable. On top of the ranges the previous check covered, this rejects fc00::/7 (which InetAddress#isSiteLocalAddress does not cover), 100.64.0.0/10, 192.0.0.0/24, 198.18.0.0/15 and 240.0.0.0/4. URLFetcher no longer lets HttpURLConnection follow redirects. It follows them itself, up to five hops, validating each one, refusing an https to http downgrade, and dropping the Authorization header on a cross-origin hop. The GitHub and GitLab URL parsers probe an unknown host to find out whether it is an SCM server, which reports the outcome back through the factory endpoint. Only publicly routable hosts are probed now. A private-network SCM server is still reached through the configured provider endpoints or a personal access token, both of which are checked before the probe. Credential allowlisting moves from bare host to origin, so a devfile can no longer make the server send a token or basic credentials over plaintext http to a host it normally reaches over https. On-prem providers configured with http keep working, since their configured origin carries that scheme. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Docker image build succeeded: quay.io/eclipse/che-server:pr-1048 kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/cheServer/deployment", "value": {containers: [{image: "quay.io/eclipse/che-server:pr-1048", name: che}]}}]" |
|
@vinokurig: The following tests 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. |
3 participants
… providers
What does this PR do?
Restrict URL fetching to
http/httpsschemes only, rejectingfile://,ftp://,jar:, and other schemes that could be exploited to read local files or access cloud metadata endpoints. Also fix incorrect null-check in ScmService that validatedrepositorytwice instead offilePath.Screenshot/screencast of this PR
What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-11956
How to test this PR?
N/A
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedRelease Notes
Reviewers
Reviewers, please comment how you tested the PR when approving it.