From 3d6479f7216dcb61e56ab6dd53fad7176930645d Mon Sep 17 00:00:00 2001 From: Shiyang Wang Date: Tue, 13 Jun 2017 08:24:40 +0800 Subject: [PATCH] Fix 401/403 apiserver errors do not return 'Status' objects --- .../apiserver/pkg/endpoints/filters/BUILD | 5 + .../pkg/endpoints/filters/authentication.go | 39 +++-- .../pkg/endpoints/filters/authorization.go | 5 +- .../pkg/endpoints/filters/impersonation.go | 7 +- .../endpoints/filters/impersonation_test.go | 4 +- .../endpoints/handlers/responsewriters/BUILD | 4 + .../handlers/responsewriters/errors.go | 19 ++- .../handlers/responsewriters/errors_test.go | 23 +-- .../src/k8s.io/apiserver/pkg/server/config.go | 6 +- test/integration/master/BUILD | 7 + test/integration/master/master_test.go | 135 ++++++++++++++---- 11 files changed, 187 insertions(+), 67 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD index e1f0112871..2dcc5badfd 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD @@ -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", @@ -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", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go index 17bc44f7b7..a53db72137 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go @@ -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" ) @@ -76,22 +82,25 @@ func WithAuthentication(handler http.Handler, mapper genericapirequest.RequestCo ) } -func Unauthorized(supportsBasicAuth bool) http.HandlerFunc { - if supportsBasicAuth { - return unauthorizedBasicAuth - } - return unauthorized -} +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 + } -// 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) + 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 diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go index 90599fe3e9..d244257a04 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go @@ -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 @@ -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) }) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go index 891b339ecc..cc4f16bb27 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go @@ -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" @@ -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 { @@ -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 } } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go index 1c9adb61bc..d43776507e 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go @@ -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" @@ -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) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/BUILD index f4ccbeab32..47986c453c 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/BUILD @@ -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", ], ) @@ -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", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go index 5c6c9bf756..6a2d226c05 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go @@ -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. @@ -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 { @@ -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. diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go index 31f45d1e10..9af22c73f3 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go @@ -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) { @@ -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: "/