Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz) #67543

Merged
merged 1 commit into from
Aug 24, 2018
Merged

delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz) #67543

merged 1 commit into from
Aug 24, 2018

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Aug 17, 2018 •
edited
Loading

Add AlwaysAllowPaths field to delegated authz. These http paths are excluded from the authz chain.

Prerequisite for #64149 and #67069.

Added --authorization-always-allow-paths to components doing delegated authorization to exclude certain HTTP paths like /healthz from authorization.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 17, 2018
@sttts sttts changed the title authn/z: add SkippedPaths to option structs (defaulting to /healthz) delegated authn/z: add SkippedPaths to option structs (defaulting to /healthz) Aug 17, 2018
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 17, 2018
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2018
liggitt
liggitt reviewed Aug 17, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go Outdated
RequestHeaderConfig *RequestHeaderConfig
}

func (c DelegatingAuthenticatorConfig) New() (authenticator.Request, *spec.SecurityDefinitions, error) {
authenticators := []authenticator.Request{}
securityDefinitions := spec.SecurityDefinitions{}

if len(c.SkippedPaths) > 0 {
authenticators = append(authenticators, path.NewAuthenticator(c.SkippedPaths))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect this to be required for authentication... if you allow anonymous requests, a request with no client cert or token will already be authenticated as the anonymous user (for all paths, not just /healthz)

Copy link
Contributor Author

@sttts sttts Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if authenticated, we want to exclude certain paths in case the kube-apiserver is down. Otherwise, with a client cert you can't query /healthz in that case.

Maybe for authn this would be fine. Authz still needs the skipped paths mechanism.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 17, 2018
@sttts sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 17, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 17, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 17, 2018

Removed the authn part for now. @liggitt ptal

@sttts sttts changed the title delegated authn/z: add SkippedPaths to option structs (defaulting to /healthz) delegated authz: add SkippedPaths to option structs (defaulting to /healthz) Aug 17, 2018
@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 17, 2018
@sttts sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 17, 2018
@sttts sttts force-pushed the sttts-auth-skip-paths branch 2 times, most recently from 751f3ed to 2dc09f7 Compare August 17, 2018 15:11
@sttts sttts changed the title delegated authz: add SkippedPaths to option struct (defaulting to /healthz) delegated authz: add AlwaysAllowPaths to option struct (defaulting to /healthz) Aug 21, 2018
@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 21, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 22, 2018

/retest

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 22, 2018
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 22, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 22, 2018

/retest

@awly awly mentioned this pull request Aug 22, 2018
Merged
@sttts
Copy link
Contributor Author

sttts commented Aug 23, 2018

@awly @liggitt @mikedanese anything left here?

@nikhita
Copy link
Member

nikhita commented Aug 23, 2018

/remove-area custom-resources

@awly
Copy link
Contributor

awly commented Aug 23, 2018

/lgtm
LGTM, but @liggitt or @mikedanese should take another look

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 24, 2018

Only change in /staging/ is for bazel and Godeps.json.

@liggitt approved?

@dims
Copy link
Member

dims commented Aug 24, 2018

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from dims August 24, 2018 12:31
@liggitt
Copy link
Member

liggitt commented Aug 24, 2018

/approve

structure looks good. looks like the /healthz default was removed (I might actually be in favor of making components opt into that as a default always-allowed path, rather than baking it into delegated authz options).

/hold
for ack on intended default behavior

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 24, 2018

the /healthz default was removed (I might actually be in favor of making components opt into that as a default always-allowed path, rather than baking it into delegated authz options).

Yes, that was the idea. Have moved it into the components.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 24, 2018

Overriding approval as only staging/BUILD blocks it.

@sttts sttts added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: awly, liggitt, sttts

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:

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

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 5ed26a3 into kubernetes:master Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@awly awly awly left review comments

@mikedanese mikedanese mikedanese left review comments

@cjcullen cjcullen Awaiting requested review from cjcullen

Assignees

@liggitt liggitt

@awly awly

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@sttts @liggitt @mikedanese @nikhita @awly @dims @k8s-ci-robot @k8s-github-robot