Simplified form of IAM Roles for ServiceAccounts #9352

justinsb · 2020-06-12T18:42:34Z

We create a featureflag "PublicJWKS" which runs in a simpler
configuration, where we expose the JWKS directly from the apiserver.

This has some limitations:

Only works when we have a public ELB
Requires enabling anonymous-auth on the apiserver

But it is a simpler configuration, and likely we can pare down the
code and get this under e2e before tackling some of the more
complicated scenarios.

Edit:

To try this out (but note that this isn't really a production config yet because of anonymous auth):

export KOPS_FEATURE_FLAGS="UseServiceAccountIAM,PublicJWKS"
kops create cluster foo.example.com --zones us-east-2a --api-loadbalancer-type public

k8s-ci-robot · 2020-06-12T18:42:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rifelpet · 2020-06-15T15:08:59Z

So we think this will be the PR that we'll land first between this, #9341 and #9271

Based on this comment: #9271 (comment) the remaining items for this PR are:

Remove the API changes. pkg/model/awsmodel/oidc_provider.go will only run if the feature flag is enabled. Also remove the creation of the jwks fitasks.ManagedFiles.
Add validation if the feature flag is enabled:
- cluster cannot use gossip, must use public DNS
- must have an internet-accessible api load balancer
- must not use an ACM certificate on the load balancer ( i think this is required? or we'd have to reference the ACM CA for thumbprints. I'm not sure we'd want to handle that complexity for initial support of this feature)
- we currently hardcode anonymous auth to enabled but maybe we go a step further and make sure the cluster spec isn't explicitly disabling anonymous auth just to be clear on what the final behavior is.
- there may be additional requirement's we'll need to check here...
Hardcode issuer URL in AWS OIDC Provider to use the cluster's api domain name and cluster's CA thumbprint
Ensure unauthenticated requests can reach the apiserver's jwks endpoints
Update pkg/resources/aws/aws.go to look for OIDC Providers with issuer URLs that match the cluster's api domain name

I'm sure theres stuff I'm missing but hopefully that makes sense so far.

justinsb · 2020-09-03T16:52:39Z

/hold cancel

johngmyers · 2020-09-03T23:03:05Z

pkg/model/components/addonmanifests/dnscontroller/remap.go

+		}
+
+		if !kubernetesRangesIntersect(versionRange, semver.MustParseRange(">= 1.19.0")) {
+			// Skip; this is an older manifest


This logic needs to be in sync with the iam_builder logic that decides whether to give the privileges to the service account or node

I think this is OK; this is saying "only try to patch the latest versions of the manifests". There is a deeper issue though, which is how to enable this on an existing cluster ... I talk about that in the top level comment.

If the Kubernetes version is 1.11 but the feature flag is on, then iam_builder will withhold the permissions from the node role, yet this won't inject the projected volume. So dns-controller won't have the permissions.

Right - there is "caveat emptor" with feature flags, I see you've got it with your suggested change though so I'm going to incorporate that!

This check can probably be removed now since context.UseServiceAccountIAM() will be false for the older manifest.

johngmyers · 2020-09-03T23:12:32Z

pkg/model/iam/iam_builder.go

 		return
 	}

 	// Legacy IAM permissions for node roles
-	if b.Cluster.Spec.IAM.Legacy {
+	if b.Cluster.Spec.IAM.Legacy && b.Role.IsNodeRole() {


No longer need this change. Legacy now implies all roles are node roles.

johngmyers · 2020-09-03T23:14:12Z

pkg/model/iam/pod_roles.go

+}
+
+// ServiceAccountForServiceAccountRole returns the kubernetes service account used by pods with the specified role
+func ServiceAccountForServiceAccountRole(serviceAccountRole ServiceAccountRole) types.NamespacedName {


Should this be a receiver?

johngmyers · 2020-09-03T23:29:33Z

pkg/model/iam/pod_roles.go

+)
+
+// PodOrNodeRole is used to specify generation of permissions for either a pod or a node
+type PodOrNodeRole struct {


I was thinking the relevant buildAWSIAMRolePolicy() logic could possibly be pushed under an interface method. And a ServiceAccount() method would return the name of any service account.

Basically I'm applying the principle that code that does type assertions (or tests for existence of union fields) suggests that the branches under the test might be better as an interface method. But if it doesn't work, it doesn't work.

justinsb · 2020-09-08T02:56:24Z

Thanks for explaining the interface comment @johngmyers - I implemented that and it seems to work really well. I think we could go even further with this reorganization, but there's likely a lot of moving around of code that would make this PR unreadably large again.

Your first comment (about keeping the logic in sync) gets at a deeper point - we don't have a great way to enable IRSA on an existing cluster right now. I suspect it'll actually work out, but it's possible we could hit a circular dependency while trying to publish the jwks document.

I think the answer will likely be:

The production flow will involve kops CLI publishing the JWKS document to an S3 bucket (i.e. no dependency on apiserver running nor on kops-controller having permissions to publish to an S3 bucket)
We'll therefore be able to enable IRSA and use it in one step; we might have some pods that temporarily don't have valid credentials but it should be a matter of a few seconds until everything converges.
When we graduate the feature-flag we'll have to include kubernetes-versions selection for whatever version we enable it on.

I don't particularly want to tackle that in this PR though - my goal here is to get test coverage on IRSA so we can get the feature enabled in upstream kubernetes (and demonstrate that kops is a good place to build test coverage of new functionality!).

johngmyers · 2020-09-08T04:50:17Z

If the plan is to condition IRSA support on the manifest version, then different service accounts are likely to have different Kubernetes-version cutoffs.

Now when we drop support for k8s 1.11 in kops 1.20 a lot of variant manifests will drop away. But there are a few CNIs that vary over 1.16 and spotinst varies over 1.14.

pkg/model/context.go

justinsb · 2020-09-08T14:42:04Z

Merged your suggested change @johngmyers and then passed down UseServiceAccountIAM to avoid duplicating the logic. I guess in some version (e.g. 1.21) we'll enable a "real" flag/field for enabling this; my hope is that we won't have to tweak the manifests at that stage, but if we need to we can cut a new version of the manifests to address any incompatibilities.

I've also been thinking more about your point about functions/templates etc... there's actually a ton of overlap with the idea of operators and their ability to do transforms nicely, but the challenge of the opacity of the resulting manifest. I don't really have a fully formed answer yet, but there's something there I think...

justinsb · 2020-09-08T19:00:40Z

(Had to add another commit to workaround AWS IAM vs Golang JSON restrictions on the generated IAM policy)

johngmyers

It looks like there's only minor stuff left. I think it's really close.

There are some parameters of type iam.Subject that are named role; perhaps they should be named subject.

johngmyers · 2020-09-09T03:18:46Z

pkg/model/components/addonmanifests/dnscontroller/remap.go

+	}
+	container := &containers[0]
+
+	if context.UseServiceAccountIAM() {


Cannot be false here.

Thanks - removed!

johngmyers · 2020-09-09T03:33:43Z

pkg/model/iam/iam_builder.go

+
+// MarshalJSON formats the IAM statement for the AWS IAM restrictions.
+// For example, `Resource: []` is not allowed, but golang would force us to use pointers.
+func (s *Statement) MarshalJSON() ([]byte, error) {


We might want unit tests for this.

Good point - we had a unit test and added test cases to cover the new cases (e.g. no Resources).

johngmyers · 2020-09-09T03:34:47Z

pkg/model/iam/pod_roles.go

 )

+// Subject represents an IAM identity, to which permissions are granted.
+// It is implemented by NodeRole objects and per-ServiceAccount objects.
+type Subject interface {


Perhaps rename this file subject.go

Good idea - done!

pkg/model/iam/pod_roles.go

k8s-ci-robot · 2020-09-09T13:31:21Z

@justinsb: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kops-e2e-cni-weave	`f5149ec`	link	`/test pull-kops-e2e-cni-weave`
pull-kops-e2e-cni-flannel	`f5149ec`	link	`/test pull-kops-e2e-cni-flannel`
pull-kops-e2e-cni-kuberouter	`f5149ec`	link	`/test pull-kops-e2e-cni-kuberouter`

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.

This should be much easier to start and to get under testing; it only works with a load balancer, it sets the apiserver into anonymous-auth allowed, it grants the anonymous auth user permission to read our jwks tokens. But it shouldn't need a second bucket or anything of that nature. Co-authored-by: John Gardiner Myers <[email protected]>

This avoids the hacky search through the list of tasks.

Hat-tip to johngmyers for the idea!

Co-authored-by: John Gardiner Myers <[email protected]>

AWS IAM is very strict and doesn't support `Resource: []` for example. We implement a custom MarshalJSON method to work around that.

johngmyers · 2020-09-10T04:40:41Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 12, 2020

k8s-ci-robot requested review from drekle and johngmyers June 12, 2020 18:42

justinsb force-pushed the irsa_with_public branch from f6f0218 to e23a81b Compare June 13, 2020 18:27

justinsb mentioned this pull request Jun 13, 2020

[WIP] Add API fields for AWS OIDC Provider #9271

Closed

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

rifelpet mentioned this pull request Jun 16, 2020

Add support for AWS OIDC Provider #9375

Merged

justinsb force-pushed the irsa_with_public branch from e23a81b to c15c8d0 Compare June 17, 2020 17:05

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

justinsb force-pushed the irsa_with_public branch from c15c8d0 to f5149ec Compare June 17, 2020 17:14

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

justinsb force-pushed the irsa_with_public branch from f5149ec to 8680bf5 Compare August 15, 2020 20:11

k8s-ci-robot added area/addons area/provider/alicloud Issues or PRs related to alicloud provider and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 15, 2020

justinsb force-pushed the irsa_with_public branch 5 times, most recently from e4fb028 to 2242ab0 Compare August 16, 2020 01:09

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2020

justinsb force-pushed the irsa_with_public branch from 06b88e8 to 206a40b Compare September 3, 2020 16:30

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2020

johngmyers reviewed Sep 3, 2020

View reviewed changes

johngmyers reviewed Sep 8, 2020

View reviewed changes

pkg/model/context.go Outdated Show resolved Hide resolved

justinsb force-pushed the irsa_with_public branch from e4b8af6 to 85529e5 Compare September 8, 2020 14:39

johngmyers reviewed Sep 9, 2020

View reviewed changes

justinsb force-pushed the irsa_with_public branch 2 times, most recently from c878d85 to 56526ca Compare September 9, 2020 13:01

justinsb and others added 6 commits September 9, 2020 09:57

Create tests for JWKS scenarios

ccc814d

IAM Policy: rely on stub resolution/unification

f05980f

This avoids the hacky search through the list of tasks.

Refactor to use interface for iam Subjects

a61ecf4

Hat-tip to johngmyers for the idea!

Add version logic to UseServiceAccountIAM

d8895c5

Co-authored-by: John Gardiner Myers <[email protected]>

JSON formatting of IAM: Workaround for optional fields

6fa8be2

AWS IAM is very strict and doesn't support `Resource: []` for example. We implement a custom MarshalJSON method to work around that.

justinsb force-pushed the irsa_with_public branch from 56526ca to 6fa8be2 Compare September 9, 2020 13:57

k8s-ci-robot assigned johngmyers Sep 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2020

k8s-ci-robot merged commit 036ea69 into kubernetes:master Sep 10, 2020

k8s-ci-robot added this to the v1.19 milestone Sep 10, 2020

mtaufen mentioned this pull request Sep 10, 2020

Graduate ServiceAccountIssuerDiscovery to beta kubernetes/kubernetes#91921

Merged

olemarkus mentioned this pull request Dec 19, 2020

AWS CSI driver #10467

Merged

olemarkus mentioned this pull request Feb 7, 2021

Instance roles for service accounts (IRSA) contd #10756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified form of IAM Roles for ServiceAccounts #9352

Simplified form of IAM Roles for ServiceAccounts #9352

justinsb commented Jun 12, 2020 •

edited

Loading

k8s-ci-robot commented Jun 12, 2020

rifelpet commented Jun 15, 2020 •

edited

Loading

justinsb commented Sep 3, 2020

johngmyers Sep 3, 2020

justinsb Sep 8, 2020

johngmyers Sep 8, 2020

justinsb Sep 8, 2020

johngmyers Sep 9, 2020 •

edited

Loading

johngmyers Sep 3, 2020

johngmyers Sep 3, 2020

justinsb Sep 8, 2020

johngmyers Sep 3, 2020 •

edited

Loading

justinsb commented Sep 8, 2020

johngmyers commented Sep 8, 2020

justinsb commented Sep 8, 2020

justinsb commented Sep 8, 2020

johngmyers left a comment

johngmyers Sep 9, 2020

justinsb Sep 9, 2020

johngmyers Sep 9, 2020

justinsb Sep 9, 2020

johngmyers Sep 9, 2020

justinsb Sep 9, 2020

k8s-ci-robot commented Sep 9, 2020 •

edited

Loading

johngmyers commented Sep 10, 2020

Simplified form of IAM Roles for ServiceAccounts #9352

Simplified form of IAM Roles for ServiceAccounts #9352

Conversation

justinsb commented Jun 12, 2020 • edited Loading

k8s-ci-robot commented Jun 12, 2020

rifelpet commented Jun 15, 2020 • edited Loading

justinsb commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

justinsb commented Sep 8, 2020

johngmyers commented Sep 8, 2020

justinsb commented Sep 8, 2020

justinsb commented Sep 8, 2020

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 9, 2020 • edited Loading

johngmyers commented Sep 10, 2020

justinsb commented Jun 12, 2020 •

edited

Loading

rifelpet commented Jun 15, 2020 •

edited

Loading

johngmyers Sep 9, 2020 •

edited

Loading

johngmyers Sep 3, 2020 •

edited

Loading

k8s-ci-robot commented Sep 9, 2020 •

edited

Loading