Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import com.fasterxml.jackson.module.scala.DefaultScalaModule
import com.typesafe.scalalogging.LazyLogging
import jakarta.ws.rs.client.{Client, ClientBuilder, Entity}
import jakarta.ws.rs.core._
import jakarta.ws.rs.{Consumes, GET, POST, Path, Produces}
import jakarta.ws.rs.{Consumes, DELETE, GET, POST, Path, Produces}
import org.apache.texera.auth.JwtParser.parseToken
import org.apache.texera.auth.SessionUser
import org.apache.texera.auth.util.{ComputingUnitAccess, HeaderField}
Expand All @@ -43,6 +43,10 @@ object AccessControlResource extends LazyLogging {
private val wsapiWorkflowWebsocket: Regex = """.*/wsapi/workflow-websocket.*""".r
private val apiExecutionsStats: Regex = """.*/api/executions/[0-9]+/stats/[0-9]+.*""".r
private val apiExecutionsResultExport: Regex = """.*/api/executions/result/export.*""".r
private val pveRoute: Regex = """.*/(?:api/|wsapi/)?pve(?:/.*)?""".r
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested by Claude:

.*/(?:api/|wsapi/)?pve(?:/.*)? is overly permissive — the leading .*/ will match any path ending in …/pve or …/pve/anything, not just the expected /api/pve / /wsapi/pve / /pve shapes. Consistent with how wsapiWorkflowWebsocket / apiExecutionsStats are written above, so not out of line for this file, but the PVE routes here are well-defined enough to anchor more tightly, e.g.:

private val pveRoute: Regex = """^/?(?:auth/)?(?:api/|wsapi/)?pve(?:/.*)?$""".r

Also applies to pvePvesCuidPath and pvePackagesCuidPath below. Worth double-checking whether uriInfo.getPath here includes the auth/ prefix from the enclosing @Path("/auth") resource — your manual test probably already covered this, but the regex shape depends on it.

// Path patterns whose cuid lives in the URL path rather than the query string.
private val pvePvesCuidPath: Regex = """.*/pve/pves/([0-9]+).*""".r
private val pvePackagesCuidPath: Regex = """.*/pve/([0-9]+)/[^/]+/packages/.+""".r

/**
* Authorize the request based on the path and headers.
Expand All @@ -60,7 +64,8 @@ object AccessControlResource extends LazyLogging {
logger.info(s"Authorizing request for path: $path")

path match {
case wsapiWorkflowWebsocket() | apiExecutionsStats() | apiExecutionsResultExport() =>
case wsapiWorkflowWebsocket() | apiExecutionsStats() | apiExecutionsResultExport() |
pveRoute() =>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description says "Tested manually." Worth adding a small unit test on AccessControlResource.authorize that covers such as:

  • /pve/system?cuid=N → 200 (query-string cuid)
  • /pve/pves/N → 200 (path-segment cuid via the DELETE route)
  • /pve/N/myenv/packages/numpy → 200 (path-segment cuid via the packages route)
  • /pve/no-cuid-anywhere → 403 (cuid extraction falls through to empty → NumberFormatException → FORBIDDEN)
  • a non-PVE garbage path → 403

checkComputingUnitAccess(uriInfo, headers, bodyOpt)
case _ =>
logger.warn(s"No authorization logic for path: $path. Denying access.")
Expand Down Expand Up @@ -95,7 +100,14 @@ object AccessControlResource extends LazyLogging {
qToken.orElse(hToken).orElse(bToken).getOrElse("")
}
logger.info(s"token extracted from request $token")
val cuid = queryParams.getOrElse("cuid", "")

val cuid = queryParams.get("cuid").filter(_.nonEmpty).getOrElse {
uriInfo.getPath match {
case pvePvesCuidPath(c) => c
case pvePackagesCuidPath(c) => c
case _ => ""
}
}
val cuidInt =
try {
cuid.toInt
Expand Down Expand Up @@ -213,6 +225,15 @@ class AccessControlResource extends LazyLogging {
logger.info("Request body: " + body)
AccessControlResource.authorize(uriInfo, headers, Option(body).map(_.trim).filter(_.nonEmpty))
}

@DELETE
@Path("/{path:.*}")
def authorizeDelete(
@Context uriInfo: UriInfo,
@Context headers: HttpHeaders
): Response = {
AccessControlResource.authorize(uriInfo, headers)
}
}

@Path("/chat")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ class PveResource {
@GET
@Path("/system")
@Produces(Array(MediaType.APPLICATION_JSON))
def getSystemPackages: util.Map[String, util.List[String]] = {
def getSystemPackages(
@QueryParam("isLocal") isLocal: Boolean
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLocal describes where the backend is running. I can see that there is a security issue where a malicious user can flip isLocal. I suggest to derive isLocal from KubernetesConfig (there's already a config flag for this) and drop the param.

): util.Map[String, util.List[String]] = {
try {

// TODO: Support Kubernetes environment handling
val isLocal = true

val systemPkgs =
PveManager.getSystemPackages(isLocal).toList.asJava

Expand Down
2 changes: 2 additions & 0 deletions bin/computing-unit-master.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,13 @@ WORKDIR /texera/amber

COPY --from=build /texera/amber/requirements.txt /tmp/requirements.txt
COPY --from=build /texera/amber/operator-requirements.txt /tmp/operator-requirements.txt
COPY --from=build /texera/amber/system-requirements-lock.txt /tmp/system-requirements-lock.txt

# Install Python runtime dependencies
RUN apt-get update && apt-get install -y \
python3-pip \
python3-dev \
python3-venv \
libpq-dev \
&& apt-get clean

Expand Down
14 changes: 14 additions & 0 deletions bin/k8s/templates/gateway-routes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,20 @@ spec:
- group: gateway.envoyproxy.io
kind: Backend
name: texera-dynamic-backend
- matches:
- path:
type: PathPrefix
value: /pve
filters:
- type: URLRewrite
urlRewrite:
path:
type: ReplacePrefixMatch
replacePrefixMatch: /api/pve
backendRefs:
- group: gateway.envoyproxy.io
kind: Backend
name: texera-dynamic-backend
---
# MinIO Route
{{- if .Values.minio.gateway.enabled }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ export class ComputingUnitSelectionComponent implements OnInit {
}));

this.workflowPveService
.getSystemPackages(isLocal)
.getSystemPackages(cuId, isLocal)
.pipe(untilDestroyed(this))
.subscribe({
next: installedResp => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ export class WorkflowPveService {
return params;
}

getSystemPackages(isLocal: boolean): Observable<PackageResponse> {
const params = this.buildBaseParams();
getSystemPackages(cuid: number, isLocal: boolean): Observable<PackageResponse> {
const params = this.buildBaseParams().set("cuid", cuid.toString()).set("isLocal", isLocal.toString());
return this.http.get<PackageResponse>("/pve/system", { params });
}

Expand Down
Loading