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

Traefik auth forward #265

Closed
wants to merge 8 commits into from
Closed

Conversation

mvanderlee
Copy link

@mvanderlee mvanderlee commented Sep 24, 2019

Behaves identical to /decisions except that it gets the url and method from the headers.
This allows integration with Traefik's ForwardAuth middleware.

Related issue

#263

Proposed changes

Add integration with Traefik by adding a new /auth_forward endpoint that behaves identical to /decisions except that it gets the url and method from the following headers:

  • X-Forwarded-Uri
  • X-Forwarded-Method

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the
    developer guide (if appropriate)

Further comments

I've copied the decision.go file, renamed where appropriate, and changed the rule matcher input. Found references to DecisionHandler and added the new AuthForwardHandler accordingly.
I've tested locally with this config https://gist.github.com/MVanderlee/2dba10f1ed6c869630eab27847bc2d12

The endpoint only has to support GET but wasn't sure how that's achieved.

Please review carefully as I'm not familiar with GoLang.

@CLAassistant
Copy link

CLAassistant commented Sep 24, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Michiel Vanderlee seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mvanderlee
Copy link
Author

I can't figure out how to get the CLA working. I have however accepted it

@mvanderlee
Copy link
Author

Just realized that the URI is only the path. Would also need to get add these:

	xForwardedProto  = "X-Forwarded-Proto"
	xForwardedHost   = "X-Forwarded-Host"

and somehow combine them into a URL object for the Matcher to understand

@aeneasr
Copy link
Member

aeneasr commented Sep 25, 2019

Thank you for this PR. Obviously we'd like to avoid having one endpoint per proxy. Is there any other solution or workaround for this? If not, I think it would make sense to rewrite the decision endpoint and add e.g. /decision/generic, /decision/traefik - that ways it's really easy to target one individual service.

Regarding the CLA, I'll look into it - sometimes the thing is buggy.

@mvanderlee
Copy link
Author

There is no alternative from Traefik's side. I agree with the decision endpoint rewrite.

@mvanderlee
Copy link
Author

I've updated the paths, and now include the protocol and host in the matcher url.

api/decision.go Outdated
@@ -30,7 +30,7 @@ import (
)

const (
DecisionPath = "/decisions"
DecisionPath = "/decisions/generic"
Copy link
Member

@aeneasr aeneasr Sep 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set up a redirect from /decisions/* (excluding "/decisions/generic(|/*)" and /decisions/traefik(|/*)) to /decisions/generic/$1 for backwards compatibility!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've handled this by simply relying on the middleware order.
This does present an issue for the metrics, and I'm not sure how to handle that.

@aeneasr
Copy link
Member

aeneasr commented Sep 28, 2019

Once the redirect is set up, I believe that all tests will pass again! :)

@aeneasr aeneasr closed this Sep 28, 2019
@aeneasr aeneasr reopened this Sep 28, 2019
@aeneasr
Copy link
Member

aeneasr commented Sep 28, 2019

Sorry, accidental close.

@derekbar90
Copy link

Any update on the status of this ticket?

@aeneasr
Copy link
Member

aeneasr commented Nov 7, 2019

@MichielVanderlee are you still up for this contribution?

@pdaher
Copy link

pdaher commented Dec 11, 2019

@jmt-vanderlee do you still plan to go forward with this PR?

@derekbar90
Copy link

@jmt-vanderlee Are you still thinking of accepting the license agreement? Would love to see this hit production

@rdehouss
Copy link

rdehouss commented Jan 24, 2020

Hi @mvanderlee, I see that your gist is not available anymore.
Do you have another one?
You could also update the README to include theTraefik compatibility + links to doc :)

Many thanks in advance!

@mrkwtz
Copy link

mrkwtz commented Jan 27, 2020

Thank you for this PR. Obviously we'd like to avoid having one endpoint per proxy. Is there any other solution or workaround for this? If not, I think it would make sense to rewrite the decision endpoint and add e.g. /decision/generic, /decision/traefik - that ways it's really easy to target one individual service.

Regarding the CLA, I'll look into it - sometimes the thing is buggy.

We like the idea to make the decisions endpoint more configurable (e.g. define the header from which it should source host, uri, etc).

We ran into the same issues with the nginx ingress controller and auth_request. Because we don't want to expose the decision endpoint via ingress we target the oathkeeper-api directly in the auth-url. However this leads oathkeeper to get his hostname as the host header (therefore it doesn't match the hostname in the rules anymore) and there is no way around this (when you want to keep using the "self-configurable" nginx controller).

@aeneasr
Copy link
Member

aeneasr commented Feb 20, 2020

I might be able to tackle this and provide a generic structure for adding proxy specific decision endpoints

Behaves identical to /decisions except that it gets the url and method from the headers.
This allows integration with Traefik's ForwardAuth middleware.
Ensure we only accept GET for /decisions/traefik
@mvanderlee
Copy link
Author

Proof of signing the CLA:
image

@aeneasr
Copy link
Member

aeneasr commented Apr 7, 2020

Thank you! The CLA is sometimes buggy :)

I take it the code is ready for review? :)

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good :) Now we need some tests for the new functionality and also the legacy decision path! Also, the documentation needs to updated here and here also also anywhere where you find /decision :)

@aeneasr
Copy link
Member

aeneasr commented Jul 29, 2020

Superseded by #486

@aeneasr aeneasr closed this Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants