add support for /token subresource in serviceaccount registry #58111

mikedanese · 2018-01-10T22:07:11Z

I'm planning on implementing the registry bits (this) in one PR and followup with an authenticator that supports new id tokens.

#58790

@kubernetes/sig-auth-pr-reviews

NONE

liggitt · 2018-02-15T21:10:00Z

pkg/registry/core/rest/storage_core.go

@@ -224,6 +230,9 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
 	if legacyscheme.Registry.IsEnabledVersion(schema.GroupVersion{Group: "policy", Version: "v1beta1"}) {
 		restStorageMap["pods/eviction"] = podStorage.Eviction
 	}
+	if c.IDTokenIssuer != nil && utilfeature.DefaultFeatureGate.Enabled(features.TokenRequest) {


would prefer checking if serviceAccountStorage.Token is non-nil here (and making it be nil if you pass in a nil IDTokenIssuer to REST construction)

liggitt · 2018-02-19T17:14:05Z

pkg/kubeapiserver/options/authentication.go

@@ -228,6 +229,10 @@ func (s *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) {

 		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.IssuerID, "service-account-issuer-id", s.ServiceAccounts.IssuerID, ""+


probably just call this service-account-issuer, and apply StringOrURI validation (if contains :, require to be a valid URI, otherwise any value is allowed)

liggitt · 2018-02-19T17:28:41Z

cmd/kube-apiserver/app/options/options.go

@@ -231,4 +233,7 @@ func (s *ServerRunOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&s.EnableAggregatorRouting, "enable-aggregator-routing", s.EnableAggregatorRouting,
 		"Turns on aggregator routing requests to endoints IP rather than cluster IP.")

+	fs.StringVar(&s.ServiceAccountSigningKeyFile, "service-account-signing-key-file", s.ServiceAccountSigningKeyFile, ""+
+		"Path to the file that contains the current private key of the service account token issuer. The issuer will sign issued ID tokens with this private key. (Ignored unless alpha TokenRequest is enabled")


do we want to ignore this unless the feature gate is on, or require the feature gate to be on to specify this (I'd prefer the latter... it's confusing to set flags that have no effect)

How do you want this to work? The feature gates are passed as flags, and at this point we haven't called flag.Parse. We could error if these are passed but the feature is not enabled. Note this is already mentioned in the flag doc.

Yeah, check at point of use against feature enablement. I think I commented there as well

liggitt · 2018-02-19T17:29:13Z

cmd/kube-apiserver/app/server.go

@@ -319,6 +322,18 @@ func CreateKubeAPIServerConfig(s *options.ServerRunOptions, nodeTunneler tunnele
 		return nil, nil, nil, nil, nil, err
 	}

+	var issuer serviceaccount.TokenGenerator
+	if s.ServiceAccountSigningKeyFile != "" && s.Authentication.ServiceAccounts.IssuerID != "" {


setting either of these without enabling TokenRequest should be an error, and when the feature gate is enabled, setting one without the other should be an error

liggitt · 2018-02-19T17:30:53Z

pkg/kubeapiserver/options/authentication.go

@@ -70,6 +70,7 @@ type PasswordFileAuthenticationOptions struct {
 type ServiceAccountAuthenticationOptions struct {
 	KeyFiles []string
 	Lookup   bool
+	IssuerID string


nit: just call this Issuer?

mikedanese · 2018-02-20T20:41:40Z

@liggitt fixed.

liggitt · 2018-02-21T20:18:55Z

test/integration/auth/svcaccttoken_test.go

+
+func TestServiceAccountTokenCreate(t *testing.T) {
+	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.TokenRequest, true)()
+	// Start the server so we know the address


might be able to drop most of this MasterHolder/custom authenticator stuff and just use:

... set feature gate masterConfig := framework.NewIntegrationTestMasterConfig() ... set signing key, issuer id master, s, closeFn := framework.RunAMaster(masterConfig) defer closeFn() c, err := clientset.NewForConfig(master.GenericAPIServer.LoopbackClientConfig)

liggitt · 2018-02-21T20:22:24Z

just the suggestion for simplifying the integration test, needs squash, lgtm
/approve

mikedanese · 2018-02-21T21:18:11Z

@liggitt done.

mikedanese · 2018-02-21T21:23:54Z

For approval:

/assign @smarterclayton

k8s-ci-robot · 2018-02-21T21:49:03Z

@mikedanese: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gke	25b05a53707fc44db088ebd424d977a538cf516b	link	`/test pull-kubernetes-e2e-gke`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

mikedanese · 2018-02-21T22:59:22Z

/retest

smarterclayton · 2018-02-21T23:05:45Z

/approve

SIG sign off

cjcullen · 2018-02-21T23:09:29Z

/lgtm

k8s-ci-robot · 2018-02-21T23:09:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~cmd/kube-apiserver/OWNERS~~ [liggitt,mikedanese,smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt,smarterclayton]
~~test/OWNERS~~ [liggitt,mikedanese,smarterclayton]

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 · 2018-02-22T06:04:54Z

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

k8s-github-robot · 2018-02-22T06:10:31Z

Automatic merge from submit-queue (batch tested with PRs 60158, 60156, 58111, 57583, 60055). If you want to cherry-pick this change to another branch, please follow the instructions here.

mikedanese self-assigned this Jan 10, 2018

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels Jan 10, 2018

mikedanese force-pushed the id-registry branch 3 times, most recently from 926c2c6 to 31e072f Compare January 17, 2018 01:09

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 17, 2018

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2018

mikedanese force-pushed the id-registry branch from 31e072f to bf0dd17 Compare February 1, 2018 23:55

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 1, 2018

mikedanese force-pushed the id-registry branch from bf0dd17 to 1fa8c7d Compare February 6, 2018 05:30

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

mikedanese force-pushed the id-registry branch from 1fa8c7d to 6313fb9 Compare February 6, 2018 05:31

k8s-github-robot removed kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels Feb 6, 2018

mikedanese force-pushed the id-registry branch 3 times, most recently from 9c2abbe to bdfe771 Compare February 6, 2018 23:23

mikedanese changed the title ~~WIP: implement registry stuff for TokenRequest~~ add support for /token subresource in serviceaccount registry Feb 6, 2018

mikedanese force-pushed the id-registry branch 2 times, most recently from 092dcb9 to 07729d4 Compare February 7, 2018 05:29

liggitt reviewed Feb 15, 2018

View reviewed changes

mikedanese force-pushed the id-registry branch from 511d960 to 443c2c4 Compare February 16, 2018 00:20

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2018

liggitt reviewed Feb 19, 2018

View reviewed changes

mikedanese force-pushed the id-registry branch from 443c2c4 to 6662692 Compare February 20, 2018 17:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2018

mikedanese added this to the v1.10 milestone Feb 21, 2018

ericchiang added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 21, 2018

liggitt reviewed Feb 21, 2018

View reviewed changes

add support for /token subresource in serviceaccount registry

8ad1c66

mikedanese force-pushed the id-registry branch from 6662692 to 8ad1c66 Compare February 21, 2018 21:17

k8s-ci-robot assigned smarterclayton Feb 21, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

k8s-github-robot merged commit 8b94ae8 into kubernetes:master Feb 22, 2018

mikedanese deleted the id-registry branch February 22, 2018 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for /token subresource in serviceaccount registry #58111

add support for /token subresource in serviceaccount registry #58111

mikedanese commented Jan 10, 2018 •

edited

liggitt Feb 15, 2018 •

edited

liggitt Feb 19, 2018 •

edited

liggitt Feb 19, 2018

mikedanese Feb 20, 2018 •

edited

liggitt Feb 20, 2018

liggitt Feb 19, 2018 •

edited

liggitt Feb 19, 2018

mikedanese commented Feb 20, 2018

liggitt Feb 21, 2018

liggitt commented Feb 21, 2018 •

edited

mikedanese commented Feb 21, 2018

mikedanese commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 •

edited

mikedanese commented Feb 21, 2018

smarterclayton commented Feb 21, 2018

cjcullen commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 22, 2018

k8s-github-robot commented Feb 22, 2018

add support for /token subresource in serviceaccount registry #58111

add support for /token subresource in serviceaccount registry #58111

Conversation

mikedanese commented Jan 10, 2018 • edited

liggitt Feb 15, 2018 • edited

Choose a reason for hiding this comment

liggitt Feb 19, 2018 • edited

Choose a reason for hiding this comment

liggitt Feb 19, 2018

Choose a reason for hiding this comment

mikedanese Feb 20, 2018 • edited

Choose a reason for hiding this comment

liggitt Feb 20, 2018

Choose a reason for hiding this comment

liggitt Feb 19, 2018 • edited

Choose a reason for hiding this comment

liggitt Feb 19, 2018

Choose a reason for hiding this comment

mikedanese commented Feb 20, 2018

liggitt Feb 21, 2018

Choose a reason for hiding this comment

liggitt commented Feb 21, 2018 • edited

mikedanese commented Feb 21, 2018

mikedanese commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 • edited

mikedanese commented Feb 21, 2018

smarterclayton commented Feb 21, 2018

cjcullen commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 22, 2018

k8s-github-robot commented Feb 22, 2018

mikedanese commented Jan 10, 2018 •

edited

liggitt Feb 15, 2018 •

edited

liggitt Feb 19, 2018 •

edited

mikedanese Feb 20, 2018 •

edited

liggitt Feb 19, 2018 •

edited

liggitt commented Feb 21, 2018 •

edited

k8s-ci-robot commented Feb 21, 2018 •

edited