Skip to content

Commit 8970c0e

Browse files
tkashembertinatto
authored andcommitted
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. squash with the previous PR during the rebase openshift#1924 squash with the previous PRs during the rebase openshift#1924 openshift#1929
1 parent 63b44ee commit 8970c0e

File tree

4 files changed

+339
-1
lines changed

4 files changed

+339
-1
lines changed

Diff for: pkg/apis/core/validation/validation.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -7024,7 +7024,12 @@ func ValidateSecret(secret *core.Secret) field.ErrorList {
70247024
func ValidateSecretUpdate(newSecret, oldSecret *core.Secret) field.ErrorList {
70257025
allErrs := ValidateObjectMetaUpdate(&newSecret.ObjectMeta, &oldSecret.ObjectMeta, field.NewPath("metadata"))
70267026

7027-
allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...)
7027+
// TODO: this is a short term fix, we can drop this patch once we
7028+
// migrate all of the affected secret objects to to intended type,
7029+
// see https://issues.redhat.com/browse/API-1800
7030+
if !openShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret) {
7031+
allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...)
7032+
}
70287033
if oldSecret.Immutable != nil && *oldSecret.Immutable {
70297034
if newSecret.Immutable == nil || !*newSecret.Immutable {
70307035
allErrs = append(allErrs, field.Forbidden(field.NewPath("immutable"), "field is immutable when `immutable` is set"))

Diff for: pkg/apis/core/validation/validation_patch.go

+68
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
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+
"k8s.io/kubernetes/pkg/apis/core"
21+
)
22+
23+
var (
24+
// we have multiple controllers reconciling the same secret,
25+
// resulting in unexpected outcomes such as the generation of new key pairs.
26+
// our goal is to prevent the generation of new key pairs by disallowing
27+
// deletions and permitting only updates, which appear to be 'safe'.
28+
//
29+
// thus we make an exception for the secrets in the following namespaces, during update
30+
// we allow the secret type to mutate from:
31+
// ["SecretTypeTLS", core.SecretTypeOpaque] -> "kubernetes.io/tls"
32+
// some of our operators were accidentally creating secrets of type
33+
// "SecretTypeTLS", and this patch enables us to move these secrets
34+
// objects to the intended type in a ratcheting manner.
35+
//
36+
// we can drop this patch when we migrate all of the affected secret
37+
// objects to to intended type: https://issues.redhat.com/browse/API-1800
38+
whitelist = map[string]struct{}{
39+
"openshift-kube-apiserver-operator": {},
40+
"openshift-kube-apiserver": {},
41+
"openshift-kube-controller-manager-operator": {},
42+
"openshift-config-managed": {},
43+
}
44+
)
45+
46+
func openShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {
47+
// initially, this check was stricter.
48+
// however, due to the platform's long history (spanning several years)
49+
// and the complexity of ensuring that resources were consistently created with only one type,
50+
// it is now permissible for (SecretTypeTLS, core.SecretTypeOpaque) type to transition to "kubernetes.io/tls".
51+
//
52+
// additionally, it should be noted that default values might also be applied in some cases.
53+
// (https://github.com/openshift/kubernetes/blob/258f1d5fb6491ba65fd8201c827e179432430627/pkg/apis/core/v1/defaults.go#L280-L284)
54+
if isOldSecretTypeMutationAllowed(oldSecret) && newSecret.Type == core.SecretTypeTLS {
55+
if _, ok := whitelist[oldSecret.Namespace]; ok {
56+
return true
57+
}
58+
}
59+
return false
60+
}
61+
62+
func isOldSecretTypeMutationAllowed(oldSecret *core.Secret) bool {
63+
// core.SecretTypeOpaque seems safe because
64+
// https://github.com/kubernetes/kubernetes/blob/8628c3c4da6746b1dc967cc520b189a04ebd78d1/pkg/apis/core/validation/validation.go#L6393
65+
//
66+
// "SecretTypeTLS" is what kas-o used
67+
return oldSecret.Type == core.SecretTypeOpaque || oldSecret.Type == "SecretTypeTLS"
68+
}

Diff for: pkg/apis/core/validation/validation_patch_test.go

+136
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+
"fmt"
21+
"testing"
22+
23+
"github.com/google/go-cmp/cmp"
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
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+
for _, secretType := range []core.SecretType{"SecretTypeTLS", core.SecretTypeOpaque} {
57+
for key := range whitelist {
58+
ns, name := key, "foo"
59+
t.Run(fmt.Sprintf("verify whitelist, key = %v, secretType = %v", key, secretType), func(t *testing.T) {
60+
// exercise a valid type mutation: "secretType" -> "kubernetes.io/tls"
61+
oldSecret, newSecret := newSecretFn(ns, name, secretType), 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(secretType)
68+
oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, secretType)
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.SecretTypeTLS)
81+
oldSecret, newSecret = newSecretFn(ns, name, "AnyOtherType"), newSecretFn(ns, name, core.SecretTypeTLS)
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 are enforced
87+
errExpected = tlsKeyRequiredErrFn()
88+
oldSecret, newSecret = newSecretFn(ns, name, secretType), 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+
97+
// we must not break secrets that are not in the whitelist
98+
tests := []struct {
99+
name string
100+
oldSecret *core.Secret
101+
newSecret *core.Secret
102+
errExpected field.ErrorList
103+
}{
104+
{
105+
name: "secret is not whitelisted, valid type transition, update not allowed",
106+
oldSecret: newSecretFn("foo", "bar", "SecretTypeTLS"),
107+
newSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
108+
errExpected: invalidTypeErrFn(core.SecretTypeTLS),
109+
},
110+
{
111+
name: "secret is not whitelisted, invalid type transition, update not allowed",
112+
oldSecret: newSecretFn("foo", "bar", "SecretTypeTLS"),
113+
newSecret: newSecretFn("foo", "bar", core.SecretTypeOpaque),
114+
errExpected: invalidTypeErrFn(core.SecretTypeOpaque),
115+
},
116+
{
117+
name: "secret is not whitelisted, no type transition, update allowed",
118+
oldSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
119+
newSecret: newSecretFn("foo", "bar", core.SecretTypeTLS),
120+
},
121+
}
122+
123+
for _, test := range tests {
124+
t.Run(test.name, func(t *testing.T) {
125+
if _, ok := whitelist[test.oldSecret.Namespace]; ok {
126+
t.Errorf("misconfigured test: secret is in whitelist: %s", test.oldSecret.Namespace)
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+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
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 apiserver
18+
19+
import (
20+
"context"
21+
"testing"
22+
23+
corev1 "k8s.io/api/core/v1"
24+
apierrors "k8s.io/apimachinery/pkg/api/errors"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26+
"k8s.io/apimachinery/pkg/util/sets"
27+
"k8s.io/client-go/kubernetes"
28+
apiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
29+
"k8s.io/kubernetes/test/integration/framework"
30+
)
31+
32+
// the list was copied from pkg/apis/core/validation/validation_patch.go
33+
var whitelistedSecretNamespaces = map[string]struct{}{
34+
"openshift-kube-apiserver-operator": {},
35+
"openshift-kube-apiserver": {},
36+
"openshift-kube-controller-manager-operator": {},
37+
"openshift-config-managed": {},
38+
}
39+
40+
// immortalNamespaces cannot be deleted, give the following error:
41+
// failed to delete namespace: "" is forbidden: this namespace may not be deleted
42+
var immortalNamespaces = sets.NewString("openshift-config-managed")
43+
44+
func TestOpenShiftValidateWhiteListedSecretTypeMutationUpdateAllowed(t *testing.T) {
45+
ctx := context.Background()
46+
server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd())
47+
if err != nil {
48+
t.Fatal(err)
49+
}
50+
t.Cleanup(server.TearDownFn)
51+
client, err := kubernetes.NewForConfig(server.ClientConfig)
52+
if err != nil {
53+
t.Fatal(err)
54+
}
55+
56+
for whiteListedSecretNamespace := range whitelistedSecretNamespaces {
57+
_, err := client.CoreV1().Namespaces().Get(ctx, whiteListedSecretNamespace, metav1.GetOptions{})
58+
if apierrors.IsNotFound(err) {
59+
testNamespace := framework.CreateNamespaceOrDie(client, whiteListedSecretNamespace, t)
60+
if !immortalNamespaces.Has(testNamespace.Name) {
61+
t.Cleanup(func() { framework.DeleteNamespaceOrDie(client, testNamespace, t) })
62+
}
63+
} else if err != nil {
64+
t.Fatal(err)
65+
}
66+
67+
secret := constructSecretWithOldType(whiteListedSecretNamespace, "foo")
68+
createdSecret, err := client.CoreV1().Secrets(whiteListedSecretNamespace).Create(ctx, secret, metav1.CreateOptions{})
69+
if err != nil {
70+
t.Errorf("failed to create secret, err = %v", err)
71+
}
72+
73+
createdSecret.Type = corev1.SecretTypeTLS
74+
updatedSecret, err := client.CoreV1().Secrets(whiteListedSecretNamespace).Update(ctx, createdSecret, metav1.UpdateOptions{})
75+
if err != nil {
76+
t.Errorf("failed to update the type of the secret, err = %v", err)
77+
}
78+
if updatedSecret.Type != corev1.SecretTypeTLS {
79+
t.Errorf("unexpected type of the secret = %v, expected = %v", updatedSecret.Type, corev1.SecretTypeTLS)
80+
}
81+
82+
// "kubernetes.io/tls" -> "SecretTypeTLS" is not allowed
83+
toUpdateSecret := updatedSecret
84+
toUpdateSecret.Type = "SecretTypeTLS"
85+
_, err = client.CoreV1().Secrets(whiteListedSecretNamespace).Update(ctx, toUpdateSecret, metav1.UpdateOptions{})
86+
if !apierrors.IsInvalid(err) {
87+
t.Errorf("unexpected error returned: %v", err)
88+
}
89+
}
90+
}
91+
92+
func TestNotWhiteListedSecretTypeMutationUpdateDisallowed(t *testing.T) {
93+
ctx := context.Background()
94+
server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd())
95+
if err != nil {
96+
t.Fatal(err)
97+
}
98+
t.Cleanup(server.TearDownFn)
99+
client, err := kubernetes.NewForConfig(server.ClientConfig)
100+
if err != nil {
101+
t.Fatal(err)
102+
}
103+
104+
testNamespace := framework.CreateNamespaceOrDie(client, "secret-type-update-disallowed", t)
105+
t.Cleanup(func() { framework.DeleteNamespaceOrDie(client, testNamespace, t) })
106+
107+
secret := constructSecretWithOldType(testNamespace.Name, "foo")
108+
createdSecret, err := client.CoreV1().Secrets(testNamespace.Name).Create(ctx, secret, metav1.CreateOptions{})
109+
if err != nil {
110+
t.Errorf("failed to create secret, err = %v", err)
111+
}
112+
113+
createdSecret.Type = corev1.SecretTypeTLS
114+
_, err = client.CoreV1().Secrets(testNamespace.Name).Update(ctx, createdSecret, metav1.UpdateOptions{})
115+
if !apierrors.IsInvalid(err) {
116+
t.Errorf("unexpected error returned: %v", err)
117+
}
118+
}
119+
120+
func constructSecretWithOldType(ns, name string) *corev1.Secret {
121+
return &corev1.Secret{
122+
ObjectMeta: metav1.ObjectMeta{
123+
Namespace: ns,
124+
Name: name,
125+
},
126+
Type: "SecretTypeTLS",
127+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
128+
}
129+
}

0 commit comments

Comments
 (0)