flyteorg / flyteorg/flyte

KServiceName() is non-injective: distinct (project, domain, name) can map to the same KService name (delimiter ambiguity + 32-bit suffix)

Open
#7,622 2 comments 0 reactions 1 assignee Claimed by @paullongtan View on GitHub
flyte2
Dominant language
Go
Stars
7.5k
Forks
886
Avg merge
1d 14h
Merged PRs (30d)
120

Description

### Summary

`KServiceName()` derives the Knative Service name for an app from its `(project, domain, name)` identity, and all apps share the single `flyte` namespace. The current derivation is **not injective** — two different app identities can produce the same KService name. Combined with `Deploy()`'s `Get → Create/Update` (last-writer-wins) behavior, a second app that resolves to an already-used name will *update the existing KService's spec* rather than create a distinct object.

Filing this as a robustness/correctness concern. There's one thing I couldn't determine from the code (the access model) — see the question near the end; the answer decides whether this is only a same-tenant footgun or can cross ownership boundaries.

Observed in `app/internal/k8s/app_client.go` (commit `5df13b9`).

### The mapping

```go
func KServiceName(id *flyteapp.Identifier) string {
raw := strings.ToLower(fmt.Sprintf("%s-%s-%s", id.GetName(), id.GetProject(), id.GetDomain()))
if len(raw) <= 63 { return raw }
sum := sha256.Sum256([]byte(id.GetProject() + "/" + id.GetDomain() + "/" + id.GetName()))
suffix := hex.EncodeToString(sum[:4]) // 4 bytes = 32-bit suffix
prefix := raw
if len(prefix) > 54 { prefix = prefix[:54] }
return prefix + "-" + suffix
}
```

### Observations

1. **Delimiter ambiguity (`raw <= 63` path).** The three fields are joined by `-`, and `project`/`domain` may themselves contain `-`, so the concatenation is ambiguous. Minimal illustration — both triples satisfy every declared field constraint, yet collide:
- `{name: "svc", project: "team", domain: "prod-x"}` → `svc-team-prod-x`
- `{name: "svc", project: "team-prod", domain: "x"}` → `svc-team-prod-x`

2. **32-bit suffix (`raw > 63` path).** When `raw` exceeds the 63-char limit, uniqueness rests on a 4-byte (32-bit) SHA-256 suffix (`sum[:4]`). That's a small space to guarantee global uniqueness across all `(project, domain, name)` triples. The retained 54-char prefix does not add uniqueness against a specific existing name, because the leading bytes can be reproduced while only the tail (beyond position 54, e.g. in a long `domain`) is varied.

3. **Field-validation asymmetry.** In `flyteidl2/app/app_definition.proto`, `name` is constrained (`max_len = 30` + DNS-1123 label pattern), but `project` and `domain` carry only `min_len = 1` — no max length and no charset restriction — so they can be arbitrarily long and contain `-`. I also didn't find a `protovalidate` interceptor wired on the app handler (only an otel interceptor in `app/setup.go`), so even these declarative constraints may not be enforced at the RPC boundary.

4. **Single shared namespace + last-writer-wins.** All KServices live in namespace `flyte`, and `Deploy()` does `Get(name)`; on hit it sets `existing.Spec = ksvc.Spec`, merges labels/annotations (including `flyte.org/app-id`), and `Update()`s. So a name collision means the later `Create`/`Update` overwrites the earlier app's KService spec instead of producing a separate object — an integrity concern for whoever created the name first.

### Access-model question (couldn't determine from the code)

`InternalAppService.Create` validates only that `id`/`spec`/payload are non-nil; it doesn't check that the caller is entitled to the requested `(project, domain)`. In the unified `flyte` binary (`manager/cmd/main.go`) the mux is wrapped only with `corsMiddleware`. Could a maintainer clarify the intended access model — is `(project, domain)` authorized against the caller's tenant somewhere upstream? That determines whether a name collision is confined to a single tenant or can affect another tenant's app.

### Suggested hardening

- Make the name derivation injective/unambiguous (length-prefix or escape the fields, or hash a canonical unambiguous encoding of the triple), and widen the truncated hash well beyond 32 bits.
- Validate `project`/`domain` (max length + DNS-safe charset, mirroring `name`) and wire a `protovalidate` interceptor so the proto constraints are actually enforced.
- Consider per-tenant namespaces rather than a single shared `flyte` namespace.
- On Deploy, reject an update whose target KService is owned by a different app identity (compare the `flyte.org/app-id` annotation before `Update`).

### Note on disclosure

Per `SECURITY.md` I'm glad to move this to the private security-advisory channel if you'd prefer to triage it there. I have a fuller local reproduction (a faithful copy of `KServiceName()` plus a `Deploy()` `Get→Update` simulation) that I can share privately.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.