From f6829bbde76b343cd1222ed3c0bfcfc0db3c9166 Mon Sep 17 00:00:00 2001 From: deads2k Date: Tue, 13 Dec 2016 09:17:46 -0500 Subject: [PATCH] improve the forbidden message --- pkg/api/errors/errors.go | 3 ++- pkg/apiserver/filters/authorization.go | 8 +++---- pkg/apiserver/filters/errors.go | 24 +++++++++++++++++++-- pkg/apiserver/filters/impersonation.go | 13 +++++------ pkg/apiserver/filters/impersonation_test.go | 4 ++-- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index b71d5eaef62..b3cb1a2b36a 100644 --- a/pkg/api/errors/errors.go +++ b/pkg/api/errors/errors.go @@ -308,7 +308,8 @@ func NewGenericServerResponse(code int, verb string, qualifiedResource schema.Gr message = "the server has asked for the client to provide credentials" case http.StatusForbidden: reason = metav1.StatusReasonForbidden - message = "the server does not allow access to the requested resource" + // the server message has details about who is trying to perform what action. Keep its message. + message = serverMessage case http.StatusMethodNotAllowed: reason = metav1.StatusReasonMethodNotAllowed message = "the server does not allow this method on the requested resource" diff --git a/pkg/apiserver/filters/authorization.go b/pkg/apiserver/filters/authorization.go index 6ac4d08673e..9a8a819ca5a 100644 --- a/pkg/apiserver/filters/authorization.go +++ b/pkg/apiserver/filters/authorization.go @@ -40,12 +40,12 @@ func WithAuthorization(handler http.Handler, requestContextMapper api.RequestCon return } - attrs, err := GetAuthorizerAttributes(ctx) + attributes, err := GetAuthorizerAttributes(ctx) if err != nil { internalError(w, req, err) return } - authorized, reason, err := a.Authorize(attrs) + authorized, reason, err := a.Authorize(attributes) if authorized { handler.ServeHTTP(w, req) return @@ -55,8 +55,8 @@ func WithAuthorization(handler http.Handler, requestContextMapper api.RequestCon return } - glog.V(4).Infof("Forbidden: %#v, Reason: %s", req.RequestURI, reason) - forbidden(w, req) + glog.V(4).Infof("Forbidden: %#v, Reason: %q", req.RequestURI, reason) + forbidden(attributes, w, req, reason) }) } diff --git a/pkg/apiserver/filters/errors.go b/pkg/apiserver/filters/errors.go index 9c430bdd28f..266d7cbc25d 100755 --- a/pkg/apiserver/filters/errors.go +++ b/pkg/apiserver/filters/errors.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" + "k8s.io/kubernetes/pkg/auth/authorizer" "k8s.io/kubernetes/pkg/util/runtime" ) @@ -30,9 +31,28 @@ func badGatewayError(w http.ResponseWriter, req *http.Request) { } // forbidden renders a simple forbidden error -func forbidden(w http.ResponseWriter, req *http.Request) { +func forbidden(attributes authorizer.Attributes, w http.ResponseWriter, req *http.Request, reason string) { + msg := forbiddenMessage(attributes) w.WriteHeader(http.StatusForbidden) - fmt.Fprintf(w, "Forbidden: %#v", req.RequestURI) + fmt.Fprintf(w, "%s: %q", msg, reason) +} + +func forbiddenMessage(attributes authorizer.Attributes) string { + username := "" + if user := attributes.GetUser(); user != nil { + username = user.GetName() + } + + resource := attributes.GetResource() + if group := attributes.GetAPIGroup(); len(group) > 0 { + resource = resource + "." + group + } + + if ns := attributes.GetNamespace(); len(ns) > 0 { + return fmt.Sprintf("User %q cannot %s %s in the namespace %q.", username, attributes.GetVerb(), resource, ns) + } + + return fmt.Sprintf("User %q cannot %s %s at the cluster scope.", username, attributes.GetVerb(), resource) } // internalError renders a simple internal error diff --git a/pkg/apiserver/filters/impersonation.go b/pkg/apiserver/filters/impersonation.go index bc76c701f80..5082a467d20 100644 --- a/pkg/apiserver/filters/impersonation.go +++ b/pkg/apiserver/filters/impersonation.go @@ -17,6 +17,7 @@ limitations under the License. package filters import ( + "errors" "fmt" "net/http" "strings" @@ -37,7 +38,7 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon impersonationRequests, err := buildImpersonationRequests(req.Header) if err != nil { glog.V(4).Infof("%v", err) - forbidden(w, req) + internalError(w, req, err) return } if len(impersonationRequests) == 0 { @@ -47,12 +48,12 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon ctx, exists := requestContextMapper.Get(req) if !exists { - forbidden(w, req) + internalError(w, req, errors.New("no context found for request")) return } requestor, exists := api.UserFrom(ctx) if !exists { - forbidden(w, req) + internalError(w, req, errors.New("no user found for request")) return } @@ -100,15 +101,15 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon userExtra[extraKey] = append(userExtra[extraKey], extraValue) default: - glog.V(4).Infof("unknown impersonation request type: %v\n", impersonationRequest) - forbidden(w, req) + glog.V(4).Infof("unknown impersonation request type: %v", impersonationRequest) + forbidden(actingAsAttributes, w, req, fmt.Sprintf("unknown impersonation request type: %v", impersonationRequest)) 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) - forbidden(w, req) + forbidden(actingAsAttributes, w, req, reason) return } } diff --git a/pkg/apiserver/filters/impersonation_test.go b/pkg/apiserver/filters/impersonation_test.go index 92f7c588074..7da35605879 100644 --- a/pkg/apiserver/filters/impersonation_test.go +++ b/pkg/apiserver/filters/impersonation_test.go @@ -118,7 +118,7 @@ func TestImpersonationFilter(t *testing.T) { expectedUser: &user.DefaultInfo{ Name: "tester", }, - expectedCode: http.StatusForbidden, + expectedCode: http.StatusInternalServerError, }, { name: "impersonating-extra-without-user", @@ -129,7 +129,7 @@ func TestImpersonationFilter(t *testing.T) { expectedUser: &user.DefaultInfo{ Name: "tester", }, - expectedCode: http.StatusForbidden, + expectedCode: http.StatusInternalServerError, }, { name: "disallowed-group",