Merge pull request #16741 from enj/enj/i/unsafeproxy_reqinfo/16619

Automatic merge from submit-queue (batch tested with PRs 16741, 16692).

Move browser safe proxy logic into an authorizer

This change moves the browser safe proxy verification from a request info resolver to an authorizer.  It is required because we moved to the upstream filters in 689cdee.  The upstream handler chain resolves request info, then authentication and then authorization.  Our old chain would mutate request info after authentication and before authorization.  This meant that we had access to the user info data.  Since the upstream handler chain does any mutation to the request info before authentication, our browser safe proxy request info resolver would never see any user data.  This meant that it would incorrectly identify safe proxy requests as unsafe.  To continue using the upstream chain, we must move this logic into an authorizer that wraps our existing authorizers.  This will guarantee that it has access to the authentication information it requires.  Since the authorizer only has access to authorization attributes, it cannot make any decisions based on the context or request info.  In this case, it means that we can no longer honor the X-CSRF-Token header as a way of confirming that the request is not originating from a browser.  This is an acceptable break from legacy behavior as that header was never documented as a way of passing the browser check.  The most likely user of this feature is the proxy command, which is easy to use in an authenticated manner with either oc or kubectl.  Administrators can continue to grant the unsafe verb to unauthenticated users should they desire to bypass this safety check entirely.

Signed-off-by: Monis Khan <[email protected]>

Fixes #16619
Fixes #16710

/assign @liggitt @deads2k

@openshift/sig-security

cc @jwendell @DirectXMan12 since you guys encountered this issue.

Author: openshift-merge-robot

pkg/authorization/authorizer/authorizer.go

@@ -6,21 +6,20 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authentication/serviceaccount"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
-	kauthorizer "k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	authorizerrbac "k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac"
 )
 
 type openshiftAuthorizer struct {
-	delegate              kauthorizer.Authorizer
+	delegate              authorizer.Authorizer
 	forbiddenMessageMaker ForbiddenMessageMaker
 }
 
 type openshiftSubjectLocator struct {
 	delegate authorizerrbac.SubjectLocator
 }
 
-func NewAuthorizer(delegate kauthorizer.Authorizer, forbiddenMessageMaker ForbiddenMessageMaker) authorizer.Authorizer {
+func NewAuthorizer(delegate authorizer.Authorizer, forbiddenMessageMaker ForbiddenMessageMaker) authorizer.Authorizer {
 	return &openshiftAuthorizer{delegate: delegate, forbiddenMessageMaker: forbiddenMessageMaker}
 }
 

pkg/authorization/authorizer/browser_safe_request_info_resolver.go

@@ -0,0 +1,79 @@
+package browsersafe
+
+import (
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
+)
+
+const (
+	proxyAction = "proxy"
+	unsafeProxy = "unsafeproxy"
+)
+
+type browserSafeAuthorizer struct {
+	delegate authorizer.Authorizer
+
+	// list of groups, any of which indicate the request is authenticated
+	authenticatedGroups sets.String
+}
+
+func NewBrowserSafeAuthorizer(delegate authorizer.Authorizer, authenticatedGroups ...string) authorizer.Authorizer {
+	return &browserSafeAuthorizer{
+		delegate:            delegate,
+		authenticatedGroups: sets.NewString(authenticatedGroups...),
+	}
+}
+
+func (a *browserSafeAuthorizer) Authorize(attributes authorizer.Attributes) (bool, string, error) {
+	browserSafeAttributes := a.getBrowserSafeAttributes(attributes)
+	return a.delegate.Authorize(browserSafeAttributes)
+}
+
+func (a *browserSafeAuthorizer) getBrowserSafeAttributes(attributes authorizer.Attributes) authorizer.Attributes {
+	if !attributes.IsResourceRequest() {
+		return attributes
+	}
+
+	isProxyVerb := attributes.GetVerb() == proxyAction
+	isProxySubresource := attributes.GetSubresource() == proxyAction
+
+	if !isProxyVerb && !isProxySubresource {
+		// Requests to non-proxy resources don't expose HTML or HTTP-handling user content to browsers
+		return attributes
+	}
+
+	if user := attributes.GetUser(); user != nil {
+		if 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 attributes
+		}
+	}
+
+	return &browserSafeAttributes{
+		Attributes:         attributes,
+		isProxyVerb:        isProxyVerb,
+		isProxySubresource: isProxySubresource,
+	}
+}
+
+type browserSafeAttributes struct {
+	authorizer.Attributes
+
+	isProxyVerb, isProxySubresource bool
+}
+
+func (b *browserSafeAttributes) GetVerb() string {
+	if b.isProxyVerb {
+		return unsafeProxy
+	}
+	return b.Attributes.GetVerb()
+}
+
+func (b *browserSafeAttributes) GetSubresource() string {
+	if b.isProxySubresource {
+		return unsafeProxy
+	}
+	return b.Attributes.GetSubresource()
+}