Skip to content

Commit 56c8474

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1810 from liouk/disable-oauth-admission-plugins
CNTRLPLANE-79: Disable oauth admission plugins
2 parents e946149 + f3f25d4 commit 56c8474

File tree

6 files changed

+522
-4
lines changed

6 files changed

+522
-4
lines changed

‎bindata/assets/config/defaultconfig.yaml‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,6 @@ apiServerArguments:
7070
- TaintNodesByCondition
7171
- ValidatingAdmissionWebhook
7272
- ValidatingAdmissionPolicy
73-
- authorization.openshift.io/RestrictSubjectBindings
74-
- authorization.openshift.io/ValidateRoleBindingRestriction
7573
- config.openshift.io/DenyDeleteClusterConfiguration
7674
- config.openshift.io/ValidateAPIServer
7775
- config.openshift.io/ValidateAuthentication

‎pkg/operator/configobservation/apiserver/observe_admission_plugins.go‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package apiserver
2+
3+
import (
4+
"fmt"
5+
"slices"
6+
7+
configv1 "github.com/openshift/api/config/v1"
8+
"github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation"
9+
"github.com/openshift/library-go/pkg/operator/configobserver"
10+
"github.com/openshift/library-go/pkg/operator/events"
11+
12+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
13+
"k8s.io/apimachinery/pkg/util/sets"
14+
)
15+
16+
type pluginCheckerFunc func(listers configobservation.Listers) (enabled, disabled []string, err error)
17+
18+
var (
19+
enableAdmissionPluginsPath = []string{"apiServerArguments", "enable-admission-plugins"}
20+
disableAdmissionPluginsPath = []string{"apiServerArguments", "disable-admission-plugins"}
21+
22+
pluginCheckers = []pluginCheckerFunc{
23+
roleBindingRestrictionPluginChecker,
24+
}
25+
)
26+
27+
// ObserveAdmissionPlugins manages the apiServerArguments.enable-admission-plugins and
28+
// apiServerArguments.disable-admission-plugins fields of the configuration. It defines a list of
29+
// plugin checkers which check the state of specific plugins, and add them to the enabled or disabled
30+
// list as required. This observer will overwrite any pre-existing values of the two fields in the existingConfig.
31+
func ObserveAdmissionPlugins(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]any) (ret map[string]any, _ []error) {
32+
defer func() {
33+
ret = configobserver.Pruned(ret, enableAdmissionPluginsPath, disableAdmissionPluginsPath)
34+
}()
35+
36+
if len(pluginCheckers) == 0 {
37+
return existingConfig, nil
38+
}
39+
40+
listers := genericListers.(configobservation.Listers)
41+
enabledSet := sets.New[string]()
42+
disabledSet := sets.New[string]()
43+
44+
for _, pluginChecker := range pluginCheckers {
45+
enabled, disabled, err := pluginChecker(listers)
46+
if err != nil {
47+
return existingConfig, []error{err}
48+
}
49+
50+
enabledSet.Insert(enabled...)
51+
disabledSet.Insert(disabled...)
52+
}
53+
54+
if intersection := enabledSet.Intersection(disabledSet); intersection.Len() > 0 {
55+
return existingConfig, []error{fmt.Errorf("plugins cannot be enabled and disabled at the same time: %v", intersection.UnsortedList())}
56+
}
57+
58+
observedConfig := map[string]any{}
59+
60+
if enabledSet.Len() > 0 {
61+
sorted := slices.Sorted[string](slices.Values(enabledSet.UnsortedList()))
62+
err := unstructured.SetNestedStringSlice(observedConfig, sorted, enableAdmissionPluginsPath...)
63+
if err != nil {
64+
return existingConfig, []error{err}
65+
}
66+
}
67+
68+
if disabledSet.Len() > 0 {
69+
sorted := slices.Sorted[string](slices.Values(disabledSet.UnsortedList()))
70+
err := unstructured.SetNestedStringSlice(observedConfig, sorted, disableAdmissionPluginsPath...)
71+
if err != nil {
72+
return existingConfig, []error{err}
73+
}
74+
}
75+
76+
return observedConfig, nil
77+
}
78+
79+
func roleBindingRestrictionPluginChecker(listers configobservation.Listers) (enabled, disabled []string, err error) {
80+
auth, err := listers.AuthConfigLister.Get("cluster")
81+
if err != nil {
82+
return
83+
}
84+
85+
rbrPlugins := []string{
86+
"authorization.openshift.io/RestrictSubjectBindings",
87+
"authorization.openshift.io/ValidateRoleBindingRestriction",
88+
}
89+
90+
switch auth.Spec.Type {
91+
case configv1.AuthenticationTypeIntegratedOAuth, "":
92+
enabled = rbrPlugins
93+
94+
case configv1.AuthenticationTypeNone, configv1.AuthenticationTypeOIDC:
95+
disabled = rbrPlugins
96+
}
97+
98+
return
99+
}

‎pkg/operator/configobservation/apiserver/observe_admission_plugins_test.go‎

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
package apiserver
2+
3+
import (
4+
"fmt"
5+
"testing"
6+
"time"
7+
8+
configv1 "github.com/openshift/api/config/v1"
9+
configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
10+
"github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/configobservation"
11+
"github.com/openshift/library-go/pkg/operator/events"
12+
13+
"k8s.io/apimachinery/pkg/api/equality"
14+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
"k8s.io/apimachinery/pkg/util/diff"
16+
"k8s.io/client-go/tools/cache"
17+
clocktesting "k8s.io/utils/clock/testing"
18+
"k8s.io/utils/ptr"
19+
)
20+
21+
func TestObserveAdmissionPlugins(t *testing.T) {
22+
for _, tt := range []struct {
23+
name string
24+
pluginCheckers []pluginCheckerFunc
25+
existingConfig map[string]any
26+
27+
expectErrors bool
28+
expectedConfig map[string]any
29+
}{
30+
{
31+
name: "no plugin checkers available",
32+
existingConfig: map[string]any{"key": "value"},
33+
pluginCheckers: []pluginCheckerFunc{},
34+
expectErrors: false,
35+
expectedConfig: map[string]any{},
36+
},
37+
{
38+
name: "observer returns pruned config",
39+
existingConfig: map[string]any{
40+
"key": "value",
41+
"apiServerArguments": map[string]any{
42+
"enable-admission-plugins": []any{"enabled1", "enabled2"},
43+
"disable-admission-plugins": []any{"disabled1", "disabled2"},
44+
},
45+
},
46+
pluginCheckers: []pluginCheckerFunc{},
47+
expectErrors: false,
48+
expectedConfig: map[string]any{
49+
"apiServerArguments": map[string]any{
50+
"enable-admission-plugins": []any{"enabled1", "enabled2"},
51+
"disable-admission-plugins": []any{"disabled1", "disabled2"},
52+
},
53+
},
54+
},
55+
{
56+
name: "plugin checker with error",
57+
existingConfig: map[string]any{"key": "value"},
58+
pluginCheckers: []pluginCheckerFunc{
59+
func(_ configobservation.Listers) ([]string, []string, error) {
60+
return nil, nil, fmt.Errorf("plugin checker error")
61+
},
62+
},
63+
expectErrors: true,
64+
expectedConfig: map[string]any{},
65+
},
66+
{
67+
name: "plugin checkers must enable and disable plugins",
68+
existingConfig: map[string]any{"key": "value"},
69+
pluginCheckers: []pluginCheckerFunc{
70+
func(_ configobservation.Listers) ([]string, []string, error) {
71+
return []string{"enabled1"}, nil, nil
72+
},
73+
func(_ configobservation.Listers) ([]string, []string, error) {
74+
return nil, []string{"disabled1"}, nil
75+
},
76+
func(_ configobservation.Listers) ([]string, []string, error) {
77+
return []string{"enabled2"}, []string{"disabled2"}, nil
78+
},
79+
},
80+
expectErrors: false,
81+
expectedConfig: map[string]any{
82+
"apiServerArguments": map[string]any{
83+
"enable-admission-plugins": []any{"enabled1", "enabled2"},
84+
"disable-admission-plugins": []any{"disabled1", "disabled2"},
85+
},
86+
},
87+
},
88+
{
89+
name: "plugin checkers must overwrite existing enabled and disabled",
90+
existingConfig: map[string]any{
91+
"key": "value",
92+
"apiServerArguments": map[string]any{
93+
"enable-admission-plugins": []any{"another1"},
94+
"disable-admission-plugins": []any{"another2"},
95+
},
96+
},
97+
pluginCheckers: []pluginCheckerFunc{
98+
func(_ configobservation.Listers) ([]string, []string, error) {
99+
return []string{"enabled1"}, nil, nil
100+
},
101+
func(_ configobservation.Listers) ([]string, []string, error) {
102+
return nil, []string{"disabled1"}, nil
103+
},
104+
func(_ configobservation.Listers) ([]string, []string, error) {
105+
return []string{"enabled2"}, []string{"disabled2"}, nil
106+
},
107+
},
108+
expectErrors: false,
109+
expectedConfig: map[string]any{
110+
"apiServerArguments": map[string]any{
111+
"enable-admission-plugins": []any{"enabled1", "enabled2"},
112+
"disable-admission-plugins": []any{"disabled1", "disabled2"},
113+
},
114+
},
115+
},
116+
{
117+
name: "plugin checkers must return disjoint enabled and disabled plugin slices",
118+
pluginCheckers: []pluginCheckerFunc{
119+
func(_ configobservation.Listers) ([]string, []string, error) {
120+
return []string{"enabled1", "enabled2"}, nil, nil
121+
},
122+
func(_ configobservation.Listers) ([]string, []string, error) {
123+
return []string{"enabled3"}, []string{"enabled2"}, nil
124+
},
125+
},
126+
expectErrors: true,
127+
},
128+
} {
129+
t.Run(tt.name, func(t *testing.T) {
130+
pluginCheckers = tt.pluginCheckers
131+
132+
eventRecorder := events.NewInMemoryRecorder("TestObserveAdmissionPlugins", clocktesting.NewFakePassiveClock(time.Now()))
133+
listers := configobservation.Listers{}
134+
gotConfig, gotErrs := ObserveAdmissionPlugins(listers, eventRecorder, tt.existingConfig)
135+
136+
if tt.expectErrors != (len(gotErrs) > 0) {
137+
t.Errorf("expected errors: %v; got %v", tt.expectErrors, gotErrs)
138+
}
139+
140+
if !equality.Semantic.DeepEqual(tt.expectedConfig, gotConfig) {
141+
t.Errorf("unexpected config diff: %s", diff.ObjectReflectDiff(tt.expectedConfig, gotConfig))
142+
}
143+
})
144+
}
145+
}
146+
147+
func TestRoleBindingRestrictionPluginChecker(t *testing.T) {
148+
for _, tt := range []struct {
149+
name string
150+
authType *configv1.AuthenticationType
151+
expectedEnabled []string
152+
expectedDisabled []string
153+
expectError bool
154+
}{
155+
{
156+
name: "authentication cluster not found",
157+
expectError: true,
158+
},
159+
{
160+
name: "auth type IntegratedOAuth",
161+
authType: ptr.To(configv1.AuthenticationTypeIntegratedOAuth),
162+
expectedEnabled: []string{
163+
"authorization.openshift.io/RestrictSubjectBindings",
164+
"authorization.openshift.io/ValidateRoleBindingRestriction",
165+
},
166+
expectedDisabled: []string{},
167+
expectError: false,
168+
},
169+
{
170+
name: "auth type empty string",
171+
authType: ptr.To(configv1.AuthenticationType("")),
172+
expectedEnabled: []string{
173+
"authorization.openshift.io/RestrictSubjectBindings",
174+
"authorization.openshift.io/ValidateRoleBindingRestriction",
175+
},
176+
expectedDisabled: []string{},
177+
expectError: false,
178+
},
179+
{
180+
name: "auth type None",
181+
authType: ptr.To(configv1.AuthenticationTypeNone),
182+
expectedEnabled: []string{},
183+
expectedDisabled: []string{
184+
"authorization.openshift.io/RestrictSubjectBindings",
185+
"authorization.openshift.io/ValidateRoleBindingRestriction",
186+
},
187+
expectError: false,
188+
},
189+
{
190+
name: "auth type OIDC",
191+
authType: ptr.To(configv1.AuthenticationTypeOIDC),
192+
expectedEnabled: []string{},
193+
expectedDisabled: []string{
194+
"authorization.openshift.io/RestrictSubjectBindings",
195+
"authorization.openshift.io/ValidateRoleBindingRestriction",
196+
},
197+
expectError: false,
198+
},
199+
} {
200+
t.Run(tt.name, func(t *testing.T) {
201+
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
202+
if tt.authType != nil {
203+
indexer.Add(&configv1.Authentication{
204+
ObjectMeta: metav1.ObjectMeta{
205+
Name: "cluster",
206+
},
207+
Spec: configv1.AuthenticationSpec{
208+
Type: *tt.authType,
209+
},
210+
})
211+
}
212+
213+
listers := configobservation.Listers{
214+
AuthConfigLister: configlistersv1.NewAuthenticationLister(indexer),
215+
}
216+
217+
enabled, disabled, err := roleBindingRestrictionPluginChecker(listers)
218+
if tt.expectError != (err != nil) {
219+
t.Errorf("expected errors: %v; got %v", tt.expectError, err)
220+
}
221+
222+
if !equality.Semantic.DeepEqual(tt.expectedEnabled, enabled) {
223+
t.Errorf("unexpected enabled plugins: %s", diff.ObjectReflectDiff(tt.expectedEnabled, enabled))
224+
}
225+
226+
if !equality.Semantic.DeepEqual(tt.expectedDisabled, disabled) {
227+
t.Errorf("unexpected disabled plugins: %s", diff.ObjectReflectDiff(tt.expectedDisabled, disabled))
228+
}
229+
})
230+
}
231+
}

‎pkg/operator/configobservation/configobservercontroller/observe_config_controller.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ func NewConfigObserver(operatorClient v1helpers.StaticPodOperatorClient, kubeInf
123123
apiserver.ObserveShutdownDelayDuration,
124124
apiserver.ObserveGracefulTerminationDuration,
125125
apiserver.ObserveSendRetryAfterWhileNotReadyOnce,
126+
apiserver.ObserveAdmissionPlugins,
126127
libgoapiserver.ObserveTLSSecurityProfile,
127128
auth.ObserveAuthMetadata,
128129
auth.ObserveServiceAccountIssuer,

0 commit comments

Comments
 (0)