Adds the design proposal for self-hosting authz webhook #1458

filmil · 2017-12-01T18:07:54Z

This proposal implements feature request:
kubernetes/enhancements#516

Please refer to the feature request for administrative details. Reviews appreciated.

cblecker · 2017-12-01T18:24:12Z

/ok-to-test

ericchiang · 2017-12-01T18:18:18Z

contributors/design-proposals/auth/self-hosting-authorization-webhook.md

+An example configuration file for authz looks like this:
+
+```yaml
+apiVersion: authorizationconfig.k8s.io/v1alpha1


I know we've had concerns in the past about API groups with only one type. In the future, should this group hold other webhook configs? e.g. audit or authentication?

What do you recommend to do here instead?

Not sure I can decide either way, not knowing the history of such concerns.

This was referencing kubernetes/kubernetes#55812 (comment)

Maybe this could be included in the existing apiserver.config.k8s.io group?

kubernetes/kubernetes@229c430#diff-77d495a77d16f1f8c941606e2e5dee42R30

My understanding from previous comments is that this addition would be a kube-apiserver feature only, and as such not suitable for generic apiservers.

See for example discussion around this comment: kubernetes/kubernetes#52511 (comment)

Would your proposal not mean that we're mixing in these specific concerns into all other apiservers?

Didn't realized deads2k had already commented on this. It's fine as is then. Thanks for the feedback.

Sure thing! It's on me as a proposer to dig up prior art.

Per https://github.com/kubernetes/community/blob/master/contributors/devel/component-config-conventions.md, component config apigroups should have the prefix: .config.k8s.io. I also don't see why this specific config format will not be useful for other webhooks (e.g. authenticator).

Changed the prefix to conform to that convention if it helps. I'm not generalizing since this is not the intention, and previous feedback has been that this should remain a kubeapiserver feature only.

ericchiang · 2017-12-01T18:20:07Z

contributors/design-proposals/auth/self-hosting-authorization-webhook.md

+externalAuthorizationHooks:
+- name: webhook-name
+  clientConfig:
+    serverCaFile: ... # the path to the CA file for the webhook server


nit: we should probably be more consistent about what we name these fields. What other config uses serverCaFile?

My concern here was that a field named serverCa, or server-certificate isn't quite clearly saying whether what follows is a file path name, or the actual quoted content of the field.

serverCaFile is very clear about what the contents are expected to be. Is this something that's not desirable? Should the name err towards consistency regardless? Do we support/endorse the use of yaml-style inline quoting?

Sorry, I mean we have a history of not being constant in our naming of TLS options kubernetes/kubernetes#54665

Admission webhooks use caBundle for example: https://kubernetes.io/docs/admin/extensible-admission-controllers/#configure-webhook-admission-controller-on-the-fly

Yes, duly noted. Do you have a specific recommendation, just so I don't need to go invent an approach if you can already see one?

Digging around the APIs there's not much precedence. I agree this is more explicit than caBundle. Let's just leave it as is. Sorry for the noise.

SGTM. I updated the comments to be way more specific as to the meaning of the fields.

ericchiang · 2017-12-01T18:26:25Z

contributors/design-proposals/auth/self-hosting-authorization-webhook.md

+Versus: this work is out of scope.
+
+Our proposed solution makes no change to the current state of the webhook
+libraries.  Unifying code paths for webhooks is a worthwhile goal, but out of


If we're going to be adding features to the authorization webhook, it seems odd to declare "how other webhooks will us this feature it's out of scope." I'd like to see it at least considered, even if it's not implemented.

Other webhooks won't use this feature per this proposal. The proposal (at least IMHO) doesn't prevent reuse.

I can expand a bit on that viewpoint in the text, but anything I'd say would be along the lines of "this is not a concern in the context of this proposal".

Also, forgot to mention: Thanks a million for jumping to review this pull request so quickly!

I seflishly would like to see this usable by the authentication webhook too. Looking at kubernetes/kubernetes#54733 it doesn't seem like that would be too difficult, the language here just worries me.

Please take another look. I tried to clarify.

filmil · 2017-12-01T23:03:35Z

/assign @deads2k

mikedanese · 2017-12-04T21:12:36Z

A bit stale but potentially related: kubernetes/kubernetes#30395

mikedanese · 2017-12-04T21:16:46Z

contributors/design-proposals/auth/self-hosting-authorization-webhook.md

+`kube-apiserver` can make use of it.  This approach has [garnered support][8]
+from the community in the earlier proposal.
+
+## Proposal


Please add a section on security implications of running an authorizer on the cluster.

Added a section on security.

Thanks. LGTM

mikedanese · 2017-12-04T21:20:36Z

contributors/design-proposals/auth/self-hosting-authorization-webhook.md

+// Top-level
+type ExternalAuthorizationHookConfiguration struct {
+  meta.TypeMeta
+  meta.ObjectMeta


Config objects should not need ObjectMeta.

k8s-ci-robot · 2017-12-11T21:38:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: filmil
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

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

Needs approval from an approver in each of these OWNERS Files:

contributors/design-proposals/auth/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

This proposal implements feature request: kubernetes/enhancements#516 Code review comments handled: - Fixed the resource naming. - Removed ObjectMeta from the configuration file description. - Added a Security section explaining some of the security recommendations.

filmil · 2017-12-11T21:59:52Z

/assign @deads2k

fejta-bot · 2018-03-12T19:30:32Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-04-11T19:46:57Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot · 2018-05-11T20:34:51Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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

filmil mentioned this pull request Dec 1, 2017

Support for self-hosting authorizer webhook kubernetes/enhancements#516

Closed

k8s-github-robot assigned deads2k and liggitt Dec 1, 2017

k8s-github-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Dec 1, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 1, 2017

ericchiang reviewed Dec 1, 2017

View reviewed changes

filmil force-pushed the self_hosting_authz_proposal branch from b57bbf3 to 4695751 Compare December 1, 2017 23:02

filmil force-pushed the self_hosting_authz_proposal branch from 4695751 to 2b96799 Compare December 1, 2017 23:06

mikedanese reviewed Dec 4, 2017

View reviewed changes

filmil force-pushed the self_hosting_authz_proposal branch 2 times, most recently from def1887 to cc4d736 Compare December 11, 2017 21:37

filmil force-pushed the self_hosting_authz_proposal branch from cc4d736 to 56c6cbc Compare December 11, 2017 21:38

k8s-github-robot added the kind/design Categorizes issue or PR as related to design. label Feb 6, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 12, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 11, 2018

k8s-ci-robot closed this May 11, 2018

liggitt mentioned this pull request Jun 14, 2018

Dynamic Auth Webhook kubernetes/kubernetes#65109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the design proposal for self-hosting authz webhook #1458

Adds the design proposal for self-hosting authz webhook #1458

filmil commented Dec 1, 2017

cblecker commented Dec 1, 2017

ericchiang Dec 1, 2017

filmil Dec 1, 2017

ericchiang Dec 1, 2017

filmil Dec 1, 2017

ericchiang Dec 1, 2017

filmil Dec 1, 2017

mikedanese Dec 4, 2017

filmil Dec 5, 2017

ericchiang Dec 1, 2017

filmil Dec 1, 2017

ericchiang Dec 1, 2017 •

edited

Loading

filmil Dec 1, 2017

ericchiang Dec 1, 2017

filmil Dec 1, 2017

ericchiang Dec 1, 2017 •

edited

Loading

filmil Dec 1, 2017

filmil Dec 1, 2017

ericchiang Dec 1, 2017

filmil Dec 1, 2017

filmil commented Dec 1, 2017 •

edited

Loading

mikedanese commented Dec 4, 2017

mikedanese Dec 4, 2017

filmil Dec 11, 2017

mikedanese Dec 12, 2017

mikedanese Dec 4, 2017

filmil Dec 5, 2017

k8s-ci-robot commented Dec 11, 2017

filmil commented Dec 11, 2017

fejta-bot commented Mar 12, 2018

fejta-bot commented Apr 11, 2018

fejta-bot commented May 11, 2018

Adds the design proposal for self-hosting authz webhook #1458

Adds the design proposal for self-hosting authz webhook #1458

Conversation

filmil commented Dec 1, 2017

cblecker commented Dec 1, 2017

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

Choose a reason for hiding this comment

ericchiang Dec 1, 2017 • edited Loading

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

ericchiang Dec 1, 2017 • edited Loading

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

filmil commented Dec 1, 2017 • edited Loading

mikedanese commented Dec 4, 2017

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

k8s-ci-robot commented Dec 11, 2017

filmil commented Dec 11, 2017

fejta-bot commented Mar 12, 2018

fejta-bot commented Apr 11, 2018

fejta-bot commented May 11, 2018

ericchiang Dec 1, 2017 •

edited

Loading

ericchiang Dec 1, 2017 •

edited

Loading

filmil commented Dec 1, 2017 •

edited

Loading