Skip to content

Commit 6352c52

Browse files
enjenj
committed
Don't fallback to cert when given invalid token
1 parent 1b04cb2 commit 6352c52

File tree

2 files changed

+223
-10
lines changed

2 files changed

+223
-10
lines changed

‎pkg/cmd/server/origin/master_config.go

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,9 @@ func newServiceAccountTokenGetter(options configapi.MasterConfig, client newetcd
346346
}
347347

348348
func newAuthenticator(config configapi.MasterConfig, restOptionsGetter restoptions.Getter, tokenGetter serviceaccount.ServiceAccountTokenGetter, apiClientCAs *x509.CertPool, groupMapper identitymapper.UserToGroupMapper) (authenticator.Request, error) {
349-
authenticators := []authenticator.Request{}
349+
tokenAuthenticators := []authenticator.Request{} // ServiceAccount and OAuth
350+
mainAuthenticators := []authenticator.Request{} // Above and x509
351+
allAuthenticators := []authenticator.Request{} // Above and anonymous
350352

351353
// ServiceAccount token
352354
if len(config.ServiceAccountConfig.PublicKeyFiles) > 0 {
@@ -359,7 +361,7 @@ func newAuthenticator(config configapi.MasterConfig, restOptionsGetter restoptio
359361
publicKeys = append(publicKeys, publicKey)
360362
}
361363
tokenAuthenticator := serviceaccount.JWTTokenAuthenticator(publicKeys, true, tokenGetter)
362-
authenticators = append(authenticators, bearertoken.New(tokenAuthenticator, true))
364+
tokenAuthenticators = append(tokenAuthenticators, bearertoken.New(tokenAuthenticator, true))
363365
}
364366

365367
// OAuth token
@@ -374,29 +376,38 @@ func newAuthenticator(config configapi.MasterConfig, restOptionsGetter restoptio
374376
paramtoken.New("access_token", tokenAuthenticator, true),
375377
}
376378

377-
authenticators = append(authenticators,
379+
tokenAuthenticators = append(tokenAuthenticators,
378380
// if you have a bearer token, you're a human (usually)
379381
// if you change this, have a look at the impersonationFilter where we attach groups to the impersonated user
380382
group.NewGroupAdder(unionrequest.NewUnionAuthentication(tokenRequestAuthenticators...), []string{bootstrappolicy.AuthenticatedOAuthGroup}))
381383
}
382384

385+
if len(tokenAuthenticators) > 0 {
386+
mainAuthenticators = append(mainAuthenticators, unionrequest.NewUnionAuthentication(tokenAuthenticators...))
387+
}
388+
383389
if configapi.UseTLS(config.ServingInfo.ServingInfo) {
384390
// build cert authenticator
385391
// TODO: add "system:" prefix in authenticator, limit cert to username
386392
// TODO: add "system:" prefix to groups in authenticator, limit cert to group name
387393
opts := x509request.DefaultVerifyOptions()
388394
opts.Roots = apiClientCAs
389395
certauth := x509request.New(opts, x509request.SubjectToUserConversion)
390-
authenticators = append(authenticators, certauth)
396+
mainAuthenticators = append(mainAuthenticators, certauth)
397+
}
398+
399+
if len(mainAuthenticators) > 0 {
400+
authenticatorsUnion := &unionrequest.Authenticator{
401+
FailOnError: true,
402+
Handlers: mainAuthenticators,
403+
}
404+
// if you change this, have a look at the impersonationFilter where we attach groups to the impersonated user
405+
allAuthenticators = append(allAuthenticators, group.NewGroupAdder(authenticatorsUnion, []string{bootstrappolicy.AuthenticatedGroup}))
391406
}
392407

393408
ret := &unionrequest.Authenticator{
394409
FailOnError: true,
395-
Handlers: []authenticator.Request{
396-
// if you change this, have a look at the impersonationFilter where we attach groups to the impersonated user
397-
group.NewGroupAdder(unionrequest.NewUnionAuthentication(authenticators...), []string{bootstrappolicy.AuthenticatedGroup}),
398-
anonymous.NewAuthenticator(),
399-
},
410+
Handlers: append(allAuthenticators, anonymous.NewAuthenticator()),
400411
}
401412

402413
return ret, nil
@@ -650,7 +661,7 @@ func (c *MasterConfig) DeploymentControllerClients() (*osclient.Client, *kclient
650661
return osClient, kClient
651662
}
652663

653-
// DeployerPodControllerClients returns the deployer pod controller client object
664+
// DeployerPodControllerClient returns the deployer pod controller client object
654665
func (c *MasterConfig) DeployerPodControllerClient() *kclient.Client {
655666
return c.PrivilegedLoopbackKubernetesClient
656667
}

‎test/integration/oauth_cert_fallback_test.go

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
// +build integration
2+
3+
package integration
4+
5+
import (
6+
"testing"
7+
8+
oclient "github.com/openshift/origin/pkg/client"
9+
"k8s.io/kubernetes/pkg/client/restclient"
10+
11+
"github.com/openshift/origin/pkg/cmd/util/tokencmd"
12+
testutil "github.com/openshift/origin/test/util"
13+
testserver "github.com/openshift/origin/test/util/server"
14+
)
15+
16+
func TestOAuthCertFallback(t *testing.T) {
17+
18+
var (
19+
invalidKeyData = []byte(`-----BEGIN RSA PRIVATE KEY-----
20+
MIIEogIBAAKCAQEAuY3YLpZsU1HNFOb6n0EoeHyRRfgbu6tXdj0EjQWR8naSm7n0
21+
HxunYDBKf2XODaLAsWcFfvQ4m7XuECtznr3gAATv1AV00POgngQi+wGTI4qmenAh
22+
a8gBdoWnkQYlpdTtnsHHzOU80aEmszoMFtnBC86DLgyT43e5KNUxTtSP0AYuop7x
23+
NRDXcWd4vjByJ8S8CLmkRvQY91EJThMZDa68BgabATJKOyixKFeFW9VpJViK9rsE
24+
R2tsLB1TuTxrH5b6c/wK6DH9hN7HVVwbC1M7UcOGQthB6+3yvqSWPbUwgosCCdaD
25+
z1OnyaMjC5Ait1xjCZxeWDll6MPCWzU2bvRsYwIDAQABAoIBAA1a7T1lLETO9XDU
26+
syM1QGFzrc0Yb36RdYkYGTTBOuD1sdWti6mVhvWAZExJGoyWs0HRhW6+yzhB3vGg
27+
/wBk8DNwJ4beIatMbboR2Cay1VFQkGztlyo3ygsq0YW5qIoIClZL4kKYGUmJTMzH
28+
l8kpQSDFa2GsHBTaMCSFO7hNylARknfm70oAgrV8j3iIAKeBB/a17wCIP78jrX4v
29+
MroarxWr32z9dWFUANyBVNSTo+Tuq4EXslWUFBCRN30zBZ0b7YT4sM86Yt/GYo9s
30+
It79Y0BnUI/ykgrNKGmm0gA79Su1Obh2WQWMW9KpT66KrWuzqtIr5eOgRzZkQsZ3
31+
klGJ7kkCgYEA2TERKA84ZXDn2zgbabejGSuONoX6TjJvT594GVHzrwm8pj8FKmXS
32+
LbbRcUhjcY8e3a4O7NznZ5hfblqjn2NhqvdvRElI9DXwQmf6dXm8lzTacSLSy8j4
33+
W1Nvcf2RD+7J9AymOjF2vvBEnksNgaMGjr6jW3J+vEF4hiTl2plOXa0CgYEA2rWW
34+
uu6bkWzrPRPsQ8iqbhJOqqz9d6hrmHmnKSBUCFpmAmNep4Y+PmHUKEUh8elWzBGh
35+
NsUuc110aK9Fccy7U+NUSnOJWYNmAez2tyVbQS/SErkPgJdAvwwqQHgLFgMnD/Wy
36+
Bm8PXLfgUWs61GZjscjmA02YpJ/HWICkvWjEFE8CgYAXjzgCNWxzrISqBfMLS604
37+
fL4Hcg8NznC+nVjEvlwFn7PEANAJolPjO5KKjESlO9YoS8o4rVm4phGsAc7/6iLd
38+
DcwXBzAPtY4jVe4YMiVf7Y7IePOOwXUXSvyqy8uhg9CKVZjudREhcySuWwvTBSEf
39+
+NP1hnzy5NMzEeuRA9I5XQKBgGtWW5d6q1cAAaOEN5w8y4gh7AHPzMYBHm1Cp0uD
40+
1joTQ6VAZ6AIPlwXXyw0Yah8QGD+9gQPWfC8mPkXrBlhxT4yf5fahDouRs4DIkJY
41+
TyT69zrBIF6X3OrmaYYiZC51daJbjvehYgS7KZhL7B958Mu8MUbFunhxAkDpQfDD
42+
jhf5AoGAKuT+Wc2aeHguZaMmfnAoL3pOu9FH2+QPkHEGQp6vA55QKcAlWLkYd4Me
43+
Pxwj3BzCwiKE44kqBn36rhow9a7N177ATDJta1iUlli2J7PtTRBmM60TOpfU+3i5
44+
UeBTTl1GjZXtgxd486Aw6BsAD/rg2C+8ZcyUba+MYzuNzY4qw6o=
45+
-----END RSA PRIVATE KEY-----`)
46+
invalidCertData = []byte(`-----BEGIN CERTIFICATE-----
47+
MIIDDTCCAfWgAwIBAgIBBzANBgkqhkiG9w0BAQsFADAmMSQwIgYDVQQDDBtvcGVu
48+
c2hpZnQtc2lnbmVyQDE0NjY3OTI3MTAwHhcNMTYwNjI0MTgyNTExWhcNMTgwNjI0
49+
MTgyNTEyWjA3MR4wHAYDVQQKExVzeXN0ZW06Y2x1c3Rlci1hZG1pbnMxFTATBgNV
50+
BAMTDHN5c3RlbTphZG1pbjCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB
51+
ALmN2C6WbFNRzRTm+p9BKHh8kUX4G7urV3Y9BI0FkfJ2kpu59B8bp2AwSn9lzg2i
52+
wLFnBX70OJu17hArc5694AAE79QFdNDzoJ4EIvsBkyOKpnpwIWvIAXaFp5EGJaXU
53+
7Z7Bx8zlPNGhJrM6DBbZwQvOgy4Mk+N3uSjVMU7Uj9AGLqKe8TUQ13FneL4wcifE
54+
vAi5pEb0GPdRCU4TGQ2uvAYGmwEySjsosShXhVvVaSVYiva7BEdrbCwdU7k8ax+W
55+
+nP8Cugx/YTex1VcGwtTO1HDhkLYQevt8r6klj21MIKLAgnWg89Tp8mjIwuQIrdc
56+
YwmcXlg5ZejDwls1Nm70bGMCAwEAAaM1MDMwDgYDVR0PAQH/BAQDAgWgMBMGA1Ud
57+
JQQMMAoGCCsGAQUFBwMCMAwGA1UdEwEB/wQCMAAwDQYJKoZIhvcNAQELBQADggEB
58+
AIKO3Qcl7lsmPImzLX+KZ1h54b0qp9LRbvNe1e9+SMkyMzNhyJExs3qqg7/Z9a8n
59+
LRqjyPPRLOFeoherM+14mnxg9BXxuhoKKZCln3hLiDgzEPZVb9vsDxMKjLy+gRiH
60+
oIyEuzexr/dldk3shmPLDAlpB0Mz+8eBWcXn8cRwXkUyEstY64nUSuMgnL72tU9y
61+
3Yt5D2gTLJ2MUpMxWv+cFz/UJZ0TBKtoKLjtLGRCGwFcUOz6rJzM7QGChfQxMzD7
62+
gydO+4blIu/i4strKbkR5jcRA1WgwS/1yW25F/hE0QglsWyXJJELf4ZCNpr+9zQb
63+
eUArEOSZyp5WmmRcn0v/YZc=
64+
-----END CERTIFICATE-----`)
65+
66+
invalidToken = "invalid"
67+
noToken = ""
68+
69+
invalidCert = restclient.TLSClientConfig{
70+
CertData: invalidCertData,
71+
KeyData: invalidKeyData,
72+
}
73+
noCert = restclient.TLSClientConfig{}
74+
75+
tokenUser = "user"
76+
certUser = "system:admin"
77+
78+
unauthorizedError = "the server has asked for the client to provide credentials (get users ~)"
79+
anonymousError = `User "system:anonymous" cannot get users at the cluster scope`
80+
)
81+
82+
testutil.RequireEtcd(t)
83+
// Build master config
84+
masterOptions, err := testserver.DefaultMasterOptions()
85+
if err != nil {
86+
t.Fatalf("unexpected error: %v", err)
87+
}
88+
89+
// Start server
90+
clusterAdminKubeConfig, err := testserver.StartConfiguredMaster(masterOptions)
91+
if err != nil {
92+
t.Fatalf("unexpected error: %v", err)
93+
}
94+
95+
adminConfig, err := testutil.GetClusterAdminClientConfig(clusterAdminKubeConfig)
96+
if err != nil {
97+
t.Fatalf("unexpected error: %v", err)
98+
}
99+
validCert := adminConfig.TLSClientConfig
100+
101+
validToken, err := tokencmd.RequestToken(adminConfig, nil, tokenUser, "pass")
102+
if err != nil {
103+
t.Fatalf("Unexpected error: %v", err)
104+
}
105+
if len(validToken) == 0 {
106+
t.Fatalf("Expected valid token, got none")
107+
}
108+
109+
for _, test := range []struct {
110+
token string
111+
cert restclient.TLSClientConfig
112+
expectedUser string
113+
errorExpected bool
114+
errorString string
115+
insecure bool
116+
}{
117+
{
118+
token: validToken,
119+
cert: validCert,
120+
expectedUser: tokenUser,
121+
},
122+
{
123+
token: validToken,
124+
cert: invalidCert,
125+
expectedUser: tokenUser,
126+
insecure: true,
127+
},
128+
{
129+
token: validToken,
130+
cert: noCert,
131+
expectedUser: tokenUser,
132+
insecure: true,
133+
},
134+
{
135+
token: invalidToken,
136+
cert: validCert,
137+
errorExpected: true,
138+
errorString: unauthorizedError,
139+
},
140+
{
141+
token: invalidToken,
142+
cert: invalidCert,
143+
errorExpected: true,
144+
errorString: unauthorizedError,
145+
insecure: true,
146+
},
147+
{
148+
token: invalidToken,
149+
cert: noCert,
150+
errorExpected: true,
151+
errorString: unauthorizedError,
152+
insecure: true,
153+
},
154+
{
155+
token: noToken,
156+
cert: validCert,
157+
expectedUser: certUser,
158+
},
159+
{
160+
token: noToken,
161+
cert: invalidCert,
162+
errorExpected: true,
163+
errorString: anonymousError, // TODO should this be unauthorizedError?
164+
insecure: true,
165+
},
166+
{
167+
token: noToken,
168+
cert: noCert,
169+
errorExpected: true,
170+
errorString: anonymousError,
171+
insecure: true,
172+
},
173+
} {
174+
config := *adminConfig
175+
config.BearerToken = test.token
176+
config.TLSClientConfig = test.cert
177+
config.Insecure = test.insecure
178+
179+
client := oclient.NewOrDie(&config)
180+
181+
user, err := client.Users().Get("~")
182+
183+
test.cert = noCert // Just to make errors more readible
184+
185+
if user.Name != test.expectedUser {
186+
t.Errorf("unexpected user %q for test %+v", user.Name, test)
187+
}
188+
189+
if test.errorExpected {
190+
if err == nil {
191+
t.Errorf("expected error but got nil for test %+v", test)
192+
} else if err.Error() != test.errorString {
193+
t.Errorf("unexpected error string %q for test %+v", err.Error(), test)
194+
}
195+
} else {
196+
if err != nil {
197+
t.Errorf("unexpected error %q for test %+v", err.Error(), test)
198+
}
199+
}
200+
}
201+
202+
}

0 commit comments

Comments
 (0)