Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decision API support for the nginx-ingress annotation nginx.ingress.kubernetes.io/auth-url #521

Closed
AlbinoDrought opened this issue Sep 24, 2020 · 9 comments
Labels
stale Feedback from one or more authors is required to proceed.

Comments

@AlbinoDrought
Copy link
Contributor

AlbinoDrought commented Sep 24, 2020

Is your feature request related to a problem? Please describe.

We are using nginx-ingress with the nginx.ingress.kubernetes.io/auth-url annotation.

We want to apply an access rule to this URL: https://admin.site.example/dashboard

We have an ingress config that looks like this:

ingress:
  enabled: true
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-url: "http://oathkeeper.site.example/decisions$request_uri"
    nginx.ingress.kubernetes.io/auth-response-headers: Authorization

Oathkeeper expects (correct me if I'm wrong) a request that looks like this:

GET /decisions/dashboard HTTP/2
Host: admin.site.example

Oathkeeper will actually receive:

GET /decisions/dashboard HTTP/2
Host: oathkeeper.site.example

The default nginx-ingress template generates an NGINX config where the Host header sent to Oathkeeper is the auth_request host oathkeeper.site.example instead of our target host admin.site.example:

Generated nginx-ingress config
	## start server admin.site.example
	server {
		server_name admin.site.example ;		
		listen 80 proxy_protocol ;
		listen 443 proxy_protocol proxy_protocol ssl http2 ;		
		set $proxy_upstream_name "-";		

		ssl_certificate_by_lua_block {
			certificate.call()
		}		

		location = /_external-auth-Lw {
			internal;			# ngx_auth_request module overrides variables in the parent request,
			# therefore we have to explicitly set this variable again so that when the parent request
			# resumes it has the correct value set for this variable so that Lua can pick backend correctly
			set $proxy_upstream_name "example-site-admin-web-80";			
			proxy_pass_request_body     off;
			proxy_set_header            Content-Length          "";
			proxy_set_header            X-Forwarded-Proto       "";
			proxy_set_header            X-Request-ID            $req_id;			
			proxy_set_header            Host                    oathkeeper.site.example;
			proxy_set_header            X-Original-URL          $scheme://$http_host$request_uri;
			proxy_set_header            X-Original-Method       $request_method;
			proxy_set_header            X-Sent-From             "nginx-ingress-controller";
			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;			
			# Pass the extracted client certificate to the auth provider			
			proxy_set_header Host "admin.site.example";			
			set $target http://oathkeeper.site.example/decisions$request_uri;
			proxy_pass $target;
		}		
		
		location / {			
			set $namespace      "example-site-admin";
			set $ingress_name   "example-site-admin-web";
			set $service_name   "example-site-admin-web";
			set $service_port   "80";
			set $location_path  "/";			rewrite_by_lua_block {
				lua_ingress.rewrite({
					force_ssl_redirect = false,
					ssl_redirect = true,
					force_no_ssl_redirect = false,
					use_port_in_redirects = false,
				})
				balancer.rewrite()
				plugins.run()
			}			
			# be careful with `access_by_lua_block` and `satisfy any` directives as satisfy any
			# will always succeed when there's `access_by_lua_block` that does not have any lua code doing `ngx.exit(ngx.DECLINED)`
			# other authentication method such as basic auth or external auth useless - all requests will be allowed.
			#access_by_lua_block {
			#}			
			header_filter_by_lua_block {
				lua_ingress.header()
				plugins.run()
			}			
			body_filter_by_lua_block {
			}			
			log_by_lua_block {
				balancer.log()				
			  monitor.call()				
			  plugins.run()
			}			
			port_in_redirect off;			set $balancer_ewma_score -1;
			set $proxy_upstream_name "example-site-admin-web-80";
			set $proxy_host          $proxy_upstream_name;
			set $pass_access_scheme  $scheme;			
			set $pass_server_port    $proxy_protocol_server_port;			
			set $best_http_host      $http_host;
			set $pass_port           $pass_server_port;			
			set $proxy_alternative_upstream_name "";			
			# this location requires authentication
			auth_request        /_external-auth-Lw;
			auth_request_set    $auth_cookie $upstream_http_set_cookie;
			add_header          Set-Cookie $auth_cookie;
			auth_request_set $authHeader0 $upstream_http_authorization;
			proxy_set_header 'Authorization' $authHeader0;			
			proxy_set_header Host                   $best_http_host;				
			# Allow websocket connections
			proxy_set_header                        Upgrade           $http_upgrade;			
			proxy_set_header                        Connection        $connection_upgrade;			
			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_pass http://upstream_balancer;			
			proxy_redirect                          off;		
		}	
	}
	## end server admin.site.example

Describe the solution you'd like

I would like to be able to point the auth-url annotation at Oathkeeper and have everything work without changing any internal nginx-ingress config templates.

Describe alternatives you've considered

The default template includes all the normal X-Forwarded-* headers. It may work out-of-the-box with the /decisions/traefik endpoint added in #486 , but I'm not sure if this is released yet.

Additional context

oryd/oathkeeper:v0.38.3-beta.1

quay.io/kubernetes-ingress-controller/nginx-ingress-controller:0.32.0

@AlbinoDrought
Copy link
Contributor Author

Also, please let me know if I'm missing something here and it's already simple to use the nginx-ingress auth-url annotation with oryd/oathkeeper:v0.38.3-beta.1

@AlbinoDrought
Copy link
Contributor Author

AlbinoDrought commented Sep 24, 2020

For what it's worth, patching 3652a77 with this works for our usecase:

diff --git a/api/decision.go b/api/decision.go
index 7347b13..0468f9a 100644
--- a/api/decision.go
+++ b/api/decision.go
@@ -22,6 +22,7 @@ package api
 
 import (
 	"net/http"
+	"net/url"
 
 	"github.com/ory/oathkeeper/pipeline/authn"
 	"github.com/ory/oathkeeper/x"
@@ -52,12 +53,33 @@ func NewJudgeHandler(r decisionHandlerRegistry) *DecisionHandler {
 
 func (h *DecisionHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
 	if len(r.URL.Path) >= len(DecisionPath) && r.URL.Path[:len(DecisionPath)] == DecisionPath {
-		r.URL.Scheme = "http"
-		r.URL.Host = r.Host
-		if r.TLS != nil {
-			r.URL.Scheme = "https"
+		originalURL := r.Header.Get("X-Original-URL")
+		if originalURL == "" {
+			r.URL.Scheme = "http"
+			r.URL.Host = r.Host
+			if r.TLS != nil {
+				r.URL.Scheme = "https"
+			}
+			r.URL.Path = r.URL.Path[len(DecisionPath):]
+		} else {
+			url, err := url.Parse(originalURL)
+			if err != nil {
+				h.r.Logger().WithError(err).
+					WithFields(map[string]interface{}{
+						"http_method":     r.Method,
+						"http_url":        r.URL.String(),
+						"http_host":       r.Host,
+						"http_user_agent": r.UserAgent(),
+						"granted":         false,
+						"x_original_url":  originalURL,
+					}).
+					Warn("Failed to parse X-Original-URL")
+				h.r.ProxyRequestHandler().HandleError(w, r, nil, err)
+				return
+			}
+
+			r.URL = url
 		}
-		r.URL.Path = r.URL.Path[len(DecisionPath):]
 
 		h.decisions(w, r)
 	} else {

@aeneasr
Copy link
Member

aeneasr commented Sep 30, 2020

Thank you for raising this. I think we'll add this with #487 as a dedicated k8s nginx ingress route (or alternative pure nginx)!

@dennislemonmarkets
Copy link

dennislemonmarkets commented Mar 22, 2021

@AlbinoDrought Just stumbled over this issue.

Aren't you able to solve the problem with the nginx.ingress.kubernetes.io/auth-proxy-set-headers annotation, by passing the original host to the oathkeeper api endpoint?

I never tested this on kubernetes, but I made a small demo with docker compose where I ran in into the same issue with the host. I could solve it by proxying the header value to the auth request. So I got high hopes it will work with the kubernetes ingress, too.

@NikChnv
Copy link

NikChnv commented Apr 28, 2021

nginx.ingress.kubernetes.io/auth-proxy-set-headers doesn't allow to rewrite Host header. So this is not working for ingress-nginx

@mdreizin
Copy link

mdreizin commented Jan 5, 2022

@aeneasr it looks like, we may introduce an additional configuration option to specify a strategy to get/build URL and Method.

What do you think about that approach?

config.yaml:

access_rules:
  # traefix, nginx, etc
  matching_url_strategy: default
package main

import (
	"errors"
	"net/http"
	"net/url"
)

type MatchURL struct {
	*url.URL
	Method string
}

type MatchURLBuilder interface {
	BuildURL(r *http.Request) (*MatchURL, error)
}

var _ MatchURLBuilder = (*DefaultMatchURLBuilder)(nil)

type DefaultMatchURLBuilder struct {}

func (b *DefaultMatchURLBuilder) BuildURL(r *http.Request) (*MatchURL, error) {
	return &MatchURL{
		URL:    r.URL,
		Method: r.Method,
	}, nil
}

type TraefikMatchURLBuilder struct {}

var _ MatchURLBuilder = (*TraefikMatchURLBuilder)(nil)

func (b *TraefikMatchURLBuilder) BuildURL(r *http.Request) (*MatchURL, error) {
	u := &url.URL{
		Scheme: r.Header.Get("X-Forwarded-Proto"),
		Host:   r.Header.Get("X-Forwarded-Host"),
		Path:   r.Header.Get("X-Forwarded-Uri"),
	}
	m := r.Header.Get("X-Forwarded-Method")
	if len(m) == 0 {
		return nil, errors.New("`X-Forwarded-Method` header is not set")
	}

	return &MatchURL{
		URL:    u,
		Method: m,
	}, nil
}

var matchURLBuilders = map[string]MatchURLBuilder {
	"default": &DefaultMatchURLBuilder{},
	"traefik": &TraefikMatchURLBuilder{},
	// etc
}

aeneasr pushed a commit that referenced this issue Feb 14, 2022
@aeneasr
Copy link
Member

aeneasr commented Apr 8, 2022

That’s also an interesting approach! Originally the idea was to have specific endpoints for the various services, which I think is still a valid path forward and also a bit easier to configure / get roght?

@raman-nbg
Copy link

I was able to integrate Oathkeeper with NGINX Ingress using the latest version of Oathkeeper (at least > v0.38.20-beta.1 that includes #904) and including the following annotations.

    nginx.ingress.kubernetes.io/auth-url: "http://oathkeeper-api.iam.svc.cluster.local:4456/decisions"
    nginx.ingress.kubernetes.io/auth-method: GET
    nginx.ingress.kubernetes.io/auth-response-headers: Authorization
    nginx.ingress.kubernetes.io/auth-snippet: |
      proxy_set_header X-Original-URL $request_uri;
      proxy_set_header X-Forwarded-Method $request_method;
      proxy_set_header X-Forwarded-Proto $scheme;
      proxy_set_header X-Forwarded-Host $host;
      proxy_set_header X-Forwarded-Uri $request_uri;

But the issue of #461 still remains unsolved.

@github-actions
Copy link

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

  • open a PR referencing and resolving the issue;
  • leave a comment on it and discuss ideas on how you could contribute towards resolving it;
  • leave a comment and describe in detail why this issue is critical for your use case;
  • open a new issue with updated details and a plan for resolving the issue.

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️

@github-actions github-actions bot added the stale Feedback from one or more authors is required to proceed. label Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Feedback from one or more authors is required to proceed.
Projects
None yet
Development

No branches or pull requests

6 participants