AWS OIDC - DeployService: configure IAM by marcoandredinis · Pull Request #28088 · gravitational/teleport

marcoandredinis · 2023-06-20T21:59:02Z

DeployService allows the user to deploy a Database Service (other services will be available later on) in a single click.
You can read more about it here:
#27035
#25521 (comment)

The IAM set up consists of:

creating a new Role with specific Policies and set a Boundary Policy
allowing the Integration role to pass that Role when starting the ECS Service
It's quite tedious and error prone.

Instead of giving users a detailed guide on how to set this up, we will ask users to run this command on their machine or Amazon CloudShell.
They must have the following permission to be able to run the configuration command:

- iam:CreatePolicy
- iam:CreateRole
- iam:PutRolePolicy
- iam:TagPolicy
- iam:TagRole

The script does not try to be smart:

only creates the resources if they don't exist previously
does not attempt to rollback any created resources if some action fails

We can add more checks to ensure a better UX, but we decided to keep things simple and safe for now.
If you think otherwise, please let me know.

Demo:

ubuntu@ip-172-31-39-120 ~/pg [1]> teleport integration configure deployservice-iam --cluster lenix --name teleportdev --aws-region eu-west-1 --role MarcoTestRoleOIDCProvider --task-role MarcoRoleForDS
2023/06/26 16:21:51 TaskRole: Boundary Policy "MarcoRoleForDSBoundary" created.
2023/06/26 16:21:51 TaskRole: Role "MarcoRoleForDS" created with Boundary "arn:aws:iam::278576220453:policy/MarcoRoleForDSBoundary".
2023/06/26 16:21:51 TaskRole: IAM Policy "MarcoRoleForDS" added to Role "MarcoRoleForDS".
2023/06/26 16:21:51 IntegrationRole: IAM Policy "DeployService" added to Role "MarcoTestRoleOIDCProvider"

Running the script again, returns an error indicating that the policy (used as Task Role Boundary) already exists

ubuntu@ip-172-31-39-120 ~/pg> teleport integration configure deployservice-iam --cluster lenix --name teleportdev --aws-region eu-west-1 --role MarcoTestRoleOIDCProvider --task-role MarcoRoleForDS
ERROR: Policy "MarcoRoleForDSBoundary" already exists, please remove it and try again.

If the IntegrationRole does not exist:

ubuntu@ip-172-31-39-120 ~/pg [1]> teleport integration configure deployservice-iam --cluster lenix --name teleportdev --aws-region eu-west-1 --role RoleThatDoesNotExist --task-role MarcoRoleForDS
2023/06/26 16:23:55 TaskRole: Boundary Policy "MarcoRoleForDSBoundary" created.
2023/06/26 16:23:55 TaskRole: Role "MarcoRoleForDS" created with Boundary "arn:aws:iam::278576220453:policy/MarcoRoleForDSBoundary".
2023/06/26 16:23:55 TaskRole: IAM Policy "MarcoRoleForDS" added to Role "MarcoRoleForDS".
ERROR: Role "RoleThatDoesNotExist" not found.

smallinsky

First pass.

smallinsky · 2023-06-27T09:57:20Z

-	Resources SliceOrString `json:"Resource"`
+	Resources SliceOrString `json:"Resource,omitempty"`
+	// Principals is a list of principals.
+	Principals map[string]SliceOrString `json:"Principal,omitempty"`


Suggested change

Principals map[string]SliceOrString `json:"Principal,omitempty"`

Principal map[string]SliceOrString `json:"Principal,omitempty"`

I was trying to be consistent with the fields above
They use plural form but then the json tag is singular
Should we change them to singular as well? Resource and Action vs Resources and Actions

Oh, I haven't notice this. Lets ignore my comment and keep this constant.

smallinsky · 2023-06-27T10:36:01Z


 	joinOpenSSH.Flag("debug", "Enable verbose logging to stderr.").Short('d').BoolVar(&ccf.Debug)

+	integrationCmd := app.Command("integration", "Integration commands")


The integration is quite generic but what is the difference between :
-- teleprot discovery bootstrap
-- db configure bootstrap

Do we need to introduce new patter for the deployservice-iam service.

Can we just create teleport deployservice-iam bootstrap/configure commands ?

This is not (yet) discovery related.
It only installs a DatabaseService in Amazon ECS for the proxy.

We can, yeah
We might have other configuration needs when setting up the IAM requirements for other integrations and I was trying to create an abstraction/layer for them. So that they don't pollute the top level teleport commands

marcoandredinis · 2023-06-30T09:29:56Z

Friendly ping @smallinsky @gabrielcorado @greedy52 🙏

smallinsky

Left some nit comments but mostly LGTM.

smallinsky · 2023-06-30T10:13:26Z

-	Resources SliceOrString `json:"Resource"`
+	Resources SliceOrString `json:"Resource,omitempty"`
+	// Principals is a list of principals.
+	Principals map[string]SliceOrString `json:"Principal,omitempty"`


Oh, I haven't notice this. Lets ignore my comment and keep this constant.

smallinsky · 2023-06-30T10:19:58Z

+func (d awsTags) ToIAMTags() []iamTypes.Tag {
+	iamTags := make([]iamTypes.Tag, 0, len(d))
+	for k, v := range d {
+		k, v := k, v


Key and Value of d awsTags[map[string]string are "value" types. Why do we need make a second copy ?

But then we use their address because iamTypes.Tag.Key/Value require string pointers.
I just removed the second copy and the tests in tags_test.go fail because of it

i think they will fix this in newer go, eventually
https://github.com/golang/go/wiki/LoopvarExperiment.
An alternative is to use aws.String() instead of &k/&v that takes care of the copy through function call

gabrielcorado

LGTM.

gabrielcorado · 2023-06-30T13:14:09Z

 }

+// PolicyARN returns the ARN representation of an AWS IAM Policy.
+func PolicyARN(partition, accountID, policy string) string {


Nit: maybe a good place to put these functions is lib/utils/aws/aws.go. There are some similar functions, but for the IAM role and user.

awsapiutils "github.com/gravitational/teleport/api/utils/aws" awslib "github.com/gravitational/teleport/lib/cloud/aws" awslibutils "github.com/gravitational/teleport/lib/utils/aws"

🥵

gabrielcorado · 2023-06-30T13:33:11Z

+}
+
+// NewDeployServiceIAMConfigureClient creates a new DeployServiceIAMConfigureClient.
+func NewDeployServiceIAMConfigureClient(ctx context.Context, region string) (DeployServiceIAMConfigureClient, error) {


A question that might be outside this PR scope: What about using the already existing cloud clients for getting AWS clients? It already initializes the connection (I know the db configurator does not rely on it too, but there is no reason not to use it). The good thing is that AWS already supports some good options, like cross-account clients, and already has a test implementation. That would also help us get everything to use the same SDK major version.

My 2c on this, we should really move to v2. I submitted a small request in v1 a few months ago. They rejected and ask me to request it in v2. Maybe we should start a cloud.ClientsV2.

Yeah, I really wanted to use V2 here
I could create the same abstraction for V2, but that seemed to be overkill for this PR

greedy52 · 2023-06-30T14:32:45Z

+// StatementForRDSDBConnect returns a statement that allows the `rds-db:connect` for all RDS DBs.
+func StatementForRDSDBConnect() *Statement {
+	return &Statement{
+		Effect:    EffectAllow,
+		Actions:   SliceOrString{"rds-db:connect"},
+		Resources: allResources,
+	}
+}


is there plan to support other DB types of deploy service? if so, ideally can share this with the db actions listed in configurators/aws. I guess we can refactor when that days comes too =D.

We will probably have more in the future 👍

There's indeed some duplication that we should get rid off.
Let's see how the DeployService evolves and then centralize all these statements.

greedy52 · 2023-06-30T14:48:27Z

+		if trace.IsAlreadyExists(awslib.ConvertIAMv2Error(err)) {
+			return trace.AlreadyExists("Role %q already exists, please remove it and try again.", req.TaskRole)
+		}
+		return trace.Wrap(err)


should here return the converted type?

I wanted to change the message so that it includes an action to resolve the issue ("remove it").
This way, the user can fix it.

If I return the converted type, then it will be the raw AWS error message.

i meant the return trace.Wrap(err) after the already exist check (like return trace.Wrap(the-converted-error). I guess not big difference.

Oh right 🙏
Yes! changing

greedy52 · 2023-06-30T15:04:42Z

+	}
+
+	_, err = clt.PutRolePolicy(ctx, &iam.PutRolePolicyInput{
+		PolicyName:     &req.IntegrationRoleDeployServicePolicy,


Can an intergration role used for multiple task roles? if so, the policy names will collide.

We might need to change this when we have multiple Deployment Modes.
For now, there's only database-service so they should not collide.

I'll make sure to add a suffix when we add a policy for another deployment.
Or we might have this policy always having the set of required statements for the deployments we have

public-teleport-github-review-bot · 2023-06-30T18:54:43Z

@marcoandredinis See the table below for backport results.

Branch	Result
branch/v13	Failed

* auto configure deployservice iam * review pt1 * review pt2

marcoandredinis marked this pull request as draft June 20, 2023 21:59

github-actions Bot added the size/md label Jun 20, 2023

github-actions Bot requested review from r0mant and rudream June 20, 2023 21:59

marcoandredinis force-pushed the marco/integration_configure branch 7 times, most recently from 264fdef to 3c0d30d Compare June 23, 2023 17:04

marcoandredinis removed request for r0mant and rudream June 26, 2023 17:09

marcoandredinis force-pushed the marco/integration_configure branch 3 times, most recently from a57099f to 1ca17fd Compare June 26, 2023 17:19

marcoandredinis marked this pull request as ready for review June 26, 2023 17:29

github-actions Bot added the size/lg label Jun 26, 2023

github-actions Bot requested review from GavinFrazar and ravicious June 26, 2023 17:30

marcoandredinis requested a review from smallinsky June 26, 2023 17:31

kimlisa mentioned this pull request Jun 26, 2023

WebDiscover: Partially implement auto deploy database server view #27924

Merged

marcoandredinis force-pushed the marco/integration_configure branch from 1ca17fd to 4ca207d Compare June 27, 2023 07:41

smallinsky requested review from gabrielcorado and greedy52 June 27, 2023 08:14

smallinsky reviewed Jun 27, 2023

View reviewed changes

marcoandredinis requested a review from smallinsky June 27, 2023 13:11

marcoandredinis mentioned this pull request Jun 28, 2023

Script to configure IAM for the DeployService #28436

Merged

marcoandredinis force-pushed the marco/integration_configure branch from 992f3be to 79ed5c9 Compare June 29, 2023 08:12

smallinsky approved these changes Jun 30, 2023

View reviewed changes

ravicious removed their request for review June 30, 2023 13:32

gabrielcorado approved these changes Jun 30, 2023

View reviewed changes

greedy52 approved these changes Jun 30, 2023

View reviewed changes

greedy52 reviewed Jun 30, 2023

View reviewed changes

Comment thread lib/integrations/awsoidc/deployservice_iam_config.go

greedy52 reviewed Jun 30, 2023

View reviewed changes

Comment thread lib/integrations/awsoidc/deployservice_iam_config.go

marcoandredinis added 3 commits June 30, 2023 18:43

auto configure deployservice iam

ad4eaa4

review pt1

e38c9ae

review pt2

279b9c6

marcoandredinis force-pushed the marco/integration_configure branch from 79ed5c9 to 279b9c6 Compare June 30, 2023 18:16

public-teleport-github-review-bot Bot removed the request for review from GavinFrazar June 30, 2023 18:17

marcoandredinis added discover Issues related to Teleport Discover backport/branch/v13 labels Jun 30, 2023

marcoandredinis enabled auto-merge June 30, 2023 18:35

marcoandredinis added this pull request to the merge queue Jun 30, 2023

Merged via the queue into master with commit e043ac0 Jun 30, 2023

marcoandredinis deleted the marco/integration_configure branch June 30, 2023 18:53

marcoandredinis added a commit that referenced this pull request Jul 3, 2023

AWS OIDC - DeployService: configure IAM (#28088)

f15cdb8

* auto configure deployservice iam * review pt1 * review pt2

marcoandredinis mentioned this pull request Jul 3, 2023

[v13] AWS OIDC - DeployService: configure IAM (#28088) #28597

Merged

github-merge-queue Bot pushed a commit that referenced this pull request Jul 3, 2023

AWS OIDC - DeployService: configure IAM (#28088) (#28597)

1088ba1

* auto configure deployservice iam * review pt1 * review pt2

r0mant mentioned this pull request Jul 14, 2023

Release 13.2.2 #29158

Closed

webvictim mentioned this pull request Oct 13, 2023

Add documentation for Teleport ECS/Fargate db_service installation via Teleport Discover #33477

Closed

	Principals map[string]SliceOrString `json:"Principal,omitempty"`
	Principal map[string]SliceOrString `json:"Principal,omitempty"`


		joinOpenSSH.Flag("debug", "Enable verbose logging to stderr.").Short('d').BoolVar(&ccf.Debug)

		integrationCmd := app.Command("integration", "Integration commands")

Conversation

marcoandredinis commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smallinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoandredinis commented Jun 30, 2023

Uh oh!

smallinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greedy52 Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielcorado left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoandredinis Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Jun 30, 2023

marcoandredinis commented Jun 20, 2023 •

edited

Loading

greedy52 Jun 30, 2023 •

edited

Loading

marcoandredinis Jun 30, 2023 •

edited

Loading