-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
implement token authenticator for new id tokens #59940
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -326,18 +326,25 @@ func CreateKubeAPIServerConfig(s *options.ServerRunOptions, nodeTunneler tunnele | |
} | ||
|
||
var issuer serviceaccount.TokenGenerator | ||
if s.ServiceAccountSigningKeyFile != "" || s.Authentication.ServiceAccounts.Issuer != "" { | ||
var apiAudiences []string | ||
if s.ServiceAccountSigningKeyFile != "" || | ||
s.Authentication.ServiceAccounts.Issuer != "" || | ||
len(s.Authentication.ServiceAccounts.APIAudiences) > 0 { | ||
if !utilfeature.DefaultFeatureGate.Enabled(features.TokenRequest) { | ||
return nil, nil, nil, nil, nil, fmt.Errorf("the TokenRequest feature is not enabled but --service-account-signing-key-file and/or --service-account-issuer-id flags were passed") | ||
} | ||
if s.ServiceAccountSigningKeyFile == "" || s.Authentication.ServiceAccounts.Issuer == "" { | ||
return nil, nil, nil, nil, nil, fmt.Errorf("service-account-signing-key-file and service-account-issuer should be specified together") | ||
if s.ServiceAccountSigningKeyFile == "" || | ||
s.Authentication.ServiceAccounts.Issuer == "" || | ||
len(s.Authentication.ServiceAccounts.APIAudiences) == 0 || | ||
len(s.Authentication.ServiceAccounts.KeyFiles) == 0 { | ||
return nil, nil, nil, nil, nil, fmt.Errorf("service-account-signing-key-file, service-account-issuer, service-account-api-audiences and service-account-key-file should be specified together") | ||
} | ||
sk, err := certutil.PrivateKeyFromFile(s.ServiceAccountSigningKeyFile) | ||
if err != nil { | ||
return nil, nil, nil, nil, nil, fmt.Errorf("failed to parse service-account-issuer-key-file: %v", err) | ||
} | ||
issuer = serviceaccount.JWTTokenGenerator(s.Authentication.ServiceAccounts.Issuer, sk) | ||
apiAudiences = s.Authentication.ServiceAccounts.APIAudiences | ||
} | ||
|
||
config := &master.Config{ | ||
|
@@ -371,7 +378,9 @@ func CreateKubeAPIServerConfig(s *options.ServerRunOptions, nodeTunneler tunnele | |
|
||
EndpointReconcilerType: reconcilers.Type(s.EndpointReconcilerType), | ||
MasterCount: s.MasterCount, | ||
ServiceAccountIssuer: issuer, | ||
|
||
ServiceAccountIssuer: issuer, | ||
ServiceAccountAPIAudiences: apiAudiences, | ||
}, | ||
} | ||
|
||
|
@@ -471,7 +480,7 @@ func BuildGenericConfig(s *options.ServerRunOptions, proxyTransport *http.Transp | |
) | ||
} | ||
|
||
genericConfig.Authentication.Authenticator, genericConfig.OpenAPIConfig.SecurityDefinitions, err = BuildAuthenticator(s, storageFactory, client, sharedInformers) | ||
genericConfig.Authentication.Authenticator, genericConfig.OpenAPIConfig.SecurityDefinitions, err = BuildAuthenticator(s, storageFactory, client, clientgoExternalClient, sharedInformers) | ||
if err != nil { | ||
return nil, nil, nil, nil, nil, fmt.Errorf("invalid authentication config: %v", err) | ||
} | ||
|
@@ -555,25 +564,10 @@ func BuildAdmissionPluginInitializers(s *options.ServerRunOptions, client intern | |
} | ||
|
||
// BuildAuthenticator constructs the authenticator | ||
func BuildAuthenticator(s *options.ServerRunOptions, storageFactory serverstorage.StorageFactory, client internalclientset.Interface, sharedInformers informers.SharedInformerFactory) (authenticator.Request, *spec.SecurityDefinitions, error) { | ||
func BuildAuthenticator(s *options.ServerRunOptions, storageFactory serverstorage.StorageFactory, client internalclientset.Interface, extclient clientgoclientset.Interface, sharedInformers informers.SharedInformerFactory) (authenticator.Request, *spec.SecurityDefinitions, error) { | ||
authenticatorConfig := s.Authentication.ToAuthenticationConfig() | ||
if s.Authentication.ServiceAccounts.Lookup { | ||
// we have to go direct to storage because the clientsets fail when they're initialized with some API versions excluded | ||
// we should stop trying to control them like that. | ||
storageConfigServiceAccounts, err := storageFactory.NewConfig(api.Resource("serviceaccounts")) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("unable to get serviceaccounts storage: %v", err) | ||
} | ||
storageConfigSecrets, err := storageFactory.NewConfig(api.Resource("secrets")) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("unable to get secrets storage: %v", err) | ||
} | ||
authenticatorConfig.ServiceAccountTokenGetter = serviceaccountcontroller.NewGetterFromStorageInterface( | ||
storageConfigServiceAccounts, | ||
storageFactory.ResourcePrefix(api.Resource("serviceaccounts")), | ||
storageConfigSecrets, | ||
storageFactory.ResourcePrefix(api.Resource("secrets")), | ||
) | ||
authenticatorConfig.ServiceAccountTokenGetter = serviceaccountcontroller.NewGetterFromClient(extclient) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1000 |
||
} | ||
if client == nil || reflect.ValueOf(client).IsNil() { | ||
// TODO: Remove check once client can never be nil. | ||
|
@@ -687,12 +681,19 @@ func defaultOptions(s *options.ServerRunOptions) error { | |
|
||
s.Authentication.ApplyAuthorization(s.Authorization) | ||
|
||
// Default to the private server key for service account token signing | ||
if len(s.Authentication.ServiceAccounts.KeyFiles) == 0 && s.SecureServing.ServerCert.CertKey.KeyFile != "" { | ||
if kubeauthenticator.IsValidServiceAccountKeyFile(s.SecureServing.ServerCert.CertKey.KeyFile) { | ||
s.Authentication.ServiceAccounts.KeyFiles = []string{s.SecureServing.ServerCert.CertKey.KeyFile} | ||
} else { | ||
glog.Warning("No TLS key provided, service account token authentication disabled") | ||
// Use (ServiceAccountSigningKeyFile != "") as a proxy to the user enabling | ||
// TokenRequest functionality. This defaulting was convenient, but messed up | ||
// a lot of people when they rotated their serving cert with no idea it was | ||
// connected to their service account keys. We are taking this oppurtunity to | ||
// remove this problematic defaulting. | ||
if s.ServiceAccountSigningKeyFile == "" { | ||
// Default to the private server key for service account token signing | ||
if len(s.Authentication.ServiceAccounts.KeyFiles) == 0 && s.SecureServing.ServerCert.CertKey.KeyFile != "" { | ||
if kubeauthenticator.IsValidServiceAccountKeyFile(s.SecureServing.ServerCert.CertKey.KeyFile) { | ||
s.Authentication.ServiceAccounts.KeyFiles = []string{s.SecureServing.ServerCert.CertKey.KeyFile} | ||
} else { | ||
glog.Warning("No TLS key provided, service account token authentication disabled") | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,9 +70,10 @@ type PasswordFileAuthenticationOptions struct { | |
} | ||
|
||
type ServiceAccountAuthenticationOptions struct { | ||
KeyFiles []string | ||
Lookup bool | ||
Issuer string | ||
KeyFiles []string | ||
Lookup bool | ||
Issuer string | ||
APIAudiences []string | ||
} | ||
|
||
type TokenFileAuthenticationOptions struct { | ||
|
@@ -236,15 +237,21 @@ func (s *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) { | |
if s.ServiceAccounts != nil { | ||
fs.StringArrayVar(&s.ServiceAccounts.KeyFiles, "service-account-key-file", s.ServiceAccounts.KeyFiles, ""+ | ||
"File containing PEM-encoded x509 RSA or ECDSA private or public keys, used to verify "+ | ||
"ServiceAccount tokens. If unspecified, --tls-private-key-file is used. "+ | ||
"The specified file can contain multiple keys, and the flag can be specified multiple times with different files.") | ||
"ServiceAccount tokens. The specified file can contain multiple keys, and the flag can "+ | ||
"be specified multiple times with different files. If unspecified, "+ | ||
"--tls-private-key-file is used. Must be specified when "+ | ||
"--service-account-signing-key is provided") | ||
|
||
fs.BoolVar(&s.ServiceAccounts.Lookup, "service-account-lookup", s.ServiceAccounts.Lookup, | ||
"If true, validate ServiceAccount tokens exist in etcd as part of authentication.") | ||
|
||
fs.StringVar(&s.ServiceAccounts.Issuer, "service-account-issuer", s.ServiceAccounts.Issuer, ""+ | ||
"Identifier of the service account token issuer. The issuer will assert this identifier "+ | ||
"in \"iss\" claim of issued tokens. This value is a string or URI.") | ||
|
||
fs.StringSliceVar(&s.ServiceAccounts.APIAudiences, "service-account-api-audiences", s.ServiceAccounts.APIAudiences, ""+ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if this was discussed previously, but why multiple audiences? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be nice when:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All for that. This is one fo the limitations of the OpenID Connect spec that's caused us pain. The JWT validation requires all the audiences to be present in the JWT: https://github.com/square/go-jose/blob/v2.1.3/jwt/validation.go#L73 It sounds like we only want to require one audience out of the set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did the token generator get updated to set audiences to the apiserver's audiences when none are requested in the TokenRequest? I expected these to be required if you wanted to use TokenRequest (possibly with a default), and to be plumbed to that rest handler There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
"Identifiers of the API. The service account token authenticator will validate that "+ | ||
"tokens used against the API are bound to at least one of these audiences.") | ||
} | ||
|
||
if s.TokenFile != nil { | ||
|
@@ -303,6 +310,8 @@ func (s *BuiltInAuthenticationOptions) ToAuthenticationConfig() authenticator.Au | |
if s.ServiceAccounts != nil { | ||
ret.ServiceAccountKeyFiles = s.ServiceAccounts.KeyFiles | ||
ret.ServiceAccountLookup = s.ServiceAccounts.Lookup | ||
ret.ServiceAccountIssuer = s.ServiceAccounts.Issuer | ||
ret.ServiceAccountAPIAudiences = s.ServiceAccounts.APIAudiences | ||
} | ||
|
||
if s.TokenFile != 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.
require
len(s.Authentication.ServiceAccounts.APIAudiences) > 0
?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 above.