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

add ability to authenticators for dynamic update of certs for delegated authn #82371

Merged
merged 1 commit into from
Oct 4, 2019
Merged

add ability to authenticators for dynamic update of certs for delegated authn #82371

merged 1 commit into from
Oct 4, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Sep 5, 2019

part 1 and 2 of #82141

For aggregated apiservers, we created delegated authentication options. These pull from kubectl get -n kube-system configmap/extension-apiserver-authentication. ConfigMaps are logically dynamic, but the delegated authentication doesn't automatically pick up the changes. This causes drift between intent and reality.

The options from here are still static, but they are wired to use the verfiyOptionsFunc variant to allow part 4 of the issue.

/kind bug
/priority important-soon
@kubernetes/sig-auth-pr-reviews

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 5, 2019
@k8s-ci-robot k8s-ci-robot added area/apiserver area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Sep 5, 2019
@deads2k deads2k changed the title add ability to authenticators for dynamic update of certs for delegated authn [WIP] add ability to authenticators for dynamic update of certs for delegated authn Sep 5, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2019
@fedebongio
Copy link
Contributor

/assign @liggitt

@k8s-ci-robot 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 Sep 5, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Sep 5, 2019

/retest

mikedanese
mikedanese reviewed Sep 6, 2019
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/options/authentication.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/requestheader.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authentication/request/x509/verify_options.go
}

// NewSimpleStaticVerifierFromFile creates a new verification func from a file. It reads the content and then fails.
// It will return a nil function if you pass an empty CA file.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should be handled externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like it should be handled externally.

It could be, but I suspect it would just be an x509verifyutil package. I can make that if you prefer.

staging/src/k8s.io/apiserver/pkg/authentication/request/x509/verify_options.go Outdated Show resolved Hide resolved
@deads2k deads2k changed the title [WIP] add ability to authenticators for dynamic update of certs for delegated authn add ability to authenticators for dynamic update of certs for delegated authn Sep 6, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2019
liggitt
liggitt reviewed Sep 24, 2019
View reviewed changes
cmd/kubelet/app/auth.go Outdated Show resolved Hide resolved
pkg/kubeapiserver/authenticator/config.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authentication/request/x509/verify_options.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go
requestHeaderAuthenticator, err := headerrequest.NewSecure(
c.RequestHeaderConfig.ClientCA,
requestHeaderAuthenticator, err := headerrequest.NewDynamicSecure(
c.RequestHeaderConfig.VerifyOptionFn,
Copy link
Member

Choose a reason for hiding this comment

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

this structure is assuming the CA is the only dynamic aspect, even though we populate all of these options dynamically, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this structure is assuming the CA is the only dynamic aspect, even though we populate all of these options dynamically, right?

I'll update to allow functions through, but I'm still planning to defer wiring until step 4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed to make the limitation clear and will continue in a separate pull to keep pull size down.

staging/src/k8s.io/apiserver/pkg/authentication/request/x509/verify_options.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/authentication.go Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Sep 24, 2019

direction looks good, a few nits, only substantive comment is #82371 (comment)

@liggitt liggitt mentioned this pull request Oct 1, 2019
Closed
@deads2k deads2k mentioned this pull request Oct 3, 2019
Merged
soltysh
soltysh approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2019
derekwaynecarr
derekwaynecarr approved these changes Oct 4, 2019
View reviewed changes
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

Approve on kubelet change.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2019
@enj
Copy link
Member

enj commented Oct 4, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 7ac6585 into kubernetes:master Oct 4, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 4, 2019
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

@mikedanese mikedanese mikedanese left review comments

@soltysh soltysh soltysh approved these changes

@derekwaynecarr derekwaynecarr derekwaynecarr approved these changes

@enj enj Awaiting requested review from enj

@hongchaodeng hongchaodeng Awaiting requested review from hongchaodeng

Assignees

@soltysh soltysh

@liggitt liggitt

@enj enj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@deads2k @fedebongio @liggitt @k8s-ci-robot @enj @soltysh @mikedanese @derekwaynecarr