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

Prevent creation of invalid App for AWS OIDC Integration #51287

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented Jan 21, 2025

When enabling AWS Access using an integration, the final address will be the concatenation of the integration name and the proxy's public address.

The proxy must present a certificate valid for that address. However, when the integration name has a dot, it will usually not work with the proxy's certificate.

We know it won't work for Teleport Cloud, where the certificates only allows for <app>.<tenant>.teleport.sh.
So, for Teleport Cloud enabling AWS Access is not possible.
For self-hosted, a warning is emitted.

New AWS OIDC Integrations can't be created with invalid DNS labels (existing ones are not affected).
Demo:

$ tctl get integration/aws.dev
kind: integration
metadata:
  name: aws.dev
  revision: fa52f979-2ed9-4c77-85bc-323f75f62076
spec:
  aws_oidc:
    role_arn: arn:aws:iam::123456789012:role/MarcoLocalClusterIntegration
  azure_oidc: {}
  github: {}
sub_kind: aws-oidc
version: v1

$ cat integration-aws.dev2.yaml
kind: integration
sub_kind: aws-oidc
version: v1
metadata:
  name: aws.dev2
spec:
  aws_oidc:
    role_arn: "arn:aws:iam::123456789012:role/MarcoLocalClusterIntegration"

$ tctl create -f integration-aws.dev2.yaml
ERROR: integration name "aws.dev2" must be a valid DNS subdomain so that it can be used to allow Web/CLI access

Fixes #44516

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Can we just prohibit dots in integration names?

@camscale
Copy link
Contributor

Can we just prohibit dots in integration names?

I was wondering the same. I did not get to trace the code back to validating an integration name yet, but are there other allowed characters in an integration name that are not a valid dns "label"? https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1 specifies that letters, digits and hyphens are all that's allowed.

Kubernetes addresses similar concerns in https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

@marcoandredinis
Copy link
Contributor Author

Can we just prohibit dots in integration names?

Yes, but we must keep this code to ensure existing integrations keep working (ie, we don't break backwards compat).

but are there other allowed characters in an integration name that are not a valid dns "label"?

Creating the AppServer ensures we have a valid URL.
So, at worst, we'll have an error being returned here.

We can apply the RFC 1035 here (which is the more strict).
So, for new Integrations we'll have the following rules:

  • only allow lowercase alphanumeric characters and '-'
  • max 63 chars
  • must start and end with alphanumeric

I'll start implementing that

@marcoandredinis marcoandredinis force-pushed the marco/prevent_awsoidc_invalid_appname branch from e31d2c3 to a793f72 Compare January 23, 2025 17:44
@camscale
Copy link
Contributor

  • must start and end with alphanumeric

According to that Kubernetes link, that would be RFC 1123. RFC 1035 says it must start with alpha, not alphanumeric.

When enabling AWS Access using an integration, the final address will be
a concatenation of the integration name and the proxy's public address.

The proxy must present a certificate valid for that address.
However, when the integration name has a dot, it will usually not work
with the proxy's certificate.
We know it won't work for Teleport Cloud, where the certificates only
allow for `<app>.<tenant>.teleport.sh`.

So, for Teleport Cloud enabling AWS Access is not possible.
For self-hosted, a warning is emitted.
@marcoandredinis marcoandredinis force-pushed the marco/prevent_awsoidc_invalid_appname branch from a793f72 to 5b5e856 Compare January 24, 2025 17:14
@marcoandredinis
Copy link
Contributor Author

@r0mant @camscale Can you please take another look?

@marcoandredinis marcoandredinis force-pushed the marco/prevent_awsoidc_invalid_appname branch from 5b5e856 to 88ee4c0 Compare January 24, 2025 17:17
@marcoandredinis marcoandredinis force-pushed the marco/prevent_awsoidc_invalid_appname branch from 88ee4c0 to b37c23d Compare January 24, 2025 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Periods in AWS integration name causes issues with AWS Console/CLI resource
3 participants