Prevent creation of invalid App for AWS OIDC Integration (gravitation…

…al#51287)

* Prevent creation of invalid App for AWS OIDC Integration

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.

* prevent new aws oidc integrations with invalid DNS labels

* add integration name to error message

* improve readability of validation.IsDNS1035Label call

integration/integrations/integration_test.go

@@ -94,7 +94,7 @@ func TestIntegrationCRUD(t *testing.T) {
 
 	// Create Integration
 	createIntegrationReq := ui.Integration{
-		Name:    "MyAWSAccount",
+		Name:    "my-aws-account",
 		SubKind: types.IntegrationSubKindAWSOIDC,
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN:        "arn:aws:iam::123456789012:role/DevTeam",
@@ -108,7 +108,7 @@ func TestIntegrationCRUD(t *testing.T) {
 
 	// Create Integration without S3 location
 	createIntegrationWithoutS3LocationReq := ui.Integration{
-		Name:    "MyAWSAccountWithoutS3",
+		Name:    "my-aws-account-without-s3",
 		SubKind: types.IntegrationSubKindAWSOIDC,
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN: "arn:aws:iam::123456789012:role/DevTeam",
@@ -119,14 +119,14 @@ func TestIntegrationCRUD(t *testing.T) {
 	require.Equal(t, http.StatusOK, respStatusCode, string(respBody))
 
 	// Get One Integration by name
-	respStatusCode, respBody = webPack.DoRequest(t, http.MethodGet, integrationsEndpoint+"/MyAWSAccount", nil)
+	respStatusCode, respBody = webPack.DoRequest(t, http.MethodGet, integrationsEndpoint+"/my-aws-account", nil)
 	require.Equal(t, http.StatusOK, respStatusCode, string(respBody))
 
 	integrationResp := ui.Integration{}
 	require.NoError(t, json.Unmarshal(respBody, &integrationResp))
 
 	require.Equal(t, ui.Integration{
-		Name:    "MyAWSAccount",
+		Name:    "my-aws-account",
 		SubKind: types.IntegrationSubKindAWSOIDC,
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN:        "arn:aws:iam::123456789012:role/DevTeam",
@@ -136,7 +136,7 @@ func TestIntegrationCRUD(t *testing.T) {
 	}, integrationResp, string(respBody))
 
 	// Update the integration to another RoleARN
-	respStatusCode, respBody = webPack.DoRequest(t, http.MethodPut, integrationsEndpoint+"/MyAWSAccount", ui.UpdateIntegrationRequest{
+	respStatusCode, respBody = webPack.DoRequest(t, http.MethodPut, integrationsEndpoint+"/my-aws-account", ui.UpdateIntegrationRequest{
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN:        "arn:aws:iam::123456789012:role/OpsTeam",
 			IssuerS3Bucket: "my-bucket",
@@ -149,7 +149,7 @@ func TestIntegrationCRUD(t *testing.T) {
 	require.NoError(t, json.Unmarshal(respBody, &integrationResp))
 
 	require.Equal(t, ui.Integration{
-		Name:    "MyAWSAccount",
+		Name:    "my-aws-account",
 		SubKind: types.IntegrationSubKindAWSOIDC,
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN:        "arn:aws:iam::123456789012:role/OpsTeam",
@@ -159,7 +159,7 @@ func TestIntegrationCRUD(t *testing.T) {
 	}, integrationResp, string(respBody))
 
 	// Update the integration to remove the S3 Location
-	respStatusCode, respBody = webPack.DoRequest(t, http.MethodPut, integrationsEndpoint+"/MyAWSAccount", ui.UpdateIntegrationRequest{
+	respStatusCode, respBody = webPack.DoRequest(t, http.MethodPut, integrationsEndpoint+"/my-aws-account", ui.UpdateIntegrationRequest{
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN: "arn:aws:iam::123456789012:role/OpsTeam2",
 		},
@@ -170,15 +170,15 @@ func TestIntegrationCRUD(t *testing.T) {
 	require.NoError(t, json.Unmarshal(respBody, &integrationResp))
 
 	require.Equal(t, ui.Integration{
-		Name:    "MyAWSAccount",
+		Name:    "my-aws-account",
 		SubKind: types.IntegrationSubKindAWSOIDC,
 		AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 			RoleARN: "arn:aws:iam::123456789012:role/OpsTeam2",
 		},
 	}, integrationResp, string(respBody))
 
 	// Delete resource
-	respStatusCode, respBody = webPack.DoRequest(t, http.MethodDelete, integrationsEndpoint+"/MyAWSAccount", nil)
+	respStatusCode, respBody = webPack.DoRequest(t, http.MethodDelete, integrationsEndpoint+"/my-aws-account", nil)
 	require.Equal(t, http.StatusOK, respStatusCode, string(respBody))
 
 	// Add multiple integrations to test pagination
@@ -187,7 +187,7 @@ func TestIntegrationCRUD(t *testing.T) {
 	totalItems := pageSize*2 + 1
 	for i := 0; i < totalItems; i++ {
 		createIntegrationReq := ui.Integration{
-			Name:    fmt.Sprintf("AWSIntegration%d", i),
+			Name:    fmt.Sprintf("aws-integration-%d", i),
 			SubKind: types.IntegrationSubKindAWSOIDC,
 			AWSOIDC: &ui.IntegrationAWSOIDCSpec{
 				RoleARN:        "arn:aws:iam::123456789012:role/DevTeam",

lib/auth/integration/integrationv1/service.go

@@ -28,6 +28,7 @@ import (
 	"github.com/jonboulle/clockwork"
 	"golang.org/x/crypto/ssh"
 	"google.golang.org/protobuf/types/known/emptypb"
+	"k8s.io/apimachinery/pkg/util/validation"
 
 	"github.com/gravitational/teleport"
 	integrationpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/integration/v1"
@@ -230,6 +231,13 @@ func (s *Service) CreateIntegration(ctx context.Context, req *integrationpb.Crea
 		if err := s.createGitHubCredentials(ctx, req.Integration); err != nil {
 			return nil, trace.Wrap(err)
 		}
+	case types.IntegrationSubKindAWSOIDC:
+		// AWS OIDC Integration can be used as source of credentials to access AWS Web/CLI.
+		// This creates a new AppServer whose endpoint is <integrationName>.<proxyURL>, which can fail if integrationName is not a valid DNS Label.
+		// Instead of failing when the integration is already created, it fails at creation time.
+		if errs := validation.IsDNS1035Label(req.GetIntegration().GetName()); len(errs) > 0 {
+			return nil, trace.BadParameter("integration name %q must be a valid DNS subdomain so that it can be used to allow Web/CLI access", req.GetIntegration().GetName())
+		}
 	}
 
 	ig, err := s.backend.CreateIntegration(ctx, req.Integration)

lib/auth/integration/integrationv1/service_test.go

@@ -19,6 +19,7 @@
 package integrationv1
 
 import (
+	"cmp"
 	"context"
 	"testing"
 
@@ -68,13 +69,14 @@ func TestIntegrationCRUD(t *testing.T) {
 	}
 
 	tt := []struct {
-		Name         string
-		Role         types.RoleSpecV6
-		Setup        func(t *testing.T, igName string)
-		Test         func(ctx context.Context, resourceSvc *Service, igName string) error
-		Validate     func(t *testing.T, igName string)
-		Cleanup      func(t *testing.T, igName string)
-		ErrAssertion func(error) bool
+		Name            string
+		Role            types.RoleSpecV6
+		IntegrationName string
+		Setup           func(t *testing.T, igName string)
+		Test            func(ctx context.Context, resourceSvc *Service, igName string) error
+		Validate        func(t *testing.T, igName string)
+		Cleanup         func(t *testing.T, igName string)
+		ErrAssertion    func(error) bool
 	}{
 		// Read
 		{
@@ -186,13 +188,30 @@ func TestIntegrationCRUD(t *testing.T) {
 					Verbs:     []string{types.VerbCreate},
 				}}},
 			},
+			IntegrationName: "integration-allow-create-access",
 			Test: func(ctx context.Context, resourceSvc *Service, igName string) error {
 				ig := sampleIntegrationFn(t, igName)
 				_, err := resourceSvc.CreateIntegration(ctx, &integrationpb.CreateIntegrationRequest{Integration: ig.(*types.IntegrationV1)})
 				return err
 			},
 			ErrAssertion: noError,
 		},
+		{
+			Name: "access to create integrations but name is invalid",
+			Role: types.RoleSpecV6{
+				Allow: types.RoleConditions{Rules: []types.Rule{{
+					Resources: []string{types.KindIntegration},
+					Verbs:     []string{types.VerbCreate},
+				}}},
+			},
+			IntegrationName: "integration-awsoidc-invalid.name",
+			Test: func(ctx context.Context, resourceSvc *Service, igName string) error {
+				ig := sampleIntegrationFn(t, igName)
+				_, err := resourceSvc.CreateIntegration(ctx, &integrationpb.CreateIntegrationRequest{Integration: ig.(*types.IntegrationV1)})
+				return err
+			},
+			ErrAssertion: trace.IsBadParameter,
+		},
 		{
 			Name: "create github integrations",
 			Role: types.RoleSpecV6{
@@ -553,7 +572,7 @@ func TestIntegrationCRUD(t *testing.T) {
 		tc := tc
 		t.Run(tc.Name, func(t *testing.T) {
 			localCtx := authorizerForDummyUser(t, ctx, tc.Role, localClient)
-			igName := uuid.NewString()
+			igName := cmp.Or(tc.IntegrationName, uuid.NewString())
 			if tc.Setup != nil {
 				tc.Setup(t, igName)
 			}

lib/web/integrations_awsoidc.go

@@ -1078,6 +1078,18 @@ func (h *Handler) awsOIDCCreateAWSAppAccess(w http.ResponseWriter, r *http.Reque
 		return nil, trace.Wrap(err)
 	}
 
+	// If the integration name contains a dot, then the proxy must provide a certificate allowing *.<something>.<proxyPublicAddr>
+	if strings.Contains(integrationName, ".") {
+		// Teleport Cloud only provides certificates for *.<tenant>.teleport.sh, so this would generate an invalid address.
+		if h.GetClusterFeatures().Cloud {
+			return nil, trace.BadParameter(`Invalid integration name (%q) for enabling AWS Access. Please re-create the integration without the "."`, integrationName)
+		}
+
+		// Typically, self-hosted clusters will also have a single wildcard for the name.
+		// Logging a warning message should help debug the problem in case the certificate is not valid.
+		h.logger.WarnContext(ctx, `Enabling AWS Access using an integration with a "." might not work unless your Proxy's certificate is valid for the address`, "public_addr", appServer.GetApp().GetPublicAddr())
+	}
+
 	if _, err := clt.UpsertApplicationServer(ctx, appServer); err != nil {
 		return nil, trace.Wrap(err)
 	}

lib/web/integrations_awsoidc_test.go

@@ -1032,6 +1032,37 @@ func TestAWSOIDCAppAccessAppServerCreationDeletion(t *testing.T) {
 		_, err = pack.clt.PostJSON(ctx, endpoint, nil)
 		require.NoError(t, err)
 	})
+
+	t.Run("using a period in the name fails when running in cloud", func(t *testing.T) {
+		enableCloudFeatureProxy(t, proxy)
+
+		// Creating an Integration using the account id as name should not return an error if the proxy is listening at the default HTTPS port
+		myIntegrationWithAccountID, err := types.NewIntegrationAWSOIDC(types.Metadata{
+			Name: "env.prod",
+		}, &types.AWSOIDCIntegrationSpecV1{
+			RoleARN: "arn:aws:iam::123456789012:role/teleport",
+		})
+		require.NoError(t, err)
+
+		_, err = env.server.Auth().CreateIntegration(ctx, myIntegrationWithAccountID)
+		require.NoError(t, err)
+		endpoint = pack.clt.Endpoint("webapi", "sites", "localhost", "integrations", "aws-oidc", "env.prod", "aws-app-access")
+		_, err = pack.clt.PostJSON(ctx, endpoint, nil)
+		require.Error(t, err)
+		require.ErrorContains(t, err, `Invalid integration name ("env.prod") for enabling AWS Access.`)
+	})
+}
+
+func enableCloudFeatureProxy(t *testing.T, proxy *testProxy) {
+	t.Helper()
+
+	existingFeatures := proxy.handler.handler.clusterFeatures
+	existingFeatures.Cloud = true
+	proxy.handler.handler.clusterFeatures = existingFeatures
+	t.Cleanup(func() {
+		existingFeatures.Cloud = false
+		proxy.handler.handler.clusterFeatures = existingFeatures
+	})
 }
 
 func TestAWSOIDCAppAccessAppServerCreationWithUserProvidedLabels(t *testing.T) {