From 7dbefee35548114346aabcea31636119194d7d1f Mon Sep 17 00:00:00 2001 From: Anthony Regeda Date: Tue, 31 Dec 2024 17:23:07 +0100 Subject: [PATCH] feat: return query_parameters_to_remove with OkResponse (#621) Envoy External Auth v3 API supports the `query_parameters_to_remove` attribute with `OkResponse`. See https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-okhttpresponse The change allows to return from OPA which query parameters should be removed from the upstream query. Signed-off-by: Anthony Regeda --- envoyauth/response.go | 38 +++++++++++-------- envoyauth/response_test.go | 78 +++++++++++++++++++++++++++++++++++--- internal/internal.go | 15 ++++++-- internal/internal_test.go | 48 ++++++++++++++++++++++- 4 files changed, 154 insertions(+), 25 deletions(-) diff --git a/envoyauth/response.go b/envoyauth/response.go index be0097af1..e54c9de55 100644 --- a/envoyauth/response.go +++ b/envoyauth/response.go @@ -114,43 +114,51 @@ func (result *EvalResult) IsAllowed() (bool, error) { return false, result.invalidDecisionErr() } -// GetRequestHTTPHeadersToRemove - returns the http headers to remove from the original request before dispatching -// it to the upstream -func (result *EvalResult) GetRequestHTTPHeadersToRemove() ([]string, error) { - headersToRemove := []string{} - +func (result *EvalResult) getStringSliceFromDecision(fieldName string) ([]string, error) { switch decision := result.Decision.(type) { case bool: - return headersToRemove, nil + return nil, nil case map[string]interface{}: var ok bool var val interface{} - if val, ok = decision["request_headers_to_remove"]; !ok { - return headersToRemove, nil + if val, ok = decision[fieldName]; !ok { + return nil, nil } switch val := val.(type) { case []string: return val, nil case []interface{}: - for _, vval := range val { - header, ok := vval.(string) + ss := make([]string, len(val)) + for i, v := range val { + s, ok := v.(string) if !ok { - return nil, fmt.Errorf("type assertion error, expected request_headers_to_remove value to be of type 'string' but got '%T'", vval) + return nil, fmt.Errorf("type assertion error, expected %s value to be of type 'string' but got '%T'", fieldName, v) } - - headersToRemove = append(headersToRemove, header) + ss[i] = s } - return headersToRemove, nil + return ss, nil default: - return nil, fmt.Errorf("type assertion error, expected request_headers_to_remove to be of type '[]string' but got '%T'", val) + return nil, fmt.Errorf("type assertion error, expected %s to be of type '[]string' but got '%T'", fieldName, val) } } return nil, result.invalidDecisionErr() } +// GetRequestQueryParametersToRemove - returns the query parameters to remove from the original request before dispatching +// it to the upstream +func (result *EvalResult) GetRequestQueryParametersToRemove() ([]string, error) { + return result.getStringSliceFromDecision("query_parameters_to_remove") +} + +// GetRequestHTTPHeadersToRemove - returns the http headers to remove from the original request before dispatching +// it to the upstream +func (result *EvalResult) GetRequestHTTPHeadersToRemove() ([]string, error) { + return result.getStringSliceFromDecision("request_headers_to_remove") +} + // GetResponseHTTPHeaders - returns the http headers to return if they are part of the decision func (result *EvalResult) GetResponseHTTPHeaders() (http.Header, error) { var responseHeaders = make(http.Header) diff --git a/envoyauth/response_test.go b/envoyauth/response_test.go index 6d151384d..717c50503 100644 --- a/envoyauth/response_test.go +++ b/envoyauth/response_test.go @@ -46,6 +46,74 @@ func TestIsAllowed(t *testing.T) { } } +func TestGetRequestQueryParametersToRemove(t *testing.T) { + tests := map[string]struct { + decision interface{} + exp []string + wantErr bool + }{ + "bool_eval_result": { + true, + nil, + false, + }, + "invalid_eval_result": { + "hello", + nil, + true, + }, + "empty_map_result": { + map[string]interface{}{}, + nil, + false, + }, + "bad_param_value": { + map[string]interface{}{"query_parameters_to_remove": "test"}, + nil, + true, + }, + "string_array_param_value": { + map[string]interface{}{"query_parameters_to_remove": []string{"foo", "bar"}}, + []string{"foo", "bar"}, + false, + }, + "interface_array_param_value": { + map[string]interface{}{"query_parameters_to_remove": []interface{}{"foo", "bar", "fuz"}}, + []string{"foo", "bar", "fuz"}, + false, + }, + "interface_array_bad_param_value": { + map[string]interface{}{"query_parameters_to_remove": []interface{}{1}}, + nil, + true, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + er := EvalResult{ + Decision: tc.decision, + } + + result, err := er.GetRequestQueryParametersToRemove() + + if tc.wantErr { + if err == nil { + t.Fatal("Expected error but got nil") + } + } else { + if err != nil { + t.Fatalf("Unexpected error %v", err) + } + + if !reflect.DeepEqual(tc.exp, result) { + t.Fatalf("Expected result %v but got %v", tc.exp, result) + } + } + }) + } +} + func TestGetRequestHTTPHeadersToRemove(t *testing.T) { tests := map[string]struct { decision interface{} @@ -54,22 +122,22 @@ func TestGetRequestHTTPHeadersToRemove(t *testing.T) { }{ "bool_eval_result": { true, - []string{}, + nil, false, }, "invalid_eval_result": { "hello", - []string{}, + nil, true, }, "empty_map_result": { map[string]interface{}{}, - []string{}, + nil, false, }, "bad_header_value": { map[string]interface{}{"request_headers_to_remove": "test"}, - []string{}, + nil, true, }, "string_array_header_value": { @@ -84,7 +152,7 @@ func TestGetRequestHTTPHeadersToRemove(t *testing.T) { }, "interface_array_bad_header_value": { map[string]interface{}{"request_headers_to_remove": []interface{}{1}}, - []string{}, + nil, true, }, } diff --git a/internal/internal.go b/internal/internal.go index b5dffdf53..0fcbaf91a 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -525,11 +525,20 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (* return nil, stop, internalErr } + var queryParamsToRemove []string + queryParamsToRemove, err = result.GetRequestQueryParametersToRemove() + if err != nil { + err = errors.Wrap(err, "failed to get request query parameters to remove") + internalErr = newInternalError(EnvoyAuthResultErr, err) + return nil, stop, internalErr + } + resp.HttpResponse = &ext_authz_v3.CheckResponse_OkResponse{ OkResponse: &ext_authz_v3.OkHttpResponse{ - Headers: responseHeaders, - HeadersToRemove: headersToRemove, - ResponseHeadersToAdd: responseHeadersToAdd, + Headers: responseHeaders, + HeadersToRemove: headersToRemove, + ResponseHeadersToAdd: responseHeadersToAdd, + QueryParametersToRemove: queryParamsToRemove, }, } } else { diff --git a/internal/internal_test.go b/internal/internal_test.go index f4edb1f28..852a554df 100644 --- a/internal/internal_test.go +++ b/internal/internal_test.go @@ -8,8 +8,6 @@ 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" @@ -21,6 +19,8 @@ import ( ext_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ext_authz_v2 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v2" ext_authz "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" + ext_type_v2 "github.com/envoyproxy/go-control-plane/envoy/type" + ext_type_v3 "github.com/envoyproxy/go-control-plane/envoy/type/v3" _structpb "github.com/golang/protobuf/ptypes/struct" "github.com/prometheus/client_golang/prometheus" "google.golang.org/genproto/googleapis/rpc/code" @@ -1518,6 +1518,50 @@ func TestCheckAllowBooleanDecisionDynamicMetadataDecisionID(t *testing.T) { assertDynamicMetadataDecisionID(t, output.GetDynamicMetadata()) } +func TestCheckAllowObjectDecisionReqQueryParamsToRemove(t *testing.T) { + var req ext_authz.CheckRequest + if err := util.Unmarshal([]byte(exampleAllowedRequest), &req); err != nil { + panic(err) + } + + module := ` + package envoy.authz + + default allow = true + + query_parameters_to_remove := ["foo", "bar"] + + result["allowed"] = allow + result["query_parameters_to_remove"] = query_parameters_to_remove` + + server := testAuthzServerWithModule(module, "envoy/authz/result", nil, withCustomLogger(&testPlugin{})) + ctx := context.Background() + output, err := server.Check(ctx, &req) + if err != nil { + t.Fatal(err) + } + + if output.Status.Code != int32(code.Code_OK) { + t.Fatalf("Expected request to be allowed but got: %v", output) + } + + response := output.GetOkResponse() + if response == nil { + t.Fatal("Expected OkHttpResponse struct but got nil") + } + + queryParams := response.GetQueryParametersToRemove() + if len(queryParams) != 2 { + t.Fatalf("Expected two query params but got %v", len(queryParams)) + } + + expectedQueryParams := []string{"foo", "bar"} + + if !reflect.DeepEqual(expectedQueryParams, queryParams) { + t.Fatalf("Expected query params %v but got %v", expectedQueryParams, queryParams) + } +} + func TestCheckAllowObjectDecisionReqHeadersToRemove(t *testing.T) { var req ext_authz.CheckRequest if err := util.Unmarshal([]byte(exampleAllowedRequestParsedPath), &req); err != nil {