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.

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

RDS's 'BuildAuthToken' should validate that the port is present in the database endpoint string #1771

Closed
gaultier opened this issue Jul 20, 2022 · 4 comments · Fixed by #1837
Assignees
Labels
feature-request A feature should be added or improved.

Comments

@gaultier
Copy link

gaultier commented Jul 20, 2022

Describe the bug

When generating a RDS database token to connect to the database over IAM, e.g on the CLI:
aws rds generate-db-auth-token --hostname <host> -user <user> --port 3306 --region <region>,
the port is required.

When leaving it out, there is an error: aws: error: the following arguments are required: --port.

However, the Go sdk v2 does not perform such validation. Since the host and the port are passed in one string (the endpoint argument) in the function BuildAuthToken, e.g. foo.cluster-bar.eu-central-1.rds.amazonaws.com:3306, it is easy to forget to pass the port at the end of this string.

This leads to this function generating an invalid token, which will in turn lead to an error from mysql:
Error 1045: Access denied for user '<user>'@'<ip>' (using password: YES)

The docs in the Go sdk v2 do state that the port is required but it's an easy mistake to make which will lead to hours of troubleshooting. Especially when the endpoint is not defined in code but in a per-environment configuration.

Expected Behavior

Suggestion: validate that the endpoint parameter contains the port at the end and return an error otherwise.

Current Behavior

No validation occurs and a wrong token is generated (without an error being returned). This token will be rejected by mysql when trying to connect with it.

Reproduction Steps

Run:

cfg, _ := config.LoadDefaultConfig(context.Background())

token, err := auth.BuildAuthToken(context.Background(), "foo.cluster-bar.eu-central-1.rds.amazonaws.com", "eu-central-1", "baz", cfg.Credentials)
if err != nil {
  panic(err)
}

And see that a token (which is silently invalid) and no error is returned.

Possible Solution

Suggestion: validate that the endpoint parameter is contains the port at the end and return an error otherwise, like the CLI does.

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

	github.com/aws/aws-sdk-go-v2 v1.16.3
	github.com/aws/aws-sdk-go-v2/config v1.11.1
	github.com/aws/aws-sdk-go-v2/feature/rds/auth v1.1.1

Compiler and Version used

go1.18.4 darwin/amd64

Operating System and version

Darwin Kernel Version 21.5.0 x86_64

@gaultier gaultier added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 20, 2022
@RanVaknin RanVaknin self-assigned this Sep 7, 2022
@RanVaknin RanVaknin added investigating This issue is being investigated and/or work is in progress to resolve the issue. feature-request A feature should be added or improved. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 8, 2022
@RanVaknin
Copy link
Contributor

Hi @gaultier,

Thanks for opening this issue. I can recognize that this is not an optimal user experience, however since the behavior is documented, I would not consider it a bug.

I have converted this into a feature-request and will discuss with the team to see if we can fit it in our backlog.

Many thanks,
Ran~

@RanVaknin RanVaknin added pending-release This issue will be fixed by an approved PR that hasn't been released yet. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Sep 9, 2022
@RanVaknin RanVaknin linked a pull request Sep 11, 2022 that will close this issue
@RanVaknin
Copy link
Contributor

Hi @gaultier,

I have added port validation so you'll get an error if a port is missing or isn't a valid number.
Feature should be available in our next release, hopefully in the coming week.

Thanks again for engaging in the community and helping us make the SDK better!

All the best,
Ran~

@RanVaknin RanVaknin removed the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Sep 12, 2022
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@gaultier
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants