gocached: support namespaces#34
Conversation
| // from [GlobalNamespace]. See [WithNamespaceMapping]. Namespace is case- | ||
| // insensitive, but is otherwise treated as an opaque string. |
There was a problem hiding this comment.
case-insensitive is a little weird. Then you have to specify how it's case insensitive. And are those rules the same between Go and SQLite? Doesn't seem worth it for an opaque string.
But it's fine if you want to document it more.
There was a problem hiding this comment.
This bubbled up from the existing table constraint where it gets persisted:
Namespace TEXT NOT NULL UNIQUE CHECK (Namespace = lower(Namespace))
I can definitely document it more, but it sounds like maybe you think we should get rid of this constraint altogether? Or maybe we shouldn't allow user-defined strings to end up in the db? Although I couldn't think of a better way to map from claims to stable unique namespaces in a way that wouldn't require storage on the caller side too.
There was a problem hiding this comment.
yeah, making it just raw bytes (but with some validation that it's utf-8 and matches some regexp) sounds good. then remove this lower(Namespace) thing here and use whatever the application sends (not raw user/attacker-controlled, but e.g. sending "pr-%d" or "user-[validated-email]" or something
There was a problem hiding this comment.
Dropping the constraint was a bit annoying to manage (tips welcome if it looks silly to you), but I think this is addressed now.
|
Sorry to jump in out of the blue here will this work address the CVE https://nvd.nist.gov/vuln/detail/CVE-2025-36852? Specifically "allows any contributor with pull request privileges to inject compromised artifacts from an untrusted environment into trusted production environments without detection". |
Remove the special-case concept of "global writes" and instead allow callers to provide a namespace mapping function that makes policy decisions about which clients are globally trusted and which clients should be isolated from each other. As a result, all clients are now able to write, but not necessarily into the shared global namespace. All clients can still read from the global namespace, as well as their own. The Namespaces table already existed in the schema, but we drop the lowercase constraint. However, there are some breaking changes in the package API, WithJWTAuth now takes issuer URLs only and policy moves into the new WithNamespaceMapping option. cmd/gocached implements the spirit of the old API in terms of a namespace mapping function, with the main difference that it now allows writes if you don't have the global claims, but just into your own isolated namespace. Updates tailscale/corp#38092 Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>
|
Hi @ryancurrah - I wouldn't say it directly addresses that CVE, but it is relevant. Currently, gocached only provides very coarse controls (global write or not) and to mitigate the risk you need to ensure you only hand out global write to trusted clients. In practice, I would normally expect that to mean only give global write to sessions that come from a protected branch, e.g. main or release branches. Then, when you pull from the global namespace on building a release, you know it only has content from trusted clients. With this PR, you get a bit more granular control. For example, you could assign every GitHub "actor" their own namespace to write in, so you can safely allow everyone to write to the cache and get cache hits on their untrusted PRs. And it's safe because attackers can only poison their own namespace which no one else will read from. |
3 participants
Remove the special-case concept of "global writes" and instead allow callers to provide a namespace mapping function that makes policy decisions about which clients are globally trusted and which clients should be isolated from each other. As a result, all clients are now able to write, but not necessarily into the shared global namespace. All clients can still read from the global namespace, as well as their own. The Namespaces table already existed in the schema, so no schema updates required.
However, there are some breaking changes in the package API, WithJWTAuth now takes issuer URLs only and policy moves into the new WithNamespaceMapping option. cmd/gocached implements the spirit of the old API in terms of a namespace mapping function, with the main difference that it now allows writes if you don't have the global claims, but just into your own isolated namespace.
Updates tailscale/corp#38092