Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid nil user special-casing in unsecured endpoint #43888

Merged
merged 1 commit into from
Apr 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/kubeapiserver/server/BUILD
Expand Up @@ -13,6 +13,7 @@ go_library(
tags = ["automanaged"],
deps = [
"//vendor:github.com/golang/glog",
"//vendor:k8s.io/apiserver/pkg/authentication/user",
"//vendor:k8s.io/apiserver/pkg/endpoints/filters",
"//vendor:k8s.io/apiserver/pkg/endpoints/request",
"//vendor:k8s.io/apiserver/pkg/server",
Expand Down
14 changes: 14 additions & 0 deletions pkg/kubeapiserver/server/insecure_handler.go
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/golang/glog"

"k8s.io/apiserver/pkg/authentication/user"
genericapifilters "k8s.io/apiserver/pkg/endpoints/filters"
apirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/server"
Expand All @@ -35,6 +36,7 @@ import (

func BuildInsecureHandlerChain(apiHandler http.Handler, c *server.Config) http.Handler {
handler := genericapifilters.WithAudit(apiHandler, c.RequestContextMapper, c.AuditWriter)
handler = genericapifilters.WithAuthentication(handler, c.RequestContextMapper, insecureSuperuser{}, nil)
handler = genericfilters.WithCORS(handler, c.CorsAllowedOriginList, nil, nil, nil, "true")
handler = genericfilters.WithPanicRecovery(handler, c.RequestContextMapper)
handler = genericfilters.WithTimeoutForNonLongRunningRequests(handler, c.RequestContextMapper, c.LongRunningFunc)
Expand Down Expand Up @@ -111,3 +113,15 @@ func serveInsecurely(insecureServingInfo *InsecureServingInfo, insecureHandler h
_, err = server.RunServer(insecureServer, insecureServingInfo.BindNetwork, stopCh)
return err
}

// insecureSuperuser implements authenticator.Request to always return a superuser.
// This is functionally equivalent to skipping authentication and authorization,
// but allows apiserver code to stop special-casing a nil user to skip authorization checks.
type insecureSuperuser struct{}

func (insecureSuperuser) AuthenticateRequest(req *http.Request) (user.Info, bool, error) {
return &user.DefaultInfo{
Name: "system:unsecured",
Groups: []string{user.SystemPrivilegedGroup, user.AllAuthenticated},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt - Was it intentional to add the AllAuthenticated (as opposed to AllUnauthenticated) here? I know it's a "sure user" identity, but it seems a little funny to add the authenticated group when we've effectively skipped authentication.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was... pairing the privileged group and the unauthenticated group seemed more bizarre than this. just don't think of this as unauthenticated, think of it as having authenticated the superuser by virtue of them arriving at the localhost port :-(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relatedly, I'd like to consider defaulting this port off in a future release and making deployments that want it explicitly enable it... it's too easy to run not knowing this exists on localhost

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I noticed it while reading through the audit logs of an e2e test, and it struck me as odd.

2017-06-02T02:21:03.555917358Z AUDIT: id="286a4484-480d-41b8-b9b4-4c30305fe0f3" stage="ResponseComplete" ip="127.0.0.1" method="get" user="system:unsecured" groups="\"system:masters\",\"system:authenticated\"" as="<self>" asgroups="<lookup>" namespace="kube-system" uri="/api/v1/namespaces/kube-system/configmaps/ingress-uid" response="200"

}, true, nil
}
4 changes: 1 addition & 3 deletions pkg/registry/rbac/escalation_check.go
Expand Up @@ -29,9 +29,7 @@ import (
func EscalationAllowed(ctx genericapirequest.Context) bool {
u, ok := genericapirequest.UserFrom(ctx)
if !ok {
// the only way to be without a user is to either have no authenticators by explicitly saying that's your preference
// or to be connecting via the insecure port, in which case this logically doesn't apply
return true
return false
}

// system:masters is special because the API server uses it for privileged loopback connections
Expand Down
3 changes: 1 addition & 2 deletions plugin/pkg/admission/security/podsecuritypolicy/admission.go
Expand Up @@ -288,8 +288,7 @@ func getMatchingPolicies(lister extensionslisters.PodSecurityPolicyLister, user
}

for _, constraint := range list {
// if no user info exists then the API is being hit via the unsecured port. In this case authorize the request.
if user == nil || authorizedForPolicy(user, namespace, constraint, authz) || authorizedForPolicy(sa, namespace, constraint, authz) {
if authorizedForPolicy(user, namespace, constraint, authz) || authorizedForPolicy(sa, namespace, constraint, authz) {
matchedPolicies = append(matchedPolicies, constraint)
}
}
Expand Down
45 changes: 37 additions & 8 deletions plugin/pkg/admission/security/podsecuritypolicy/admission_test.go
Expand Up @@ -1612,25 +1612,34 @@ func TestGetMatchingPolicies(t *testing.T) {
},
expectedPolicies: sets.NewString("policy1", "policy2", "policy4", "policy5"),
},
"policies are allowed for nil user info": {
user: nil,
sa: &user.DefaultInfo{Name: "sa"},
ns: "test",
allowed: map[string]map[string]map[string]bool{}, // authorizer not consulted
"policies are not allowed for nil user info": {
user: nil,
sa: &user.DefaultInfo{Name: "sa"},
ns: "test",
allowed: map[string]map[string]map[string]bool{
"sa": {
"test": {"policy1": true},
},
"user": {
"test": {"policy2": true},
},
},
inPolicies: []*extensions.PodSecurityPolicy{
policyWithName("policy1"),
policyWithName("policy2"),
policyWithName("policy3"),
},
// all policies are allowed regardless of the permissions when user info is nil
// (ie. a request hitting the unsecure port)
expectedPolicies: sets.NewString("policy1", "policy2", "policy3"),
// only the policies for the sa are allowed when user info is nil
expectedPolicies: sets.NewString("policy1"),
},
"policies are not allowed for nil sa info": {
user: &user.DefaultInfo{Name: "user"},
sa: nil,
ns: "test",
allowed: map[string]map[string]map[string]bool{
"sa": {
"test": {"policy1": true},
},
"user": {
"test": {"policy2": true},
},
Expand All @@ -1643,6 +1652,26 @@ func TestGetMatchingPolicies(t *testing.T) {
// only the policies for the user are allowed when sa info is nil
expectedPolicies: sets.NewString("policy2"),
},
"policies are not allowed for nil sa and user info": {
user: nil,
sa: nil,
ns: "test",
allowed: map[string]map[string]map[string]bool{
"sa": {
"test": {"policy1": true},
},
"user": {
"test": {"policy2": true},
},
},
inPolicies: []*extensions.PodSecurityPolicy{
policyWithName("policy1"),
policyWithName("policy2"),
policyWithName("policy3"),
},
// no policies are allowed if sa and user are both nil
expectedPolicies: sets.NewString(),
},
}
for k, v := range tests {
informerFactory := informers.NewSharedInformerFactory(nil, controller.NoResyncPeriodFunc())
Expand Down