Skip to content

Commit 56ebf57

Browse files
liggittliggitt
committed
policy unsafe proxy requests separately
1 parent 06b1616 commit 56ebf57

File tree

5 files changed

+239
-4
lines changed

5 files changed

+239
-4
lines changed

‎pkg/authorization/authorizer/attributes_builder.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,14 @@ import (
55
"strings"
66

77
kapi "k8s.io/kubernetes/pkg/api"
8-
kapiserver "k8s.io/kubernetes/pkg/apiserver"
98
)
109

1110
type openshiftAuthorizationAttributeBuilder struct {
1211
contextMapper kapi.RequestContextMapper
13-
infoResolver *kapiserver.RequestInfoResolver
12+
infoResolver RequestInfoResolver
1413
}
1514

16-
func NewAuthorizationAttributeBuilder(contextMapper kapi.RequestContextMapper, infoResolver *kapiserver.RequestInfoResolver) AuthorizationAttributeBuilder {
15+
func NewAuthorizationAttributeBuilder(contextMapper kapi.RequestContextMapper, infoResolver RequestInfoResolver) AuthorizationAttributeBuilder {
1716
return &openshiftAuthorizationAttributeBuilder{contextMapper, infoResolver}
1817
}
1918

‎pkg/authorization/authorizer/browser_safe_request_info_resolver.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
package authorizer
2+
3+
import (
4+
"net/http"
5+
6+
kapi "k8s.io/kubernetes/pkg/api"
7+
kapiserver "k8s.io/kubernetes/pkg/apiserver"
8+
"k8s.io/kubernetes/pkg/util/sets"
9+
)
10+
11+
type browserSafeRequestInfoResolver struct {
12+
// infoResolver is used to determine info for the request
13+
infoResolver RequestInfoResolver
14+
15+
// contextMapper is used to look up the context corresponding to a request
16+
// to obtain the user associated with the request
17+
contextMapper kapi.RequestContextMapper
18+
19+
// list of groups, any of which indicate the request is authenticated
20+
authenticatedGroups sets.String
21+
}
22+
23+
func NewBrowserSafeRequestInfoResolver(contextMapper kapi.RequestContextMapper, authenticatedGroups sets.String, infoResolver RequestInfoResolver) RequestInfoResolver {
24+
return &browserSafeRequestInfoResolver{
25+
contextMapper: contextMapper,
26+
authenticatedGroups: authenticatedGroups,
27+
infoResolver: infoResolver,
28+
}
29+
}
30+
31+
func (a *browserSafeRequestInfoResolver) GetRequestInfo(req *http.Request) (kapiserver.RequestInfo, error) {
32+
requestInfo, err := a.infoResolver.GetRequestInfo(req)
33+
if err != nil {
34+
return requestInfo, err
35+
}
36+
37+
if !requestInfo.IsResourceRequest {
38+
return requestInfo, nil
39+
}
40+
41+
isProxyVerb := requestInfo.Verb == "proxy"
42+
isProxySubresource := requestInfo.Subresource == "proxy"
43+
44+
if !isProxyVerb && !isProxySubresource {
45+
// Requests to non-proxy resources don't expose HTML or HTTP-handling user content to browsers
46+
return requestInfo, nil
47+
}
48+
49+
if len(req.Header.Get("X-CSRF-Token")) > 0 {
50+
// Browsers cannot set custom headers on direct requests
51+
return requestInfo, nil
52+
}
53+
54+
if ctx, hasContext := a.contextMapper.Get(req); hasContext {
55+
user, hasUser := kapi.UserFrom(ctx)
56+
if hasUser && a.authenticatedGroups.HasAny(user.GetGroups()...) {
57+
// An authenticated request indicates this isn't a browser page load.
58+
// Browsers cannot make direct authenticated requests.
59+
// This depends on the API not enabling basic or cookie-based auth.
60+
return requestInfo, nil
61+
}
62+
63+
}
64+
65+
// TODO: compare request.Host to a list of hosts allowed for the requestInfo.Namespace (e.g. <namespace>.proxy.example.com)
66+
67+
if isProxyVerb {
68+
requestInfo.Verb = "unsafeproxy"
69+
}
70+
if isProxySubresource {
71+
requestInfo.Subresource = "unsafeproxy"
72+
}
73+
74+
return requestInfo, nil
75+
}

‎pkg/authorization/authorizer/browser_safe_request_info_resolver_test.go

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
package authorizer
2+
3+
import (
4+
"net/http"
5+
"testing"
6+
7+
kapi "k8s.io/kubernetes/pkg/api"
8+
kapiserver "k8s.io/kubernetes/pkg/apiserver"
9+
"k8s.io/kubernetes/pkg/auth/user"
10+
"k8s.io/kubernetes/pkg/util/sets"
11+
)
12+
13+
func TestUpstreamInfoResolver(t *testing.T) {
14+
subresourceRequest, _ := http.NewRequest("GET", "/api/v1/namespaces/myns/pods/mypod/proxy", nil)
15+
proxyRequest, _ := http.NewRequest("GET", "/api/v1/proxy/nodes/mynode", nil)
16+
17+
testcases := map[string]struct {
18+
Request *http.Request
19+
ExpectedVerb string
20+
ExpectedSubresource string
21+
}{
22+
"unsafe proxy subresource": {
23+
Request: subresourceRequest,
24+
ExpectedVerb: "get",
25+
ExpectedSubresource: "proxy", // should be "unsafeproxy" or similar once check moves upstream
26+
},
27+
"unsafe proxy verb": {
28+
Request: proxyRequest,
29+
ExpectedVerb: "proxy", // should be "unsafeproxy" or similar once check moves upstream
30+
},
31+
}
32+
33+
for k, tc := range testcases {
34+
resolver := &kapiserver.RequestInfoResolver{
35+
APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"),
36+
GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi"),
37+
}
38+
39+
info, err := resolver.GetRequestInfo(tc.Request)
40+
if err != nil {
41+
t.Errorf("%s: unexpected error: %v", k, err)
42+
continue
43+
}
44+
45+
if info.Verb != tc.ExpectedVerb {
46+
t.Errorf("%s: expected verb %s, got %s. If kapiserver.RequestInfoResolver now adjusts attributes for proxy safety, investigate removing the NewBrowserSafeRequestInfoResolver wrapper.", k, tc.ExpectedVerb, info.Verb)
47+
}
48+
if info.Subresource != tc.ExpectedSubresource {
49+
t.Errorf("%s: expected verb %s, got %s. If kapiserver.RequestInfoResolver now adjusts attributes for proxy safety, investigate removing the NewBrowserSafeRequestInfoResolver wrapper.", k, tc.ExpectedSubresource, info.Subresource)
50+
}
51+
}
52+
}
53+
54+
func TestBrowserSafeRequestInfoResolver(t *testing.T) {
55+
testcases := map[string]struct {
56+
RequestInfo kapiserver.RequestInfo
57+
Context kapi.Context
58+
Host string
59+
Headers http.Header
60+
61+
ExpectedVerb string
62+
ExpectedSubresource string
63+
}{
64+
"non-resource": {
65+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: false, Verb: "GET"},
66+
ExpectedVerb: "GET",
67+
},
68+
69+
"non-proxy": {
70+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "logs"},
71+
ExpectedVerb: "get",
72+
ExpectedSubresource: "logs",
73+
},
74+
75+
"unsafe proxy subresource": {
76+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "proxy"},
77+
ExpectedVerb: "get",
78+
ExpectedSubresource: "unsafeproxy",
79+
},
80+
"unsafe proxy verb": {
81+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: true, Verb: "proxy", Resource: "nodes"},
82+
ExpectedVerb: "unsafeproxy",
83+
},
84+
"unsafe proxy verb anonymous": {
85+
Context: kapi.WithUser(kapi.NewContext(), &user.DefaultInfo{Name: "system:anonymous", Groups: []string{"system:unauthenticated"}}),
86+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: true, Verb: "proxy", Resource: "nodes"},
87+
ExpectedVerb: "unsafeproxy",
88+
},
89+
90+
"proxy subresource authenticated": {
91+
Context: kapi.WithUser(kapi.NewContext(), &user.DefaultInfo{Name: "bob", Groups: []string{"system:authenticated"}}),
92+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "proxy"},
93+
ExpectedVerb: "get",
94+
ExpectedSubresource: "proxy",
95+
},
96+
"proxy subresource custom header": {
97+
RequestInfo: kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "proxy"},
98+
Headers: http.Header{"X-Csrf-Token": []string{"1"}},
99+
ExpectedVerb: "get",
100+
ExpectedSubresource: "proxy",
101+
},
102+
}
103+
104+
for k, tc := range testcases {
105+
resolver := NewBrowserSafeRequestInfoResolver(
106+
&testContextMapper{tc.Context},
107+
sets.NewString("system:authenticated"),
108+
&testInfoResolver{tc.RequestInfo},
109+
)
110+
111+
req, _ := http.NewRequest("GET", "/", nil)
112+
req.Host = tc.Host
113+
req.Header = tc.Headers
114+
115+
info, err := resolver.GetRequestInfo(req)
116+
if err != nil {
117+
t.Errorf("%s: unexpected error: %v", k, err)
118+
continue
119+
}
120+
121+
if info.Verb != tc.ExpectedVerb {
122+
t.Errorf("%s: expected verb %s, got %s", k, tc.ExpectedVerb, info.Verb)
123+
}
124+
if info.Subresource != tc.ExpectedSubresource {
125+
t.Errorf("%s: expected verb %s, got %s", k, tc.ExpectedSubresource, info.Subresource)
126+
}
127+
}
128+
}
129+
130+
type testContextMapper struct {
131+
context kapi.Context
132+
}
133+
134+
func (t *testContextMapper) Get(req *http.Request) (kapi.Context, bool) {
135+
return t.context, t.context != nil
136+
}
137+
func (t *testContextMapper) Update(req *http.Request, ctx kapi.Context) error {
138+
return nil
139+
}
140+
141+
type testInfoResolver struct {
142+
info kapiserver.RequestInfo
143+
}
144+
145+
func (t *testInfoResolver) GetRequestInfo(req *http.Request) (kapiserver.RequestInfo, error) {
146+
return t.info, nil
147+
}

‎pkg/authorization/authorizer/interfaces.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"net/http"
55

66
kapi "k8s.io/kubernetes/pkg/api"
7+
kapiserver "k8s.io/kubernetes/pkg/apiserver"
78
"k8s.io/kubernetes/pkg/auth/user"
89
"k8s.io/kubernetes/pkg/util/sets"
910
)
@@ -17,6 +18,10 @@ type AuthorizationAttributeBuilder interface {
1718
GetAttributes(request *http.Request) (AuthorizationAttributes, error)
1819
}
1920

21+
type RequestInfoResolver interface {
22+
GetRequestInfo(req *http.Request) (kapiserver.RequestInfo, error)
23+
}
24+
2025
type AuthorizationAttributes interface {
2126
GetVerb() string
2227
GetAPIVersion() string

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -375,7 +375,16 @@ func newAuthorizer(policyClient policyclient.ReadOnlyPolicyClient, projectReques
375375
}
376376

377377
func newAuthorizationAttributeBuilder(requestContextMapper kapi.RequestContextMapper) authorizer.AuthorizationAttributeBuilder {
378-
authorizationAttributeBuilder := authorizer.NewAuthorizationAttributeBuilder(requestContextMapper, &apiserver.RequestInfoResolver{APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"), GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi")})
378+
// Default API request resolver
379+
requestInfoResolver := &apiserver.RequestInfoResolver{APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"), GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi")}
380+
// Wrap with a resolver that detects unsafe requests and modifies verbs/resources appropriately so policy can address them separately
381+
browserSafeRequestInfoResolver := authorizer.NewBrowserSafeRequestInfoResolver(
382+
requestContextMapper,
383+
sets.NewString(bootstrappolicy.AuthenticatedGroup),
384+
requestInfoResolver,
385+
)
386+
387+
authorizationAttributeBuilder := authorizer.NewAuthorizationAttributeBuilder(requestContextMapper, browserSafeRequestInfoResolver)
379388
return authorizationAttributeBuilder
380389
}
381390

0 commit comments

Comments
 (0)