Cross account access feature #58

sesh-kebab · 2018-04-25T22:53:49Z

What does this pull request do?

Allows other aws accounts to access repositories in the ECR that the service is backed by. This allows for other aws account that has access, to pull images using the full ECR repo name rather than having the request forwarded through d.ims.io.

PR exposes a new account entity and allows you to list, add and delete aws account ids via the rest api. Additionally, when you add an account id, all the repositories in ECR have their policies updated to allow access from that account. The same applies for when you delete an account, all the repositories' policies are updated, to remove access for the deleted aws account id.

How should this be tested?

🔴 WARNING Do not use us-west-2 region when running the service locally. That region's ECR is backing our prod d.ims.io instance. So please make use of the --aws-region flag when running d.ims.io locally.

Test:

/accounts api endpoint GET, POST, DELETE
ensure POST and DELETE update the repository policy document. For example, adding account 123, should result in a policy document for all repositories with that account permission
note Sid and Principal properties below. They should match the account id added.
Adding 123 won't actually work as you will need a valid aws account id of sufficient length

{
    "Version": "2008-10-17",
    "Statement": [
        {
            "Sid": "123",
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::123:root"
            },
            "Action": [
                "ecr:GetDownloadUrlForLayer",
                "ecr:BatchGetImage",
                "ecr:BatchCheckLayerAvailability"
            ]
        }
}

Other notes:
This work is in part to primarily address the private auth issue quintilesims/layer0#540.

Checklist

Unit tests
Terraform

closes: #57

- new accounts entity. can add/remove aws account ids. - creating new repos will now add permissions for all accounts.

- when accounts are added - when accounts are deleted

zpatrick · 2018-04-26T19:16:47Z

auth/dynamo_account_manager.go

+
+	response := make([]string, len(output.Items))
+	i := 0
+	for _, v := range output.Items {


The first arg in range is the index!

for i, v := range output.Items { response[i] = aws.StringValue(v["AccountID"].S) }

hah yes it is - not sure what happened there 🤷‍♂️
✅

zpatrick · 2018-04-26T19:19:55Z

auth/dynamo_account_manager_test.go

+			t.Errorf("Column 'AccountID' was '%v', expected '%v'", v, want)
+		}
+
+		if input.Item["AccountID"].S == nil {


This seems redundant after the if v, want := aws.StringValue(input.Item["AccountID"].S) check above?

zpatrick · 2018-04-26T19:20:28Z

auth/dynamo_account_manager_test.go

+	target := NewDynamoAccountManager("table", mockDynamoDB)
+
+	validatePutItemInput := func(input *dynamodb.PutItemInput) {
+		if v, want := aws.StringValue(input.TableName), "table"; v != want {


[nit] you can use assert.Equal(t, expected, actual) for these guys

zpatrick · 2018-04-26T19:24:11Z

auth/dynamo_account_manager.go

+}
+
+func (d *DynamoAccountManager) RevokeAccess(accountID string) error {
+	key := map[string]*dynamodb.AttributeValue{


I think this variable name should be item for consistency and because it seems appropriate with:

input.SetItem(item)

zpatrick · 2018-04-26T19:38:42Z

auth/dynamo_account_manager_test.go

+	}
+
+	mockDynamoDB.EXPECT().
+		PutItem(gomock.Any()).


We typically reserve the mock.Do pattern when we only want to validate a subset of the input or if gomock can't handle checking equality correctly (typically, time.Time objects)

With that in mind, I think this would work just fine:

item := map[string]*dynamodb.AttributeValue{ "AccountID": { S: aws.String("account-id"), }, } input := &dynamodb.DeleteItemInput{} input.SetTableName("table") input.SetItem(item) mockDynamoDB.EXPECT(). PutItem(input). Return(&dynamodb.PutItemOutput{}, nil)

zpatrick · 2018-04-26T20:07:39Z

controllers/common.go

+		}
+	}
+
+	if getPolicyOutput.PolicyText != nil && aws.StringValue(getPolicyOutput.PolicyText) != "" {


I think we can make this more readable as well:

policy := models.PolicyDocument{} if text := aws.StringValue(output.PolicyText); text != "" { if err := json.Unmarshal([]byte(text), &policy); err != nil { return nil, err } } return policy, nil

zpatrick · 2018-04-26T20:09:57Z

controllers/common.go

+	return policyDoc, nil
+}
+
+func setRepositoryPolicy(e ecriface.ECRAPI, repositoryName, policyText string) error {


From above - Seems like just re-purposing this function a bit would allow us to remove most of the other helpers here. Looks like the callers only really ever need set since they grab all of the accounts prior to this call.

I had a look at maximizing re-use but I'm not seeing a clear path to expand this function to re-purpose it. The problem is that there are 3 cases that require slightly different logic before setting the policy which seem better encapsulated in their own function:

add all accounts to a single new repo's policy

add single account to all repos' policies

remove single account from all repos' policies
✅

zpatrick · 2018-04-26T20:10:59Z

controllers/repository.go

@@ -84,6 +87,15 @@ func (r *RepositoryController) CreateRepository(c *fireball.Context) (fireball.R
 		return nil, err
 	}

+	accounts, err := r.account.Accounts()


I'm not sure why this is here?

We iterate over all the accounts to create a repository policy text every time a new repository is created. In hindsight though, this should probably be in addToRepositoryPolicy.

Right, I'm just not sure why this needs to happen during a repository creation though? As far as I can tell, no changes to the account table happen during this API call, so this seems like it wouldn't ever change the policy?

Not sure I follow. Are you suggesting that I cache a list of accounts so that I don't have to get the list of accounts from the dynamo db table?

Oh I see what you mean - the policy is for each repository. So every time you create a new repo, the policy needs to be explicitly set for that repository (which by default is empty).

Ooooh I see - I was thinking this was a single policy. That's why I was confused by the efficiency of using setPolicy above.

zpatrick · 2018-04-26T20:15:28Z

models/policy_document.go

+	Statement []statement
+}
+
+type statement struct {


does aws-sdk-go have a struct for this? nothing wrong with this implementation; just more curious if anything.
Maybe if we're lucky aws-sdk-go has some nice helpers around this stuff?

policy := aws.PolicyDoc{} policy.Statements.AddAction(...)

I did a quick search in the aws-sdk-go repo on github but couldn't find such a struct. https://github.com/aws/aws-sdk-go/search?p=1&q=PolicyDocument&type=
✅

zpatrick · 2018-04-26T20:17:03Z

models/policy_document.go

+	p.Statement = temp
+}
+
+func (p *PolicyDocument) RenderPolicyText() string {


hmm - the error swallowing is a little concerning here. Maybe we remove this helper function and allow the caller to properly handle errors?

yea I wasn't quite sure what to do here either. In theory though this error should never be triggered as we're not trying to serialize a non-serializable type.

zpatrick · 2018-04-27T23:11:25Z

auth/dynamo_account_manager.go

+	response := make([]string, len(output.Items))
+	for i, v := range output.Items {
+		response[i] = aws.StringValue(v["AccountID"].S)
+		i++


this needs to be removed since i is already updated as part of range

zpatrick · 2018-04-27T23:14:28Z

controllers/account.go

+	}
+
+	if err := request.Validate(); err != nil {
+		return nil, fireball.NewError(400, err, nil)


With the current implementation you are returning a non-json error in a json-formatted api. I would be pretty unsatisfied as a consumer of this API with this implementation so I think it does need to change.

If you wanted to do the least amount of work, I'd expect you to do something like:

if err := foo(); err != nil { return fireball.NewJSONError(400, err) }

See the Layer0 controllers package to see how to unit test responses from fireball.

zpatrick · 2018-04-27T23:17:11Z

controllers/account.go

+	}
+
+	accounts = append(accounts, request.Account)
+	for _, r := range repositories {


Efficient in the sense that this (and RevokeAccess) turn into:

accounts = append(accounts, request.Account) if err := setRepositoryPolicy(a.erc, r, accounts); err != nil { return nil, err }

zpatrick · 2018-04-27T23:17:50Z

controllers/account_test.go

+		Times(2)
+
+	mockAccountManager.EXPECT().
+		GrantAccess(gomock.Any()).


this still needs addressed

zpatrick · 2018-04-27T23:18:07Z

auth/dynamo_account_manager_test.go

+	}
+
+	expected := []string{"1", "2", "3"}
+	assert.Equal(t, expected, result)


this still needs addressed

zpatrick · 2018-04-27T23:23:00Z

controllers/common.go

+		}
+	}
+
+	if text := aws.StringValue(output.PolicyText); text != "" {


I think it's best practice in Go to declare variables as close to when they are used as possible - but I can say for sure that's the pattern we've been using in this codebase so I think for consistency policy should be declared right above the if text := ... line.

zpatrick · 2018-04-28T16:14:22Z

controllers/common.go

+	input.SetPolicyText(policyText)
+	input.SetRepositoryName(repositoryName)
+	if err := input.Validate(); err != nil {
+		return fireball.NewError(400, err, nil)


Let's leave fireball errors up to the controllers/handlers.

zpatrick · 2018-04-28T16:15:46Z

controllers/common.go

+	}
+
+	statementCount := len(policyDoc.Statement)
+	policyDoc.RemoveAWSAccountPrincipal(accountID)


[nit] another pattern I tend to see for this type of logic is to return a bool here to indicate if something actually changed.

zpatrick · 2018-04-28T16:17:32Z

controllers/repository.go

@@ -84,6 +87,15 @@ func (r *RepositoryController) CreateRepository(c *fireball.Context) (fireball.R
 		return nil, err
 	}

+	accounts, err := r.account.Accounts()


Ooooh I see - I was thinking this was a single policy. That's why I was confused by the efficiency of using setPolicy above.

zpatrick · 2018-04-28T16:19:31Z

controllers/account.go

+	}
+
+	accounts = append(accounts, request.Account)
+	for _, r := range repositories {


ah, ok, I didn't realize there was one policy per repo; I was thinking there was only one policy total

zpatrick · 2018-05-01T16:15:05Z

controllers/repository_test.go

@@ -29,6 +30,36 @@ func TestCreateRepository(t *testing.T) {
 		Do(validateCreateRepositoryInput).
 		Return(&ecr.CreateRepositoryOutput{}, nil)

+	validateGetRepositoryPolicyInput := func(input *ecr.GetRepositoryPolicyInput) {


[nit] ditto - I'm not sure we why aren't using this pattern:

gomock.EXPECT(). GetRepositoryPolicy(input). Return(...)

Not sure? I also thought it was odd.

zpatrick · 2018-05-01T16:17:37Z

controllers/repository_test.go

@@ -16,7 +16,8 @@ func TestCreateRepository(t *testing.T) {
 	defer ctrl.Finish()

 	mockECR := mock.NewMockECRAPI(ctrl)
-	controller := NewRepositoryController(mockECR)
+	mockAccountManager := mock.NewMockAccountManager(ctrl)
+	controller := NewRepositoryController(mockECR, mockAccountManager)

 	validateCreateRepositoryInput := func(input *ecr.CreateRepositoryInput) {


[nit] While we're here - can we change this to be consistent with the other gomock input patterns?

mockECR.EXPECT(). CreateRepository(input). Return(...)

zpatrick · 2018-05-01T16:26:34Z

models/policy_document.go

+	return result
+}
+
+func (p *PolicyDocument) RenderPolicyText() string {


I know it's not a huge deal as-is, but I'm more worried about some dev seeing this in the future, thinking this is an acceptable pattern, and then using it somewhere where it actually does become an issue.
I think it would be wise to either:

Change the return to string, error

Remove this function and leave marshalling up to the caller

Also, just in case you didn't already know - you can customize how objects get marshaled with json.Marshal by implementing the Marshaler interface.

I don't think this is a necessary or even a very useful option in this context, but figured it was a nice thing worth knowing.

* CreateRepository: Return error if 'name' field contains '/' * Spec out 'image' endpoints; ListImages * Add DeleteImage * Add GetImage * '/image' endpoint tests * remove swagger host from configuration * change image model. return list of strings for get repo images * 429 and throttling work * add zpatrick's 'go-bytesize' to vendor dir * improve backoff logic * test Auth0Manager retries on 429s * code review fixes * add auth0 request logging * Empty user pass fix (#54) * do not attempt to authenticate with an empty user or pass * added test case for authenticating with empty user and pass * Cross account access feature (#58) * Private auth support - new accounts entity. can add/remove aws account ids. - creating new repos will now add permissions for all accounts. * Added code to update policies for all repositories: - when accounts are added - when accounts are deleted * add migrator tool * fix repo uri bug * Update migrate.py * Update migrate.py * Update router.go * Update variables.tf * Update main.tf * make debug mode configurable * add if else logic to template * use dep for vendoring * add log decorator * remove unused registry endpoint config value * add full debug authentication output * change order of decorator * update cache dep * fix swagger doc delete tag * remove 401 response code from auth unit test * remove comment

Chemudugunta, Seshi added 8 commits April 23, 2018 14:43

Private auth support

f5d8be7

- new accounts entity. can add/remove aws account ids. - creating new repos will now add permissions for all accounts.

Added and updated unit tests.

7f7dcbe

Added code to update policies for all repositories:

ace5749

- when accounts are added - when accounts are deleted

Updated 'Access*' -> 'Account*'

c013842

Updated unit tests to cover the default code path.

456fb51

Render empty policy if the no statements exist in the policy document.

f532675

Added unit tests to test controller validation logic.

4a33fe1

Updated accounts table to 'd.ims.io.accounts' to d.ims.io.account'

f110dda

sesh-kebab added the enhancement label Apr 25, 2018

sesh-kebab requested a review from zpatrick April 25, 2018 22:53

zpatrick suggested changes Apr 26, 2018

View reviewed changes

Code review feedback.

4d30b20

zpatrick suggested changes Apr 28, 2018

View reviewed changes

additional review feedback

7eacc89

quintilesims deleted a comment from sesh-kebab May 1, 2018

zpatrick suggested changes May 1, 2018

View reviewed changes

Addressing more code review feedback.

5c5f161

zpatrick approved these changes May 2, 2018

View reviewed changes

sesh-kebab merged commit 757a913 into develop May 2, 2018

zpatrick deleted the cross-account-access-feature branch May 3, 2018 22:52

sesh-kebab mentioned this pull request May 11, 2018

Add docker repo override feature to layer0 quintilesims/layer0#618

Closed

2 tasks

Cross account access feature #58

Cross account access feature #58

Conversation

sesh-kebab commented Apr 25, 2018 • edited Loading

What does this pull request do?

How should this be tested?

Choose a reason for hiding this comment

sesh-kebab Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sesh-kebab Apr 26, 2018 • 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

Choose a reason for hiding this comment

zpatrick Apr 26, 2018 • 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

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

sesh-kebab Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sesh-kebab Apr 26, 2018 • 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

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

sesh-kebab commented Apr 25, 2018 •

edited

Loading

sesh-kebab Apr 26, 2018 •

edited

Loading

sesh-kebab Apr 26, 2018 •

edited

Loading

zpatrick Apr 26, 2018 •

edited

Loading

sesh-kebab Apr 26, 2018 •

edited

Loading

sesh-kebab Apr 26, 2018 •

edited

Loading