Skip to content

Commit

Permalink
set invalid request status if request parsing fails
Browse files Browse the repository at this point in the history 
Signed-off-by: Rudrakh Panigrahi <[email protected]>
  • Loading branch information
@rudrakhp
rudrakhp committed Aug 7, 2024
1 parent 48a1f99 commit 143b712
Show file tree
Hide file tree
Showing 4 changed files with 86 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
15 changes: 15 additions & 0 deletions test/bats/test.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,21 @@ SLEEP_TIME=1
[ "$result" == 200 ]
}

@test "alice cannot access invalid url /%rr" {
host="$(docker_ip 'kind-control-plane')"
port="$(ingress_port)"

echo "$host"
echo "$port"

cmd="docker exec kind-control-plane curl --connect-timeout 30 --max-time 60 --retry 10 --retry-delay 0 --retry-max-time 600 \
--retry-connrefused --user alice:password -s -o /dev/null -w "%{http_code}" http://"$host":"$port"/%rr"

result=`$cmd`
echo "result: $result"
[ "$result" == 400 ]
}

@test "alice cannot access /api/v1/products" {
host="$(docker_ip 'kind-control-plane')"
port="$(ingress_port)"
Expand Down

0 comments on commit 143b712

Please sign in to comment.