make sure set-cookie is retained from external auth endpoint #4872

ElvinEfendi · 2019-12-30T04:39:57Z

What this PR does / why we need it:

After the workaround in #4859, we no longer retain Set-Cookie header set by external authentication server. This PR fixes that.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

k8s-ci-robot · 2019-12-30T04:40:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov-io · 2019-12-30T04:47:11Z

Codecov Report

Merging #4872 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4872      +/-   ##
==========================================
+ Coverage   58.37%   58.39%   +0.01%     
==========================================
  Files          88       88              
  Lines        6720     6720              
==========================================
+ Hits         3923     3924       +1     
+ Misses       2369     2367       -2     
- Partials      428      429       +1

Impacted Files	Coverage Δ
internal/watch/file_watcher.go	`84.61% <0%> (+3.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aba58d6...9915b52. Read the comment docs.

aledbf · 2019-12-30T09:42:48Z

/retest

ElvinEfendi · 2019-12-30T18:15:54Z

/test pull-ingress-nginx-e2e-1-15

ElvinEfendi · 2019-12-30T19:05:13Z

The regression test is currently wrong. We should use authentication server that returns 401 and also sets cookie. Currently the authentication server only returns cookie and 302 status code.

I'll do something like https://github.com/kubernetes/ingress-nginx/pull/4826/files#diff-d1dbbabd02e531d86463819dfef2f7b7R131 and use echoserver for this instead of HTTPBIN.

aledbf · 2019-12-30T23:32:48Z

@ElvinEfendi I think such a test is not going to test this is working. What about using something like https://github.com/RichardKnop/go-oauth2-server to deploy a real app and run a real e2e test?

travisgroth · 2020-02-10T11:24:44Z

Hi - is this being worked on? If I understand the related PRs fully, external auth has been broken for over a month now. Even if the regression testing isn't perfect, it is better to have this merged than not.

aledbf · 2020-02-10T11:29:50Z

external auth has been broken for over a month now.

What do you mean? I have dozens of services exposed with oauth2 running without any issue using the latest version.

travisgroth · 2020-02-10T14:19:59Z

I'm only familiar with the flow that pomerium uses, but it depends on setting a cookie during a 302 to the auth-signin url.

Specifically:

[no session] -> nginx -> auth check [fails]
302 sign-in page -> pomerium
302 original url + pomerium parameters -> nginx -> auth check [returns cookie]
302 sign-in page -> pomerium -> cookie/session already set
302 original url with cookie from step 3 -> nginx -> auth check [succeeds, normal request proceeds]
further requests pass cookie without redirect flow

This cookie identifies futures requests via nginx when it makes the subrequest to auth-url. The auth-url endpoint verifies the request's cookies as the auth check.

Based on testing, I believe #4859 broke the setting of this cookie and this PR restores it. I assumed that other external auth implementations used this same strategy to set up sessions, but I might be wrong. Currently, Pomerium uses this for both nginx and traefik integration.

Curiosity - Are you using oauth2_proxy or something else?

aledbf · 2020-02-10T14:21:08Z

Curiosity - Are you using oauth2_proxy or something else?

the majority uses oauth2_proxy

aledbf · 2020-02-10T14:26:18Z

I'm only familiar with the flow that pomerium uses, but it depends on setting a cookie during a 302 to the auth-signin url.

If you have problems with this I suggest you open a separate issue. It would be awesome if you could provide the steps to reproduce it, something like https://gist.github.com/aledbf/48237bd5d2dd23df4b9a2cf83d28d228

travisgroth · 2020-02-11T13:37:14Z

If you have problems with this I suggest you open a separate issue.

I will file a formal issue, but the lack of Set-Cookie did break the auth flow. I've tried manually patching the config with the changes in this PR, and it fixes the behavior. I'm just hoping to see this merged.

It would be awesome if you could provide the steps to reproduce it

I'll have full reproduction in the issue I open.

Directly related to this PR - I have a simple nginx config that can simulate the responses at play. This may be useful for the regression test:

    server {
        server_name _;
        listen 80;
        location /check {
            if ($cookie_authed = "yes") {
                return 200;
            }

            if ($http_x_original_url ~ "authed=yes") {
                add_header Set-Cookie "authed=yes" always;
                return 401;    
            }
            
            return 401;
        }

        location /sign-in {
            return 302 $arg_uri?authed=yes;
        }
    }

Corresponding annotations. Replace EXTAUTHENDPOINT:

nginx.ingress.kubernetes.io/auth-signin: http://EXTAUTHENDPOINT/sign-in?uri=$scheme://$host$request_uri
nginx.ingress.kubernetes.io/auth-url: http://EXTAUTHENDPOINT/check?uri=$scheme://$host$request_uri

travisgroth · 2020-02-11T14:13:00Z

@aledbf #5054

aledbf · 2020-02-11T15:22:24Z

/retest

k8s-ci-robot · 2020-02-11T15:47:19Z

@ElvinEfendi: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-ingress-nginx-e2e-1-15	`9915b52`	link	`/test pull-ingress-nginx-e2e-1-15`
pull-ingress-nginx-e2e-1-16	`9915b52`	link	`/test pull-ingress-nginx-e2e-1-16`
pull-ingress-nginx-e2e-1-17	`9915b52`	link	`/test pull-ingress-nginx-e2e-1-17`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

aledbf · 2020-02-12T00:40:36Z

test/e2e/annotations/auth.go

+		for _, err := range errs {
+			Expect(err).NotTo(HaveOccurred())
+		}
+		Expect(resp.StatusCode).Should(Equal(http.StatusFound))


Remove this line

aledbf · 2020-02-12T00:40:42Z

test/e2e/annotations/auth.go

+			Expect(err).NotTo(HaveOccurred())
+		}
+		Expect(resp.StatusCode).Should(Equal(http.StatusFound))
+		Expect(resp.Header.Get("Location")).Should(Equal(fmt.Sprintf("http://%s/auth/start?rd=http://%s%s", host, host, url.QueryEscape("/?a=b&c=d"))))


Remove this line

aledbf · 2020-02-12T00:40:57Z

test/e2e/annotations/auth.go

+		httpbinIP = e.Subsets[0].Addresses[0].IP
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/auth-url":    fmt.Sprintf("http://%s/cookies/set?alma=armud", httpbinIP),


change to /cookies/set/alma/armud

cmluciano · 2020-02-18T20:36:28Z

@aledbf Is this now stale since #5067 is merged?

k8s-ci-robot · 2020-02-18T20:36:36Z

@ElvinEfendi: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aledbf · 2020-02-18T20:56:03Z

@cmluciano yes. Closed

ElvinEfendi · 2020-02-19T20:58:50Z

@aledbf thanks for addressing this in another PR - I did not have time to complete this.

This changes introduces another edge case where when there's an incorrect annotation that renders a Location as invalid then controller will generate invalid Nginx configuration and will not be able to reload it. Here's the bug report with more details: #5130

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 30, 2019

k8s-ci-robot requested review from aledbf and cmluciano December 30, 2019 04:40

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 30, 2019

make sure set-cookie is retained from external auth endpoint

9915b52

This was referenced Jan 9, 2020

Forward auth redirect loop using pomerium 0.5.2 and nginx ingress 0.26.2 pomerium/pomerium#442

Closed

[stable/nginx-ingress] Controller version 0.26.2 breaks external auth helm/charts#20001

Closed

travisgroth mentioned this pull request Feb 11, 2020

set-cookie not retained from external auth endpoint #5054

Closed

aledbf reviewed Feb 12, 2020

View reviewed changes

aledbf mentioned this pull request Feb 13, 2020

Make sure set-cookie is retained from external auth endpoint #5067

Merged

8 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2020

aledbf closed this Feb 18, 2020

ElvinEfendi deleted the retain-external-auth-cookie branch February 19, 2020 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make sure set-cookie is retained from external auth endpoint #4872

make sure set-cookie is retained from external auth endpoint #4872

ElvinEfendi commented Dec 30, 2019

k8s-ci-robot commented Dec 30, 2019

codecov-io commented Dec 30, 2019 •

edited

Loading

aledbf commented Dec 30, 2019

ElvinEfendi commented Dec 30, 2019

ElvinEfendi commented Dec 30, 2019 •

edited

Loading

aledbf commented Dec 30, 2019

travisgroth commented Feb 10, 2020

aledbf commented Feb 10, 2020

travisgroth commented Feb 10, 2020

aledbf commented Feb 10, 2020 •

edited

Loading

aledbf commented Feb 10, 2020

travisgroth commented Feb 11, 2020

travisgroth commented Feb 11, 2020

aledbf commented Feb 11, 2020

k8s-ci-robot commented Feb 11, 2020

aledbf Feb 12, 2020

aledbf Feb 12, 2020

aledbf Feb 12, 2020

cmluciano commented Feb 18, 2020

k8s-ci-robot commented Feb 18, 2020

aledbf commented Feb 18, 2020

ElvinEfendi commented Feb 19, 2020 •

edited

Loading

make sure set-cookie is retained from external auth endpoint #4872

make sure set-cookie is retained from external auth endpoint #4872

Conversation

ElvinEfendi commented Dec 30, 2019

k8s-ci-robot commented Dec 30, 2019

codecov-io commented Dec 30, 2019 • edited Loading

Codecov Report

aledbf commented Dec 30, 2019

ElvinEfendi commented Dec 30, 2019

ElvinEfendi commented Dec 30, 2019 • edited Loading

aledbf commented Dec 30, 2019

travisgroth commented Feb 10, 2020

aledbf commented Feb 10, 2020

travisgroth commented Feb 10, 2020

aledbf commented Feb 10, 2020 • edited Loading

aledbf commented Feb 10, 2020

travisgroth commented Feb 11, 2020

travisgroth commented Feb 11, 2020

aledbf commented Feb 11, 2020

k8s-ci-robot commented Feb 11, 2020

aledbf Feb 12, 2020

Choose a reason for hiding this comment

aledbf Feb 12, 2020

Choose a reason for hiding this comment

aledbf Feb 12, 2020

Choose a reason for hiding this comment

cmluciano commented Feb 18, 2020

k8s-ci-robot commented Feb 18, 2020

aledbf commented Feb 18, 2020

ElvinEfendi commented Feb 19, 2020 • edited Loading

codecov-io commented Dec 30, 2019 •

edited

Loading

ElvinEfendi commented Dec 30, 2019 •

edited

Loading

aledbf commented Feb 10, 2020 •

edited

Loading

ElvinEfendi commented Feb 19, 2020 •

edited

Loading