Skip to content

support cross-account AWS db access#23680

Merged
GavinFrazar merged 2 commits intomasterfrom
gavinfrazar/support-cross-account-aws-db-access
Apr 8, 2023
Merged

support cross-account AWS db access#23680
GavinFrazar merged 2 commits intomasterfrom
gavinfrazar/support-cross-account-aws-db-access

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

This PR is a follow-up for #23158 as part of the larger issue: #21872

The database agent will now assume a configured AWS role for a database when connecting to it, fetching metadata, or setting up IAM permissions.

I split out the implementation for database discovery separately for a follow-up PR; this PR is just for individual db config.

Note: assume_role_arn should be in the same account as the configured database, so that when the db agent assumes that role it will be able to access the database.

Testing

I have tested that this works with RDS and Aurora (MySQL and Postgres), Keyspaces, DynamoDB, Redshift Serverless.

I have not tested: MemoryDB, ElastiCache, RDS Proxy, or Redshift. Working on doing those next, but it's time consuming to setup all these different databases.

Special Cases

Three protocols that use IAM roles for --db-user are: AWS DynamoDB, Keyspaces, and Redshift Serverless. These protocols will assume the configured role (assume_role_arn) before assuming the role from --db-user. If there is no assume_role_arn in the database config, it will attempt to assume the --db-user role directly (prior behavior), except that now all 3 protocols will use external_id, if it's available. So technically, these protocols don't need a configured assume_role_arn, but we should advise customers to use that, as I explain below:

Cross-account AWS requires a bit more setup than assuming a role within the same account, namely:

  • within one account, a role's trust policy is enough to grant access to another principal, but assuming an external account role requires that the trust policy allows it AND the IAM identity assuming the role has sts:AssumeRole permission allowed for the role it will assume.
  • it is strongly recommended security practice to use an external ID when allowing an external account to assume a role.

For these reasons, in the doc guide update PR I will advise customers to use a configured assume_role_arn and external_id to access an external account, which can then be used to assume other roles within that account for DynamoDB, Keyspaces, or Redshift Serverless (from --db-user). This is a simpler setup than configuring cross-account access for multiple roles; instead we only need to configure one role for cross-account access.

Example setup

Example permissions attached to an identity in AWS account "111111111111":

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": "sts:AssumeRole",
            "Resource": "arn:aws:iam::222222222222:role/some-role"
        }
    ]
}

Example trust policy for "some-role" in AWS account "222222222222":

        {
            "Sid": "ExternalAccess",
            "Effect": "Allow",
            "Principal": {
                "AWS": [
                    "arn:aws:iam::111111111111:root",
                ]
            },
            "Action": "sts:AssumeRole",
            "Condition": {
                "StringEquals": {
                    "sts:ExternalId": "someUniqueID"
                }
            }
        }

Example configuration for RDS postgres for a database agent running with credentials in account "111111111111":

  databases:
    - name: "postgres-rds"
      protocol: "postgres"
      uri: "postgres-rds.abcdef123456.us-west-1.rds.amazonaws.com:5432"
      aws:
        region: "us-west-1"
        assume_role_arn: "arn:aws:iam::222222222222:role/some-role"
        external_id: "someUniqueID"
        rds:
          instance_id: "postgres-rds"

Comment thread lib/srv/db/cloud/iam.go Outdated
Comment on lines +252 to +254
// Use full identity string as the semaphore name, since two roles
// may have the same name in different AWS accounts.
SemaphoreName: configurator.cfg.identity.String(),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smallinsky the comment explains why I did this, but I just want to make sure this is ok

@greedy52
Copy link
Copy Markdown
Contributor

greedy52 commented Mar 28, 2023

I'll test out MemoryDB, ElastiCache, and RDS Proxy later this week when reviewing the PR.

I should be able to use an assume_role_arn with the same account ID as the host session right? It's a cool use case of this feature that databases can be sharded to different roles in the same account (for better organization of permissions, bypassing max policy size, etc.). I will test it out this way for my testing.

@GavinFrazar
Copy link
Copy Markdown
Contributor Author

@greedy52 Yes you can use any assume role arn, even the same account as the agent identity.

And thank you for testing those other protocols!

Copy link
Copy Markdown
Contributor

@mdwn mdwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a code perspective this looks good to me. Surprisingly straightforward given the size. :-) But I haven't actually gone and tested any of this myself -- I just wanted to make sure that if you're waiting for code reviews you're not blocked on me.

Comment thread lib/srv/db/cloud/iam.go Outdated
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first pass. good job tracking a millions places that need to be updated. I've tested elasticache/memorydb/rdsproxy with auto discovery and they worked out of the box. Really love this feature that now I can have separates roles for holding permissions for different database types.

Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/srv/db/common/auth.go Outdated
Comment thread lib/cloud/clients.go Outdated
@greedy52
Copy link
Copy Markdown
Contributor

greedy52 commented Apr 2, 2023

may want to use the account from assume role arn for db user matcher here.

teleport/lib/services/role.go

Lines 2178 to 2179 in f238c1b

func makeAlternativeNamesForAWSRole(db types.Database, user string) []string {
metadata := db.GetAWS()

@GavinFrazar GavinFrazar force-pushed the gavinfrazar/support-cross-account-aws-db-access branch from 998d3e2 to 1df39cc Compare April 4, 2023 02:04
@GavinFrazar
Copy link
Copy Markdown
Contributor Author

GavinFrazar commented Apr 4, 2023

I decided to refactor this and it's still WIP to fix tests, but should be ready again tomorrow. Implementation itself should be working for all protocols still, although the discovery PR stacked on this one will have to be updated as well.

TODO:

  • update/fix tests after the cloudclients refactor ✅
  • hex encode semaphore name per Mike's suggestion ✅
  • use account from assume role arn for db user matcher per Steve's suggestion
  • update discovery PR stacked on this one to use the new cloudclients interface as well

Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
Comment thread lib/cloud/clients.go Outdated
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/support-cross-account-aws-db-access branch from 0023a9c to b62d5dd Compare April 4, 2023 21:36
@GavinFrazar
Copy link
Copy Markdown
Contributor Author

may want to use the account from assume role arn for db user matcher here.

teleport/lib/services/role.go

Lines 2178 to 2179 in f238c1b

func makeAlternativeNamesForAWSRole(db types.Database, user string) []string {
metadata := db.GetAWS()

On closer look I don't think this needs to be changed. Database AccountID must match the account ID for the assumed role. If there is no assumed role, then the account ID must not be blank.

DynamoDB and Cassandra both return error from CheckAndSetDefaults if account ID is empty, Redshift Serverless checks for empty account ID and sets it from the parsed endpoint uri, and ValidateDatabase verifies that the account ID matches assume_role_arn account ID for all databases.

And if account ID is blank, servicecfg.Database.CheckAndSetDefaults will set the account ID by parsing assume_role_arn.

@GavinFrazar GavinFrazar force-pushed the gavinfrazar/support-cross-account-aws-db-access branch from b62d5dd to b9ee156 Compare April 4, 2023 23:06
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

FYI: #24214

Comment thread lib/cloud/clients.go Outdated
Comment thread lib/kube/proxy/cluster_details.go Outdated
@GavinFrazar GavinFrazar removed this pull request from the merge queue due to a manual request Apr 7, 2023
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/support-cross-account-aws-db-access branch from 6fa8821 to 7e631b1 Compare April 7, 2023 17:26
@GavinFrazar GavinFrazar enabled auto-merge April 7, 2023 17:26
* update aws cloud clients, engines, metadata
* base64 encode semaphor
* update tests for refactored cloud clients
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/support-cross-account-aws-db-access branch from 7e631b1 to 9a34bc7 Compare April 7, 2023 17:57
@GavinFrazar GavinFrazar added this pull request to the merge queue Apr 7, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 7, 2023
@GavinFrazar GavinFrazar enabled auto-merge April 7, 2023 23:49
@GavinFrazar GavinFrazar added this pull request to the merge queue Apr 8, 2023
Merged via the queue into master with commit a190a2e Apr 8, 2023
@GavinFrazar GavinFrazar deleted the gavinfrazar/support-cross-account-aws-db-access branch April 8, 2023 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

database-access Database access related issues and PRs kubernetes-access size/lg

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants