Improve the external authorization concept from opt-in to secure-by-default #3506

haassConnyun · 2018-12-03T16:06:19Z

What this PR does / why we need it:
Assuming the cluster uses an external authorization service,
currently activation of external authorization is per ingress rule, that means the annotation auth-url needs to be added to every service that is publicly available. We call this security by opt-in.
To improve this concept to security-by-default be propose to add a parameter to the configmap which takes care of authorizing every request automatically.

The existing no-auth-locations is used to manually whitelist services, the new code creates the needed internal location and adds a sub-auth request to every regular location, just as happens with the auth-url annotation

k8s-ci-robot · 2018-12-03T16:06:23Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

haassConnyun · 2018-12-04T14:40:44Z

CLA signed

okryvoshapka-connyun · 2018-12-04T14:53:26Z

CLA signed

haassConnyun · 2018-12-04T14:56:04Z

/assign @antoineco

haassConnyun · 2018-12-04T15:30:16Z

/shrug

okryvoshapka-connyun · 2018-12-04T15:40:07Z

PR raised from issue #3276

haassConnyun · 2018-12-05T09:04:45Z

/unassign @antoineco
/unshrug
was following the bot's advise, apparently not always wanted. sorry.

k8s-ci-robot · 2018-12-05T09:04:45Z

@haassConnyun: ¯\_(ツ)_/¯

In response to this:

/unassign @antoineco
/unshrug
was following the bot's advise, apparently not always wanted. sorry.

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.

haassConnyun · 2019-01-23T08:40:57Z

@aledbf sorry to bump this - is there something we should do to get a review or a comment?

ElvinEfendi · 2019-01-24T15:39:28Z

Sorry for delay, I'll try to review this by the end of this week.

ElvinEfendi · 2019-01-28T17:50:27Z

Locations that should not get authenticated can be listed using no-auth-locations. See no-auth-locations

@haassConnyun no-auth-locations is a global setting, it won't let users to whitelist a path for a given service. Do you have any plan to implement something so that users can opt out of this global auth setting per ingress? Maybe something like auth-url-off annotation where when it's set to true it will not enable external auth.

ElvinEfendi · 2019-01-28T17:53:37Z

internal/ingress/controller/template/configmap.go

+			glog.Warningf("Global auth location denied, reason: url scheme is empty")
+		} else if authURL.Host == "" {
+			glog.Warningf("Global auth location denied, reason: url host is empty")
+		} else if strings.Contains(authURL.Host, "..") {


Is this some kind of edge case where url.Parse does not err?

This was taken from auth-url annotation parser.
https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/annotations/authreq/main.go#L137
The idea behind was to follow the syntax and style of the existing solution

Yes, it is the case url.Parse does not catch.

ElvinEfendi · 2019-01-28T17:54:11Z

internal/ingress/controller/template/configmap.go

+
+		authURL, err := url.Parse(val)
+		if err != nil {
+			glog.Warningf("Global auth location denied, reason: url is not")


The log message seems incomplete. Also can you include the err in the message as well?

I assume it should be ".. is not valid"))
Yes, will append the err to the end.

ElvinEfendi · 2019-01-28T17:55:59Z

internal/ingress/controller/template/configmap.go

@@ -150,6 +152,25 @@ func ReadConfig(src map[string]string) config.Configuration {
 		}
 	}

+	// Verify that the configured global auth is parsable as URL. if not, set the default value


Also can you extract this verification logic into a helper/util function and reuse it both here and in auth-url annotation parsing?

ElvinEfendi · 2019-01-28T17:59:07Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -1163,6 +1164,18 @@ stream {
            {{- end }}
            {{ end }}

+            {{ if not (isLocationInLocationList $location $all.Cfg.NoAuthLocations) }}


What if both auth-url annotation and global setting is set? If I read this correctly it'll have duplicate auth request setup. We should make sure annotation overrides global setting.

Okay, can do it. Was not sure about the priorities. Now it is clear.

ElvinEfendi · 2019-01-28T18:04:57Z

@haassConnyun thanks for the PR! I've made few inline comments. On top of them I think Nginx template should not have separate rendering for global and annotation settings. When the setting is set you can create the annotation manually in the controller and make sure every (except the whitelisted ones) ingress has it. That way you'd not need to touch nginx.tmpl.

okryvoshapka-connyun · 2019-02-25T13:20:17Z

Hi @ElvinEfendi,

As requested changes were made:

It is possible now to disable the globally configured auth with Ingress annotation enable-global-auth: false.
The proper priority is kept: auth-url annotation has higher priority then global-auth-url configration if both are set.
URL verification logic was extracted into separate function and used in annotation auth-url and controller's config global-auth-url.
Modification to the tmpl.conf was minimized, the existing solution for external auth was reused.

To assist you, I have drawn 2 tables:

When enable-global-auth is enabled (default)

A	auth-url	global-auth-url	enable-global-auth	expected behavior
1	/a	/b	true	Request goes to /a
2	/a	none	true	Request goes to /a
3	none	/b	true	Request goes to /b
4	none	none	true	No external auth at all

When enable-global-auth is disabled

B	auth-url	global-auth-url	enable-global-auth	expected behavior
1	/a	/b	false	Request goes to /a
2	/a	none	false	Request goes to /a
3	none	/b	false	No external auth at all
4	none	none	false	No external auth at all

okryvoshapka-connyun · 2019-02-28T13:33:34Z

Hi @ElvinEfendi

Rebased, tested, green.
Sorry for delay, faced with some e2e-test environment and go-formatting issues after rebase.

okryvoshapka-connyun · 2019-03-19T09:39:37Z

Hi @ElvinEfendi ,

I hope the change is not too dramatic.
So the solution was extended to be complete and fully mirror the existing external auth solution.
Nothing was changed from previously implemented logic, only minor renaming.
It contains all the addons implemented in external auth via Ingress Rule, naming concept was kept:

Annotation	ConfigMap setting
auth-method	global-auth-method
auth-signin	global-auth-signin
auth-response-headers	global-auth-response-headers
auth-request-redirect	global-auth-request-redirect
auth-snippet	global-auth-snippet

The switch between 2 solutions is full, so or you are using location specific external auth, or go with global external auth (including all addons in both cases). For example, it is not possible to have global-auth-url: /foo and global-auth-method: POST but for one location change only the nginx.ingress.kubernetes.io/auth-method: GET.

Modification of tmpl.conf template was reduced to minimum, existing external auth solution was reused to maximum.

Can I get another feedback round?

P.S. I am working on rebase.

ElvinEfendi · 2019-03-27T23:49:53Z

@aledbf any reason why do you not wanna merge this? (I saw you added do-not-merge above)

okryvoshapka-connyun · 2019-05-02T06:47:20Z

Hi @ElvinEfendi and @aledbf
Can we assist you somehow to speed up the te review of this PR?

aledbf · 2019-05-02T12:47:35Z

@okryvoshapka-connyun apologies for the delay. Please squash the commits.
After we merge the webhook PR I'll review this one

… incl. addons

okryvoshapka-connyun · 2019-05-03T11:06:03Z

@aledbf , rebased and squashed.

ElvinEfendi · 2019-05-03T15:16:11Z

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

@@ -389,6 +390,14 @@ nginx.ingress.kubernetes.io/auth-snippet: |
 !!! example
    Please check the [external-auth](../../examples/auth/external-auth/README.md) example.

+#### Global External Authentication
+
+By default the controller redirects all requests to an existing service that provides authentication if `global-auth-url` is set in the NGINX ConfigMap. If you want to disable this behavior for that ingress, you can use ssl-redirect: "false" in the NGINX ConfigMap.


What does ssl-redirect has to do with global-auth-url?

ElvinEfendi · 2019-05-03T15:21:31Z

internal/ingress/controller/config/config.go


 	defIPCIDR = append(defIPCIDR, "0.0.0.0/0")
 	defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1")
 	defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1")
 	defProxyDeadlineDuration := time.Duration(5) * time.Second
+	degGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", append(defResponseHeaders, ""), "", ""}


ElvinEfendi · 2019-05-03T15:33:36Z

The PR is looking good! I still think we can avoid touching nginx.tmpl as I mentioned in #3506 (comment).

The location objects are built in controller.go, and when building we assign the annotations, including external auth. In the code path why can you not check global external auth and set external auth of location to the global one if the annotation is not set?

Or even better in the external auth annotation, why don't you set default value to the global external auth value from the configmap? For an example you can check

ingress-nginx/internal/ingress/annotations/proxy/main.go

Line 113 in b4f2880

func (a proxy) Parse(ing *extensions.Ingress) (interface{}, error) {

. You can see there that for example if proxy-connect-timeout is given as annotation then it will be used, otherwise its default/fallback value will be taken from configmap (you can trace GetDefaultBackend function to see that).

okryvoshapka-connyun · 2019-05-06T07:32:40Z

@ElvinEfendi , thank you for feedback.
The change in nginx.tmpl was done intentionally because I simply have not found an example of functionality you mentioned with proxy/main.go)))
My intentions were:

not to flood and corrupt the structure of annotations with imports from the controller or vice-versa.
make the switch the between annotation and global configuration easily visible and not hidden in the code of annotation parser or controller parser.

As you have pointed to an example of "proxy-connect-timeout", I can change it, but it will take some time as I am a bit busy with another project.
Should I change?

P.S. After simply typos fixes, e2e tests have failed for an unclear reason No API token found for service account "ingress-nginx-e2e", retry after the token is automatically created and added to the service account. Any hint?)

ElvinEfendi · 2019-05-07T15:01:08Z

/approve

aledbf · 2019-05-07T16:11:25Z

/lgtm

k8s-ci-robot · 2019-05-07T16:11:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 3, 2018

k8s-ci-robot requested review from aledbf and antoineco December 3, 2018 16:06

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 4, 2018

k8s-ci-robot assigned antoineco Dec 4, 2018

k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Dec 4, 2018

k8s-ci-robot unassigned antoineco Dec 5, 2018

k8s-ci-robot removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Dec 5, 2018

aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2019

ElvinEfendi reviewed Jan 28, 2019

View reviewed changes

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 Feb 25, 2019

okryvoshapka-connyun force-pushed the feature/extrenal-auth-security-opt-out branch from b4c8c26 to e786a47 Compare February 27, 2019 21:31

okryvoshapka-connyun force-pushed the feature/extrenal-auth-security-opt-out branch from baff365 to 53713fe Compare March 1, 2019 09:35

okryvoshapka-connyun force-pushed the feature/extrenal-auth-security-opt-out branch from 28366dc to a16a12d Compare March 19, 2019 16:06

okryvoshapka-connyun force-pushed the feature/extrenal-auth-security-opt-out branch from d7800f8 to 1aae430 Compare May 3, 2019 07:42

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

okryvoshapka-connyun force-pushed the feature/extrenal-auth-security-opt-out branch from 1aae430 to edc66f0 Compare May 3, 2019 08:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2019

Added Global External Authentication settings to configmap parameters…

8cc9afe

… incl. addons

okryvoshapka-connyun force-pushed the feature/extrenal-auth-security-opt-out branch from edc66f0 to 8cc9afe Compare May 3, 2019 10:13

ElvinEfendi reviewed May 3, 2019

View reviewed changes

Fixed typos

4811168

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

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

k8s-ci-robot assigned aledbf May 7, 2019

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

k8s-ci-robot merged commit b06e114 into kubernetes:master May 7, 2019

aledbf mentioned this pull request Jun 26, 2019

global-auth-url configuration does not work #4231

Closed

reegnz mentioned this pull request Jul 18, 2019

Upgrade nginx ingress to 0.25.0 kubernetes/minikube#4785

Merged

Improve the external authorization concept from opt-in to secure-by-default #3506

Improve the external authorization concept from opt-in to secure-by-default #3506

Conversation

haassConnyun commented Dec 3, 2018 • edited Loading

k8s-ci-robot commented Dec 3, 2018

haassConnyun commented Dec 4, 2018

okryvoshapka-connyun commented Dec 4, 2018

haassConnyun commented Dec 4, 2018

haassConnyun commented Dec 4, 2018

okryvoshapka-connyun commented Dec 4, 2018

haassConnyun commented Dec 5, 2018

k8s-ci-robot commented Dec 5, 2018

haassConnyun commented Jan 23, 2019

ElvinEfendi commented Jan 24, 2019

ElvinEfendi commented Jan 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Jan 28, 2019

okryvoshapka-connyun commented Feb 25, 2019

okryvoshapka-connyun commented Feb 28, 2019

okryvoshapka-connyun commented Mar 19, 2019

ElvinEfendi commented Mar 27, 2019

okryvoshapka-connyun commented May 2, 2019

aledbf commented May 2, 2019

okryvoshapka-connyun commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented May 3, 2019

okryvoshapka-connyun commented May 6, 2019

ElvinEfendi commented May 7, 2019

aledbf commented May 7, 2019

k8s-ci-robot commented May 7, 2019

haassConnyun commented Dec 3, 2018 •

edited

Loading