Update delegated authorization options default to eliminate unnecessary SARs #98325

deads2k · 2021-01-22T21:22:37Z

healthz, readyz, and livez are canonical names for checks that the kubelet does. By default, allow access to them in the options. Callers can adjust the defaults if they have a reason to require checks.

system:masters has full power, so the authorization check is unnecessary and just uses an extra call for in-cluster access. Callers can adjust the defaults if they have a reason to require checks.

This directly impacts kube-controller-manager, kube-scheduler, and cloud-controller-manager in tree. I think these values are reasonable, but am happy to hear from @kubernetes/sig-auth-pr-reviews on the subject. And I'm willing to wire a configuration option if we think cluster-admins should be able to twiddle these knobs.

/kind cleanup
/priority important-soon
@kubernetes/sig-auth-pr-reviews @kubernetes/sig-api-machinery-pr-reviews

The default delegating authorization options now allow unauthenticated access to healthz, readyz, and livez.  A system:masters user connecting to an authz delegator will not perform an authz check.

deads2k · 2021-01-22T21:22:55Z

/hold

don't want to merge until there has been a good amount of time for comment.

lavalamp · 2021-01-22T21:43:03Z

staging/src/k8s.io/apiserver/pkg/server/options/authorization.go

@@ -78,6 +78,14 @@ func NewDelegatingAuthorizationOptions() *DelegatingAuthorizationOptions {
 		DenyCacheTTL:        10 * time.Second,
 		ClientTimeout:       10 * time.Second,
 		WebhookRetryBackoff: DefaultAuthWebhookRetryBackoff(),
+		// This allows the kubelet to always get health and readiness without causing an authorization check.
+		// This field can be cleared by callers if they don't want this behavior.
+		AlwaysAllowPaths: []string{"/healthz", "readyz", "/livez"},


I think we should at least require membership in the authenticated group for something like this. Does it? How?

Assuming you have anonymous auth enabled on your cluster, we already allow this for unauthenticated users:

kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

Lines 228 to 236 in 7b3f0cd

{

// a role which provides just enough power read insensitive cluster information

ObjectMeta: metav1.ObjectMeta{Name: "system:public-info-viewer"},

Rules: []rbacv1.PolicyRule{

rbacv1helpers.NewRule("get").URLs(

"/livez", "/readyz", "/healthz", "/version", "/version/",

).RuleOrDie(),

},

},

kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

Line 578 in 7b3f0cd

rbacv1helpers.NewClusterBinding("system:public-info-viewer").Groups(user.AllAuthenticated, user.AllUnauthenticated).BindingOrDie(),

This change just makes it so that the SAR checks are skipped.

lavalamp · 2021-01-22T21:44:40Z

uh I like the specific optimization, but I'm kinda terrified in general of optimizing away authz checks. Like will this change how calls show up in the audit log?

enj · 2021-01-22T21:56:54Z

LGTM and matches my personal experience. I have done the following before to make node health checks against aggregated API servers less wasteful:

authorizationOptions := genericapiserveroptions.NewDelegatingAuthorizationOptions().
    WithAlwaysAllowPaths("/healthz").
    WithAlwaysAllowGroups(user.SystemPrivilegedGroup)

enj · 2021-01-22T21:58:09Z

staging/src/k8s.io/apiserver/pkg/server/options/authorization.go

@@ -78,6 +78,14 @@ func NewDelegatingAuthorizationOptions() *DelegatingAuthorizationOptions {
 		DenyCacheTTL:        10 * time.Second,
 		ClientTimeout:       10 * time.Second,
 		WebhookRetryBackoff: DefaultAuthWebhookRetryBackoff(),
+		// This allows the kubelet to always get health and readiness without causing an authorization check.
+		// This field can be cleared by callers if they don't want this behavior.
+		AlwaysAllowPaths: []string{"/healthz", "readyz", "/livez"},


/readyz (missing leading slash)

/readyz (missing leading slash)

updated

deads2k · 2021-01-25T14:24:20Z

uh I like the specific optimization, but I'm kinda terrified in general of optimizing away authz checks. Like will this change how calls show up in the audit log?

The audit log will show a different authorizer as allowing the connection instead of RBAC (this is built in a layered way). The authentication pieces in the audit log will match exactly.

SARs healthz, readyz, and livez are canonical names for checks that the kubelet does. By default, allow access to them in the options. Callers can adjust the defaults if they have a reason to require checks. system:masters has full power, so the authorization check is unnecessary and just uses an extra call for in-cluster access. Callers can adjust the defaults if they have a reason to require checks.

lavalamp · 2021-01-25T18:01:32Z

The audit log will show a different authorizer as allowing the connection instead of RBAC (this is built in a layered way). The authentication pieces in the audit log will match exactly.

OK LGTM then. Will let someone from SIG auth do the official LGTM.

enj · 2021-01-26T17:34:48Z

Failing test on AlwaysAllowGroups:

Executing tests from //cmd/kube-controller-manager/app/options:go_default_test
--- FAIL: TestAddFlags (0.02s)
    options_test.go:444: Got different run options than expected.
        Difference detected on:
          &options.KubeControllerManagerOptions{
          	... // 27 identical fields
          	InsecureServing: &{DeprecatedInsecureServingOptions: &{BindAddress: s"192.168.4.10", BindPort: 10000, BindNetwork: "tcp"}},
          	Authentication:  &{RemoteKubeConfigFileOptional: true, CacheTTL: s"10s", RequestHeader: {UsernameHeaders: {"x-remote-user"}, GroupHeaders: {"x-remote-group"}, ExtraHeaderPrefixes: {"x-remote-extra-"}}, WebhookRetryBackoff: &{Duration: s"500ms", Factor: 1.5, Jitter: 0.2, Steps: 5, ...}, ...},
          	Authorization: &options.DelegatingAuthorizationOptions{
          		... // 3 identical fields
          		DenyCacheTTL:        s"10s",
          		AlwaysAllowPaths:    {"/healthz"},
        - 		AlwaysAllowGroups:   nil,
        + 		AlwaysAllowGroups:   []string{"system:masters"},
          		ClientTimeout:       s"10s",
          		WebhookRetryBackoff: &{Duration: s"500ms", Factor: 1.5, Jitter: 0.2, Steps: 5, ...},
          	},
          	Metrics: &{},
          	Logs:    &{LogFormat: "text"},
          	... // 3 identical fields
          }

Do we want to update AlwaysAllowPaths for KCM to []string{"/healthz", "readyz", "/livez"}?

deads2k · 2021-01-26T17:53:48Z

Do we want to update AlwaysAllowPaths for KCM to []string{"/healthz", "readyz", "/livez"}?

pushing update to use the defaults now.

leilajal · 2021-01-26T21:16:22Z

/triage accepted

deads2k · 2021-01-28T16:11:03Z

/hold cancel

I think we closed on this and are ok moving forward with this one.

enj · 2021-02-02T15:47:15Z

/lgtm

cheftako · 2021-02-02T16:58:39Z

/lgtm

cheftako · 2021-02-02T16:58:52Z

/approve

damemi

/approve

k8s-ci-robot · 2021-02-02T18:37:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, damemi, deads2k

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:

~~cmd/kube-controller-manager/OWNERS~~ [cheftako,deads2k]
~~cmd/kube-scheduler/OWNERS~~ [damemi]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [cheftako]

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 the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2021

k8s-ci-robot requested review from dims and ping035627 January 22, 2021 21:23

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver labels Jan 22, 2021

lavalamp reviewed Jan 22, 2021

View reviewed changes

enj reviewed Jan 22, 2021

View reviewed changes

deads2k force-pushed the update-default-authorizer branch from c46e32a to cebce29 Compare January 25, 2021 14:42

update delegated authz defaults in kube binaries

62230d3

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 26, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2021

k8s-ci-robot assigned enj Feb 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2021

k8s-ci-robot assigned cheftako Feb 2, 2021

damemi reviewed Feb 2, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2021

k8s-ci-robot merged commit d265910 into kubernetes:master Feb 2, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 2, 2021

enj mentioned this pull request Feb 2, 2021

Reduce the number of SAR checks we make vmware-tanzu/pinniped#241

Open

github-actions bot mentioned this pull request Feb 3, 2021

Week Ending January 31, 2021 dev-obs/actus#339

Open

enj mentioned this pull request Mar 3, 2021

update the kubelet authorizer to allow system:masters access without a SAR #99611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update delegated authorization options default to eliminate unnecessary SARs #98325

Update delegated authorization options default to eliminate unnecessary SARs #98325

deads2k commented Jan 22, 2021

deads2k commented Jan 22, 2021

lavalamp Jan 22, 2021

enj Jan 22, 2021

lavalamp commented Jan 22, 2021

enj commented Jan 22, 2021

enj Jan 22, 2021

deads2k Jan 25, 2021

deads2k commented Jan 25, 2021

lavalamp commented Jan 25, 2021

enj commented Jan 26, 2021

deads2k commented Jan 26, 2021

leilajal commented Jan 26, 2021

deads2k commented Jan 28, 2021

enj commented Feb 2, 2021

cheftako commented Feb 2, 2021

cheftako commented Feb 2, 2021

damemi left a comment

k8s-ci-robot commented Feb 2, 2021

	{
	// a role which provides just enough power read insensitive cluster information
	ObjectMeta: metav1.ObjectMeta{Name: "system:public-info-viewer"},
	Rules: []rbacv1.PolicyRule{
	rbacv1helpers.NewRule("get").URLs(
	"/livez", "/readyz", "/healthz", "/version", "/version/",
	).RuleOrDie(),
	},
	},

Update delegated authorization options default to eliminate unnecessary SARs #98325

Update delegated authorization options default to eliminate unnecessary SARs #98325

Conversation

deads2k commented Jan 22, 2021

deads2k commented Jan 22, 2021

lavalamp Jan 22, 2021

Choose a reason for hiding this comment

enj Jan 22, 2021

Choose a reason for hiding this comment

lavalamp commented Jan 22, 2021

enj commented Jan 22, 2021

enj Jan 22, 2021

Choose a reason for hiding this comment

deads2k Jan 25, 2021

Choose a reason for hiding this comment

deads2k commented Jan 25, 2021

lavalamp commented Jan 25, 2021

enj commented Jan 26, 2021

deads2k commented Jan 26, 2021

leilajal commented Jan 26, 2021

deads2k commented Jan 28, 2021

enj commented Feb 2, 2021

cheftako commented Feb 2, 2021

cheftako commented Feb 2, 2021

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 2, 2021