Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AWS OIDC: List Deployed Database Services - implementation #49331

Merged
merged 2 commits into from
Dec 18, 2024
Merged

AWS OIDC: List Deployed Database Services - implementation #49331

merged 2 commits into from
Dec 18, 2024

Conversation

marcoandredinis
Copy link
Contributor

This PR implements the List Deployed Database Services.

This will be used to let the user know which deployed database services were deployed during the Enroll New Resource / RDS flows.

@marcoandredinis marcoandredinis added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 backport/branch/v17 labels Nov 21, 2024
@marcoandredinis marcoandredinis mentioned this pull request Nov 21, 2024
Open
@github-actions github-actions bot requested review from nklaassen and tcsc November 21, 2024 18:05
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices branch from 6dc7b65 to d5e82f0 Compare November 22, 2024 15:38
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 46744e1 to f131a9f Compare November 22, 2024 15:38
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices branch from d5e82f0 to c20b1dd Compare November 25, 2024 10:32
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from f131a9f to 696d356 Compare November 25, 2024 10:33
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices branch from c20b1dd to 7db738c Compare November 28, 2024 13:54
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 696d356 to b7237e5 Compare November 28, 2024 13:54
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices branch from 7db738c to 4e7bb9c Compare November 28, 2024 17:40
Base automatically changed from marco/awsoidc-listdatabaseservices to master November 28, 2024 18:18
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from b7237e5 to 6cd29f8 Compare November 28, 2024 18:31
@marcoandredinis
Copy link
Contributor Author

@nklaassen @tcsc Can you please take a look when you get a chance?

@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 6cd29f8 to 557241b Compare December 2, 2024 10:59
@marcoandredinis
Copy link
Contributor Author

@nklaassen @tcsc Friendly ping :)

@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch 2 times, most recently from aef905d to c4f7b14 Compare December 9, 2024 11:44
nklaassen
nklaassen reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

sorry marco, don't know how I missed this for so long, feel free to nag me on slack if I'm not reviewing your PRs

lib/auth/integration/integrationv1/awsoidc.go Outdated Show resolved Hide resolved
lib/auth/integration/integrationv1/awsoidc.go
return nil, trace.Wrap(err)
}

if err := authCtx.CheckAccessToKind(types.KindIntegration, types.VerbUse); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have an authz check on kind db_service or anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion on that.
We don't require any other authz check for deploying.
We have been requiring only integration.use check for those methods, but we can easily add more.
If we add it here, then I think we should add it to the deployment call.

The most sensitive part of this listing is the teleport.yaml configuration, but even that only has the IAM Token Name, which we don't consider to be a secret.

I'm in favor of keeping it as is, but let me know what you think.

lib/integrations/awsoidc/deployservice.go Outdated
Comment on lines 456 to 464
func serviceDashboardURL(region, clusterName, serviceName string) string {
return fmt.Sprintf("https://%s.console.aws.amazon.com/ecs/v2/clusters/%s/services/%s", region, clusterName, serviceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make 100% sure that region, clusterName, and serviceName are validated before building a URL like this to avoid SSRF? Maybe they already are but it's not extremely clear to me where these are all coming from

Copy link
Contributor Author

@marcoandredinis marcoandredinis Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This happens in two places:

  • after deploying a service
  • when creating the list of already deployed services

So, it's always after those values were used to call the AWS API.
I'm pretty confident that invalid values do not reach this method given its current usage.

Anyway, I'll add checks ensuring that the host part is valid (region) so that future usage of this method is also not prone to SSRF.

@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from c4f7b14 to 6da2178 Compare December 10, 2024 10:44
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 6da2178 to 0bfa0b8 Compare December 10, 2024 11:35
@marcoandredinis
Copy link
Contributor Author

@tcsc @nklaassen Can you please take another look?

@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 0bfa0b8 to 5634a35 Compare December 16, 2024 10:06
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 5634a35 to 1916b75 Compare December 17, 2024 09:44
@marcoandredinis
AWS OIDC: List Deployed Database Services - implementation
31ccf3c 
This PR implements the List Deployed Database Services.

This will be used to let the user know which deployed database services
were deployed during the Enroll New Resource / RDS flows.
@marcoandredinis
validate region for dashboard url
087cbbd
@marcoandredinis marcoandredinis force-pushed the marco/awsoidc-listdatabaseservices-impl branch from 1916b75 to 087cbbd Compare December 17, 2024 15:05
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from tcsc December 17, 2024 20:02
@marcoandredinis marcoandredinis added this pull request to the merge queue Dec 18, 2024
Merged via the queue into master with commit 475753f Dec 18, 2024
41 checks passed
@marcoandredinis marcoandredinis deleted the marco/awsoidc-listdatabaseservices-impl branch December 18, 2024 09:35
@public-teleport-github-review-bot
Copy link

@marcoandredinis See the table below for backport results.

Branch Result
branch/v17 Create PR

@marcoandredinis marcoandredinis mentioned this pull request Dec 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GavinFrazar GavinFrazar GavinFrazar approved these changes

@nklaassen nklaassen nklaassen approved these changes

Assignees
No one assigned
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/lg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcoandredinis @nklaassen @GavinFrazar