Repository navigation
Commit 833b2ee
authored
fix(api): drop caller-supplied internal.cozystack.io labels in tenantsecret writes (#3599)
## What this PR does
Labels and annotations under `internal.cozystack.io/` belong to the
platform. The lineage webhook stamps
`internal.cozystack.io/tenantresource` to decide whether a Secret is
visible through this API at all, and the cacert controller stamps
`internal.cozystack.io/tenant-ca`, which an `ApplicationDefinition`
selects on in `spec.secrets` to hand a trust anchor to the tenant. The
registry copied every caller-supplied key onto the backing Secret as is,
so anyone with write access to `tenantsecrets` could plant either marker
on a Secret of their choice, or strip one to take an object out of the
platform's view.
`tenantToSecret` is the only place the write verbs go through, PATCH
included, because `rest.Patcher` is `Getter`+`Updater` and the endpoint
installer serves PATCH out of `Update`. It now skips keys with that
prefix. The update target starts as a copy of the stored Secret, so a
platform key cannot be removed either. The guard is on the whole prefix,
so a platform key added later is covered without anyone touching this
code again.
Reads are unchanged. A tenant can see the platform's verdicts, it just
cannot write them. Keys are dropped silently instead of rejected,
otherwise a GET, edit and PUT round trip would break `kubectl edit` for
no gain.
Nothing writes through this API today. Every `tenantsecrets` grant in
`cozystack-basics` is `get`/`list`/`watch`, and the console only lists
and watches. That read-only RBAC is what keeps the hole inert, and it
lives in a different package from the registry that implements the write
verbs, which is why I would rather close it here than keep relying on
it.
Two things I found next to the diff and did not fix. The `Patch` method
on this type is never reached: its signature is the `client-go` shape
and matches no interface the endpoint installer asserts, so no request
lands there. I gave it the same guard instead of leaving the weaker of
two doors, but deleting it is probably the better answer and I would
rather that call was yours. A repair after the write is weaker than the
conversion anyway: the forged key is live between the patch and the
repair, and forcing the object back to a pre-patch snapshot drops a key
that a controller stamped inside that window. Second one, and the reason
none of this is easy to spot: `Update` passes `nil` where the handler
hands the storage the current object, so `hasUID` reports false and the
patch mechanism takes its create-new-object branch. For JSON Patch,
merge patch and strategic merge patch that branch returns `NotFound`, so
all three get a 404 against an existing TenantSecret. Apply is the
exception. `applyPatcher.createNewObject` builds a fresh object of the
kind and applies onto that, so an apply does reach `Update`, carrying
only the fields the caller sent, and the merge in `tenantToSecret` is
what keeps the stored labels. Passing `secretToTenant(previous)` instead
of `nil` is the fix and it belongs in its own change, since it makes the
other three patch types live. Tell me if you want issues for either.
Closes #3464
### Screenshots
Not a UI change.
### Downstream repositories
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
Walked the trigger map against the diff: two Go files under
`pkg/registry/core/tenantsecret/`, no package added, no chart, values
schema, CRD, `ApplicationDefinition` field, namespace, `hack/` target or
release asset touched. Nothing in the map fires.
### Release note
```release-note
fix(api): TenantSecret writes no longer accept caller-supplied `internal.cozystack.io/*` labels and annotations, so a principal with write access to the API can neither plant nor strip the platform markers that decide tenant visibility on the backing Secret.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Strengthened protection of platform-managed metadata during
tenant-secret creation and updates.
* Prevented tenant-provided platform-owned labels and annotations from
overwriting protected values.
* Preserved standard metadata while hiding sensitive resource markers.
* Improved patch-based update handling and protected metadata
consistency across changes.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->2 files changed
Lines changed: 483 additions & 53 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
43 | 44 | | |
44 | 45 | | |
45 | 46 | | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
46 | 57 | | |
47 | 58 | | |
48 | | - | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
49 | 66 | | |
50 | 67 | | |
51 | 68 | | |
| |||
93 | 110 | | |
94 | 111 | | |
95 | 112 | | |
96 | | - | |
| 113 | + | |
97 | 114 | | |
98 | 115 | | |
99 | 116 | | |
| |||
115 | 132 | | |
116 | 133 | | |
117 | 134 | | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
118 | 143 | | |
119 | 144 | | |
120 | 145 | | |
121 | 146 | | |
122 | 147 | | |
123 | 148 | | |
124 | 149 | | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
125 | 153 | | |
126 | 154 | | |
127 | 155 | | |
| |||
147 | 175 | | |
148 | 176 | | |
149 | 177 | | |
150 | | - | |
151 | | - | |
152 | | - | |
153 | | - | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
154 | 185 | | |
155 | 186 | | |
156 | 187 | | |
| |||
400 | 431 | | |
401 | 432 | | |
402 | 433 | | |
403 | | - | |
404 | | - | |
405 | | - | |
406 | | - | |
407 | | - | |
408 | | - | |
409 | | - | |
410 | | - | |
411 | | - | |
412 | | - | |
413 | | - | |
414 | | - | |
415 | | - | |
416 | | - | |
417 | | - | |
418 | | - | |
419 | | - | |
420 | | - | |
421 | | - | |
422 | | - | |
423 | | - | |
424 | | - | |
425 | | - | |
426 | | - | |
427 | | - | |
428 | | - | |
429 | | - | |
430 | | - | |
431 | | - | |
432 | | - | |
433 | | - | |
434 | | - | |
435 | | - | |
436 | | - | |
437 | | - | |
438 | | - | |
439 | | - | |
440 | | - | |
441 | | - | |
442 | | - | |
443 | | - | |
444 | | - | |
445 | | - | |
446 | | - | |
447 | | - | |
448 | | - | |
449 | | - | |
450 | 434 | | |
451 | 435 | | |
452 | 436 | | |
| |||
0 commit comments