Security improvements on IAM policies #2497

chrislovecnm · 2017-05-05T00:50:57Z

This PR continues the work on #1873 for an end user to have the capability to create specific policies that will be used by kops.

Capability

Custom IAM policies can be a complicated, and maintaining those policies create technical debt. To automate the maintenance of these policies, kops can now create its own fine grain IAM policies.

The current IAM policies have been given a much shorter hair cut. Permissions such as ec2:* and s3:* no longer exist. Also, the nodes had Route53 permissions, which they should not have. The node permissions are now very limited.

TODO

Testing

I have tested the following different installs with 1.6.3.

EBS PV and PVC creation
ELB - used the go guestbook
Encrypted volumes
HA public kubenet
HA private weave
Single master public kubenet

I have not tested ECR, but I have not changed those permissions. I have not tested the new IAM permissions for working with SSL certs and ELBs, as I do not have an SSL cert with AWS.

E2E is happy as well.

This change is

chrislovecnm · 2017-05-11T04:50:28Z

@justinsb please take a look.

As we discussed I have hidden the command till we have life cycles in, once we have lifecycles in I can refactor that code just a bit, and we will have the auth lifecycle. We need GCE as well :)

chrislovecnm · 2017-05-11T04:51:43Z

The one tweak that I may want to make is prune down ECR permission to get the GetAuthToken. Let me know.

mstump · 2017-05-18T15:23:49Z

upup/pkg/fi/cloudup/awsup/aws_cloud.go

@@ -148,6 +148,7 @@ func NewAWSCloud(region string, tags map[string]string) (AWSCloud, error) {
 		c := &awsCloudImplementation{region: region}

 		config := aws.NewConfig().WithRegion(region)
+		//config := aws.NewConfig().WithRegion(region).WithLogLevel(aws.LogDebugWithHTTPBody)


I am going to add it to documentation in the go file, and file an issue to be able to turn on debugging. I will submit another PR.

mstump

+1

chrislovecnm · 2017-05-18T20:54:44Z

As @justinsb recommended, I removed the cmd portion of this PR, to limit this PR to just IAM role changes. Once we have lifecycles implemented, we will have a command for generating example security roles.

justinsb · 2017-05-20T19:55:35Z

pkg/model/iam/iam_builder.go

@@ -91,65 +109,30 @@ func (b *IAMPolicyBuilder) BuildAWSIAMPolicy() (*IAMPolicy, error) {
 	if b.Role == api.InstanceGroupRoleBastion {
 		p.Statement = append(p.Statement, &IAMStatement{
 			// We grant a trivial (?) permission (DescribeRegions), because empty policies are not allowed
+			Sid:      "kopsK8sBastion",


Not sure about this. We know it's the bastion policy. What is our goal in adding sid to our policies?

Yes, it was recommended by one of our users to add a side to all of our policies.

Recommended using the sid so you know the intent as to what each permission is for. Some of these will be obvious, such as the bastion.
If a user looks at the iam policy from the aws console it will help allow people to highlight individual fragments that may need to be modified in future

justinsb · 2017-05-20T19:57:48Z

pkg/model/iam/iam_builder.go


-	if b.HostedZoneID != "" {
-		addRoute53Permissions(p, b.HostedZoneID)
+		if b.HostedZoneID != "" {


This is a behaviour change, right? We are no longer adding route53 permissions to the nodes? That should be called out - we've broken people before with this!

Yes this is a breaking change. A lot of these changes can be breaking changes. Should we have old and new policies?

I think we're going to have to. We should probably prioritize the kube2iam work.

justinsb · 2017-05-20T19:59:16Z

pkg/model/iam/iam_builder.go

-	hostedZoneID = strings.TrimPrefix(hostedZoneID, "/")
-	hostedZoneID = strings.TrimPrefix(hostedZoneID, "hostedzone/")
+// BuildAWSIAMPolicyNode generates a custom policy for a Kubernetes master.
+func (b *IAMPolicyBuilder) BuildAWSIAMPolicyMaster() (*IAMPolicy, error) {


Duplicate code?

justinsb · 2017-05-20T20:02:19Z

pkg/model/iam/iam_builder.go

+	// We shouldn't be running lots of pods on the master, but it is perfectly reasonable to run
+	// a private logging pod or similar.
+	// At this point we allow all regions with ECR, since ECR is region specific.
+	p.Statement = append(p.Statement, &IAMStatement{


Moving these around makes them tricky to review. But if we don't end up having the duplicate function, we might not need to move them (or we can move them in a set of small PRs)

I refactorred

justinsb · 2017-05-20T20:04:12Z

So some good stuff here, but why did we duplicate the IAM generation function? That seems to then require moving everything into a function, which makes this PR much bigger than it otherwise needs to be

chrislovecnm · 2017-05-20T20:35:58Z

So the newer functions are due to how I refactored for the cli tool that use to be in this PR. I can refactor with just one method.

chrislovecnm · 2017-05-22T05:04:37Z

Everything is back to one method. Please review :)

Vince-Cercury · 2017-06-29T05:49:46Z

I'm after the minimum policies to get my security department to review. Thanks for your code.

Is that a comprehensive minimum required list?

Node:

addECRPermissions
addNodeEC2Policies

Master:

addMasterEC2Policies
addMasterELBPolicies
addMasterASPolicies
addCertIAMPolicies

I'm going to make a cloud formation out of that. Would be good to have that documented on wiki to get security folks comfortable with kub and kops

chrislovecnm · 2017-06-29T20:41:01Z

@Vincemd #1873 includes links to gists I have does that are the full policies. I am missing a policy for autoscaler in this PR. We have another PR for using a customer IAM role, and I am guessing that would be very helpful.

chrislovecnm · 2017-07-12T18:22:33Z

@justinsb what about we add these changes behind a feature flag? That way we don't introduce breaking changes, but allow for better security?

k8s-github-robot · 2017-07-26T20:30:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

Associated issue: 1103

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-08-12T00:01:12Z

@chrislovecnm PR needs rebase

chrislovecnm · 2017-08-18T19:17:43Z

@KashifSaadat thoughts on this PR? I am wondering if we get this in using an API value, or a feature flag.

Not going to rebase since @KashifSaadat has a PR inbound as well around IAM.

KashifSaadat · 2017-08-18T20:33:10Z

This is excellent! Must have taken a while to work through all the permissions.

It's quite a significant change to the policy documents so there is some risk in odd behaviour / edge case scenarios occurring. As @justinsb suggested in PR #3186, we could wrap these changes around an API flag to get it in earlier and have some more user testing around it (we're keen to use this ASAP). I've added a strict IAM flag in PR #3210 which could be used.

I think for API methods that create resources, it would be good to enforce condition keys (where they are supported). For example, specifying ec2:RequestTag so that new resources must be created with a specific tag to associate it with the cluster. I guess we'd need to check where these are called in kubernetes and ensure that tags are added to the resources within the same API call?

arnisoph · 2017-09-04T17:52:12Z

We're right in the process of testing a provided AWS account in a project for our customer, so I'm really looking forward to see this in master. :)

I'm going to do a local rebase/merge anyway since it's kinda time-critical. :D

chrislovecnm · 2017-09-04T20:44:56Z

You many be interested in this #2440 as well. @bechtoldt

chrislovecnm · 2017-09-04T20:46:41Z

A quick update on this PR. I need to refactor it into a feature flag in order to have this merged. If anyone else wants to fork my branch please feel free

KashifSaadat · 2017-09-05T15:43:13Z

I'll fork off of this and refactor, will raise a PR shortly.

bgeesaman · 2017-09-11T14:06:20Z

pkg/model/iam/iam_builder.go

-			Resource: wildcard,
-		})
-	}
+	addECRPermissions(p)


Should this be:

if b.CreateECRPerms { addECRPermissions(p) }

chrislovecnm · 2017-09-11T17:42:26Z

Closing since we have #3343

@chrislovecnm

Automatic merge from submit-queue Revision to IAM Policies created by Kops 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.

chrislovecnm added the WIP: Work In Progress label May 5, 2017

chrislovecnm self-assigned this May 5, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2017

chrislovecnm changed the title ~~Kops now can create specific IAM policies~~ kops now can create specific IAM policies May 5, 2017

chrislovecnm force-pushed the iam-tooling branch 3 times, most recently from c1fdb44 to fb610cd Compare May 5, 2017 19:11

chrislovecnm mentioned this pull request May 5, 2017

Allow the re-use of a IAM role that is not created by kops. #2440

Closed

5 tasks

chrislovecnm force-pushed the iam-tooling branch 4 times, most recently from 38740a0 to cce6f2e Compare May 11, 2017 04:47

chrislovecnm assigned justinsb May 11, 2017

chrislovecnm added needs-review and removed WIP: Work In Progress labels May 11, 2017

chrislovecnm force-pushed the iam-tooling branch 2 times, most recently from 8f6114f to 3bf2778 Compare May 17, 2017 17:10

mstump reviewed May 18, 2017

View reviewed changes

mstump approved these changes May 18, 2017

View reviewed changes

chrislovecnm force-pushed the iam-tooling branch from 3bf2778 to 3bfd8e0 Compare May 18, 2017 20:48

chrislovecnm changed the title ~~kops now can create specific IAM policies~~ Security improvements on IAM policies May 18, 2017

chrislovecnm force-pushed the iam-tooling branch from 3bfd8e0 to c6d1e38 Compare May 18, 2017 20:57

justinsb reviewed May 20, 2017

View reviewed changes

chrislovecnm mentioned this pull request May 20, 2017

Route53 mapper should add listHostedZones for master node #2427

Closed

chrislovecnm force-pushed the iam-tooling branch from c6d1e38 to 2061ae7 Compare May 22, 2017 04:10

chrislovecnm force-pushed the iam-tooling branch from 2061ae7 to 9c442f6 Compare May 22, 2017 04:46

chrislovecnm force-pushed the iam-tooling branch from 9c442f6 to 13a7a4e Compare May 22, 2017 05:36

chrislovecnm closed this May 22, 2017

chrislovecnm reopened this May 22, 2017

chrislovecnm force-pushed the iam-tooling branch from 13a7a4e to c6d1e38 Compare May 30, 2017 05:13

Kops now can create specific IAM policies.

4516ff8

chrislovecnm force-pushed the iam-tooling branch from c6d1e38 to 4516ff8 Compare July 12, 2017 19:09

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

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

KashifSaadat mentioned this pull request Sep 5, 2017

Revision to IAM Policies created by Kops #3343

Merged

bgeesaman reviewed Sep 11, 2017

View reviewed changes

chrislovecnm closed this Sep 11, 2017

chrislovecnm deleted the iam-tooling branch December 30, 2017 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security improvements on IAM policies #2497

Security improvements on IAM policies #2497

chrislovecnm commented May 5, 2017 •

edited

Loading

chrislovecnm commented May 11, 2017

chrislovecnm commented May 11, 2017

mstump May 18, 2017

chrislovecnm May 18, 2017

mstump left a comment

chrislovecnm commented May 18, 2017

justinsb May 20, 2017

chrislovecnm May 22, 2017

ajohnstone May 22, 2017

justinsb May 20, 2017

chrislovecnm May 20, 2017

justinsb Jun 9, 2017

justinsb May 20, 2017

justinsb May 20, 2017

chrislovecnm May 30, 2017

justinsb commented May 20, 2017

chrislovecnm commented May 20, 2017

chrislovecnm commented May 22, 2017

Vince-Cercury commented Jun 29, 2017 •

edited

Loading

chrislovecnm commented Jun 29, 2017

chrislovecnm commented Jul 12, 2017

k8s-github-robot commented Jul 26, 2017

k8s-github-robot commented Aug 12, 2017

chrislovecnm commented Aug 18, 2017

KashifSaadat commented Aug 18, 2017

arnisoph commented Sep 4, 2017

chrislovecnm commented Sep 4, 2017 •

edited

Loading

chrislovecnm commented Sep 4, 2017

KashifSaadat commented Sep 5, 2017

bgeesaman Sep 11, 2017

chrislovecnm commented Sep 11, 2017

Security improvements on IAM policies #2497

Security improvements on IAM policies #2497

Conversation

chrislovecnm commented May 5, 2017 • edited Loading

Capability

TODO

Testing

chrislovecnm commented May 11, 2017

chrislovecnm commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstump left a comment

Choose a reason for hiding this comment

chrislovecnm commented May 18, 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

Choose a reason for hiding this comment

justinsb commented May 20, 2017

chrislovecnm commented May 20, 2017

chrislovecnm commented May 22, 2017

Vince-Cercury commented Jun 29, 2017 • edited Loading

chrislovecnm commented Jun 29, 2017

chrislovecnm commented Jul 12, 2017

k8s-github-robot commented Jul 26, 2017

k8s-github-robot commented Aug 12, 2017

chrislovecnm commented Aug 18, 2017

KashifSaadat commented Aug 18, 2017

arnisoph commented Sep 4, 2017

chrislovecnm commented Sep 4, 2017 • edited Loading

chrislovecnm commented Sep 4, 2017

KashifSaadat commented Sep 5, 2017

Choose a reason for hiding this comment

chrislovecnm commented Sep 11, 2017

chrislovecnm commented May 5, 2017 •

edited

Loading

Vince-Cercury commented Jun 29, 2017 •

edited

Loading

chrislovecnm commented Sep 4, 2017 •

edited

Loading