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.

Sign up for GitHub

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

Jump to bottom

Fix 401/403 apiserver errors do not return 'Status' objects #47384

Merged
merged 1 commit into from Jul 5, 2017
Merged

Fix 401/403 apiserver errors do not return 'Status' objects #47384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD
View file
Open in desktop
Expand Up @@ -23,6 +23,8 @@ go_test(
deps = [
"//vendor/k8s.io/api/authentication/v1:go_default_library",
"//vendor/k8s.io/api/batch/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//vendor/k8s.io/apiserver/pkg/apis/audit:go_default_library",
Expand Down Expand Up @@ -54,7 +56,10 @@ go_library(
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
"//vendor/k8s.io/api/authentication/v1:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
"//vendor/k8s.io/apiserver/pkg/apis/audit:go_default_library",
Expand Down
41 changes: 25 additions & 16 deletions staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go
View file
Open in desktop
Expand Up @@ -17,13 +17,19 @@ limitations under the License.
package filters

import (
"errors"
"net/http"
"strings"

"github.com/golang/glog"
"github.com/prometheus/client_golang/prometheus"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/authentication/authenticator"
"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
"k8s.io/apiserver/pkg/endpoints/request"
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
)

Expand Down Expand Up @@ -76,22 +82,25 @@ func WithAuthentication(handler http.Handler, mapper genericapirequest.RequestCo
)
}

func Unauthorized(supportsBasicAuth bool) http.HandlerFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about adding the mapper and serializer to this function? Then we can keep the (now failing) unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, how do you think about now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good

if supportsBasicAuth {
return unauthorizedBasicAuth
}
return unauthorized
}

// unauthorizedBasicAuth serves an unauthorized message to clients.
func unauthorizedBasicAuth(w http.ResponseWriter, req *http.Request) {
w.Header().Set("WWW-Authenticate", `Basic realm="kubernetes-master"`)
http.Error(w, "Unauthorized", http.StatusUnauthorized)
}

// unauthorized serves an unauthorized message to clients.
func unauthorized(w http.ResponseWriter, req *http.Request) {
http.Error(w, "Unauthorized", http.StatusUnauthorized)
func Unauthorized(requestContextMapper request.RequestContextMapper, s runtime.NegotiatedSerializer, supportsBasicAuth bool) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
if supportsBasicAuth {
w.Header().Set("WWW-Authenticate", `Basic realm="kubernetes-master"`)
}
ctx, ok := requestContextMapper.Get(req)
if !ok {
responsewriters.InternalError(w, req, errors.New("no context found for request"))
return
}
requestInfo, found := request.RequestInfoFrom(ctx)
if !found {
responsewriters.InternalError(w, req, errors.New("no RequestInfo found in the context"))
return
}

gv := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
responsewriters.ErrorNegotiated(ctx, apierrors.NewUnauthorized("Unauthorized"), s, gv, w, req)
})
}

// compressUsername maps all possible usernames onto a small set of categories
Expand Down
5 changes: 3 additions & 2 deletions staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go
View file
Open in desktop
Expand Up @@ -22,13 +22,14 @@ import (

"github.com/golang/glog"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
"k8s.io/apiserver/pkg/endpoints/request"
)

// WithAuthorizationCheck passes all authorized requests on to handler, and returns a forbidden error otherwise.
func WithAuthorization(handler http.Handler, requestContextMapper request.RequestContextMapper, a authorizer.Authorizer) http.Handler {
func WithAuthorization(handler http.Handler, requestContextMapper request.RequestContextMapper, a authorizer.Authorizer, s runtime.NegotiatedSerializer) http.Handler {
if a == nil {
glog.Warningf("Authorization is disabled")
return handler
Expand Down Expand Up @@ -56,7 +57,7 @@ func WithAuthorization(handler http.Handler, requestContextMapper request.Reques
}

glog.V(4).Infof("Forbidden: %#v, Reason: %q", req.RequestURI, reason)
responsewriters.Forbidden(attributes, w, req, reason)
responsewriters.Forbidden(ctx, attributes, w, req, reason, s)
})
}

Expand Down
7 changes: 4 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ import (

authenticationv1 "k8s.io/api/authentication/v1"
"k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/authentication/serviceaccount"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/apiserver/pkg/authorization/authorizer"
Expand All @@ -35,7 +36,7 @@ import (
)

// WithImpersonation is a filter that will inspect and check requests that attempt to change the user.Info for their requests
func WithImpersonation(handler http.Handler, requestContextMapper request.RequestContextMapper, a authorizer.Authorizer) http.Handler {
func WithImpersonation(handler http.Handler, requestContextMapper request.RequestContextMapper, a authorizer.Authorizer, s runtime.NegotiatedSerializer) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
impersonationRequests, err := buildImpersonationRequests(req.Header)
if err != nil {
Expand Down Expand Up @@ -104,14 +105,14 @@ func WithImpersonation(handler http.Handler, requestContextMapper request.Reques

default:
glog.V(4).Infof("unknown impersonation request type: %v", impersonationRequest)
responsewriters.Forbidden(actingAsAttributes, w, req, fmt.Sprintf("unknown impersonation request type: %v", impersonationRequest))
responsewriters.Forbidden(ctx, actingAsAttributes, w, req, fmt.Sprintf("unknown impersonation request type: %v", impersonationRequest), s)
return
}

allowed, reason, err := a.Authorize(actingAsAttributes)
if err != nil || !allowed {
glog.V(4).Infof("Forbidden: %#v, Reason: %s, Error: %v", req.RequestURI, reason, err)
responsewriters.Forbidden(actingAsAttributes, w, req, reason)
responsewriters.Forbidden(ctx, actingAsAttributes, w, req, reason, s)
return
}
}
Expand Down
4 changes: 3 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go
View file
Open in desktop
Expand Up @@ -26,6 +26,8 @@ import (
"testing"

authenticationapi "k8s.io/api/authentication/v1"
"k8s.io/apimachinery/pkg/runtime"
serializer "k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/apiserver/pkg/endpoints/request"
Expand Down Expand Up @@ -356,7 +358,7 @@ func TestImpersonationFilter(t *testing.T) {

delegate.ServeHTTP(w, req)
})
}(WithImpersonation(doNothingHandler, requestContextMapper, impersonateAuthorizer{}))
}(WithImpersonation(doNothingHandler, requestContextMapper, impersonateAuthorizer{}, serializer.NewCodecFactory(runtime.NewScheme())))
handler = request.WithRequestContext(handler, requestContextMapper)

server := httptest.NewServer(handler)
Expand Down
4 changes: 4 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/BUILD
View file
Open in desktop
Expand Up @@ -19,9 +19,12 @@ go_test(
deps = [
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
"//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
],
)

Expand All @@ -35,6 +38,7 @@ go_library(
],
tags = ["automanaged"],
deps = [
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
Expand Down
19 changes: 13 additions & 6 deletions staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go
View file
Open in desktop
Expand Up @@ -21,8 +21,12 @@ import (
"net/http"
"strings"

"k8s.io/apimachinery/pkg/util/runtime"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/apiserver/pkg/endpoints/request"
)

// Avoid emitting errors that look like valid HTML. Quotes are okay.
Expand All @@ -37,17 +41,20 @@ func BadGatewayError(w http.ResponseWriter, req *http.Request) {
}

// Forbidden renders a simple forbidden error
func Forbidden(attributes authorizer.Attributes, w http.ResponseWriter, req *http.Request, reason string) {
func Forbidden(ctx request.Context, attributes authorizer.Attributes, w http.ResponseWriter, req *http.Request, reason string, s runtime.NegotiatedSerializer) {
msg := sanitizer.Replace(forbiddenMessage(attributes))
w.Header().Set("Content-Type", "text/plain")
w.Header().Set("X-Content-Type-Options", "nosniff")
w.WriteHeader(http.StatusForbidden)

var errMsg string
if len(reason) == 0 {
fmt.Fprintf(w, "%s", msg)
errMsg = fmt.Sprintf("%s", msg)
} else {
fmt.Fprintf(w, "%s: %q", msg, reason)
errMsg = fmt.Sprintf("%s: %q", msg, reason)
}
gv := schema.GroupVersion{Group: attributes.GetAPIGroup(), Version: attributes.GetAPIVersion()}
gr := schema.GroupResource{Group: attributes.GetAPIGroup(), Resource: attributes.GetResource()}
ErrorNegotiated(ctx, apierrors.NewForbidden(gr, attributes.GetName(), fmt.Errorf(errMsg)), s, gv, w, req)
}

func forbiddenMessage(attributes authorizer.Attributes) string {
Expand Down Expand Up @@ -81,7 +88,7 @@ func InternalError(w http.ResponseWriter, req *http.Request, err error) {
w.Header().Set("X-Content-Type-Options", "nosniff")
w.WriteHeader(http.StatusInternalServerError)
fmt.Fprintf(w, "Internal Server Error: %q: %v", sanitizer.Replace(req.RequestURI), err)
runtime.HandleError(err)
utilruntime.HandleError(err)
}

// NotFound renders a simple not found error.
Expand Down
23 changes: 14 additions & 9 deletions staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go
View file
Open in desktop
Expand Up @@ -22,8 +22,11 @@ import (
"net/http/httptest"
"testing"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/apiserver/pkg/endpoints/request"
)

func TestErrors(t *testing.T) {
Expand Down Expand Up @@ -64,18 +67,20 @@ func TestForbidden(t *testing.T) {
attributes authorizer.Attributes
reason string
}{
{`User "NAME" cannot GET path "/whatever".`,
authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/whatever"}, ""},
{`User "NAME" cannot GET path "/<script>".`,
authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/<script>"}, ""},
{`User "NAME" cannot GET pod at the cluster scope.`,
authorizer.AttributesRecord{User: u, Verb: "GET", Resource: "pod", ResourceRequest: true}, ""},
{`User "NAME" cannot GET pod.v2/quota in the namespace "test".`,
authorizer.AttributesRecord{User: u, Verb: "GET", Namespace: "test", APIGroup: "v2", Resource: "pod", Subresource: "quota", ResourceRequest: true}, ""},
{`{"metadata":{},"status":"Failure","message":" \"\" is forbidden: User \"NAME\" cannot GET path \"/whatever\".","reason":"Forbidden","details":{},"code":403}
`, authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/whatever"}, ""},
{`{"metadata":{},"status":"Failure","message":" \"\" is forbidden: User \"NAME\" cannot GET path \"/\u0026lt;script\u0026gt;\".","reason":"Forbidden","details":{},"code":403}
`, authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/<script>"}, ""},
{`{"metadata":{},"status":"Failure","message":"pod \"\" is forbidden: User \"NAME\" cannot GET pod at the cluster scope.","reason":"Forbidden","details":{"kind":"pod"},"code":403}
`, authorizer.AttributesRecord{User: u, Verb: "GET", Resource: "pod", ResourceRequest: true}, ""},
{`{"metadata":{},"status":"Failure","message":"pod.v2 \"\" is forbidden: User \"NAME\" cannot GET pod.v2/quota in the namespace \"test\".","reason":"Forbidden","details":{"group":"v2","kind":"pod"},"code":403}
`, authorizer.AttributesRecord{User: u, Verb: "GET", Namespace: "test", APIGroup: "v2", Resource: "pod", Subresource: "quota", ResourceRequest: true}, ""},
}
for _, test := range cases {
observer := httptest.NewRecorder()
Forbidden(test.attributes, observer, &http.Request{}, test.reason)
scheme := runtime.NewScheme()
negotiatedSerializer := serializer.DirectCodecFactory{CodecFactory: serializer.NewCodecFactory(scheme)}
Forbidden(request.NewDefaultContext(), test.attributes, observer, &http.Request{}, test.reason, negotiatedSerializer)
result := string(observer.Body.Bytes())
if result != test.expected {
t.Errorf("Forbidden(%#v...) != %#v, got %#v", test.attributes, test.expected, result)
Expand Down
6 changes: 3 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/server/config.go
View file
Open in desktop
Expand Up @@ -467,14 +467,14 @@ func (c completedConfig) New(name string, delegationTarget DelegationTarget) (*G
}

func DefaultBuildHandlerChain(apiHandler http.Handler, c *Config) http.Handler {
handler := genericapifilters.WithAuthorization(apiHandler, c.RequestContextMapper, c.Authorizer)
handler = genericapifilters.WithImpersonation(handler, c.RequestContextMapper, c.Authorizer)
handler := genericapifilters.WithAuthorization(apiHandler, c.RequestContextMapper, c.Authorizer, c.Serializer)
handler = genericapifilters.WithImpersonation(handler, c.RequestContextMapper, c.Authorizer, c.Serializer)
if utilfeature.DefaultFeatureGate.Enabled(features.AdvancedAuditing) {
handler = genericapifilters.WithAudit(handler, c.RequestContextMapper, c.AuditBackend, c.AuditPolicyChecker, c.LongRunningFunc)
} else {
handler = genericapifilters.WithLegacyAudit(handler, c.RequestContextMapper, c.LegacyAuditWriter)
}
handler = genericapifilters.WithAuthentication(handler, c.RequestContextMapper, c.Authenticator, genericapifilters.Unauthorized(c.SupportsBasicAuth))
handler = genericapifilters.WithAuthentication(handler, c.RequestContextMapper, c.Authenticator, genericapifilters.Unauthorized(c.RequestContextMapper, c.Serializer, c.SupportsBasicAuth))
handler = genericfilters.WithCORS(handler, c.CorsAllowedOriginList, nil, nil, nil, "true")
handler = genericfilters.WithPanicRecovery(handler)
handler = genericfilters.WithTimeoutForNonLongRunningRequests(handler, c.RequestContextMapper, c.LongRunningFunc)
Expand Down
7 changes: 7 additions & 0 deletions test/integration/master/BUILD
View file
Open in desktop
Expand Up @@ -19,13 +19,20 @@ go_test(
"//pkg/api/testapi:go_default_library",
"//pkg/client/clientset_generated/clientset/typed/core/v1:go_default_library",
"//pkg/client/clientset_generated/internalclientset:go_default_library",
"//pkg/master:go_default_library",
"//test/integration:go_default_library",
"//test/integration/framework:go_default_library",
"//vendor/github.com/ghodss/yaml:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authentication/group:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authentication/request/bearertoken:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authorization/authorizerfactory:go_default_library",
"//vendor/k8s.io/apiserver/plugin/pkg/authenticator/token/tokentest:go_default_library",
"//vendor/k8s.io/client-go/rest:go_default_library",
],
)
Expand Down