Skip to content

Commit

Permalink
set invalid request status if request parsing fails
Browse files Browse the repository at this point in the history 
If request parsing fails, instead of an error send a denied HTTP response with status 400 Bad Request.

Signed-off-by: Rudrakh Panigrahi <[email protected]>
  • Loading branch information
@rudrakhp
rudrakhp authored Sep 16, 2024
1 parent dc328ea commit f16fcd7
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 39 deletions.
4 changes: 2 additions & 2 deletions internal/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,6 @@ func (e *Error) Unwrap() error {
return e.err
}

func internalError(code string, err error) Error {
return Error{Code: code, err: err}
func newInternalError(code string, err error) *Error {
return &Error{Code: code, err: err}
}
72 changes: 43 additions & 29 deletions internal/internal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,22 +370,22 @@ func (p *envoyExtAuthzGrpcServer) Check(ctx context.Context, req *ext_authz_v3.C
func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*ext_authz_v3.CheckResponse, func() *rpc_status.Status, *Error) {
var err error
var evalErr error
var internalErr Error
var internalErr *Error
start := time.Now()
logger := p.manager.Logger()

result, stopeval, err := envoyauth.NewEvalResult()
if err != nil {
logger.WithFields(map[string]interface{}{"err": err}).Error("Unable to start new evaluation.")
internalErr = internalError(StartCheckErr, err)
return nil, func() *rpc_status.Status { return nil }, &internalErr
internalErr = newInternalError(StartCheckErr, err)
return nil, func() *rpc_status.Status { return nil }, internalErr
}

txn, txnClose, err := result.GetTxn(ctx, p.Store())
if err != nil {
logger.WithFields(map[string]interface{}{"err": err}).Error("Unable to start new storage transaction.")
internalErr = internalError(StartTxnErr, err)
return nil, func() *rpc_status.Status { return nil }, &internalErr
internalErr = newInternalError(StartTxnErr, err)
return nil, func() *rpc_status.Status { return nil }, internalErr
}

result.Txn = txn
Expand All @@ -398,9 +398,9 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
stopeval()
if p.cfg.EnablePerformanceMetrics {
var topdownError *topdown.Error
if internalErr.Unwrap() != nil && errors.As(internalErr.Unwrap(), &topdownError) {
if internalErr != nil && errors.As(internalErr.Unwrap(), &topdownError) {
p.metricErrorCounter.With(prometheus.Labels{"reason": topdownError.Code}).Inc()
} else if internalErr.Code != "" {
} else if internalErr != nil && internalErr.Code != "" {
p.metricErrorCounter.With(prometheus.Labels{"reason": internalErr.Code}).Inc()
}
}
Expand All @@ -422,27 +422,41 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*

input, err = envoyauth.RequestToInput(req, logger, p.cfg.protoSet, p.cfg.SkipRequestBodyParse)
if err != nil {
internalErr = internalError(RequestParseErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(RequestParseErr, err)
return &ext_authz_v3.CheckResponse{
Status: &rpc_status.Status{
Code: int32(code.Code_PERMISSION_DENIED),
Message: internalErr.Error(),
},
HttpResponse: &ext_authz_v3.CheckResponse_DeniedResponse{
DeniedResponse: &ext_authz_v3.DeniedHttpResponse{
Status: &ext_type_v3.HttpStatus{
Code: ext_type_v3.StatusCode(ext_type_v3.StatusCode_BadRequest),
},
Body: internalErr.Error(),
},
},
DynamicMetadata: nil,
}, stop, nil
}

if ctx.Err() != nil {
err = errors.Wrap(ctx.Err(), "check request timed out before query execution")
internalErr = internalError(CheckRequestTimeoutErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(CheckRequestTimeoutErr, err)
return nil, stop, internalErr
}

var inputValue ast.Value
inputValue, err = ast.InterfaceToValue(input)
if err != nil {
internalErr = internalError(InputParseErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(InputParseErr, err)
return nil, stop, internalErr
}

if err = envoyauth.Eval(ctx, p, inputValue, result); err != nil {
evalErr = err
internalErr = internalError(EnvoyAuthEvalErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthEvalErr, err)
return nil, stop, internalErr
}

resp := &ext_authz_v3.CheckResponse{}
Expand All @@ -451,8 +465,8 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
allowed, err = result.IsAllowed()
if err != nil {
err = errors.Wrap(err, "failed to get response status")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

status := int32(code.Code_PERMISSION_DENIED)
Expand All @@ -467,16 +481,16 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
responseHeaders, err = result.GetResponseEnvoyHeaderValueOptions()
if err != nil {
err = errors.Wrap(err, "failed to get response headers")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

var dynamicMetadata *_structpb.Struct
dynamicMetadata, err = result.GetDynamicMetadata()
if err != nil {
err = errors.Wrap(err, "failed to get dynamic metadata")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}
resp.DynamicMetadata = dynamicMetadata

Expand All @@ -486,16 +500,16 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
headersToRemove, err = result.GetRequestHTTPHeadersToRemove()
if err != nil {
err = errors.Wrap(err, "failed to get request headers to remove")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

var responseHeadersToAdd []*ext_core_v3.HeaderValueOption
responseHeadersToAdd, err = result.GetResponseHTTPHeadersToAdd()
if err != nil {
err = errors.Wrap(err, "failed to get response headers to send to client")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

resp.HttpResponse = &ext_authz_v3.CheckResponse_OkResponse{
Expand All @@ -510,16 +524,16 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
body, err = result.GetResponseBody()
if err != nil {
err = errors.Wrap(err, "failed to get response body")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

var httpStatus *ext_type_v3.HttpStatus
httpStatus, err = result.GetResponseEnvoyHTTPStatus()
if err != nil {
err = errors.Wrap(err, "failed to get response http status")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

deniedResponse := &ext_authz_v3.DeniedHttpResponse{
Expand Down
34 changes: 26 additions & 8 deletions internal/internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"context"
"errors"
"fmt"
ext_type_v2 "github.com/envoyproxy/go-control-plane/envoy/type"
ext_type_v3 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
"net/http"
"net/http/httptest"
"reflect"
Expand Down Expand Up @@ -862,12 +864,20 @@ func TestCheckBadDecisionWithLogger(t *testing.T) {
ctx := context.Background()
output, err := server.Check(ctx, &req)

if err == nil {
t.Fatal("Expected error but got nil")
if err != nil {
t.Fatal("Expected nil err")
}

if output != nil {
t.Fatalf("Expected no output but got %v", output)
if output == nil {
t.Fatal("Expected output but got nil")
}
if output.Status.Code != int32(code.Code_PERMISSION_DENIED) {
t.Fatalf("Expected status %v status: %v", int32(code.Code_PERMISSION_DENIED), output.Status.Code)
}
if deniedResponse, ok := output.HttpResponse.(*ext_authz.CheckResponse_DeniedResponse); !ok {
t.Fatalf("Expected http response of type ext_authz.CheckResponse_DeniedResponse")
} else if deniedResponse.DeniedResponse.Status.Code != ext_type_v3.StatusCode_BadRequest {
t.Fatalf("Unexpected http status code: %v", deniedResponse.DeniedResponse.Status.Code)
}

if len(customLogger.events) != 1 {
Expand Down Expand Up @@ -1056,12 +1066,20 @@ func TestCheckBadDecisionWithLoggerV2(t *testing.T) {
ctx := context.Background()
output, err := server.Check(ctx, &req)

if err == nil {
t.Fatal("Expected error but got nil")
if err != nil {
t.Fatal("Expected nil err")
}

if output != nil {
t.Fatalf("Expected no output but got %v", output)
if output == nil {
t.Fatal("Expected output but got nil")
}
if output.Status.Code != int32(code.Code_PERMISSION_DENIED) {
t.Fatalf("Expected status %v status: %v", int32(code.Code_PERMISSION_DENIED), output.Status.Code)
}
if deniedResponse, ok := output.HttpResponse.(*ext_authz_v2.CheckResponse_DeniedResponse); !ok {
t.Fatalf("Expected http response of type ext_authz.CheckResponse_DeniedResponse")
} else if deniedResponse.DeniedResponse.Status.Code != ext_type_v2.StatusCode_BadRequest {
t.Fatalf("Unexpected http status code: %v", deniedResponse.DeniedResponse.Status.Code)
}

if len(customLogger.events) != 1 {
Expand Down

0 comments on commit f16fcd7

Please sign in to comment.