Skip to content

Commit db872ce

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 db872ce

File tree

3 files changed

+176
-1
lines changed

3 files changed

+176
-1
lines changed

pkg/apis/core/validation/validation.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -6407,7 +6407,11 @@ 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, the long term goal is to improve the
6411+
// cert rotation logic that is not reliant on this hack.
6412+
if !OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret) {
6413+
allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...)
6414+
}
64116415
if oldSecret.Immutable != nil && *oldSecret.Immutable {
64126416
if newSecret.Immutable == nil || !*newSecret.Immutable {
64136417
allErrs = append(allErrs, field.Forbidden(field.NewPath("immutable"), "field is immutable when `immutable` is set"))
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
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+
"fmt"
21+
22+
"k8s.io/kubernetes/pkg/apis/core"
23+
)
24+
25+
var (
26+
// we make an exception for the following secret objects, during update
27+
// we allow the secret type to mutate from:
28+
// "SecretTypeTLS" -> "kubernetes.io/tls"
29+
whitelist = map[string]struct{}{
30+
"openshift-kube-apiserver-operator/aggregator-client-signer": {},
31+
"openshift-kube-apiserver-operator/kube-apiserver-to-kubelet-signer": {},
32+
"openshift-kube-apiserver-operator/localhost-serving-signer": {},
33+
"openshift-kube-apiserver-operator/service-network-serving-signer": {},
34+
"openshift-kube-apiserver-operator/loadbalancer-serving-signer": {},
35+
"openshift-kube-apiserver-operator/localhost-recovery-serving-signer": {},
36+
"openshift-kube-apiserver-operator/kube-control-plane-signer": {},
37+
"openshift-kube-apiserver-operator/node-system-admin-signer": {},
38+
}
39+
)
40+
41+
func OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {
42+
// we allow "SecretTypeTLS" -> "kubernetes.io/tls" only
43+
if oldSecret.Type == "SecretTypeTLS" && newSecret.Type == core.SecretTypeTLS {
44+
key := fmt.Sprintf("%s/%s", oldSecret.Namespace, oldSecret.Name)
45+
if _, ok := whitelist[key]; ok {
46+
return true
47+
}
48+
}
49+
return false
50+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
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+
"fmt"
21+
"strings"
22+
"testing"
23+
24+
"github.com/google/go-cmp/cmp"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26+
"k8s.io/apimachinery/pkg/util/validation/field"
27+
"k8s.io/kubernetes/pkg/apis/core"
28+
)
29+
30+
func TestOpenShiftValidateSecretUpdate(t *testing.T) {
31+
newSecretFn := func(ns, name string, secretType core.SecretType) *core.Secret {
32+
return &core.Secret{
33+
ObjectMeta: metav1.ObjectMeta{
34+
Name: name,
35+
Namespace: ns,
36+
ResourceVersion: "1",
37+
},
38+
Type: secretType,
39+
Data: map[string][]byte{
40+
"tls.key": []byte("foo"),
41+
"tls.crt": []byte("bar"),
42+
},
43+
}
44+
}
45+
errFn := func(secretType core.SecretType) field.ErrorList {
46+
return field.ErrorList{
47+
field.Invalid(field.NewPath("type"), secretType, "field is immutable"),
48+
}
49+
}
50+
51+
t.Run("verify whitelist", func(t *testing.T) {
52+
for key := range whitelist {
53+
split := strings.Split(key, "/")
54+
if len(split) != 2 {
55+
t.Errorf("invalid key in whitelist - %s", key)
56+
continue
57+
}
58+
ns, name := split[0], split[1]
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+
// no type change, no validation failure expected
67+
oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, core.SecretTypeTLS)
68+
if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
69+
t.Errorf("unexpected error: %v", errs)
70+
}
71+
72+
// exercise an invalid type mutation, we expect validation failure
73+
errExpected := errFn(core.SecretTypeOpaque)
74+
oldSecret, newSecret = newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeOpaque)
75+
if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
76+
t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
77+
}
78+
}
79+
})
80+
81+
// we must not break secrets that are not in the whitelist
82+
tests := []struct {
83+
name string
84+
oldSecret *core.Secret
85+
newSecret *core.Secret
86+
errExpected field.ErrorList
87+
}{
88+
{
89+
name: "secret is not whitelisted, valid type transition, update not allowed",
90+
oldSecret: newSecretFn("foo", "bar", "SecretTypeTLS"),
91+
newSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
92+
errExpected: errFn(core.SecretTypeTLS),
93+
},
94+
{
95+
name: "secret is not whitelisted, invalid type transition, update not allowed",
96+
oldSecret: newSecretFn("foo", "bar", "SecretTypeTLS"),
97+
newSecret: newSecretFn("foo", "bar", core.SecretTypeOpaque),
98+
errExpected: errFn(core.SecretTypeOpaque),
99+
},
100+
{
101+
name: "secret is not whitelisted, no type transition, update allowed",
102+
oldSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
103+
newSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
104+
},
105+
}
106+
107+
for _, test := range tests {
108+
t.Run(test.name, func(t *testing.T) {
109+
key := fmt.Sprintf("%s/%s", test.oldSecret.Namespace, test.oldSecret.Name)
110+
if _, ok := whitelist[key]; ok {
111+
t.Errorf("secret is in whitelist: %s", key)
112+
return
113+
}
114+
115+
errGot := ValidateSecretUpdate(test.newSecret, test.oldSecret)
116+
if !cmp.Equal(test.errExpected, errGot) {
117+
t.Errorf("expected error: %v, diff: %s", test.errExpected, cmp.Diff(test.errExpected, errGot))
118+
}
119+
})
120+
}
121+
}

0 commit comments

Comments
 (0)