policy unsafe proxy requests separately

pkg/authorization/authorizer/attributes_builder.go

@@ -5,15 +5,14 @@ import (
 	"strings"
 
 	kapi "k8s.io/kubernetes/pkg/api"
-	kapiserver "k8s.io/kubernetes/pkg/apiserver"
 )
 
 type openshiftAuthorizationAttributeBuilder struct {
 	contextMapper kapi.RequestContextMapper
-	infoResolver  *kapiserver.RequestInfoResolver
+	infoResolver  RequestInfoResolver
 }
 
-func NewAuthorizationAttributeBuilder(contextMapper kapi.RequestContextMapper, infoResolver *kapiserver.RequestInfoResolver) AuthorizationAttributeBuilder {
+func NewAuthorizationAttributeBuilder(contextMapper kapi.RequestContextMapper, infoResolver RequestInfoResolver) AuthorizationAttributeBuilder {
 	return &openshiftAuthorizationAttributeBuilder{contextMapper, infoResolver}
 }
 

pkg/authorization/authorizer/browser_safe_request_info_resolver.go

@@ -0,0 +1,75 @@
+package authorizer
+
+import (
+	"net/http"
+
+	kapi "k8s.io/kubernetes/pkg/api"
+	kapiserver "k8s.io/kubernetes/pkg/apiserver"
+	"k8s.io/kubernetes/pkg/util/sets"
+)
+
+type browserSafeRequestInfoResolver struct {
+	// infoResolver is used to determine info for the request
+	infoResolver RequestInfoResolver
+
+	// contextMapper is used to look up the context corresponding to a request
+	// to obtain the user associated with the request
+	contextMapper kapi.RequestContextMapper
+
+	// list of groups, any of which indicate the request is authenticated
+	authenticatedGroups sets.String
+}
+
+func NewBrowserSafeRequestInfoResolver(contextMapper kapi.RequestContextMapper, authenticatedGroups sets.String, infoResolver RequestInfoResolver) RequestInfoResolver {
+	return &browserSafeRequestInfoResolver{
+		contextMapper:       contextMapper,
+		authenticatedGroups: authenticatedGroups,
+		infoResolver:        infoResolver,
+	}
+}
+
+func (a *browserSafeRequestInfoResolver) GetRequestInfo(req *http.Request) (kapiserver.RequestInfo, error) {
+	requestInfo, err := a.infoResolver.GetRequestInfo(req)
+	if err != nil {
+		return requestInfo, err
+	}
+
+	if !requestInfo.IsResourceRequest {
+		return requestInfo, nil
+	}
+
+	isProxyVerb := requestInfo.Verb == "proxy"
+	isProxySubresource := requestInfo.Subresource == "proxy"
+
+	if !isProxyVerb && !isProxySubresource {
+		// Requests to non-proxy resources don't expose HTML or HTTP-handling user content to browsers
+		return requestInfo, nil
+	}
+
+	if len(req.Header.Get("X-CSRF-Token")) > 0 {
+		// Browsers cannot set custom headers on direct requests
+		return requestInfo, nil
+	}
+
+	if ctx, hasContext := a.contextMapper.Get(req); hasContext {
+		user, hasUser := kapi.UserFrom(ctx)
+		if hasUser && a.authenticatedGroups.HasAny(user.GetGroups()...) {
+			// An authenticated request indicates this isn't a browser page load.
+			// Browsers cannot make direct authenticated requests.
+			// This depends on the API not enabling basic or cookie-based auth.
+			return requestInfo, nil
+		}
+
+	}
+
+	// TODO: compare request.Host to a list of hosts allowed for the requestInfo.Namespace (e.g. <namespace>.proxy.example.com)
+
+	if isProxyVerb {
+		requestInfo.Verb = "unsafeproxy"
+	}
+	if isProxySubresource {
+		requestInfo.Subresource = "unsafeproxy"
+	}
+
+	return requestInfo, nil
+}

pkg/authorization/authorizer/browser_safe_request_info_resolver_test.go

@@ -0,0 +1,147 @@
+package authorizer
+
+import (
+	"net/http"
+	"testing"
+
+	kapi "k8s.io/kubernetes/pkg/api"
+	kapiserver "k8s.io/kubernetes/pkg/apiserver"
+	"k8s.io/kubernetes/pkg/auth/user"
+	"k8s.io/kubernetes/pkg/util/sets"
+)
+
+func TestUpstreamInfoResolver(t *testing.T) {
+	subresourceRequest, _ := http.NewRequest("GET", "/api/v1/namespaces/myns/pods/mypod/proxy", nil)
+	proxyRequest, _ := http.NewRequest("GET", "/api/v1/proxy/nodes/mynode", nil)
+
+	testcases := map[string]struct {
+		Request             *http.Request
+		ExpectedVerb        string
+		ExpectedSubresource string
+	}{
+		"unsafe proxy subresource": {
+			Request:             subresourceRequest,
+			ExpectedVerb:        "get",
+			ExpectedSubresource: "proxy", // should be "unsafeproxy" or similar once check moves upstream
+		},
+		"unsafe proxy verb": {
+			Request:      proxyRequest,
+			ExpectedVerb: "proxy", // should be "unsafeproxy" or similar once check moves upstream
+		},
+	}
+
+	for k, tc := range testcases {
+		resolver := &kapiserver.RequestInfoResolver{
+			APIPrefixes:          sets.NewString("api", "osapi", "oapi", "apis"),
+			GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi"),
+		}
+
+		info, err := resolver.GetRequestInfo(tc.Request)
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", k, err)
+			continue
+		}
+
+		if info.Verb != tc.ExpectedVerb {
+			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)
+		}
+		if info.Subresource != tc.ExpectedSubresource {
+			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)
+		}
+	}
+}
+
+func TestBrowserSafeRequestInfoResolver(t *testing.T) {
+	testcases := map[string]struct {
+		RequestInfo kapiserver.RequestInfo
+		Context     kapi.Context
+		Host        string
+		Headers     http.Header
+
+		ExpectedVerb        string
+		ExpectedSubresource string
+	}{
+		"non-resource": {
+			RequestInfo:  kapiserver.RequestInfo{IsResourceRequest: false, Verb: "GET"},
+			ExpectedVerb: "GET",
+		},
+
+		"non-proxy": {
+			RequestInfo:         kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "logs"},
+			ExpectedVerb:        "get",
+			ExpectedSubresource: "logs",
+		},
+
+		"unsafe proxy subresource": {
+			RequestInfo:         kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "proxy"},
+			ExpectedVerb:        "get",
+			ExpectedSubresource: "unsafeproxy",
+		},
+		"unsafe proxy verb": {
+			RequestInfo:  kapiserver.RequestInfo{IsResourceRequest: true, Verb: "proxy", Resource: "nodes"},
+			ExpectedVerb: "unsafeproxy",
+		},
+		"unsafe proxy verb anonymous": {
+			Context:      kapi.WithUser(kapi.NewContext(), &user.DefaultInfo{Name: "system:anonymous", Groups: []string{"system:unauthenticated"}}),
+			RequestInfo:  kapiserver.RequestInfo{IsResourceRequest: true, Verb: "proxy", Resource: "nodes"},
+			ExpectedVerb: "unsafeproxy",
+		},
+
+		"proxy subresource authenticated": {
+			Context:             kapi.WithUser(kapi.NewContext(), &user.DefaultInfo{Name: "bob", Groups: []string{"system:authenticated"}}),
+			RequestInfo:         kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "proxy"},
+			ExpectedVerb:        "get",
+			ExpectedSubresource: "proxy",
+		},
+		"proxy subresource custom header": {
+			RequestInfo:         kapiserver.RequestInfo{IsResourceRequest: true, Verb: "get", Resource: "pods", Subresource: "proxy"},
+			Headers:             http.Header{"X-Csrf-Token": []string{"1"}},
+			ExpectedVerb:        "get",
+			ExpectedSubresource: "proxy",
+		},
+	}
+
+	for k, tc := range testcases {
+		resolver := NewBrowserSafeRequestInfoResolver(
+			&testContextMapper{tc.Context},
+			sets.NewString("system:authenticated"),
+			&testInfoResolver{tc.RequestInfo},
+		)
+
+		req, _ := http.NewRequest("GET", "/", nil)
+		req.Host = tc.Host
+		req.Header = tc.Headers
+
+		info, err := resolver.GetRequestInfo(req)
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", k, err)
+			continue
+		}
+
+		if info.Verb != tc.ExpectedVerb {
+			t.Errorf("%s: expected verb %s, got %s", k, tc.ExpectedVerb, info.Verb)
+		}
+		if info.Subresource != tc.ExpectedSubresource {
+			t.Errorf("%s: expected verb %s, got %s", k, tc.ExpectedSubresource, info.Subresource)
+		}
+	}
+}
+
+type testContextMapper struct {
+	context kapi.Context
+}
+
+func (t *testContextMapper) Get(req *http.Request) (kapi.Context, bool) {
+	return t.context, t.context != nil
+}
+func (t *testContextMapper) Update(req *http.Request, ctx kapi.Context) error {
+	return nil
+}
+
+type testInfoResolver struct {
+	info kapiserver.RequestInfo
+}
+
+func (t *testInfoResolver) GetRequestInfo(req *http.Request) (kapiserver.RequestInfo, error) {
+	return t.info, nil
+}

pkg/authorization/authorizer/interfaces.go

@@ -4,6 +4,7 @@ import (
 	"net/http"
 
 	kapi "k8s.io/kubernetes/pkg/api"
+	kapiserver "k8s.io/kubernetes/pkg/apiserver"
 	"k8s.io/kubernetes/pkg/auth/user"
 	"k8s.io/kubernetes/pkg/util/sets"
 )
@@ -17,6 +18,10 @@ type AuthorizationAttributeBuilder interface {
 	GetAttributes(request *http.Request) (AuthorizationAttributes, error)
 }
 
+type RequestInfoResolver interface {
+	GetRequestInfo(req *http.Request) (kapiserver.RequestInfo, error)
+}
+
 type AuthorizationAttributes interface {
 	GetVerb() string
 	GetAPIVersion() string

pkg/cmd/server/origin/master_config.go

@@ -375,7 +375,16 @@ func newAuthorizer(policyClient policyclient.ReadOnlyPolicyClient, projectReques
 }
 
 func newAuthorizationAttributeBuilder(requestContextMapper kapi.RequestContextMapper) authorizer.AuthorizationAttributeBuilder {
-	authorizationAttributeBuilder := authorizer.NewAuthorizationAttributeBuilder(requestContextMapper, &apiserver.RequestInfoResolver{APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"), GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi")})
+	// Default API request resolver
+	requestInfoResolver := &apiserver.RequestInfoResolver{APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"), GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi")}
+	// Wrap with a resolver that detects unsafe requests and modifies verbs/resources appropriately so policy can address them separately
+	browserSafeRequestInfoResolver := authorizer.NewBrowserSafeRequestInfoResolver(
+		requestContextMapper,
+		sets.NewString(bootstrappolicy.AuthenticatedGroup),
+		requestInfoResolver,
+	)
+
+	authorizationAttributeBuilder := authorizer.NewAuthorizationAttributeBuilder(requestContextMapper, browserSafeRequestInfoResolver)
 	return authorizationAttributeBuilder
 }