Revision to IAM Policies created by Kops #3343

KashifSaadat · 2017-09-05T16:47:55Z

Based off of the work done by @chrislovecnm in PR #2497.

This PR tightens down the IAM policies created for Master & Node instance groups. The Cluster Spec IAMSpec.Legacy flag is used to control application of stricter policy rules, which is defaulted to true for existing clusters (to limit potential regression impact), and false for new cluster creation.

KashifSaadat · 2017-09-05T16:51:27Z

Ping @gambol99 @chrislovecnm
/assign @justinsb

This will need some thorough review to make sure I've not accidentally dropped any permissions for the legacy behaviour (I think I've captured it all), also maybe some SID renaming and refactoring to make it more readable.

I've tested on a couple local clusters (creation, updates, rolling-updates, both with and without flag) and it seems fine so far. Maybe the Jenkins E2E test will pick something up!

KashifSaadat · 2017-09-05T17:22:04Z

Not sure why the E2E tests failed, there's no log output. Will kick off again.

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-09-06T01:02:17Z

E2E wth kops is not happy currently. Known issue that @justinsb is workin on

KashifSaadat · 2017-09-06T15:48:23Z

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-09-06T22:05:57Z

This is a biggy. Need to set aside some time to review. The conversation between @justinsb and I was to initially get this under a feature flag, or do you think the API settings that I think you are using are sufficient?

chrislovecnm · 2017-09-06T22:06:03Z

/assign

KashifSaadat · 2017-09-07T07:47:32Z

Hey @chrislovecnm! Yeah this will need some careful review, it's easy for a permission to slip by and potentially break something.

I think when we initially discussed about this back in PR #3210 the plan was to have any future policy revisions wrapped around the Cluster Spec IAM Legacy flag, so I've tried to re-use that (it was already in use for the S3 and EC2 permissions). Do you think a different flag should be used instead?

chrislovecnm · 2017-09-10T19:32:31Z

@KashifSaadat I think we can use your API element just fine :)

chrislovecnm

Great start. This is going to need a lot of testing.

ELB
EBS
KMS
S3

What else does k-c-m play with?

chrislovecnm · 2017-09-10T19:34:10Z

pkg/model/iam/iam_builder.go

@@ -14,6 +14,13 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

+// TODO: We have a couple different code paths until with do lifecycles, and


How are these TODOs updated??

I was a little unsure about the reference to using lifecycles and refactor of s3 code, so kept your comment as-is from the previous PR. I'll update the second TODO as we're now doing some more restrictive behaviour with use of resource paths and condition keys (but still room for improvement).

chrislovecnm · 2017-09-10T19:35:02Z

pkg/model/iam/iam_builder.go

@@ -48,6 +55,7 @@ func (p *IAMPolicy) AsJSON() (string, error) {
 }

 type IAMStatementEffect string
+type IAMSid string


Sounds good. All of the types / constants are being picked up by golint for stuttering (iam.IAMSid etc) so I'll clean it up.

chrislovecnm · 2017-09-10T19:35:55Z

pkg/model/iam/iam_builder.go

+	case kops.InstanceGroupRoleBastion:
+		p, err = b.BuildAWSIAMPolicyBastion()
+		if err != nil {
+			return nil, err


fmt please.

Whoops, I've done that in a few places. Will update.

chrislovecnm · 2017-09-10T19:37:25Z

pkg/model/iam/iam_builder.go

+	HostedZoneID          string
+	S3Bucket              string
+	ResourceARN           *string
+	CreateECRPerms        bool


How are we wiring this in? What is setting this?

I pulled this in from #2497 but I don't believe anything is actually making use of it yet (aside from me, setting KMSKeys in the unit tests). Shall I take the extra fields out for now?

Please remove

I've cleaned up the unused fields, leaving in KMSKeys, ResourceARN & CreateECRPerms (although those aren't set elsewhere on use of the struct yet) as we try to make use of these within the builder.

chrislovecnm · 2017-09-10T19:40:56Z

pkg/model/iam/iam_builder.go

-				"ecr:DescribeRepositories",
-				"ecr:ListImages",
-				"ecr:BatchGetImage",
+				"ec2:AllocateAddress",


We need to prune these. The kops installer needs a lot of these, but the master and nodes to do not. Take a look at https://gist.github.com/chrislovecnm/0e23d11903cc36b99ccf73a013d5ae56 and https://gist.github.com/chrislovecnm/a93941b4afbbd5afe746354af0cfddcc

I'll take this function out based on the other comment below, we can raise as a separate PR and review it independently.

chrislovecnm · 2017-09-10T20:06:27Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "autoscaling:GetAsgForInstance"
+      ],
+      "Resource": [
+        "*"


Can we limit on ASGs that are tagged?

(this is read metadata for ASGs, so perhaps could have one statement with Sid "ReadAWSMetadataAllResources")

chrislovecnm · 2017-09-10T20:07:20Z

pkg/model/iam/tests/iam_builder_master_strict.json

+    {
+      "Effect": "Allow",
+      "Action": [
+        "s3:List*"


Can we tighten and remove the *?

Yup, I'll update and test (I think I can remove this completely, will validate).

chrislovecnm · 2017-09-10T20:08:19Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "kms:ReEncrypt*"
+      ],
+      "Resource": [
+        "key-id-1",


Can we test by adding a different key for a ebs volume? This may be too restricting. I am not familiar with kms perms, we need to double check if we have too many.

@faraazkhan do you know IAM kms perms?

As far as I understand this is a complete set to work with (and delegate use of ) a specific key. There are some other conditions that delegate use only to aws resources (ie to EC2 so that it can mount the volumes) but I have never got good information out of AWS for exactly what is required for a minimal set here.

chrislovecnm · 2017-09-10T20:11:01Z

pkg/model/iam/tests/iam_builder_node_strict.json

+    {
+      "Effect": "Allow",
+      "Action": [
+        "ec2:DescribeInstances"


DescribeRegion? Why do we need DescribeInstances?

This was introduced in #2497 with a comment that Protokube makes a DescribeInstances call. If it's not needed at all we can drop the Node EC2 policy completely.

Please check. We may only need for gossip.

This permission is still required, I got the below error when dropping it for compute nodes:

kubelet[8706]: error: failed to run Kubelet: could not init cloud provider "aws": error finding instance i-12345678: error listing AWS instances: UnauthorizedOperation: You are not authorized to perform this operation

chrislovecnm · 2017-09-10T20:11:50Z

pkg/model/iam/tests/iam_builder_node_strict.json

+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/cluster.spec",
+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/config",
+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/instancegroup/*",
+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/pki/issued/*",


Which pki keys do we actually need?

Given there are limits on policy sizes, I would suggest you consider this role to own the whole iam-builder-test.k8s.local/* prefix - particularly for read actions. If there are specific prefixes under here that might hold secrets that a node should not be able to read, then you could use a DENY on those, or move that to a different prefix.

Back in PR #3158 I initially trialled this with open access and then explicit DENY rules for certain resources, however it became a bit more complex to manage. The /pki/private/ path can differ on content depending on your deployment configuration and there would be more rules to add in as the nodes shouldn't have access to most of these files, hence why I switched to this method.

The open access to /pki/issued/ was also brought up in #3158, but deemed acceptable (#3158 (review)).

chrislovecnm · 2017-09-10T20:16:19Z

@KashifSaadat thanks for your patience with this PR, it is a bear. How are u testing this? We really need to figure out if we can run e2e against this so that we can get better testing coverage. Otherwise, we can install a bunch of example apps.

chrislovecnm · 2017-09-10T20:32:06Z

@KashifSaadat let me know if you want to go over these changes on a video call. This is a complicated enough PR that we may want to review on a call.

lwoggardner · 2017-09-10T22:59:13Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "ec2:DescribeVolumes"
+      ],
+      "Resource": [
+        "*"


Hi Chris, Grant Gardner here as requested. I would not worry too much about the "read metadata" permissions such as ec2:Describe* in a first pass at a stricter policy. Would concentrate on permissions that can read end user data (eg S3) or can create/update/delete AWS resources.

I have just commented on the *strict permissions json, if there is anything else you'd like me to look at please let me know.

lwoggardner · 2017-09-10T23:04:06Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "ec2:ModifyInstanceAttribute"
+      ],
+      "Resource": [
+        "*"


In some cases you can use the aws:RequestTags condition to force the creation of a resource (Volume/Security group) to contain a specific tag value (which you use in the next statement to ensure that you are only working on your own resources)
and then move the Delete* actions into the "with tagged resources" below.
with Create/ModifyTags you can use a ForAllValues:StringEquals condition on Aws::TagKeys to limit which tags this role allows you to play with (ie only the tags associated with Kubernetes)

lwoggardner · 2017-09-10T23:05:32Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "autoscaling:GetAsgForInstance"
+      ],
+      "Resource": [
+        "*"


(this is read metadata for ASGs, so perhaps could have one statement with Sid "ReadAWSMetadataAllResources")

lwoggardner · 2017-09-10T23:07:38Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "autoscaling:UpdateAutoScalingGroup"
+      ],
+      "Resource": [
+        "*"


AutoScaling now has resource level permissions so instead of tags you also have the option of using a prefix on your AutoScalingGroup and LaunchConfiguration names.

I like that idea

That would be good to add in. Would you be able to point me in the right direction of retrieving the autoscaling & launch configuration names and account id to populate the resources list?

lwoggardner · 2017-09-10T23:12:22Z

pkg/model/iam/tests/iam_builder_master_strict.json

+        "kms:ReEncrypt*"
+      ],
+      "Resource": [
+        "key-id-1",


As far as I understand this is a complete set to work with (and delegate use of ) a specific key. There are some other conditions that delegate use only to aws resources (ie to EC2 so that it can mount the volumes) but I have never got good information out of AWS for exactly what is required for a minimal set here.

lwoggardner · 2017-09-10T23:15:00Z

pkg/model/iam/tests/iam_builder_node_strict.json

+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/cluster.spec",
+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/config",
+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/instancegroup/*",
+        "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/pki/issued/*",


Given there are limits on policy sizes, I would suggest you consider this role to own the whole iam-builder-test.k8s.local/* prefix - particularly for read actions. If there are specific prefixes under here that might hold secrets that a node should not be able to read, then you could use a DENY on those, or move that to a different prefix.

k8s-github-robot · 2017-09-11T00:51:15Z

@KashifSaadat PR needs rebase

KashifSaadat · 2017-09-11T07:35:07Z

Thanks for the detailed feedback guys! I'll go through it later and try to address as much as I can :)

KashifSaadat · 2017-09-11T13:55:40Z

I've made some changes based on the feedback and updated tests accordingly (hopefully the E2E will still be clean). A call would be good to discuss! Maybe something to chat about on the next Friday kops call?

KashifSaadat · 2017-09-12T16:00:37Z

/retest

KashifSaadat · 2017-09-12T16:58:11Z

@chrislovecnm do you know if there's any known issues with the E2E tests at the moment? My latest update didn't include any policy changes (just unused code cleanup) and still works locally, but getting failures in the E2E tests now. It looks like the E2E tests are missing some log files (kube-controller-manager not present).

KashifSaadat · 2017-09-13T07:46:33Z

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-09-14T01:30:44Z

Looks like it passed

chrislovecnm · 2017-09-14T21:11:29Z

@KashifSaadat how do we want to proceed with this? File some issues for the things we did not address and iterate? With these changes I assume this is opt in changes. We will not be changing the default behavior?

IAM Legacy flag.

KashifSaadat · 2017-09-15T07:08:50Z

Hi @chrislovecnm, yeah it would be good if we could get this one in and then separately pick up the remaining tasks, such as optional ASG permissions, IAM SSL certs, validation around ECR permissions.

I've aimed to keep the existing permission set / behaviour under the legacy flag, which will be enabled by default for existing clusters so users shouldn't be impacted (unless they explicitly set the flag to false in their cluster spec).

chrislovecnm · 2017-09-15T13:34:31Z

Can you open some issues on those remaining items? We will need release notes and probably more documentation ;)

/lgtm

k8s-github-robot · 2017-09-15T13:35:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-15T14:50:12Z

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

k8s-github-robot · 2017-09-15T15:32:46Z

Automatic merge from submit-queue

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 5, 2017

k8s-ci-robot assigned justinsb Sep 5, 2017

KashifSaadat force-pushed the iam-policy-revision branch from d2d669f to 89ea4d7 Compare September 6, 2017 17:05

k8s-ci-robot assigned chrislovecnm Sep 6, 2017

chrislovecnm suggested changes Sep 10, 2017

View reviewed changes

lwoggardner reviewed Sep 10, 2017

View reviewed changes

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

KashifSaadat force-pushed the iam-policy-revision branch from 89ea4d7 to 6a99c76 Compare September 11, 2017 13:52

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

KashifSaadat force-pushed the iam-policy-revision branch 2 times, most recently from 5395b84 to 8c9d1af Compare September 11, 2017 15:41

chrislovecnm mentioned this pull request Sep 11, 2017

Security improvements on IAM policies #2497

Closed

8 tasks

KashifSaadat force-pushed the iam-policy-revision branch 2 times, most recently from 409ee5f to 7d99013 Compare September 12, 2017 15:04

Revision to IAM Policies created by Kops, and wrapped in Cluster Spec

2e6b7ee

IAM Legacy flag.

KashifSaadat force-pushed the iam-policy-revision branch from 7d99013 to 2e6b7ee Compare September 15, 2017 07:06

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2017

k8s-github-robot merged commit 52fe2ce into kubernetes:master Sep 15, 2017

KashifSaadat mentioned this pull request Sep 15, 2017

AWS IAM policy cleanup for Master & Node Instance Groups #3386

Open

kerin mentioned this pull request Sep 18, 2017

Add kubernetes cluster Terraform module, moving all cluster creation and almost all cluster management tasks into Terraform ministryofjustice/analytics-platform-ops#108

Merged

Revision to IAM Policies created by Kops #3343

Revision to IAM Policies created by Kops #3343

Conversation

KashifSaadat commented Sep 5, 2017

KashifSaadat commented Sep 5, 2017

KashifSaadat commented Sep 5, 2017

chrislovecnm commented Sep 6, 2017

KashifSaadat commented Sep 6, 2017

chrislovecnm commented Sep 6, 2017

chrislovecnm commented Sep 6, 2017

KashifSaadat commented Sep 7, 2017

chrislovecnm commented Sep 10, 2017 • edited

chrislovecnm 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KashifSaadat Sep 11, 2017 • edited

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

KashifSaadat Sep 12, 2017 • edited

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

chrislovecnm commented Sep 10, 2017

chrislovecnm commented Sep 10, 2017

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

Choose a reason for hiding this comment

k8s-github-robot commented Sep 11, 2017

KashifSaadat commented Sep 11, 2017

KashifSaadat commented Sep 11, 2017

KashifSaadat commented Sep 12, 2017 • edited

KashifSaadat commented Sep 12, 2017 • edited

KashifSaadat commented Sep 13, 2017

chrislovecnm commented Sep 14, 2017

chrislovecnm commented Sep 14, 2017

KashifSaadat commented Sep 15, 2017 • edited

chrislovecnm commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

chrislovecnm commented Sep 10, 2017 •

edited

KashifSaadat Sep 11, 2017 •

edited

KashifSaadat Sep 12, 2017 •

edited

KashifSaadat commented Sep 12, 2017 •

edited

KashifSaadat commented Sep 12, 2017 •

edited

KashifSaadat commented Sep 15, 2017 •

edited