Skip to content

Commit b7d80e8

Browse files
committed
UPSTREAM: <carry>: allow type mutation for specific secrets
This is a short term fix, once we improve the cert rotation logic in library-go that does not depend on this hack, then we can remove this carry patch.
1 parent 258f1d5 commit b7d80e8

File tree

3 files changed

+199
-1
lines changed

3 files changed

+199
-1
lines changed

pkg/apis/core/validation/validation.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6407,7 +6407,12 @@ func ValidateSecret(secret *core.Secret) field.ErrorList {
64076407
func ValidateSecretUpdate(newSecret, oldSecret *core.Secret) field.ErrorList {
64086408
allErrs := ValidateObjectMetaUpdate(&newSecret.ObjectMeta, &oldSecret.ObjectMeta, field.NewPath("metadata"))
64096409

6410-
allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...)
6410+
// TODO: this is a short term fix, we can drop this patch once we
6411+
// migrate all of the affected secret objects to to intended type,
6412+
// see https://issues.redhat.com/browse/API-1800
6413+
if !OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret) {
6414+
allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...)
6415+
}
64116416
if oldSecret.Immutable != nil && *oldSecret.Immutable {
64126417
if newSecret.Immutable == nil || !*newSecret.Immutable {
64136418
allErrs = append(allErrs, field.Forbidden(field.NewPath("immutable"), "field is immutable when `immutable` is set"))
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/*
2+
Copyright 2024 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package validation
18+
19+
import (
20+
apimachinerytypes "k8s.io/apimachinery/pkg/types"
21+
"k8s.io/kubernetes/pkg/apis/core"
22+
)
23+
24+
var (
25+
// we make an exception for the following secret objects, during update
26+
// we allow the secret type to mutate from:
27+
// "SecretTypeTLS" -> "kubernetes.io/tls"
28+
// some of our operators were accidentally creating secrets of type
29+
// "SecretTypeTLS", and this patch enables us to move these secrest
30+
// objects to the intended type in a ratcheting manner.
31+
//
32+
// we can drop this patch when we migrate all of the affected secret
33+
// objects to to intended type: https://issues.redhat.com/browse/API-1800
34+
whitelist = map[apimachinerytypes.NamespacedName]struct{}{
35+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "aggregator-client-signer"}: {},
36+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "kube-apiserver-to-kubelet-signer"}: {},
37+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "localhost-serving-signer"}: {},
38+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "service-network-serving-signer"}: {},
39+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "loadbalancer-serving-signer"}: {},
40+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "localhost-recovery-serving-signer"}: {},
41+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "kube-control-plane-signer"}: {},
42+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "node-system-admin-signer"}: {},
43+
apimachinerytypes.NamespacedName{Namespace: "openshift-etcd-operator", Name: "etcd-client"}: {},
44+
apimachinerytypes.NamespacedName{Namespace: "openshift-kube-controller-manager-operator", Name: "csr-signer-signer"}: {},
45+
}
46+
)
47+
48+
func OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {
49+
// we allow "SecretTypeTLS" -> "kubernetes.io/tls" only
50+
if oldSecret.Type == "SecretTypeTLS" && newSecret.Type == core.SecretTypeTLS {
51+
key := apimachinerytypes.NamespacedName{Namespace: oldSecret.Namespace, Name: oldSecret.Name}
52+
if _, ok := whitelist[key]; ok {
53+
return true
54+
}
55+
}
56+
return false
57+
}
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
Copyright 2024 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package validation
18+
19+
import (
20+
"testing"
21+
22+
"github.com/google/go-cmp/cmp"
23+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
apimachinerytypes "k8s.io/apimachinery/pkg/types"
25+
"k8s.io/apimachinery/pkg/util/validation/field"
26+
"k8s.io/kubernetes/pkg/apis/core"
27+
)
28+
29+
func TestOpenShiftValidateSecretUpdate(t *testing.T) {
30+
newSecretFn := func(ns, name string, secretType core.SecretType) *core.Secret {
31+
return &core.Secret{
32+
ObjectMeta: metav1.ObjectMeta{
33+
Name: name,
34+
Namespace: ns,
35+
ResourceVersion: "1",
36+
},
37+
Type: secretType,
38+
Data: map[string][]byte{
39+
"tls.key": []byte("foo"),
40+
"tls.crt": []byte("bar"),
41+
},
42+
}
43+
}
44+
invalidTypeErrFn := func(secretType core.SecretType) field.ErrorList {
45+
return field.ErrorList{
46+
field.Invalid(field.NewPath("type"), secretType, "field is immutable"),
47+
}
48+
}
49+
tlsKeyRequiredErrFn := func() field.ErrorList {
50+
return field.ErrorList{
51+
field.Required(field.NewPath("data").Key(core.TLSCertKey), ""),
52+
field.Required(field.NewPath("data").Key(core.TLSPrivateKeyKey), ""),
53+
}
54+
}
55+
56+
t.Run("verify whitelist", func(t *testing.T) {
57+
for key := range whitelist {
58+
ns, name := key.Namespace, key.Name
59+
60+
// exercise a valid type mutation: "SecretTypeTLS" -> "kubernetes.io/tls"
61+
oldSecret, newSecret := newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeTLS)
62+
if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
63+
t.Errorf("unexpected error: %v", errs)
64+
}
65+
66+
// the reverse should not be allowed
67+
errExpected := invalidTypeErrFn("SecretTypeTLS")
68+
oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, "SecretTypeTLS")
69+
if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
70+
t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
71+
}
72+
73+
// no type change, no validation failure expected
74+
oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, core.SecretTypeTLS)
75+
if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
76+
t.Errorf("unexpected error: %v", errs)
77+
}
78+
79+
// exercise an invalid type mutation, we expect validation failure
80+
errExpected = invalidTypeErrFn(core.SecretTypeOpaque)
81+
oldSecret, newSecret = newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeOpaque)
82+
if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
83+
t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
84+
}
85+
86+
// verify that kbernetes.io/tls validation ar enforced
87+
errExpected = tlsKeyRequiredErrFn()
88+
oldSecret, newSecret = newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeTLS)
89+
newSecret.Data = nil
90+
if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
91+
t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
92+
}
93+
}
94+
})
95+
96+
// we must not break secrets that are not in the whitelist
97+
tests := []struct {
98+
name string
99+
oldSecret *core.Secret
100+
newSecret *core.Secret
101+
errExpected field.ErrorList
102+
}{
103+
{
104+
name: "secret is not whitelisted, valid type transition, update not allowed",
105+
oldSecret: newSecretFn("foo", "bar", "SecretTypeTLS"),
106+
newSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
107+
errExpected: invalidTypeErrFn(core.SecretTypeTLS),
108+
},
109+
{
110+
name: "secret is not whitelisted, invalid type transition, update not allowed",
111+
oldSecret: newSecretFn("foo", "bar", "SecretTypeTLS"),
112+
newSecret: newSecretFn("foo", "bar", core.SecretTypeOpaque),
113+
errExpected: invalidTypeErrFn(core.SecretTypeOpaque),
114+
},
115+
{
116+
name: "secret is not whitelisted, no type transition, update allowed",
117+
oldSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
118+
newSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
119+
},
120+
}
121+
122+
for _, test := range tests {
123+
t.Run(test.name, func(t *testing.T) {
124+
key := apimachinerytypes.NamespacedName{Namespace: test.oldSecret.Namespace, Name: test.oldSecret.Name}
125+
if _, ok := whitelist[key]; ok {
126+
t.Errorf("misconfigured test: secret is in whitelist: %s", key)
127+
return
128+
}
129+
130+
errGot := ValidateSecretUpdate(test.newSecret, test.oldSecret)
131+
if !cmp.Equal(test.errExpected, errGot) {
132+
t.Errorf("expected error: %v, diff: %s", test.errExpected, cmp.Diff(test.errExpected, errGot))
133+
}
134+
})
135+
}
136+
}

0 commit comments

Comments
 (0)