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.

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

tokenreview: authenticator interface changes #69582

Merged
merged 3 commits into from Oct 23, 2018

Conversation

@mikedanese
Member

mikedanese commented Oct 9, 2018

Broken out of #62692

NONE
@neolit123

This comment has been minimized.

Member

neolit123 commented Oct 10, 2018

/kind api-change

type Response struct {
// Audience is the audience selected by the authenticator. If the
// authenticator is not audience aware, this field will be nil.
Audience *string

This comment has been minimized.

@enj

enj Oct 10, 2018

Member

I still think this should be a slice:

type Audiences []string

type Response struct {
	Audiences Audiences

	...
}

You can distinguish between nil, len() == 0, len() > 0 as needed (though I am unsure if the distinction is required).

This comment has been minimized.

@mikedanese

mikedanese Oct 15, 2018

Member

Do you think it's valuable for an Authorizer to return multiple audiences that it was able to validate the token against? My idea wast that 1 valid audience is probably enough to return.

This comment has been minimized.

@enj

enj Oct 16, 2018

Member

I guess it depends on what question is being asked: "did any audience validate?" or "what audiences validated?"

If we are going with the former, would it simpler to just say Audience *bool?

nil == API server does not understand audiences
true/false == API server does understand audiences + validation was performed + the field holds the result

Show resolved Hide resolved staging/src/k8s.io/apiserver/pkg/authentication/authenticator/interfaces.go
@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 16, 2018

/approve

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 16, 2018

@liggitt @enj PTAL

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 16, 2018

/retest

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 16, 2018

a couple changes needed pre-merge, and a lot of TODOs, lgtm otherwise

Show resolved Hide resolved pkg/kubelet/server/server.go Outdated
Show resolved Hide resolved pkg/registry/authentication/tokenreview/storage.go Outdated
@@ -177,7 +177,7 @@ func (j *jwtTokenAuthenticator) AuthenticateToken(tokenData string) (user.Info,
return nil, false, err
}
return sa.UserInfo(), true, nil
return &authenticator.Response{User: sa.UserInfo()}, true, nil

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

needs a TODO to verify the token audience and echo in the response if ctx has an audience limitation

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

I think this (and the other related TODOs) means most of the authenticators will need to know the implicit apiserver audiences at construction time. If the token has no audience (legacy service account token), then we should accept AuthenticateToken requests for the implicit apiserver audiences.

return &user.DefaultInfo{
Name: bootstrapapi.BootstrapUserPrefix + string(id),
Groups: groups,
return &authenticator.Response{

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODO to take apiserver audiences at TokenAuthenticator construction time, and if ctx is audience-limited, ensure it intersects with the apiserver audiences, and echo an audience in the response

@@ -30,7 +30,12 @@ const (
)
func NewAuthenticator() authenticator.Request {
return authenticator.RequestFunc(func(req *http.Request) (user.Info, bool, error) {
return &user.DefaultInfo{Name: anonymousUser, Groups: []string{unauthenticatedGroup}}, true, nil
return authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODO to echo audience in response if present in ctx?

@@ -88,10 +90,10 @@ func NewCSV(path string) (*TokenAuthenticator, error) {
}, nil
}
func (a *TokenAuthenticator) AuthenticateToken(value string) (user.Info, bool, error) {
func (a *TokenAuthenticator) AuthenticateToken(ctx context.Context, value string) (*authenticator.Response, bool, error) {

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODO to take implicit audiences at TokenAuthenticator construction time, and if ctx is audience-limited, ensure it intersects with the implicit audiences, and echo an audience in the response

@@ -71,9 +71,9 @@ func WithAuthentication(handler http.Handler, auth authenticator.Request, failed
// authorization header is not required anymore in case of a successful authentication.
req.Header.Del("Authorization")

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODO to check if the response has a valid audience if apiAuds is non-empty

This comment has been minimized.

@mikedanese

mikedanese Oct 16, 2018

Member

Added TODO here and in TokenReview storage.

@@ -78,13 +80,13 @@ func NewCSV(path string) (*PasswordAuthenticator, error) {
return &PasswordAuthenticator{users}, nil
}
func (a *PasswordAuthenticator) AuthenticatePassword(username, password string) (user.Info, bool, error) {
func (a *PasswordAuthenticator) AuthenticatePassword(ctx context.Context, username, password string) (*authenticator.Response, bool, error) {

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODO to take implicit audiences at PasswordAuthenticator construction time, and if ctx is audience-limited, ensure it intersects with the implicit audiences, and echo an audience in the response

@@ -529,7 +531,7 @@ func (r *claimResolver) resolve(endpoint endpoint, allClaims claims) error {
return nil
}
func (a *Authenticator) AuthenticateToken(token string) (user.Info, bool, error) {
func (a *Authenticator) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) {

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODO to take implicit audiences at Authenticator construction time, and if ctx is audience-limited, ensure it intersects with the implicit audiences, and echo an audience in the response. Interestingly, this will likely be totally different than the audience in the token itself.

@@ -69,7 +70,7 @@ func newWithBackoff(tokenReview authenticationclient.TokenReviewInterface, ttl,
}
// AuthenticateToken implements the authenticator.Token interface.
func (w *WebhookTokenAuthenticator) AuthenticateToken(token string) (user.Info, bool, error) {
func (w *WebhookTokenAuthenticator) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) {

This comment has been minimized.

@liggitt

liggitt Oct 16, 2018

Member

TODOs here:

  • take implicit audiences at WebhookTokenAuthenticator construction time
  • if ctx is audience-limited, add into the tokenreview object
  • if the tokenreview returns with an audience set that matches the ctx, copy into the response and return success
  • if the tokenreview returns without an audience set, ensure the ctx audiences intersect with the implicit audiences taken at WebhookTokenAuthenticator construction time, and pick one to add into the response
@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 16, 2018

I moved all the authenticator handling of context todos to here to track:

#69893

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 17, 2018

/retest

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 18, 2018

one nit, retitle the "make everything compile" commit, then lgtm

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 18, 2018

@liggitt fixed

for pkg/kubelet approval
/assign @tallclair

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 19, 2018

bazel failure

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 22, 2018

@liggitt Fixed.

@tallclair

This comment has been minimized.

Member

tallclair commented Oct 22, 2018

For pkg/kubelet:
/approve

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 22, 2018

/retest

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 23, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 23, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese, tallclair

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

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 23, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit baa8d80 into kubernetes:master Oct 23, 2018

17 of 18 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation mikedanese authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@mikedanese mikedanese deleted the mikedanese:trev7 branch Oct 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment