Merge pull request #16110 from deads2k/server-40-normal-authz

Automatic merge from submit-queue

use the upstream authorization filters

This brings in a config option from upstream and uses it to remove custom handler code we had.

@openshift/sig-security

pkg/authorization/authorizer/browser_safe_request_info_resolver.go

@@ -9,7 +9,7 @@ import (
 
 type browserSafeRequestInfoResolver struct {
 	// infoFactory is used to determine info for the request
-	infoFactory RequestInfoFactory
+	infoFactory apirequest.RequestInfoResolver
 
 	// contextMapper is used to look up the context corresponding to a request
 	// to obtain the user associated with the request
@@ -19,7 +19,7 @@ type browserSafeRequestInfoResolver struct {
 	authenticatedGroups sets.String
 }
 
-func NewBrowserSafeRequestInfoResolver(contextMapper apirequest.RequestContextMapper, authenticatedGroups sets.String, infoFactory RequestInfoFactory) RequestInfoFactory {
+func NewBrowserSafeRequestInfoResolver(contextMapper apirequest.RequestContextMapper, authenticatedGroups sets.String, infoFactory apirequest.RequestInfoResolver) apirequest.RequestInfoResolver {
 	return &browserSafeRequestInfoResolver{
 		contextMapper:       contextMapper,
 		authenticatedGroups: authenticatedGroups,

pkg/authorization/authorizer/interfaces.go

@@ -1,25 +1,14 @@
 package authorizer
 
 import (
-	"net/http"
-
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
-	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 )
 
 type SubjectLocator interface {
 	GetAllowedSubjects(attributes authorizer.Attributes) (sets.String, sets.String, error)
 }
 
-type AuthorizationAttributeBuilder interface {
-	GetAttributes(request *http.Request) (authorizer.Attributes, error)
-}
-
-type RequestInfoFactory interface {
-	NewRequestInfo(req *http.Request) (*apirequest.RequestInfo, error)
-}
-
 // ForbiddenMessageMaker creates a forbidden message from Attributes
 type ForbiddenMessageMaker interface {
 	MakeMessage(attrs authorizer.Attributes) (string, error)

pkg/authorization/authorizer/personal_subjectaccessreview.go

@@ -8,17 +8,18 @@ import (
 
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apiserver/pkg/endpoints/request"
+	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	kapi "k8s.io/kubernetes/pkg/api"
 
 	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
 )
 
 type personalSARRequestInfoResolver struct {
 	// infoFactory is used to determine info for the request
-	infoFactory RequestInfoFactory
+	infoFactory apirequest.RequestInfoResolver
 }
 
-func NewPersonalSARRequestInfoResolver(infoFactory RequestInfoFactory) RequestInfoFactory {
+func NewPersonalSARRequestInfoResolver(infoFactory apirequest.RequestInfoResolver) apirequest.RequestInfoResolver {
 	return &personalSARRequestInfoResolver{
 		infoFactory: infoFactory,
 	}

pkg/authorization/authorizer/project_request_info_resolver.go

@@ -3,21 +3,21 @@ package authorizer
 import (
 	"net/http"
 
-	"k8s.io/apiserver/pkg/endpoints/request"
+	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 )
 
 type projectRequestInfoResolver struct {
 	// infoFactory is used to determine info for the request
-	infoFactory RequestInfoFactory
+	infoFactory apirequest.RequestInfoResolver
 }
 
-func NewProjectRequestInfoResolver(infoFactory RequestInfoFactory) RequestInfoFactory {
+func NewProjectRequestInfoResolver(infoFactory apirequest.RequestInfoResolver) apirequest.RequestInfoResolver {
 	return &projectRequestInfoResolver{
 		infoFactory: infoFactory,
 	}
 }
 
-func (a *projectRequestInfoResolver) NewRequestInfo(req *http.Request) (*request.RequestInfo, error) {
+func (a *projectRequestInfoResolver) NewRequestInfo(req *http.Request) (*apirequest.RequestInfo, error) {
 	requestInfo, err := a.infoFactory.NewRequestInfo(req)
 	if err != nil {
 		return requestInfo, err

pkg/cmd/server/handlers/authorization.go

@@ -14,10 +14,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	kauthorizer "k8s.io/apiserver/pkg/authorization/authorizer"
-	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	kapi "k8s.io/kubernetes/pkg/api"
-
-	"github.com/openshift/origin/pkg/authorization/authorizer"
 )
 
 type bypassAuthorizer struct {
@@ -38,33 +35,6 @@ func (a bypassAuthorizer) Authorize(attributes kauthorizer.Attributes) (allowed
 	return a.authorizer.Authorize(attributes)
 }
 
-// AuthorizationFilter imposes normal authorization rules
-func AuthorizationFilter(handler http.Handler, authorizer kauthorizer.Authorizer, authorizationAttributeBuilder authorizer.AuthorizationAttributeBuilder, contextMapper apirequest.RequestContextMapper) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		attributes, err := authorizationAttributeBuilder.GetAttributes(req)
-		if err != nil {
-			Forbidden(err.Error(), attributes, w, req)
-			return
-		}
-		if attributes == nil {
-			Forbidden("No attributes", attributes, w, req)
-			return
-		}
-
-		allowed, reason, err := authorizer.Authorize(attributes)
-		if err != nil {
-			Forbidden(err.Error(), attributes, w, req)
-			return
-		}
-		if !allowed {
-			Forbidden(reason, attributes, w, req)
-			return
-		}
-
-		handler.ServeHTTP(w, req)
-	})
-}
-
 // Forbidden renders a simple forbidden error to the response
 func Forbidden(reason string, attributes kauthorizer.Attributes, w http.ResponseWriter, req *http.Request) {
 	kind := ""

pkg/cmd/server/kubernetes/master/master_config.go

@@ -59,9 +59,11 @@ import (
 	kversion "k8s.io/kubernetes/pkg/version"
 
 	"github.com/openshift/origin/pkg/api"
+	oauthorizer "github.com/openshift/origin/pkg/authorization/authorizer"
 	"github.com/openshift/origin/pkg/authorization/authorizer/scope"
 	"github.com/openshift/origin/pkg/cmd/flagtypes"
 	configapi "github.com/openshift/origin/pkg/cmd/server/api"
+	"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
 	"github.com/openshift/origin/pkg/cmd/server/cm"
 	"github.com/openshift/origin/pkg/cmd/server/crypto"
 	"github.com/openshift/origin/pkg/cmd/server/election"
@@ -449,6 +451,7 @@ func buildKubeApiserverConfig(
 	genericConfig.DisabledPostStartHooks.Insert("extensions/third-party-resources")
 	genericConfig.AdmissionControl = admissionControl
 	genericConfig.RequestContextMapper = requestContextMapper
+	genericConfig.RequestInfoResolver = openshiftRequestInfoResolver(genericConfig.RequestContextMapper)
 	genericConfig.OpenAPIConfig = defaultOpenAPIConfig(masterConfig)
 	genericConfig.SwaggerConfig = apiserver.DefaultSwaggerConfig()
 	genericConfig.SwaggerConfig.PostBuildHandler = customizeSwaggerDefinition
@@ -781,3 +784,18 @@ func readCAorNil(file string) ([]byte, error) {
 func newMasterLeases(storage storage.Interface, masterEndpointReconcileTTL int) election.Leases {
 	return election.NewLeases(storage, "/masterleases/", uint64(masterEndpointReconcileTTL))
 }
+
+func openshiftRequestInfoResolver(requestContextMapper apirequest.RequestContextMapper) apirequest.RequestInfoResolver {
+	// Default API request info factory
+	requestInfoFactory := &apirequest.RequestInfoFactory{APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"), GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi")}
+	// Wrap with a request info factory that detects unsafe requests and modifies verbs/resources appropriately so policy can address them separately
+	browserSafeRequestInfoResolver := oauthorizer.NewBrowserSafeRequestInfoResolver(
+		requestContextMapper,
+		sets.NewString(bootstrappolicy.AuthenticatedGroup),
+		requestInfoFactory,
+	)
+	personalSARRequestInfoResolver := oauthorizer.NewPersonalSARRequestInfoResolver(browserSafeRequestInfoResolver)
+	projectRequestInfoResolver := oauthorizer.NewProjectRequestInfoResolver(personalSARRequestInfoResolver)
+
+	return projectRequestInfoResolver
+}

pkg/cmd/server/kubernetes/master/master_config_test.go

@@ -35,6 +35,7 @@ var expectedGroupPreferredVersions []string = []string{
 	"admissionregistration.k8s.io/v1alpha1",
 	"apps/v1beta1,authentication.k8s.io/v1",
 	"authorization.k8s.io/v1",
+	"authorization.openshift.io/v1",
 	"autoscaling/v1",
 	"batch/v1",
 	"certificates.k8s.io/v1beta1",

pkg/cmd/server/origin/controller/standalone_apiserver.go

@@ -19,7 +19,6 @@ import (
 	authzwebhook "k8s.io/apiserver/plugin/pkg/authorizer/webhook"
 	clientgoclientset "k8s.io/client-go/kubernetes"
 
-	"github.com/openshift/origin/pkg/authorization/authorizer"
 	configapi "github.com/openshift/origin/pkg/cmd/server/api"
 	serverauthenticator "github.com/openshift/origin/pkg/cmd/server/authenticator"
 	"github.com/openshift/origin/pkg/cmd/server/crypto"
@@ -57,11 +56,10 @@ func RunControllerServer(servingInfo configapi.HTTPServingInfo, kubeExternal cli
 	requestInfoResolver := apiserver.NewRequestInfoResolver(&apiserver.Config{})
 	// the request context mapper for controllers is always separate
 	requestContextMapper := apirequest.NewRequestContextMapper()
-	authorizationAttributeBuilder := authorizer.NewAuthorizationAttributeBuilder(requestContextMapper, requestInfoResolver)
 
 	// we use direct bypass to allow readiness and health to work regardless of the master health
 	authz := serverhandlers.NewBypassAuthorizer(remoteAuthz, "/healthz", "/healthz/ready")
-	handler := serverhandlers.AuthorizationFilter(mux, authz, authorizationAttributeBuilder, requestContextMapper)
+	handler := apifilters.WithAuthorization(mux, requestContextMapper, authz)
 	handler = apifilters.WithAuthentication(handler, requestContextMapper, authn, apifilters.Unauthorized(false))
 	handler = apiserverfilters.WithPanicRecovery(handler)
 	handler = apifilters.WithRequestInfo(handler, requestInfoResolver, requestContextMapper)

pkg/cmd/server/origin/master.go

@@ -287,7 +287,7 @@ func (c *MasterConfig) buildHandlerChain() (func(apiHandler http.Handler, kc *ap
 
 			// these are all equivalent to the kube handler chain
 			///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
-			handler = serverhandlers.AuthorizationFilter(handler, c.Authorizer, c.AuthorizationAttributeBuilder, genericConfig.RequestContextMapper)
+			handler = apifilters.WithAuthorization(handler, c.RequestContextMapper, c.Authorizer)
 			handler = serverhandlers.ImpersonationFilter(handler, c.Authorizer, cache.NewGroupCache(c.UserInformers.User().InternalVersion().Groups()), genericConfig.RequestContextMapper)
 			// audit handler must comes before the impersonationFilter to read the original user
 			if c.Options.AuditConfig.Enabled {
@@ -318,7 +318,7 @@ func (c *MasterConfig) buildHandlerChain() (func(apiHandler http.Handler, kc *ap
 			// execution - updates vs reads, long reads vs short reads, fat reads vs skinny reads.
 			// NOTE: read vs. write is implemented in Kube 1.6+
 			handler = apiserverfilters.WithMaxInFlightLimit(handler, genericConfig.MaxRequestsInFlight, genericConfig.MaxMutatingRequestsInFlight, genericConfig.RequestContextMapper, genericConfig.LongRunningFunc)
-			handler = apifilters.WithRequestInfo(handler, apiserver.NewRequestInfoResolver(genericConfig), genericConfig.RequestContextMapper)
+			handler = apifilters.WithRequestInfo(handler, genericConfig.RequestInfoResolver, genericConfig.RequestContextMapper)
 			handler = apirequest.WithRequestContext(handler, genericConfig.RequestContextMapper)
 			handler = apiserverfilters.WithPanicRecovery(handler)
 			///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

pkg/cmd/server/origin/master_config.go

@@ -100,9 +100,6 @@ type MasterConfig struct {
 	Authorizer     kauthorizer.Authorizer
 	SubjectLocator authorizer.SubjectLocator
 
-	// TODO(sttts): replace AuthorizationAttributeBuilder with apiserverfilters.NewRequestAttributeGetter
-	AuthorizationAttributeBuilder authorizer.AuthorizationAttributeBuilder
-
 	ProjectAuthorizationCache     *projectauth.AuthorizationCache
 	ProjectCache                  *projectcache.ProjectCache
 	ClusterQuotaMappingController *clusterquotamapping.ClusterQuotaMappingController
@@ -300,11 +297,10 @@ func BuildMasterConfig(options configapi.MasterConfig, informers InformerAccess)
 
 		RESTOptionsGetter: restOptsGetter,
 
-		RuleResolver:                  ruleResolver,
-		Authenticator:                 authenticator,
-		Authorizer:                    authorizer,
-		SubjectLocator:                subjectLocator,
-		AuthorizationAttributeBuilder: newAuthorizationAttributeBuilder(requestContextMapper),
+		RuleResolver:   ruleResolver,
+		Authenticator:  authenticator,
+		Authorizer:     authorizer,
+		SubjectLocator: subjectLocator,
 
 		ProjectAuthorizationCache: newProjectAuthorizationCache(
 			subjectLocator,
@@ -782,22 +778,6 @@ func newAuthorizer(kubeAuthorizer kauthorizer.Authorizer, kubeSubjectLocator rba
 	return authorizer, subjectLocator
 }
 
-func newAuthorizationAttributeBuilder(requestContextMapper apirequest.RequestContextMapper) authorizer.AuthorizationAttributeBuilder {
-	// Default API request info factory
-	requestInfoFactory := &apirequest.RequestInfoFactory{APIPrefixes: sets.NewString("api", "osapi", "oapi", "apis"), GrouplessAPIPrefixes: sets.NewString("api", "osapi", "oapi")}
-	// Wrap with a request info factory that detects unsafe requests and modifies verbs/resources appropriately so policy can address them separately
-	browserSafeRequestInfoResolver := authorizer.NewBrowserSafeRequestInfoResolver(
-		requestContextMapper,
-		sets.NewString(bootstrappolicy.AuthenticatedGroup),
-		requestInfoFactory,
-	)
-	personalSARRequestInfoResolver := authorizer.NewPersonalSARRequestInfoResolver(browserSafeRequestInfoResolver)
-	projectRequestInfoResolver := authorizer.NewProjectRequestInfoResolver(personalSARRequestInfoResolver)
-
-	authorizationAttributeBuilder := authorizer.NewAuthorizationAttributeBuilder(requestContextMapper, projectRequestInfoResolver)
-	return authorizationAttributeBuilder
-}
-
 // KubeClientsetInternal returns the kubernetes client object
 func (c *MasterConfig) KubeClientsetInternal() kclientsetinternal.Interface {
 	return c.PrivilegedLoopbackKubernetesClientsetInternal

test/cmd/authentication.sh

@@ -69,7 +69,7 @@ os::cmd::expect_success_and_text "oc get user/~ --token='${allescalatingpowersto
 os::cmd::expect_success "oc get secrets --token='${allescalatingpowerstoken}' -n '${project}'"
 # scopes allow it, but authorization doesn't
 os::cmd::try_until_failure "oc get secrets --token='${allescalatingpowerstoken}' -n default"
-os::cmd::expect_failure_and_text "oc get secrets --token='${allescalatingpowerstoken}' -n default" 'cannot list secrets in project'
+os::cmd::expect_failure_and_text "oc get secrets --token='${allescalatingpowerstoken}' -n default" 'cannot list secrets in the namespace'
 os::cmd::expect_success_and_text "oc get projects --token='${allescalatingpowerstoken}'" "${project}"
 os::cmd::expect_success_and_text "oc policy can-i --list --token='${allescalatingpowerstoken}' -n '${project}'" 'get.*pods'
 

test/cmd/status.sh

@@ -44,15 +44,15 @@ os::cmd::expect_success_and_text "oc login --server=${KUBERNETES_MASTER} --certi
 os::cmd::expect_success_and_text 'oc status' "You don't have any projects. You can try to create a new project, by running"
 os::cmd::expect_success_and_text 'oc status --all-namespaces' "Showing all projects on server"
 # make sure `oc status` does not re-use the "no projects" message from `oc login` if -n is specified
-os::cmd::expect_failure_and_text 'oc status -n forbidden' 'Error from server \(Forbidden\): User "test-user" cannot get project "forbidden"'
+os::cmd::expect_failure_and_text 'oc status -n forbidden' 'Error from server \(Forbidden\): User "test-user" cannot get projects in the namespace "forbidden"'
 
 # create a new project
 os::cmd::expect_success "oc new-project project-bar --display-name='my project' --description='test project'"
 os::cmd::expect_success_and_text "oc project" 'Using project "project-bar"'
 
 # make sure `oc status` does not use "no projects" message if there is a project created
 os::cmd::expect_success_and_text 'oc status' "In project my project \(project-bar\) on server"
-os::cmd::expect_failure_and_text 'oc status -n forbidden' 'Error from server \(Forbidden\): User "test-user" cannot get project "forbidden"'
+os::cmd::expect_failure_and_text 'oc status -n forbidden' 'Error from server \(Forbidden\): User "test-user" cannot get projects in the namespace "forbidden"'
 
 # create a second project
 os::cmd::expect_success "oc new-project project-bar-2 --display-name='my project 2' --description='test project 2'"
@@ -62,7 +62,7 @@ os::cmd::expect_success_and_text "oc project" 'Using project "project-bar-2"'
 # message since `project-bar` still exists
 os::cmd::expect_success_and_text "oc delete project project-bar-2" 'project "project-bar-2" deleted'
 # the deletion is asynchronous and can take a while, so wait until we see the error
-os::cmd::try_until_text "oc status" 'Error from server \(Forbidden\): User "test-user" cannot get project "project-bar-2"'
+os::cmd::try_until_text "oc status" 'Error from server \(Forbidden\): User "test-user" cannot get projects in the namespace "project-bar-2"'
 
 # delete "project-bar" and test that `oc status` still does not return the "no projects" message.
 # Although we are deleting the last remaining project, the current context's namespace is still set
@@ -71,7 +71,7 @@ os::cmd::try_until_text "oc status" 'Error from server \(Forbidden\): User "test
 os::cmd::expect_success "oc project project-bar"
 os::cmd::expect_success "oc delete project project-bar"
 # the deletion is asynchronous and can take a while, so wait until we see the error
-os::cmd::try_until_text "oc status" 'Error from server \(Forbidden\): User "test-user" cannot get project "project-bar"'
+os::cmd::try_until_text "oc status" 'Error from server \(Forbidden\): User "test-user" cannot get projects in the namespace "project-bar"'
 os::cmd::try_until_not_text "oc get projects" "project-bar"
 os::cmd::try_until_not_text "oc get projects" "project-bar-2"
 os::cmd::expect_success "oc logout"