From aadbcd23bd84a7bc2da19a44e2e3fb615d43d8d6 Mon Sep 17 00:00:00 2001 From: bobby <1544881+desimone@users.noreply.github.com> Date: Mon, 19 Oct 2020 08:09:13 -0700 Subject: [PATCH] fwd-auth: fix nginx-ingress forward-auth (#1505 / #1497) Signed-off-by: Bobby DeSimone --- .devcontainer/envs/nginx.yaml | 2 - authorize/grpc.go | 81 +++++++--------- authorize/grpc_test.go | 147 ++++++++++++++++++++++------- examples/nginx/auth.conf | 11 --- examples/nginx/docker-compose.yaml | 2 - examples/nginx/ext_authz.conf | 16 ---- examples/nginx/httpbin.conf | 68 ++++++++++--- examples/nginx/proxy.conf | 84 +++++++++++------ 8 files changed, 257 insertions(+), 154 deletions(-) delete mode 100644 examples/nginx/auth.conf delete mode 100644 examples/nginx/ext_authz.conf diff --git a/.devcontainer/envs/nginx.yaml b/.devcontainer/envs/nginx.yaml index d3cbfda18..40e443c17 100644 --- a/.devcontainer/envs/nginx.yaml +++ b/.devcontainer/envs/nginx.yaml @@ -10,8 +10,6 @@ services: - ../../examples/nginx/httpbin.conf:/etc/nginx/conf.d/httpbin.conf - ../../examples/nginx/pomerium.conf:/etc/nginx/conf.d/pomerium.conf - ../../examples/nginx/proxy.conf:/etc/nginx/proxy.conf - - ../../examples/nginx/auth.conf:/etc/nginx/auth.conf - - ../../examples/nginx/ext_authz.conf:/etc/nginx/ext_authz.conf - ../../examples/nginx/_wildcard.localhost.pomerium.io.pem:/etc/nginx/nginx.pem - ../../examples/nginx/_wildcard.localhost.pomerium.io-key.pem:/etc/nginx/nginx-key.pem diff --git a/authorize/grpc.go b/authorize/grpc.go index 5028a2144..7345a088b 100644 --- a/authorize/grpc.go +++ b/authorize/grpc.go @@ -39,9 +39,21 @@ func (a *Authorize) Check(ctx context.Context, in *envoy_service_auth_v2.CheckRe state := a.state.Load() - // maybe rewrite http request for forward auth - isForwardAuth := a.handleForwardAuth(in) + // convert the incoming envoy-style http request into a go-style http request hreq := getHTTPRequestFromCheckRequest(in) + + isForwardAuth := a.isForwardAuth(in) + if isForwardAuth { + // update the incoming http request's uri to match the forwarded URI + fwdAuthURI := getForwardAuthURL(hreq) + in.Attributes.Request.Http.Scheme = fwdAuthURI.Scheme + in.Attributes.Request.Http.Host = fwdAuthURI.Host + in.Attributes.Request.Http.Path = fwdAuthURI.Path + if fwdAuthURI.RawQuery != "" { + in.Attributes.Request.Http.Path += "?" + fwdAuthURI.RawQuery + } + } + rawJWT, _ := loadRawSession(hreq, a.currentOptions.Load(), state.encoder) sessionState, _ := loadSession(state.encoder, rawJWT) @@ -65,7 +77,7 @@ func (a *Authorize) Check(ctx context.Context, in *envoy_service_auth_v2.CheckRe case reply.Status == http.StatusOK: return a.okResponse(reply), nil case reply.Status == http.StatusUnauthorized: - if isForwardAuth { + if isForwardAuth && hreq.URL.Path == "/verify" { return a.deniedResponse(in, http.StatusUnauthorized, "Unauthenticated", nil), nil } return a.redirectResponse(in), nil @@ -172,7 +184,23 @@ func (a *Authorize) getEnvoyRequestHeaders(signedJWT string) ([]*envoy_api_v2_co return hvos, nil } -func (a *Authorize) handleForwardAuth(req *envoy_service_auth_v2.CheckRequest) bool { +func getForwardAuthURL(r *http.Request) *url.URL { + urqQuery := r.URL.Query().Get("uri") + u, _ := urlutil.ParseAndValidateURL(urqQuery) + if u == nil { + u = &url.URL{ + Scheme: r.Header.Get(httputil.HeaderForwardedProto), + Host: r.Header.Get(httputil.HeaderForwardedHost), + Path: r.Header.Get(httputil.HeaderForwardedURI), + } + } + // todo(bdd): handle httputil.HeaderOriginalURL which incorporates + // path and query params + return u +} + +// isForwardAuth returns if the current request is a forward auth route. +func (a *Authorize) isForwardAuth(req *envoy_service_auth_v2.CheckRequest) bool { opts := a.currentOptions.Load() if opts.ForwardAuthURL == nil { @@ -180,37 +208,8 @@ func (a *Authorize) handleForwardAuth(req *envoy_service_auth_v2.CheckRequest) b } checkURL := getCheckRequestURL(req) - if urlutil.StripPort(checkURL.Host) != urlutil.StripPort(opts.GetForwardAuthURL().Host) { - return false - } - uriQuery := checkURL.Query().Get("uri") - if headers := req.GetAttributes().GetRequest().GetHttp().GetHeaders(); uriQuery == "" && headers != nil { - uriQuery = headers[http.CanonicalHeaderKey(httputil.HeaderForwardedProto)] + "://" + - headers[http.CanonicalHeaderKey(httputil.HeaderForwardedHost)] - if xfu := headers[http.CanonicalHeaderKey(httputil.HeaderForwardedURI)]; xfu != "/" { - uriQuery += xfu - } - } - - if (checkURL.Path != "/" && checkURL.Path != "/verify") || uriQuery == "" { - return false - } - verifyURL, err := url.Parse(uriQuery) - if err != nil { - log.Warn().Str("uri", checkURL.Query().Get("uri")).Err(err).Msg("failed to parse uri for forward authentication") - return false - } - - req.Attributes.Request.Http.Scheme = verifyURL.Scheme - req.Attributes.Request.Http.Host = verifyURL.Host - req.Attributes.Request.Http.Path = verifyURL.Path - - // envoy sends the query string as part of the path - if verifyURL.RawQuery != "" { - req.Attributes.Request.Http.Path += "?" + verifyURL.RawQuery - } - return true + return urlutil.StripPort(checkURL.Host) == urlutil.StripPort(opts.GetForwardAuthURL().Host) } func (a *Authorize) getEvaluatorRequestFromCheckRequest(in *envoy_service_auth_v2.CheckRequest, sessionState *sessions.State) *evaluator.Request { @@ -291,20 +290,6 @@ func getCheckRequestURL(req *envoy_service_auth_v2.CheckRequest) *url.URL { } else { u.Path = path } - - // check to make sure this is _not_ a verify endpoint and that forwarding - // headers are set. If so, infer the true authorization location from thos - if u.Path != "/verify" && h.GetHeaders() != nil { - if val, ok := h.GetHeaders()["x-forwarded-proto"]; ok && val != "" { - u.Scheme = val - } - if val, ok := h.GetHeaders()["x-forwarded-host"]; ok && val != "" { - u.Host = val - } - if val, ok := h.GetHeaders()["x-forwarded-uri"]; ok && val != "" && val != "/" { - u.Path = val - } - } return u } diff --git a/authorize/grpc_test.go b/authorize/grpc_test.go index 793570fc4..5225db73c 100644 --- a/authorize/grpc_test.go +++ b/authorize/grpc_test.go @@ -8,8 +8,11 @@ import ( envoy_service_auth_v2 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v2" "github.com/golang/protobuf/ptypes" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/genproto/googleapis/rpc/status" "google.golang.org/grpc" "github.com/pomerium/pomerium/authorize/evaluator" @@ -95,7 +98,7 @@ func Test_getEvaluatorRequest(t *testing.T) { }, HTTP: evaluator.RequestHTTP{ Method: "GET", - URL: "https://example.com/some/path?qs=1", + URL: "http://example.com/some/path?qs=1", Headers: map[string]string{ "Accept": "text/html", "X-Forwarded-Proto": "https", @@ -108,12 +111,12 @@ func Test_getEvaluatorRequest(t *testing.T) { } func Test_handleForwardAuth(t *testing.T) { + tests := []struct { name string checkReq *envoy_service_auth_v2.CheckRequest - attrCtxHTTPReq *envoy_service_auth_v2.AttributeContext_HttpRequest forwardAuthURL string - isForwardAuth bool + want bool }{ { name: "enabled", @@ -132,21 +135,14 @@ func Test_handleForwardAuth(t *testing.T) { }, }, }, - attrCtxHTTPReq: &envoy_service_auth_v2.AttributeContext_HttpRequest{ - Method: "GET", - Path: "/some/path?qs=1", - Host: "example.com", - Scheme: "https", - }, forwardAuthURL: "https://forward-auth.example.com", - isForwardAuth: true, + want: true, }, { name: "disabled", checkReq: nil, - attrCtxHTTPReq: nil, forwardAuthURL: "", - isForwardAuth: false, + want: false, }, { name: "honor x-forwarded-uri set", @@ -170,19 +166,8 @@ func Test_handleForwardAuth(t *testing.T) { }, }, }, - attrCtxHTTPReq: &envoy_service_auth_v2.AttributeContext_HttpRequest{ - Method: "GET", - Path: "/foo/bar", - Host: "example.com", - Scheme: "https", - Headers: map[string]string{ - httputil.HeaderForwardedURI: "/foo/bar", - httputil.HeaderForwardedProto: "https", - httputil.HeaderForwardedHost: "example.com", - }, - }, forwardAuthURL: "https://forward-auth.example.com", - isForwardAuth: true, + want: true, }, { name: "request with invalid forward auth url", @@ -201,9 +186,8 @@ func Test_handleForwardAuth(t *testing.T) { }, }, }, - attrCtxHTTPReq: nil, forwardAuthURL: "https://forward-auth.example.com", - isForwardAuth: false, + want: false, }, { name: "request with invalid path", @@ -222,9 +206,8 @@ func Test_handleForwardAuth(t *testing.T) { }, }, }, - attrCtxHTTPReq: nil, forwardAuthURL: "https://forward-auth.example.com", - isForwardAuth: false, + want: true, }, { name: "request with empty uri", @@ -243,9 +226,8 @@ func Test_handleForwardAuth(t *testing.T) { }, }, }, - attrCtxHTTPReq: nil, forwardAuthURL: "https://forward-auth.example.com", - isForwardAuth: false, + want: true, }, { name: "request with invalid uri", @@ -264,9 +246,8 @@ func Test_handleForwardAuth(t *testing.T) { }, }, }, - attrCtxHTTPReq: nil, forwardAuthURL: "https://forward-auth.example.com", - isForwardAuth: false, + want: true, }, } @@ -279,9 +260,11 @@ func Test_handleForwardAuth(t *testing.T) { fau = mustParseURL(tc.forwardAuthURL) } a.currentOptions.Store(&config.Options{ForwardAuthURL: fau}) - assert.Equal(t, tc.isForwardAuth, a.handleForwardAuth(tc.checkReq)) - if tc.attrCtxHTTPReq != nil { - assert.Equal(t, tc.attrCtxHTTPReq, tc.checkReq.Attributes.Request.Http) + + got := a.isForwardAuth(tc.checkReq) + + if diff := cmp.Diff(got, tc.want); diff != "" { + t.Errorf("Authorize.Check() = %s", diff) } }) } @@ -325,7 +308,7 @@ func Test_getEvaluatorRequestWithPortInHostHeader(t *testing.T) { Session: evaluator.RequestSession{}, HTTP: evaluator.RequestHTTP{ Method: "GET", - URL: "https://example.com/some/path?qs=1", + URL: "http://example.com/some/path?qs=1", Headers: map[string]string{ "Accept": "text/html", "X-Forwarded-Proto": "https", @@ -486,3 +469,95 @@ type mockDataBrokerServiceClient struct { func (m mockDataBrokerServiceClient) Get(ctx context.Context, in *databroker.GetRequest, opts ...grpc.CallOption) (*databroker.GetResponse, error) { return m.get(ctx, in, opts...) } + +func TestAuthorize_Check(t *testing.T) { + opt := config.NewDefaultOptions() + opt.AuthenticateURL = mustParseURL("https://authenticate.example.com") + opt.DataBrokerURL = mustParseURL("https://databroker.example.com") + opt.SharedKey = "E8wWIMnihUx+AUfRegAQDNs8eRb3UrB5G3zlJW9XJDM=" + a, err := New(&config.Config{Options: opt}) + if err != nil { + t.Fatal(err) + } + a.currentOptions.Store(&config.Options{ForwardAuthURL: mustParseURL("https://forward-auth.example.com")}) + + cmpOpts := []cmp.Option{ + cmpopts.IgnoreUnexported(envoy_service_auth_v2.CheckResponse{}), + cmpopts.IgnoreUnexported(status.Status{}), + cmpopts.IgnoreTypes(envoy_service_auth_v2.DeniedHttpResponse{}), + } + tests := []struct { + name string + in *envoy_service_auth_v2.CheckRequest + want *envoy_service_auth_v2.CheckResponse + wantErr bool + }{ + {"basic deny", + &envoy_service_auth_v2.CheckRequest{ + Attributes: &envoy_service_auth_v2.AttributeContext{ + Source: &envoy_service_auth_v2.AttributeContext_Peer{ + Certificate: url.QueryEscape(certPEM), + }, + Request: &envoy_service_auth_v2.AttributeContext_Request{ + Http: &envoy_service_auth_v2.AttributeContext_HttpRequest{ + Id: "id-1234", + Method: "GET", + Headers: map[string]string{ + "accept": "application/json", + "x-forwarded-proto": "https", + }, + Path: "/some/path?qs=1", + Host: "example.com", + Scheme: "http", + Body: "BODY", + }, + }, + }, + }, + &envoy_service_auth_v2.CheckResponse{ + Status: &status.Status{Code: 7, Message: "Access Denied"}, + HttpResponse: &envoy_service_auth_v2.CheckResponse_DeniedResponse{ + DeniedResponse: &envoy_service_auth_v2.DeniedHttpResponse{}, + }, + }, + false}, + {"basic forward-auth deny", + &envoy_service_auth_v2.CheckRequest{ + Attributes: &envoy_service_auth_v2.AttributeContext{ + Source: &envoy_service_auth_v2.AttributeContext_Peer{ + Certificate: url.QueryEscape(certPEM), + }, + Request: &envoy_service_auth_v2.AttributeContext_Request{ + Http: &envoy_service_auth_v2.AttributeContext_HttpRequest{ + Method: "GET", + Path: "/verify?uri=" + url.QueryEscape("https://example.com/some/path?qs=1"), + Host: "forward-auth.example.com", + Scheme: "https", + }, + }, + }, + }, + &envoy_service_auth_v2.CheckResponse{ + Status: &status.Status{Code: 7, Message: "Access Denied"}, + HttpResponse: &envoy_service_auth_v2.CheckResponse_DeniedResponse{ + DeniedResponse: &envoy_service_auth_v2.DeniedHttpResponse{}, + }, + }, + false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + got, err := a.Check(context.TODO(), tt.in) + if (err != nil) != tt.wantErr { + t.Errorf("Authorize.Check() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if diff := cmp.Diff(got, tt.want, cmpOpts...); diff != "" { + t.Errorf("NewStore() = %s", diff) + } + + }) + } +} diff --git a/examples/nginx/auth.conf b/examples/nginx/auth.conf deleted file mode 100644 index 4ccf0b861..000000000 --- a/examples/nginx/auth.conf +++ /dev/null @@ -1,11 +0,0 @@ -# Send auth check to /authorize location. -auth_request /authorize; - -auth_request_set $target_url $scheme://$http_host$request_uri; - -# Set cookies we get back from the auth check -auth_request_set $saved_set_cookie $upstream_http_set_cookie; -add_header Set-Cookie $saved_set_cookie; - -# If we get a 401, respond with a named location -error_page 401 =302 https://fwdauth.localhost.pomerium.io/?uri=$target_url; diff --git a/examples/nginx/docker-compose.yaml b/examples/nginx/docker-compose.yaml index bb002c186..cff0793c7 100644 --- a/examples/nginx/docker-compose.yaml +++ b/examples/nginx/docker-compose.yaml @@ -12,8 +12,6 @@ services: - ./_wildcard.localhost.pomerium.io.pem:/etc/nginx/nginx.pem - ./_wildcard.localhost.pomerium.io-key.pem:/etc/nginx/nginx-key.pem - ./proxy.conf:/etc/nginx/proxy.conf - - ./auth.conf:/etc/nginx/auth.conf - - ./ext_authz.conf:/etc/nginx/ext_authz.conf httpbin: image: kennethreitz/httpbin:latest diff --git a/examples/nginx/ext_authz.conf b/examples/nginx/ext_authz.conf deleted file mode 100644 index 8e5ae6750..000000000 --- a/examples/nginx/ext_authz.conf +++ /dev/null @@ -1,16 +0,0 @@ -location /authorize { - proxy_pass http://pomerium/verify?uri=$scheme://$http_host$request_uri; - proxy_set_header Host fwdauth.localhost.pomerium.io; - proxy_http_version 1.1; - - proxy_set_header X-Original-URL $scheme://$http_host$request_uri; - proxy_set_header X-Real-IP $remote_addr; - proxy_set_header X-Forwarded-For $remote_addr; - proxy_set_header X-Forwarded-Proto $scheme; - proxy_set_header X-Forwarded-Host $http_host; - proxy_set_header X-Forwarded-Uri $request_uri; - proxy_cache_bypass $cookie_session; - proxy_no_cache $cookie_session; - proxy_buffers 4 32k; - proxy_pass_request_body off; -} diff --git a/examples/nginx/httpbin.conf b/examples/nginx/httpbin.conf index 6e384f824..aabd52c32 100644 --- a/examples/nginx/httpbin.conf +++ b/examples/nginx/httpbin.conf @@ -1,16 +1,62 @@ # Protected application server { - listen 443 ssl; - listen 80; - server_name httpbin.localhost.pomerium.io; - ssl_certificate /etc/nginx/nginx.pem; - ssl_certificate_key /etc/nginx/nginx-key.pem; + listen 80; + listen 443 ssl http2; - include /etc/nginx/ext_authz.conf; + server_name httpbin.localhost.pomerium.io; + ssl_certificate /etc/nginx/nginx.pem; + ssl_certificate_key /etc/nginx/nginx-key.pem; - location / { - proxy_pass http://httpbin; - include /etc/nginx/auth.conf; - include /etc/nginx/proxy.conf; - } + + location = /ext_authz { + internal; + + proxy_pass_request_body off; + proxy_set_header Content-Length ""; + proxy_set_header X-Forwarded-Proto ""; + + proxy_set_header Host fwdauth.localhost.pomerium.io; + proxy_set_header X-Original-URL $scheme://$http_host$request_uri; + proxy_set_header X-Original-Method $request_method; + proxy_set_header X-Real-IP $remote_addr; + + proxy_set_header X-Forwarded-For $remote_addr; + + proxy_set_header X-Auth-Request-Redirect $request_uri; + + proxy_buffering off; + + proxy_buffer_size 4k; + proxy_buffers 4 4k; + proxy_request_buffering on; + proxy_http_version 1.1; + + proxy_ssl_server_name on; + proxy_pass_request_headers on; + + client_max_body_size 1m; + + # Pass the extracted client certificate to the auth provider + set $target http://pomerium/verify?uri=$scheme://$http_host$request_uri; + proxy_pass $target; + } + + location @authredirect { + internal; + add_header Set-Cookie $auth_cookie; + return 302 + https://fwdauth.localhost.pomerium.io/?uri=$scheme://$host$request_uri; + } + + location / { + proxy_pass http://httpbin; + + include /etc/nginx/proxy.conf; + # If we get a 401, respond with a named location + error_page 401 = @authredirect; + # this location requires authentication + auth_request /ext_authz; + auth_request_set $auth_cookie $upstream_http_set_cookie; + add_header Set-Cookie $auth_cookie; + } } diff --git a/examples/nginx/proxy.conf b/examples/nginx/proxy.conf index 36b51d6bc..79d735c1f 100644 --- a/examples/nginx/proxy.conf +++ b/examples/nginx/proxy.conf @@ -1,33 +1,61 @@ -client_body_buffer_size 128k; +set $pass_access_scheme $scheme; -#Timeout if the real server is dead -proxy_next_upstream error timeout invalid_header http_500 http_502 http_503; +set $pass_server_port $server_port; -# Advanced Proxy Config -send_timeout 5m; -proxy_read_timeout 360; -proxy_send_timeout 360; -proxy_connect_timeout 360; +set $best_http_host $http_host; +set $pass_port $pass_server_port; + +set $proxy_alternative_upstream_name ""; + +client_max_body_size 1m; + + +proxy_set_header Host $best_http_host; + +# Pass the extracted client certificate to the backend + +# Allow websocket connections +proxy_set_header Upgrade $http_upgrade; -# Basic Proxy Config -proxy_set_header Host $host; -proxy_set_header X-Real-IP $remote_addr; -proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; -proxy_set_header X-Forwarded-Proto $scheme; -proxy_set_header X-Forwarded-Host $http_host; -proxy_set_header X-Forwarded-Uri $request_uri; -proxy_set_header X-Forwarded-Ssl on; -proxy_redirect http:// $scheme://; -proxy_http_version 1.1; proxy_set_header Connection ""; -proxy_cache_bypass $cookie_session; -proxy_no_cache $cookie_session; -proxy_buffers 64 256k; -# If behind reverse proxy, forwards the correct IP -set_real_ip_from 10.0.0.0/8; -set_real_ip_from 172.0.0.0/8; -set_real_ip_from 192.168.0.0/16; -set_real_ip_from fc00::/7; -real_ip_header X-Forwarded-For; -real_ip_recursive on; +# proxy_set_header X-Request-ID $req_id; +proxy_set_header X-Real-IP $remote_addr; +proxy_set_header X-Forwarded-For $remote_addr; +proxy_set_header X-Forwarded-Host $best_http_host; +proxy_set_header X-Forwarded-Port $pass_port; +proxy_set_header X-Forwarded-Proto $pass_access_scheme; + +proxy_set_header X-Scheme $pass_access_scheme; + +# Pass the original X-Forwarded-For +proxy_set_header X-Original-Forwarded-For $http_x_forwarded_for; + +# mitigate HTTPoxy Vulnerability +# https://www.nginx.com/blog/mitigating-the-httpoxy-vulnerability-with-nginx/ +proxy_set_header Proxy ""; + +# Custom headers to proxied server + +proxy_connect_timeout 5s; +proxy_send_timeout 60s; +proxy_read_timeout 60s; + +proxy_buffering off; +proxy_buffer_size 4k; +proxy_buffers 4 4k; + +proxy_max_temp_file_size 1024m; + +proxy_request_buffering on; +proxy_http_version 1.1; + +proxy_cookie_domain off; +proxy_cookie_path off; + +# In case of errors try the next upstream server before returning an error +proxy_next_upstream error timeout; +proxy_next_upstream_timeout 0; +proxy_next_upstream_tries 3; + +proxy_redirect off;