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
kube-apiserver: add a bootstrap token authenticator for TLS bootstrapping #41281
kube-apiserver: add a bootstrap token authenticator for TLS bootstrapping #41281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | ||
|
||
// SecretLister is a Kubernetes clients which can list secrets in a namespace. | ||
type SecretLister interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead do something like this so it's possible to pass a client-go ClientSet to it for example?
// SecretsGetter has a method to return a SecretInterface.
// A group's client should implement this interface.
type SecretsGetter interface {
Secrets(namespace string) SecretInterface
}
// SecretInterface has methods to work with Secret resources.
type SecretInterface interface {
List(opts meta_v1.ListOptions) (*v1.SecretList, error)
}
(taken from https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/secret.go)
Maybe the client-go clientset would do the caching for us then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably some kind of informer should be used for secret lookup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericchiang Can you add a secret informer here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Informer or internal storage access?
@@ -0,0 +1,97 @@ | |||
/* | |||
Copyright 2016 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017 now :)
|
||
import ( | ||
"k8s.io/apiserver/pkg/authentication/user" | ||
"k8s.io/kubernetes/pkg/api" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to depend on client-go here? I see that other parts of k8s.io/apiserver
do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, use client-go from the apiserver for kube types. Compare e.g. the webhook authn plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to mimic the service account authenticator[0] and that uses the API server's internal storage directly[1]. That uses the API server package[2].
I planned to hook it up the same way we do the service account authenticator, and that's why it's this way. Is the service account authenticator using old practices?
[0]
ServiceAccountTokenGetter serviceaccount.ServiceAccountTokenGetter |
[1]
kubernetes/cmd/kube-apiserver/app/server.go
Line 252 in 8621bd3
authenticatorConfig.ServiceAccountTokenGetter = serviceaccountcontroller.NewGetterFromStorageInterface(storageConfig, storageFactory.ResourcePrefix(api.Resource("serviceaccounts")), storageFactory.ResourcePrefix(api.Resource("secrets"))) |
[2]
kubernetes/pkg/serviceaccount/jwt.go
Line 32 in 8621bd3
"k8s.io/kubernetes/pkg/api/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will your authenticator run if it is part of a kube-aggregator setup? On the aggregator side it won't have access to the internal storage, but has to use client-go. In contrast, some kind of delegation has to take place if it runs on the kube-apiserver side. /cc @deads2k
continue | ||
} | ||
|
||
if ts, ok := secret.Data[TokenSecret]; !ok || token != string(ts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the argument token
should equal fmt.Sprintf("%s:%s", secret.Data[TokenID], secret.Data[TokenSecret])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't that encourage setups reusing the same token-secret
s for each token and just switching out the token-id
? Why would this be preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure I follow...
The full token tokenid:tokensecret
is given as the token
arg, right?
Then comparing that arg to only tokensecret
will yield the wong behaviour. Or am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed the header would look like:
Authorization: Bearer (token secret)
Would then map to userid: (token id)
Not:
Authorization: Bearer (token id):(token secret)
Happy to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jbeda @mikedanese I think the token will be
Authorization: Bearer (token id):(token secret)
because that's what we set in kubeconfig.AuthInfo[].Token
in the kubeconfig that the kubelet uses
@ericchiang no need to wait for k8s.io/apiserver to sync. For internal consumation we have a link in the vendor directory. The staging directory is authorative, just change the code there. |
@ericchiang Do you need more information or can you update this with the changes requested? |
@luxas fine to make the changes, but I'm still a little confused about where this gets hooked up to the API server. I've normally been adding authentication plugins here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubeapiserver/authenticator/config.go But that code gets called from here: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/server.go where we don't use client-go. |
a couple lines down, you can see us building a client for portions of the API (like admission plugins, etc) to talk to the API:
I'm not sure about using that for an auth plugin, though. @deads2k, opinions? |
@liggitt I think I can compose an internal secret informer from that client: https://godoc.org/k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion/core/internalversion Should I use that instead? |
You can use the loopback client config since it's recognized locally and ahead of others. |
@deads2k to reuse the loopback client's shared informer[0], I was going to use the factory it creates[1], however there's no secret informer on that interface so I was going to add one. Is that code generated or manually editable? [0] kubernetes/cmd/kube-apiserver/app/server.go Line 272 in 436fa5c
[1] https://godoc.org/k8s.io/kubernetes/pkg/controller/informers#SharedInformerFactory |
@ncdc do you have a shared secret informer in a PR yet? |
@liggitt nobody should be using |
02781a9
to
94b7a86
Compare
Updated plugin to use |
@@ -66,7 +66,8 @@ type AuthenticatorConfig struct { | |||
RequestHeaderConfig *authenticatorfactory.RequestHeaderConfig | |||
|
|||
// TODO, this is the only non-serializable part of the entire config. Factor it out into a clientconfig | |||
ServiceAccountTokenGetter serviceaccount.ServiceAccountTokenGetter | |||
ServiceAccountTokenGetter serviceaccount.ServiceAccountTokenGetter | |||
BootstrapTokenAuthenticator authenticator.Token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with it for now, would still like these two fields better factored
cmd/kube-apiserver/app/server.go
Outdated
// 'pkg/client/informers/informers_generated', the second informer | ||
// created here. | ||
sharedInformers := informers.NewSharedInformerFactory(nil, client, 10*time.Minute) | ||
internalSharedInformers := internalversion.NewSharedInformerFactory(client, 10*time.Minute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm tracking my work for that here: https://trello.com/c/kpiFmic9/121-use-generated-shared-informers-listers-everywhere-possible. We also need to decide if the kube-apiserver should switch to using versioned informers instead of internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the 10*time.Minute
do here? Does the user have to wait 10mins for the APIServer to notice that a new token was added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After hunting it down it looks like how often it calls the underlying cache's resync method. Which seems to be this:
func (c *threadSafeMap) Resync() error {
// Nothing to do
return nil
}
So, I'm a bit confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how often a full resync occurs. A full resync is defined as the shared informer's DeltaFIFO cache re-enqueing every item it has so all listeners can have an opportunity to reprocess them. The correct Resync() function is here (or, the equivalent in the kubernetes repo): https://github.com/kubernetes/client-go/blob/204f12b1f3125cc660f989655af4b5bd83cbd9dd/tools/cache/delta_fifo.go#L507
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And no, the user doesn't have to wait 10 minutes for the server to notice a new token was added. The shared informer uses a Reflector, which performs a list of all items currently in existence, then starts watching for modified items. It performs a full resync every 10 minutes (in this example). Also, in the event the watch is closed for some reason, the Reflector relists and rewatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! I had some nits that could be addressed.
Will test this on my cluster as soon as possible :)
BootstrapUserPrefix = "system:bootstrap:" | ||
BootstrapGroup = "system:bootstrappers" | ||
|
||
kubeSystemNamespace = "kube-system" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a constant for this already: k8s.io/apimachinery/apis/meta/v1.NamespaceSystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was avoiding the import. I'll switch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make the namespace an option when constructing the authenticator and inject it
// name: node-bootstrap-token | ||
// namespace: kube-system | ||
// data: | ||
// token-secret: ( bearer token ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
( the private part of the token )
maybe?
// apiVersion: v1 | ||
// kind: Secret | ||
// metadata: | ||
// name: node-bootstrap-token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bootstrap-token-( token id )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the token shouldn't matter as we are keying off of the SecretType. We shouldn't make assumptions about the name.
// type: bootstrap.kubernetes.io/token | ||
// | ||
func (t *TokenAuthenticator) AuthenticateToken(token string) (user.Info, bool, error) { | ||
secrets, err := t.lister.List(labels.Everything()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we only return Secrets of type bootstrap.kubernetes.io/token
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not here, but there might be a fancier way of doing it on the informer. Let me look into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using the shared informer, but look at https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/bootstrap/tokencleaner.go#L75
94b7a86
to
70fa725
Compare
"k8s.io/kubernetes/pkg/controller" | ||
) | ||
|
||
type secretLister struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops this isn't used anymore.
74e53ae
to
c5c237e
Compare
@@ -172,6 +183,12 @@ func (s *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) { | |||
|
|||
} | |||
|
|||
if s.BootstrapToken != nil { | |||
fs.BoolVar(&s.BootstrapToken.Allow, "experimental-bootstrap-token-auth", s.BootstrapToken.Allow, ""+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag added here. Defaults to false.
secretName := tokenNamePrefix + tokenID | ||
secret, err := t.lister.Get(secretName) | ||
if err != nil { | ||
if errors.IsNotFound(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope this is the correct way to do this detection for the internalverison api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts ^
I guess so, yes
Having trouble running the hack script to update the docs with the new flag. Seems I need a bigger machine :/
Expecting verification to fail because of the added flag. Going to try to fix it tomorrow. |
@@ -270,7 +268,32 @@ func Run(s *options.ServerRunOptions) error { | |||
// TODO: get rid of KUBE_API_VERSIONS or define sane behaviour if set | |||
glog.Errorf("Failed to create clientset with KUBE_API_VERSIONS=%q. KUBE_API_VERSIONS is only for testing. Things will break.", kubeAPIVersions) | |||
} | |||
|
|||
// TODO: Internal informers should switch to using 'pkg/client/informers/informers_generated', | |||
// the second informer created here. Refactor clients which take the former to accept the latter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be done before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts no, but as a follow-up. It would be really important to get this in today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please clean up the duplicated comments below at least.
cmd/kube-apiserver/app/server.go
Outdated
internalSharedInformers := internalversion.NewSharedInformerFactory(client, 10*time.Minute) | ||
|
||
if client == nil { | ||
// If the client is nil, the internalSharedInformers is not safe to use. However, we only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the client==nil case here looks "over commented" ;) 3x more or less the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok, he actually got asked to clarify it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll try to make this more concise.
cmd/kube-apiserver/app/server.go
Outdated
|
||
apiAuthenticator, securityDefinitions, err := authenticatorConfig.New() | ||
if err != nil { | ||
return fmt.Errorf("invalid Authentication Config: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lower case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for adding it behind a flag
@@ -270,7 +268,32 @@ func Run(s *options.ServerRunOptions) error { | |||
// TODO: get rid of KUBE_API_VERSIONS or define sane behaviour if set | |||
glog.Errorf("Failed to create clientset with KUBE_API_VERSIONS=%q. KUBE_API_VERSIONS is only for testing. Things will break.", kubeAPIVersions) | |||
} | |||
|
|||
// TODO: Internal informers should switch to using 'pkg/client/informers/informers_generated', | |||
// the second informer created here. Refactor clients which take the former to accept the latter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts no, but as a follow-up. It would be really important to get this in today
secretName := tokenNamePrefix + tokenID | ||
secret, err := t.lister.Get(secretName) | ||
if err != nil { | ||
if errors.IsNotFound(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts ^
I guess so, yes
// because the existing functionality isn't exported or because it is in a | ||
// package that shouldn't be directly imported by this packages. | ||
|
||
const tokenNamePrefix = "bootstrap-token-" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does now exist in bootstrapapi.BootstrapTokenSecretPrefix
c5c237e
to
a0df658
Compare
return nil, false, nil | ||
} | ||
|
||
secretName := bootstrapapi.BootstrapTokenSecretPrefix + tokenID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now uses constant defined in pkg/bootstrap/api
@luxas comments addressed and figured out the flags check. |
internalSharedInformers := internalversion.NewSharedInformerFactory(client, 10*time.Minute) | ||
|
||
if client == nil { | ||
// TODO: Remove check once client can never be nil. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like more logging here, to be honest. But it isn't worth holding this up and that can come in a new PR.
} | ||
|
||
secretName := bootstrapapi.BootstrapTokenSecretPrefix + tokenID | ||
secret, err := t.lister.Get(secretName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like adding some logging for each of these failure cases might be really helpful for people. When auth fails it can be super frustrating to figure out what is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Will follow up once this is merged.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: ericchiang, jbeda Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 41812, 41665, 40007, 41281, 41771) |
Automatic merge from submit-queue Remove the kube-discovery binary from the tree **What this PR does / why we need it**: kube-discovery was a temporary solution to implementing proposal: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/bootstrap-discovery.md However, this functionality is now gonna be implemented in the core for v1.6 and will fully replace kube-discovery: - #36101 - #41281 - #41417 So due to that `kube-discovery` isn't used in any v1.6 code, it should be removed. The image `gcr.io/google_containers/kube-discovery-${ARCH}:1.0` should and will continue to exist so kubeadm <= v1.5 continues to work. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note Remove cmd/kube-discovery from the tree since it's not necessary anymore ``` @jbeda @dgoodwin @mikedanese @dmmcquay @lukemarsden @errordeveloper @pires
Follows up on #36101
Still needs:
cc @kubernetes/sig-auth-api-reviews @liggitt @luxas @jbeda