Add configuration for cross-account AWS database-access#23158
Add configuration for cross-account AWS database-access#23158GavinFrazar merged 7 commits intomasterfrom
Conversation
GavinFrazar
added
tctl
GavinFrazar
force-pushed
the
gavinfrazar/improve-aws-utils-and-database-validation
branch
from
abbc5ca to
834bc0b
Compare
GavinFrazar
force-pushed
the
gavinfrazar/support-cross-account-aws-discovery-config
branch
from
5299b24 to
eb0f6c6
Compare
* log a warning when an invalid AWS IAM role ARN is filtered out
* add tests for nested role arn path and remove superfluous test parallelism
* add extra godoc info for BuildRoleARN
* improve mock IAM error messages
- Failing tests' error messages were not specific enough to know what
happened.
* refactor code to call GetAWS() once
- Avoids constantly fetching a full copy of the AWS metadata.
- Reduces verbosity of code.
* reuse MetadataFromRedshiftCluster in meta fetcher
- Avoids repeating the same logic inside the fetcher.
* improve database-access DynamoDB config checking
- Don't early return from database CheckAndSetDefaults so AWS account ID
is checked properly, and any other future checks added will apply to
DynamoDB as well.
* add aws utils for parsing AWS IAM roles
- ParseRoleARN will parse an AWS ARN and verify that it is a valid
AWS IAM Role resource
- CheckARNPartitionAndAccount is a helper that verifies an ARN partition
and account ID matches the caller's expectations.
* use cmp.Diff instead of require.Equal in db tests
- require.Equal spams a lot of protobuf noise.
- cmp.Diff yields a precise, readable diff on test failure.
GavinFrazar
force-pushed
the
gavinfrazar/improve-aws-utils-and-database-validation
branch
from
90126a8 to
0b75973
Compare
GavinFrazar
force-pushed
the
gavinfrazar/support-cross-account-aws-discovery-config
branch
from
eb0f6c6 to
0a629bc
Compare
Co-authored-by: STeve (Xin) Huang <xin.huang@goteleport.com>
* external_id does not require assume_role_arn for DynamoDB, Keyspaces, or Redshift Serverless * keyspaces gives a hint that URI could not be derived because region was also empty * make some error messages more consistent with each other regarding missing/empty fields.
| // If AWS account ID is missing, but assume role ARN is given, | ||
| // try to parse the role arn and set the account id to match. | ||
| if d.AWS.AccountID == "" && d.AWS.AssumeRoleARN != "" { | ||
| parsed, err := awsutils.ParseRoleARN(d.AWS.AssumeRoleARN) | ||
| if err != nil { | ||
| return trace.BadParameter("database %q invalid AWS assume_role_arn: %v", | ||
| d.Name, err) | ||
| } | ||
| d.AWS.AccountID = parsed.AccountID | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: Not a problem started by this change but ideally we should do this in api.Database's CheckAndSetDefaults so that databases created by tctl and webapi can benefit from it. (maybe the same for the TLS and AD CheckAndSetDefaults above, if we can move them to api if possible)
@smallinsky @GavinFrazar do you think if worth copying the AWS arn implementation into our api lib?
https://github.com/aws/aws-sdk-go/blob/v1.44.224/aws/arn/arn.go
It's not that much code. And if we have our own implementation, we can even enhance it with more functions we need.
There was a problem hiding this comment.
👍 Yeah I put it here to avoid pulling AWS sdk into our api. I have the same check in ValidateDatabase, but it just errors for empty AccountID because we don't set defaults in that function. Would definitely be a better UX for tctl and webapi if we can move these all into the api CheckAndSetDefaults.
There was a problem hiding this comment.
do you think if worth copying the AWS arn implementation into our api lib?
https://github.com/aws/aws-sdk-go/blob/v1.44.224/aws/arn/arn.go
Yea we can do that but why not just embed the aws.ARN into own custom type and add additional logic ?
There was a problem hiding this comment.
Yea we can do that but why not just embed the aws.ARN into own custom type and add additional logic ?
I think we hesitate whether to bring the whole AWS SDK dependency to api's go.mod.
Another alternative is to get aws-sdk-go-v2 without other services.
There was a problem hiding this comment.
I think I want to just pull in aws-sdk-go-v2 since the stripped binary size change is tiny and v2 doesn't download the whole sdk when fetching dependencies, but I should move this code in another PR to get feedback from other people first. It enables only a small UX improvement for tctl and web api. I can move the TLS and AD stuff too in that PR.
public-teleport-github-review-bot
Bot
removed request for
EdwardDowling and
espadolini
GavinFrazar
deleted the
gavinfrazar/support-cross-account-aws-discovery-config
branch
4 participants
I am splitting up PRs for the implementation of #21872 to reduce PR size.
This PR adds configuration for AWS
assume_role_arnin database config and discovery matchers.It does not add the logic that actually uses this new configuration - that will be added in PRs to follow.
This new config is available in:
teleport db configure createteleport db start--aws-external-idwas already a flag and--aws-assume-role-arnnaturally pairs with that flag.tctl create/editteleportmerge base is set to the branch for #23157 which has various changes I split out for backporting and will update to master once that branch merges.