Skip to content

Commit 833b2ee

Browse files
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 parents 5f7c1e9 + 52f2de3 commit 833b2ee

2 files changed

Lines changed: 483 additions & 53 deletions

File tree

‎pkg/registry/core/tenantsecret/rest.go‎

Lines changed: 37 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"net/http"
1212
"strconv"
13+
"strings"
1314
"time"
1415

1516
corev1 "k8s.io/api/core/v1"
@@ -43,9 +44,25 @@ const (
4344
singularName = "tenantsecret"
4445
kindTenantSecret = "TenantSecret"
4546
kindTenantSecretList = "TenantSecretList"
47+
48+
// internalPrefix namespaces the labels and annotations the platform owns:
49+
// the lineage webhook's tenantresource verdict, the cacert controller's
50+
// tenant-ca selector and its ownership markers. They are the platform's
51+
// answer to "may this tenant see this Secret", and an ApplicationDefinition
52+
// selects on them, so a caller writing through this API must not be able to
53+
// plant or strip one on the backing Secret. The guard is on the prefix
54+
// rather than on a list of keys so a platform key added later is protected
55+
// without anyone remembering to come back here.
56+
internalPrefix = "internal.cozystack.io/"
4657
)
4758

48-
func stripInternal(m map[string]string) map[string]string {
59+
// isInternalKey reports whether a metadata key belongs to the platform.
60+
func isInternalKey(k string) bool { return strings.HasPrefix(k, internalPrefix) }
61+
62+
// stripTenantMarker hides the tenant-resource marker, and only that key. The
63+
// read side is deliberately narrower than the write side: a tenant may see the
64+
// platform's other verdicts, tenant-ca among them, it just cannot write them.
65+
func stripTenantMarker(m map[string]string) map[string]string {
4966
if m == nil {
5067
return nil
5168
}
@@ -93,7 +110,7 @@ func secretToTenant(sec *corev1.Secret) *corev1alpha1.TenantSecret {
93110
UID: sec.UID,
94111
ResourceVersion: sec.ResourceVersion,
95112
CreationTimestamp: sec.CreationTimestamp,
96-
Labels: stripInternal(sec.Labels),
113+
Labels: stripTenantMarker(sec.Labels),
97114
Annotations: sec.Annotations,
98115
},
99116
Type: string(sec.Type),
@@ -115,13 +132,24 @@ func tenantToSecret(ts *corev1alpha1.TenantSecret, cur *corev1.Secret) *corev1.S
115132
}
116133
out.Labels[tsLabelKey] = tsLabelValue
117134
for k, v := range ts.Labels {
135+
// Platform keys are dropped rather than rejected: a caller that GETs an
136+
// object, edits it and PUTs it back sends them straight back at us, and
137+
// failing that round-trip would break `kubectl edit` for no gain. On an
138+
// update out starts as a copy of the stored Secret, so ignoring the
139+
// caller here also means a platform key cannot be stripped.
140+
if isInternalKey(k) {
141+
continue
142+
}
118143
out.Labels[k] = v
119144
}
120145

121146
if out.Annotations == nil {
122147
out.Annotations = map[string]string{}
123148
}
124149
for k, v := range ts.Annotations {
150+
if isInternalKey(k) {
151+
continue
152+
}
125153
out.Annotations[k] = v
126154
}
127155

@@ -147,10 +175,13 @@ func nsFrom(ctx context.Context) (string, error) {
147175
// -----------------------------------------------------------------------------
148176

149177
var (
150-
_ rest.Creater = &REST{}
151-
_ rest.Getter = &REST{}
152-
_ rest.Lister = &REST{}
153-
_ rest.Updater = &REST{}
178+
_ rest.Creater = &REST{}
179+
_ rest.Getter = &REST{}
180+
_ rest.Lister = &REST{}
181+
_ rest.Updater = &REST{}
182+
// rest.Patcher is Getter+Updater: a PATCH request is decoded against the
183+
// object Get returns and applied through Update, so there is no separate
184+
// patch entry point and the guard in tenantToSecret covers PATCH too.
154185
_ rest.Patcher = &REST{}
155186
_ rest.GracefulDeleter = &REST{}
156187
_ rest.Watcher = &REST{}
@@ -400,53 +431,6 @@ func (r *REST) Delete(
400431
return nil, err == nil, err
401432
}
402433

403-
func (r *REST) Patch(
404-
ctx context.Context,
405-
name string,
406-
pt types.PatchType,
407-
data []byte,
408-
opts *metav1.PatchOptions,
409-
subresources ...string,
410-
) (runtime.Object, error) {
411-
if len(subresources) > 0 {
412-
return nil, fmt.Errorf("TenantSecret does not have subresources")
413-
}
414-
ns, err := nsFrom(ctx)
415-
if err != nil {
416-
return nil, err
417-
}
418-
current := &corev1.Secret{}
419-
if err := r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, current, &client.GetOptions{Raw: &metav1.GetOptions{}}); err != nil {
420-
return nil, err
421-
}
422-
if current.Labels == nil || current.Labels[tsLabelKey] != tsLabelValue {
423-
return nil, apierrors.NewNotFound(r.gvr.GroupResource(), name)
424-
}
425-
out := &corev1.Secret{
426-
ObjectMeta: metav1.ObjectMeta{
427-
Namespace: ns,
428-
Name: name,
429-
},
430-
}
431-
patch := client.RawPatch(pt, data)
432-
err = r.c.Patch(ctx, out, patch, &client.PatchOptions{Raw: opts})
433-
if err != nil {
434-
return nil, err
435-
}
436-
437-
// Ensure tenant secret label is preserved
438-
if out.Labels == nil {
439-
out.Labels = make(map[string]string)
440-
}
441-
442-
if out.Labels[tsLabelKey] != tsLabelValue {
443-
out.Labels[tsLabelKey] = tsLabelValue
444-
_ = r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}})
445-
}
446-
447-
return secretToTenant(out), nil
448-
}
449-
450434
// -----------------------------------------------------------------------------
451435
// Watcher
452436
// -----------------------------------------------------------------------------

0 commit comments

Comments
 (0)