traefik-forward-auth icon indicating copy to clipboard operation
traefik-forward-auth copied to clipboard

AuthCallback handlers aren't fired when using rules

Open JoshuaSjoding opened this issue 4 years ago • 5 comments

I would like to make use of #169 to provide host-specific domains and whitelists. Consider these two approaches:

# Global configuration
domain=a.example.com,b.example.com
# Rule-based configuration
rule.a.rule=Host(`a.example.com`)
rule.a.domains=a.example.com

rule.b.rule=Host(`b.example.com`)
rule.b.domains=b.example.com

In the above example, I would like to use rule-based configuration instead of global configuration.

When I use global configuration everything works.

When I use rule-based configuration the authorization process gets stuck in a loop. Logging shows that redirects are landing back in the Auth handler instead of the AuthCallback handler like they should be. As a result, the Auth process kicks the user back out to the authorization server whenever a redirect comes in. From the user's perspective, they're asked to select their account over and over again.

JoshuaSjoding avatar Feb 13 '21 00:02 JoshuaSjoding

I suspect this is a route order/priority problem in Server.buildRoutes:

func (s *Server) buildRoutes() {
	var err error
	s.router, err = rules.NewRouter()
	if err != nil {
		log.Fatal(err)
	}

	// Let's build a router
	for name, rule := range config.Rules {
		matchRule := rule.formattedRule()
		if rule.Action == "allow" {
			s.router.AddRoute(matchRule, 1, s.AllowHandler(name))
		} else {
			s.router.AddRoute(matchRule, 1, s.AuthHandler(rule.Provider, name))
		}
	}

	// Add callback handler
	s.router.Handle(config.Path, s.AuthCallbackHandler())

	// Add logout handler
	s.router.Handle(config.Path+"/logout", s.LogoutHandler())

	// Add a default handler
	if config.DefaultAction == "allow" {
		s.router.NewRoute().Handler(s.AllowHandler("default"))
	} else {
		s.router.NewRoute().Handler(s.AuthHandler(config.DefaultProvider, "default"))
	}
}

It looks like Traefik's Router is based on Gorilla's Router, which means that:

Routes are tested in the order they were added to the router. If two routes match, the first one wins

With that in mind, consider the route order in buildRoutes():

  1. AuthHandler (rules)
  2. AuthCallbackHandler
  3. LogoutHandler
  4. AuthHandler (global)

If all of these routes have the same priority, AuthCallbackHandler will never be called when matching rules.

JoshuaSjoding avatar Feb 13 '21 00:02 JoshuaSjoding

I wonder if the rule routes should be handled in two passes, one for AllowHandler and one for AuthHandler. This would let the allow actions continue to have precedence, but let the callback and logout handlers fire when they need to.

An order like this is what I have in mind:

  1. AllowHandler (rules)
  2. AuthCallbackHandler
  3. LogoutHandler
  4. AuthHandler (rules)
  5. AuthHandler (global)

JoshuaSjoding avatar Feb 13 '21 01:02 JoshuaSjoding

Hi - thanks for sticking with this one, you're correct in that this is a problem with the current implementation. My normal workaround is to ensure that rules don't match the callback/logout endpoint, but your proposed solution sounds really good!

I'll keep this issue open to track.

thomseddon avatar Apr 19 '21 20:04 thomseddon