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

redirection from /auth to /auth/$STATE fails when VP is hosted at a path such as /vouch (redirects to /auth/$STATE instead of /vouch/auth/$STATE) #373

Closed
pommedeterresautee opened this issue Mar 11, 2021 · 36 comments
Labels

Comments

@pommedeterresautee
Copy link

pommedeterresautee commented Mar 11, 2021

Use a Paste Service

Link to logs with vouch.testing set to true

Describe the problem

Vouch has been configured to work with Okta and nginx.
With all version up to 0.19.2 it works well.
When we update to 0.20.0 or newer versions, the process works until the last redirection after being correctly logged.
The reason is that the redirect URL is wrong, it forgets a part.

Instead of redirecting to https://app-dev.our-domain.something/vouch-webapp-dev/auth it sends to https://app-dev.our-domain.something/auth and it finishes with a 404 error.

Last lines of the kindof Vouch report generated in testing mode through the browser (after the Okta login screen):
image

Expected behavior

We expect that version 0.20.0 or newer to work like version 0.19.2 or older, in particular to not get

Additional context

Vouch config updated (with secure: true)

We run Vouch on docker-swarm: image: voucher/vouch-proxy:0.19.2

Tests has been run on Chrome 0.89 and Ubuntu 20.10

@bnfinet
Copy link
Member

bnfinet commented Mar 11, 2021

@pommedeterresautee interesting. I can't reproduce the behavior.

Could you please provide your nginx config in a gist as described in the README. Please move your VP config there too. Line numbers really help for this stuff.

Why do you have the rewrite? What does that help you accomplish?

@bnfinet
Copy link
Member

bnfinet commented Mar 11, 2021

@pommedeterresautee your VP log only shows calls to a healthz endpoint

Where are the errant requests with the URL you've described? Why is there no return call to /auth from Okta?

@pommedeterresautee
Copy link
Author

I will recapture and repost, obviously I failed in my copy pasting tentative.
Regarding the config, VP config, you want me to remove it from the issue, even if secrets have been redacted, right?

@bnfinet
Copy link
Member

bnfinet commented Mar 11, 2021

Your cookie.secure is false but your auth endpoint is https. Usually they should align. Probably should be removed to default secure: true.

@pommedeterresautee
Copy link
Author

pommedeterresautee commented Mar 11, 2021

I have updated the log (same link as before) and moved the VP/nginx config to gist (link in the first message too).
There is no more the many health check calls.
That time, secure is set to true

@bnfinet
Copy link
Member

bnfinet commented Mar 17, 2021

@pommedeterresautee sorry for the delay

I now see the call to /auth but then what happens? Why are there no more logs after that?

Please offer at least two full roundtrips including all errors and failures validate, login, okta, auth, auth/state.

Thanks

@bnfinet
Copy link
Member

bnfinet commented Mar 17, 2021

@gsx95 I'm wondering if this might be related to #349

If /auth is run at https://protectedapp.mydomain.com/vouch-in-a-path/auth

would the 302 redirect at
https://github.com/vouch/vouch-proxy/blob/master/handlers/auth.go#L46-L47

push the request to /auth/$STATE instead of /vouch-in-a-path/auth/$STATE

hmm. That's an nginx/VP alignment issue I think.

@pommedeterresautee
Copy link
Author

pommedeterresautee commented Mar 17, 2021

Hi, thank you for your message.
I have made several run login, validate. When I get to auth, because it misses a part of the path I get a 404 and it's not captured in VP log because it never sees it (the path being wrong...). However in the last log I fixed the address manually and tried /auth and /auth/state, got error too but it's captured in the log.

Logs have been updated in the link present in the first message.

Regarding auth/state I did it literally but i have the feeling it's not what you expect. Let me know if this is the case.

@gsx95
Copy link
Contributor

gsx95 commented Mar 26, 2021

You are right @bnfinet, this is caused by VP's hard redirect to /auth/state, which doesn't account for vouch running on its own context path as in this case.
A possible fix could be to redirect to ./auth/state instead to keep the context path. One would have to test this of course, but I think this could be a valid solution. Let me know what you think.

@pommedeterresautee
Copy link
Author

I would be happy to test that version when ready and report here for results.

@bnfinet
Copy link
Member

bnfinet commented Apr 6, 2021

thanks @gsx95 would you be able to publish a fix and confirm it works for the common case (VP at vouch.mydomain.com/auth) and then we can ask @pommedeterresautee to build and test on his end?

Otherwise it will have to wait for me to get to it :)

@bnfinet bnfinet added the bug label Apr 6, 2021
@bnfinet bnfinet changed the title vouch-proxy with Okta working on v0.19.2 but stops working on v0.20.0 redirection from /auth to /auth/$STATE fails when VP is hosted at a path such as /vouch (redirects to /auth/$STATE instead of /vouch/auth/$STATE) Apr 6, 2021
bnfinet added a commit that referenced this issue May 21, 2021
@bnfinet
Copy link
Member

bnfinet commented May 21, 2021

@pommedeterresautee would you be in a position to pull the branch fix/373_vouch_in_a_path and test in your setup.

It looks to me like this simple fix is working.

@pommedeterresautee
Copy link
Author

Tks for the fix.
I have executed the following commands:

git switch fix/373_vouch_in_a_path
docker build -t ghcr.io/xxx/vouch:latest -t ghcr.io/xxx/vouch:1.0.1 .
docker push ghcr.io/xxx/vouch:latest
docker push ghcr.io/xxx/vouch:1.0.1

Then I deployed that image on our stack, but I am still redirected to https://xxx.xxx.eu/auth/abcdef/?code=abcdef&state=abcdef

@MCOfficer
Copy link

MCOfficer commented Jul 7, 2021

In the meantime, here's a dirty little workaround, assuming you don't need that /auth path for something else.

        location /auth/ {
            auth_request off;
			# Pretty much mirror your proxy settings here.
			# You can also proxy_pass to the vouch port directly.
			# make sure to get your trailing slashes right!
            proxy_pass http://your.vouch.url/auth/; 
            proxy_set_header Host $http_host;
        }

@beingamarnath
Copy link

@bnfinet I'm just new to this project and looking into how to configure it in my k8s cluster. I have all my apps under path context and not as sub domains. Do you think its possible or this will only work if the apps are under subdomain?
P.S: I can enable subdomain for vouch but not for the apps.

@MCOfficer
Copy link

@bnfinet I'm just new to this project and looking into how to configure it in my k8s cluster. I have all my apps under path context and not as sub domains. Do you think its possible or this will only work if the apps are under subdomain?
P.S: I can enable subdomain for vouch but not for the apps.

I have made it work with all apps being under path behind an nginx reverse proxy. As long as your can configure your kubernetes similar to the workaround above, it should be possible.

@beingamarnath
Copy link

Thanks @MCOfficer I would give it a try.

bnfinet added a commit that referenced this issue Jul 30, 2021
When VP is run behind  Nginx "in a path" such as `/vouch/validate`,
`/vouch/login` etc, its necessary to adjust the path that the session
cookie is set into and the 302 redirect from `/vouch/auth` to
`/vouch/auth/$STATE`
@bnfinet
Copy link
Member

bnfinet commented Jul 30, 2021

@beingamarnath @pommedeterresautee @MCOfficer

I've just published a fix to the fix/373_vouch_in_a_path branch. If any one of you is in a position to test it would much appreciate.

By configuring Nginx to send the X-Original-Uri header to VP we can use that as a hint to use the correct path for the session cookie...

        location /vouch/login {
          proxy_set_header X-Original-URI $request_uri;
          proxy_set_header Host $http_host;
          proxy_pass http://vouch.yourdomain.com:9090/login;
        }

        location /vouch/auth {
          proxy_set_header X-Original-URI $request_uri;
          proxy_set_header Host $http_host;
          proxy_pass http://vouch.yourdomain.com:9090/auth;
        }

thanks much!

@beingamarnath
Copy link

beingamarnath commented Aug 11, 2021

@bnfinet Since I'm configuring Vouch for the first time, it took me some time to get it working with pure domain based approach as I wanted a working setup first to compare the behavior with. Anyhow I picked up this path based setup for testing.

App: https://example.com/app
Vouch: https://example.com/vouch
Provider: ADFS

I configured the X-Original-URI to be passed on from the Nginx. Since I was using Nginx ingress controller, I had to add the setting as mentioned here: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#proxy-add-original-uri-header via configmaps

Also the Vouch ingress had to be adapted like

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/rewrite-target: /$2
  name: vouch-proxy
spec:
  rules:
  - host: example.com
    http:
      paths:
      - backend:
          service:
            name: vouch-proxy
            port:
              name: http
        path: /vouch(/|$)(.*)
        pathType: ImplementationSpecific

With the above mentioned changes, vouch was able to figure out its hosted under path(using the X-Original-URI header) and the redirections worked until auth.

But then it failed with the following error /auth Invalid session state: stored %!s(<nil>), returned 5Hx82NXoZo5usK07wqwbFG5Esh5SMGJ" at AuthStateHandler
I guess the sessions state information couldn't be figured out here ? I will try to debug this further but let me know if you have any hints..

@bnfinet
Copy link
Member

bnfinet commented Aug 11, 2021

@beingamarnath thank you for testing.

To be clear... did you build the branch fix/373_vouch_in_a_path and then test the setup with X-Original-URI? Could you offer your Nginx config too?

@beingamarnath
Copy link

beingamarnath commented Aug 11, 2021

I just picked up the image with from repository: quay.io/vouch/vouch-proxy and tag: "fix_373_vouch_in_a_path".

My nginx config is the default that comes out when we install Ingress nginx controller via Helm Chart. (I'm not sure how to pick up this info at the moment as its not a static setup)

@bnfinet
Copy link
Member

bnfinet commented Aug 11, 2021

@beingamarnath thanks, what does the VP log show for the X-Original-URI? Does it look correct?

@bnfinet
Copy link
Member

bnfinet commented Aug 11, 2021

Is this the source that the ConfigMap is interacting with?

https://github.com/kubernetes/ingress-nginx/blob/9a9ad4785704ef7ec0f6428e5cfb2a32d9af5cf3/rootfs/etc/nginx/template/nginx.tmpl#L1003

is if $externalAuth.Method being triggered?

@beingamarnath
Copy link

beingamarnath commented Aug 11, 2021

All the logs for one single request - https://gist.github.com/beingamarnath/127a6277e2085cdde744fa9456c330d5
I anonymized the endpoints and auth codes, otherwise it contains the whole info.

The ingress configuration of the app itself is available here https://gist.github.com/beingamarnath/0e1cd213edb4b6eab946099b7a5900af

The external auth is triggered and it redirects to the adfs server and I'm able to login.

To configure the X-Original-URI, I basically update the https://github.com/kubernetes/ingress-nginx/blob/9a9ad4785704ef7ec0f6428e5cfb2a32d9af5cf3/charts/ingress-nginx/values.yaml#L37 with proxy-add-original-uri-header : true

@bnfinet
Copy link
Member

bnfinet commented Aug 11, 2021

@beingamarnath those logs do show X-Original-URI is set. It appears that the session cookie is not being set and read back properly which is usually an issue with the domain which the cookie is being set into.

Could you please offer your VP config and also show the VP startup in your logs. Pretty much I need the stuff outlined in the README.

@beingamarnath
Copy link

jskrzypek pushed a commit to jskrzypek/vouch-proxy that referenced this issue Aug 12, 2021
jskrzypek pushed a commit to jskrzypek/vouch-proxy that referenced this issue Aug 12, 2021
When VP is run behind  Nginx "in a path" such as `/vouch/validate`,
`/vouch/login` etc, its necessary to adjust the path that the session
cookie is set into and the 302 redirect from `/vouch/auth` to
`/vouch/auth/$STATE`
jskrzypek pushed a commit to jskrzypek/vouch-proxy that referenced this issue Aug 12, 2021
@jskrzypek
Copy link

jskrzypek commented Aug 13, 2021

I tried using the above-mentioned setup with the image tag for that branch, (only with /sso/ as the prefix) but only got a series of redirected back to /sso/auth/$STATE/?code=$CODE&state=$STATE, which gave me a 400 error. I also tried this with versions of this fix rebased against both master and v0.32.0, but neither of them worked, and they both redirected to /auth/$STATE/?code=$CODE&state=$STATE finally.

@jskrzypek
Copy link

On the other hand I've found a partial workaround with the alpine-0.32 image. I set up my ingress like this (the PLACEHOLDER gets patched by kustomize later):

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/rewrite-target: /$2
  name: vouch-proxy
spec:
  rules:
    - host: PLACEHOLDER
      http:
        paths:
          - backend:
              service:
                name: vouch-proxy
                port:
                  name: http
            path: /sso(/|$)(.*)
            pathType: Prefix
          - backend:
              service:
                name: vouch-proxy
                port:
                  name: http
            path: /()(auth.*)
            pathType: Prefix
  tls:
    - hosts:
        - PLACEHOLDER
      secretName: PLACEHOLDER

When I do this I get redirected back to /sso/auth/$STATE/?code=$CODE&state=$STATE and then to /auth/$STATE/?code=$CODE&state=$STATE which shows me a 400, but then if I manually change the url to /auth/$STATE?code=$CODE&state=$STATE (i.e. I remove the last / just before the query params) it works for some reason.

I am able to get this to kind of work automatically by adding the following configuration snippet to my ingress annotations above, BUT it still lands you on the 400 error page, and then you need to reload/re-hit that url before it will save the correct session in vouch and let me access the secured page:

    nginx.ingress.kubernetes.io/configuration-snippet: |
      rewrite (/auth/[^/]+)/(\?.*$) $1$2;

@bnfinet
Copy link
Member

bnfinet commented Aug 13, 2021

@jskrzypek thanks for testing and hacking on this a bit

@jskrzypek @beingamarnath and all, could you please try with the latest updates to fix/373_vouch_in_a_path.

FYI - the docker images are not currently getting built for branches (see #406)

@jskrzypek
Copy link

@bnfinet I'll try to test that next week as I'm already nearing EoD. Fwiw my coworkers on Chrome are not having that issue with needing to reload to get the configuration snippet to work

@bnfinet
Copy link
Member

bnfinet commented Aug 25, 2021

@jskrzypek friendly nudge. If you had any time to test I'd love to confirm its working and then get it merged.

@jskrzypek
Copy link

jskrzypek commented Aug 25, 2021 via email

@balee
Copy link

balee commented Aug 26, 2021

The current code in fix/373_vouch_in_a_path seem to work.

(I suffered a few hours missing the info that the image quay.io/vouch/vouch-proxy:fix_373_vouch_in_a_path is not up to date and took time to figure out that the path of VouchSession cookie set by /login is wrong and how I can tweak it with nginx... xD).

However, wouldn't be the correct solution to introduce a config property for the context path in VP and deal with it internally, instead of requiring it to be tweaked in nginx? I have seen such for example in Kibana and Prometheus. I think it would be reasonable as this setup seems to be pretty common, and the nginx tweak is not trivial.

@bnfinet
Copy link
Member

bnfinet commented Aug 28, 2021

@balee thanks for testing

I've added a config option for vouch.document_root: /vouch_in_a_path to VP in the following branch:

https://github.com/vouch/vouch-proxy/tree/fix/373_vouch_in_a_path_document_root

If you (or @jskrzypek or anyone else) was able to test and to check the README for clarity it would be much appreciated.

Really I'm on the fence about this. I'm always hesitant to add new knobs to the VP config.

proxy_set_header X-Original-URI $request_uri; feels more correct from an HTTP standpoint. In general I'd prefer to glean the intent from the original request if possible.

However I do see that it seems clunky and requires all of the paths to be set explicitly (location (/vouch_in_a_path/auth | /vouch_in_a_path/login | ...) and gets into other issues for static assets and whatnot and having only one cleaner location stanza in the Nginx config is a bit future proof. So I went and did it in the spirit of ergonomics.

Sorry for the confusion on the docker image. I have removed the image quay.io/vouch/vouch-proxy:fix_373_vouch_in_a_path

@balee
Copy link

balee commented Aug 28, 2021

@bnfinet I can confirm that vouch.document_root config is working fine.

I use it in k8s with nginx ingress, so it is a bit different, but I`d raise up a few things:

  1. In config.yml_example, maybe there is a missing reference for something?
# document_root - VOUCH_DOCUMENT_ROOT
# see README for "
  1. There is a difference in location @error401 { part compared to the "original" config, see the missig &X-Vouch-Token=$auth_resp_jwt&error=$auth_resp_err. I don't know whether it is intentional and what it actually affects. Anyway, it works both ways.

  2. I think we still need this part (or at least the last line depending on 2.), which is now missing:

# these return values are used by the @error401 call
auth_request_set $auth_resp_jwt $upstream_http_x_vouch_jwt;
auth_request_set $auth_resp_err $upstream_http_x_vouch_err;
auth_request_set $auth_resp_failcount $upstream_http_x_vouch_failcount;

bnfinet added a commit that referenced this issue Aug 30, 2021
@bnfinet
Copy link
Member

bnfinet commented Aug 30, 2021

released in v0.33.0

Thanks again to @balee @jskrzypek @beingamarnath @MCOfficer and @pommedeterresautee for the testing and debugging support. Very pleased to get this fix into VP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants