Skip to content
This repository was archived by the owner on Jan 24, 2019. It is now read-only.

RD form value on login page #456

Open
krogon-dp opened this issue Sep 27, 2017 · 11 comments
Open

RD form value on login page #456

krogon-dp opened this issue Sep 27, 2017 · 11 comments
Labels
Milestone

Comments

@krogon-dp
Copy link

Correct rd form value when param not specified.
https://example.domain.com/oauth2/sign_in

<form method="GET" action="/oauth2/start">
  <input type="hidden" name="rd" value="/">

  <button type="submit" class="btn">Sign in with a GitHub Account</button><br>
</form>

Incorrect rd form value when param specified, making redirect to login_page again (loop)
https://example.domain.com/oauth2/sign_in?rd=/

<form method="GET" action="/oauth2/start">
  <input type="hidden" name="rd" value="/oauth2/sign_in?rd=/">

  <button type="submit" class="btn">Sign in with a GitHub Account</button><br>
</form>

Version: 2.2.1-alpha

      containers:
      - args:
        - --provider=github
        - --email-domain=*
        - --github-org=exampleOrganisation
        - --github-team=exampleGroup
        - --upstream=file:///dev/null
        - --cookie-domain=example.com
        - --cookie-name=_oauth2_proxy_test
        - --http-address=0.0.0.0:4180
        - --proxy-prefix=/oauth2
        image: docker.io/colemickens/oauth2_proxy:latest
       [...]

@ploxiln
Copy link
Contributor

ploxiln commented Sep 27, 2017

Currently it's not supported to add rd=... param to the sign-in page url. You have two options:

@krogon-dp
Copy link
Author

I see it now why it stopped working for me. nginx-intress-controller for kubernetes fixed the rd param handling in nginx-0.9.0-beta.12 version with this commit: kubernetes/ingress-nginx@b2be9f0#diff-b7803798d356c6c17a90d93cc58bdbaaR588

Correct me if I am wrong, but adding X-Auth-Request-Redirect header won't solve the problem unless rd param is still being set in error_page 401 directive.
The only way is custom nginx template for ingress which reverts changes from above mentioned commit.

@krogon-dp
Copy link
Author

Reading code seems like X-Auth-Request-Redirect would work if set to static destination.

The preferred way would be to remove SingInPath from RequestURI near that code:

oauth2_proxy/oauthproxy.go

Lines 367 to 373 in b1e29c3

redirect_url := req.URL.RequestURI()
if req.Header.Get("X-Auth-Request-Redirect") != "" {
redirect_url = req.Header.Get("X-Auth-Request-Redirect")
}
if redirect_url == p.SignInPath {
redirect_url = "/"
}

@ploxiln
Copy link
Contributor

ploxiln commented Sep 27, 2017

see #427

(also I forgot about a third option: don't use the sign-in page at all, redirect straight to /start)

krogon-dp added a commit to krogon-dp/oauth2_proxy that referenced this issue Sep 27, 2017
@krogon-dp
Copy link
Author

krogon-dp commented Sep 27, 2017

Assuming I still want to use login page I prepared a PR #457 which fixes both mentioned use-cases.
The corresponding test is also in-place.

@JordanP
Copy link

JordanP commented Oct 17, 2017

Yes, I am facing the exact same issue. I tried to upgrade from nginx-0.9.0-beta11 to nginx-0.9.0-beta15 and authentication started to fail (infinite redirect loop). I tracked this to the exact change @krogon-dp mentioned.

PR #457 and #427 seem to be stuck so I am not sure what to do. For now I am sticking to nginx-controller-beta11 which is suboptimal. @aledbf @ploxiln any suggestion ?

Thanks guys

@xeor
Copy link

xeor commented Jan 12, 2018

Is everyone using an old ingress-controller? Or using proxy_set_header X-Auth-Request-Redirect /?

I think that the problem can be solved more elegantly if https://domain.com/oauth2/sign_in?rd=https://domain.com/original-url didnt log you out, but instead checked if you was authenticated, and then did the redirect... https://github.com/bitly/oauth2_proxy/blob/master/oauthproxy.go#L366 should have a check if the user is logged in, and if she is, then redirect.. If not, clear session..

@xeor
Copy link

xeor commented Jan 12, 2018

Another option would be to use proxy_set_header X-Auth-Request-Redirect $arg_rd;. That does set the correct rd in the hidden form, but it is not being redirected to after the login, which

oauth2_proxy/oauthproxy.go

Lines 420 to 423 in b0c1c85

redirect = req.Form.Get("rd")
if redirect == "" || !strings.HasPrefix(redirect, "/") || strings.HasPrefix(redirect, "//") {
redirect = "/"
}
are to blame.. In my setup, my redirect are complete urls, including urls to other sub-domains, which uses this as authentication backend.

Maybe changing this logic to allow same domain/subdomains is better..?

@xeor
Copy link

xeor commented Jan 12, 2018

Sorry for spam :) But this ended up working just like wanted...

In the ingress definition, add:

metadata:
  annotations:
    nginx.ingress.kubernetes.io/configuration-snippet: |
      proxy_set_header X-Auth-Request-Redirect /?redirect=$arg_rd;

Then on the index-page, add something like

    <script>
    // https://stackoverflow.com/a/901144/452081
    function getParameterByName(name, url) {
      if (!url) url = window.location.href;
      name = name.replace(/[\[\]]/g, "\\$&");
      var regex = new RegExp("[?&]" + name + "(=([^&#]*)|&|#|$)"), results = regex.exec(url);
      if (!results) return null;
      if (!results[2]) return '';
      return decodeURIComponent(results[2].replace(/\+/g, " "));
    }
    var redirect_url = getParameterByName("redirect", window.location.href) || "";

    if(redirect_url.match('^https://([a-z0-9-]+\.)?domain\.com/.*') window.location = redirect_url;
    </script>

This redirects me, to the original url, even if it in another sub-domain..

@ploxiln
Copy link
Contributor

ploxiln commented Jan 12, 2018

If it works for you, great, but that's what would be called an "open redirect"

@xeor
Copy link

xeor commented Jan 13, 2018

@ploxiln yes, sorry about that.. It was ment as a POC for those needing such a solution. I just wanted to post it and go to bed :)

I'm updating with a comment for verification of the url..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

5 participants