feat: auth-req caching #4278

moolen · 2019-07-07T16:58:13Z

What this PR does / why we need it:
This PR adds a way to configure the proxy_cache_* (docs) directive for external authentication.

It adds these annotations, as proposed by @Stono in #2862 :

nginx.ingress.kubernetes.io/auth-cache-key
nginx.ingress.kubernetes.io/auth-cache-duration

The first one sets the proxy_cache_key (e.g. $uri$cookie__auth_proxy) and enables the feature. The latter sets the cache duration (defaults to 10m)

Note:
The user-defined cache_key may contain sensitive information (e.g. Authorization header). That's why we want to store only a hash of that key, not the key itself on disk.

fixes #2862

k8s-ci-robot · 2019-07-07T16:58:14Z

Welcome @moolen!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-07-07T16:58:20Z

Hi @moolen. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2019-07-07T18:24:36Z

@moolen please add e2e tests

aledbf · 2019-07-07T18:24:45Z

/ok-to-test

aledbf · 2019-07-07T18:24:51Z

/hold

codecov-io · 2019-07-07T18:29:19Z

Codecov Report

Merging #4278 into master will increase coverage by 0.16%.
The diff coverage is 82.75%.

@@            Coverage Diff             @@
##           master    #4278      +/-   ##
==========================================
+ Coverage    58.3%   58.47%   +0.16%     
==========================================
  Files          87       87              
  Lines        6478     6528      +50     
==========================================
+ Hits         3777     3817      +40     
- Misses       2274     2282       +8     
- Partials      427      429       +2

Impacted Files	Coverage Δ
internal/ingress/controller/config/config.go	`98.57% <100%> (ø)`	⬆️
internal/ingress/controller/template/configmap.go	`76.63% <66.66%> (-0.52%)`	⬇️
internal/ingress/annotations/authreq/main.go	`70.83% <85.41%> (+6.27%)`	⬆️

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 84af99b...23504db. Read the comment docs.

moolen · 2019-07-07T22:41:49Z

/retest

aledbf · 2019-07-08T18:36:48Z

@moolen two comments:

I think auth-cache-key should prepend the nginx server_name variable
auth-cache-duration could be a list to allow different durations (not sure 10 minutes is ok for 401)

moolen · 2019-07-09T08:18:55Z

@aledbf Thanks for your feedback!

I think auth-cache-key should prepend the nginx server_name variable

good catch, that makes sense!

auth-cache-duration could be a list to allow different durations

You're right, it should be a list to allow configuration of multiple durations.

But while i think about that i'm not really sure if we really need this annotation at all. The user could set proxy_cache_valid using the auth-snippet annotation anyway. It would be semantically more precise to use a specific annotation, but adds more complexity to the codebase.

I'll implement it as a list for now. If you like using auth-snippet better i can remove it again (not a big deal).

not sure 10 minutes is ok for 401

It depends on the user's context i would say. For my use-case ($corp internal web-apps) i use Authorization header as cache_key and i want every auth-response (!= 5xx) to be cached.

I'd propose to lower the default value to 5m. The user can override the value using the duration annotation. What do you think?

moolen · 2019-07-09T11:56:15Z

i did touch this; The last runs indicate this test is flaky; let's retest and maybe dig deeper.
/test pull-ingress-nginx-test

aledbf · 2019-07-09T12:12:48Z

I'll implement it as a list for now.

👍

I'd propose to lower the default value to 5m

👍

Also, please squash the commits

aledbf · 2019-07-10T17:29:30Z

/approve

ElvinEfendi · 2019-07-11T20:25:48Z

I think auth-cache-key should prepend the nginx server_name variable

should it include location identifier too? since one can configure different external authentication for different locations in the same server.

moolen · 2019-07-11T20:54:23Z

again, that flaky test :/
/test pull-ingress-nginx-test

aledbf · 2019-07-11T20:59:45Z

again, that flaky test :/

Apologies for that :(
We are removing the static SSL feature in this release. With that, this will not be an issue anymore.

moolen · 2019-07-15T12:19:55Z

@ElvinEfendi i added the location identifier and squashed the commits.

docs/user-guide/nginx-configuration/configmap.md

ElvinEfendi · 2019-07-15T18:30:03Z

internal/ingress/controller/template/configmap.go

+					}
+				}
+			}
+		}


Is there any way to not duplicate this logic? @aledbf do we have means to share code between here and annotation? like a utils package or something

I centralized the "parsing" logic for now in d6b9014. Let me know if you want it in a utils package. I digged into this topic - it seems there's a bunch of code that is copy/pasted with slight adaptions (e.g. returning vs. logging an error).

rootfs/etc/nginx/template/nginx.tmpl

ElvinEfendi · 2019-07-15T18:44:40Z

Can you add a test to assert that the cache key includes location and server identifiers? For location the test would like like this

Sign in on i.e /foo
Kill the external auth server
Assert that /foo still works
Assert that /bar does not work

moolen · 2019-07-16T21:16:10Z

Thanks for your feedback!

I added tests to verify the scope of the cache key (location/server) and addressed the other comments too.
I'm not sure about introducing a utils package. I centralized the logic for now. please let me know if that works for you!

moolen · 2019-07-16T22:15:48Z

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

ElvinEfendi · 2019-07-17T15:17:50Z

docs/user-guide/nginx-configuration/annotations.md

@@ -378,6 +380,10 @@ Additionally it is possible to set:
  `<Response_Header_1, ..., Response_Header_n>` to specify headers to pass to backend once authentication request completes.
 * `nginx.ingress.kubernetes.io/auth-request-redirect`:
  `<Request_Redirect_URL>`  to specify the X-Auth-Request-Redirect header value.
+* `nginx.ingress.kubernetes.io/auth-cache-key`:
+  `<Cache_Key>` this enables caching for auth requests. specify a lookup key for auth responses. e.g. `$remote_user$http_authorization`.


Should we make a note that this will be hashed and encoded?

I think the implementation details are not important - the mechanics are tho!

I appended this:

Each server and location has it's own keyspace. Hence a cached response is only valid on a per-server and per-location basis

ElvinEfendi · 2019-07-17T15:18:53Z

@moolen this is looking great! Once you squash the commits we can merge this.

add a way to configure the `proxy_cache_*` [1] directive for external-auth. The user-defined cache_key may contain sensitive information (e.g. Authorization header). We want to store *only* a hash of that key, not the key itself on disk. [1] http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_key Signed-off-by: Moritz Johner <[email protected]>

ElvinEfendi · 2019-07-17T17:46:51Z

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

ElvinEfendi · 2019-07-17T19:05:13Z

/lgtm

thanks @moolen !

k8s-ci-robot · 2019-07-17T19:05:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, moolen

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,aledbf]

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

Stono · 2019-07-18T05:49:17Z

Hey a quick one - thanks for all your effort in this everyone, finally able to rid it of my monkeypatch

vaskozl · 2022-01-27T18:38:30Z

Isn't this dangerous in the case where the user specified cache key ends up being an empty variable?

E.g. if the cache key is set to $cookie_foo and the upstream auth provider decides to change the auth cookie name form foo to bar, then $cookie_foo would become empty and we start caching simply on { $server.Hostname }}{{ $authPath }} which as I see it will let everyone through for the duration of the cache?

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 7, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2019

k8s-ci-robot requested review from aledbf and ElvinEfendi July 7, 2019 16:58

moolen force-pushed the feat/auth-req-cache branch from 4a495bb to 255ece5 Compare July 7, 2019 17:00

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 7, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2019

aledbf assigned aledbf and ElvinEfendi Jul 8, 2019

moolen force-pushed the feat/auth-req-cache branch from b23ec18 to d4c3553 Compare July 9, 2019 11:29

moolen force-pushed the feat/auth-req-cache branch from d4c3553 to 1262b85 Compare July 9, 2019 12:16

aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2019

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

moolen force-pushed the feat/auth-req-cache branch from 1262b85 to 67647c2 Compare July 11, 2019 20:45

ElvinEfendi reviewed Jul 15, 2019

View reviewed changes

docs/user-guide/nginx-configuration/configmap.md Outdated Show resolved Hide resolved

ElvinEfendi reviewed Jul 15, 2019

View reviewed changes

rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved

ElvinEfendi reviewed Jul 15, 2019

View reviewed changes

rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved

ElvinEfendi reviewed Jul 15, 2019

View reviewed changes

rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 16, 2019

moolen force-pushed the feat/auth-req-cache branch from 9071d9e to 836a5fd Compare July 16, 2019 21:37

ElvinEfendi reviewed Jul 17, 2019

View reviewed changes

moolen force-pushed the feat/auth-req-cache branch from 836a5fd to 23504db Compare July 17, 2019 16:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2019

k8s-ci-robot merged commit 589c9a2 into kubernetes:master Jul 17, 2019

moolen deleted the feat/auth-req-cache branch July 18, 2019 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auth-req caching #4278

feat: auth-req caching #4278

moolen commented Jul 7, 2019

k8s-ci-robot commented Jul 7, 2019

k8s-ci-robot commented Jul 7, 2019

aledbf commented Jul 7, 2019

aledbf commented Jul 7, 2019

aledbf commented Jul 7, 2019

codecov-io commented Jul 7, 2019 •

edited

Loading

moolen commented Jul 7, 2019

aledbf commented Jul 8, 2019

moolen commented Jul 9, 2019 •

edited

Loading

moolen commented Jul 9, 2019

aledbf commented Jul 9, 2019

aledbf commented Jul 10, 2019

ElvinEfendi commented Jul 11, 2019

moolen commented Jul 11, 2019

aledbf commented Jul 11, 2019

moolen commented Jul 15, 2019

ElvinEfendi Jul 15, 2019

moolen Jul 16, 2019

ElvinEfendi commented Jul 15, 2019

moolen commented Jul 16, 2019 •

edited

Loading

moolen commented Jul 16, 2019

ElvinEfendi Jul 17, 2019

moolen Jul 17, 2019

ElvinEfendi commented Jul 17, 2019

ElvinEfendi commented Jul 17, 2019

ElvinEfendi commented Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

Stono commented Jul 18, 2019

vaskozl commented Jan 27, 2022

feat: auth-req caching #4278

feat: auth-req caching #4278

Conversation

moolen commented Jul 7, 2019

k8s-ci-robot commented Jul 7, 2019

k8s-ci-robot commented Jul 7, 2019

aledbf commented Jul 7, 2019

aledbf commented Jul 7, 2019

aledbf commented Jul 7, 2019

codecov-io commented Jul 7, 2019 • edited Loading

Codecov Report

moolen commented Jul 7, 2019

aledbf commented Jul 8, 2019

moolen commented Jul 9, 2019 • edited Loading

moolen commented Jul 9, 2019

aledbf commented Jul 9, 2019

aledbf commented Jul 10, 2019

ElvinEfendi commented Jul 11, 2019

moolen commented Jul 11, 2019

aledbf commented Jul 11, 2019

moolen commented Jul 15, 2019

ElvinEfendi Jul 15, 2019

Choose a reason for hiding this comment

moolen Jul 16, 2019

Choose a reason for hiding this comment

ElvinEfendi commented Jul 15, 2019

moolen commented Jul 16, 2019 • edited Loading

moolen commented Jul 16, 2019

ElvinEfendi Jul 17, 2019

Choose a reason for hiding this comment

moolen Jul 17, 2019

Choose a reason for hiding this comment

ElvinEfendi commented Jul 17, 2019

ElvinEfendi commented Jul 17, 2019

ElvinEfendi commented Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

Stono commented Jul 18, 2019

vaskozl commented Jan 27, 2022

codecov-io commented Jul 7, 2019 •

edited

Loading

moolen commented Jul 9, 2019 •

edited

Loading

moolen commented Jul 16, 2019 •

edited

Loading