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

Fix AWS App Access creation for AWS OIDC Integration when using the account number as name #44480

Merged
merged 3 commits into from
Aug 26, 2024

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented Jul 19, 2024

If the integration name is digits only, the resulting address for the application will look like this:
123456789012.proxy.example.com:443
This fails to parse with go's url.Parse.

This PR keeps prepends the protocol to the URL, and that makes url.Parse happy.

changelog: Fix an issue that prevented the creation of AWS App Access for an Integration that used digits only (eg, AWS Account ID).

Copy link
Contributor

@timothyb89 timothyb89 left a comment

Choose a reason for hiding this comment

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

Approving since I think this works most of the time and doesn't require other changes which is nice.

As a possible alternative, have we looked at specifying a protocol? https:////123456789012.proxy.example.com:443 or even //123456789012.proxy.example.com:443 parse just fine and will work with arbitrary proxy ports, if we can prepend some characters just before we url.Parse() the address.

(I imagine things would break if we added a protocol to the app server's URL so presumably any fix like I'm suggesting would need to happen elsewhere if it would work at all)

@marcoandredinis marcoandredinis force-pushed the marco/fix-awsappaccess_accountid_integration branch from 7a40218 to d95593f Compare July 26, 2024 10:36
@marcoandredinis
Copy link
Contributor Author

@timothyb89 Thank you for the suggestion. I changed to use the protocol and it works.

I'm just not able to do a final test because of this issue
#44553

@marcoandredinis marcoandredinis force-pushed the marco/fix-awsappaccess_accountid_integration branch from d95593f to 909848f Compare August 13, 2024 11:41
@marcoandredinis marcoandredinis force-pushed the marco/fix-awsappaccess_accountid_integration branch from 909848f to b377196 Compare August 23, 2024 08:16
@marcoandredinis marcoandredinis changed the title Fix AWS App Access creation for AWS OIDC Integration Fix AWS App Access creation for AWS OIDC Integration when using the account number as name Aug 23, 2024
@marcoandredinis marcoandredinis force-pushed the marco/fix-awsappaccess_accountid_integration branch from b377196 to 5902fe6 Compare August 23, 2024 08:20
If the integration name is digits only, the resulting address for the
application will look like this:
`123456789012.proxy.example.com:443`
This fails to parse with go's `url.Parse`.

This PR keeps the existing logic for creating the AWS App Access but
does a best effort to fix this issue by removing the `:443` from the
public proxy addr.
If another port is used, we would still get the error.
@marcoandredinis marcoandredinis force-pushed the marco/fix-awsappaccess_accountid_integration branch from 5902fe6 to 7996cfc Compare August 23, 2024 09:20
@marcoandredinis
Copy link
Contributor Author

I've changed some things, can you please take another look @timothyb89 ?

Can you also please take a look @atburke ?

@marcoandredinis marcoandredinis added this pull request to the merge queue Aug 26, 2024
Merged via the queue into master with commit 50d50ed Aug 26, 2024
40 checks passed
@marcoandredinis marcoandredinis deleted the marco/fix-awsappaccess_accountid_integration branch August 26, 2024 08:31
@public-teleport-github-review-bot

@marcoandredinis See the table below for backport results.

Branch Result
branch/v15 Create PR
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants