From e9b92a2898aa5cfc8107102bc13ee248e91ac344 Mon Sep 17 00:00:00 2001 From: Marco Dinis Date: Tue, 3 Dec 2024 14:33:57 +0000 Subject: [PATCH 1/2] AWS OIDC: add aws account id as label to AWS App We were not setting any labels in the AWS App when using the Discover Flow for a given AWS OIDC integration. This is a bad practice because this means that users must have `app_labels: *:*` in order to access this particular app. This is not recommended because it grants access to every app. This PR changes this so that the account id can be used to gate access. --- api/types/appserver.go | 8 +++++--- api/types/appserver_test.go | 6 +++++- lib/web/integrations_awsoidc.go | 13 ++++++++++++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/api/types/appserver.go b/api/types/appserver.go index b3ce94c684697..f2094e25391af 100644 --- a/api/types/appserver.go +++ b/api/types/appserver.go @@ -86,13 +86,15 @@ func NewAppServerV3FromApp(app *AppV3, hostname, hostID string) (*AppServerV3, e // NewAppServerForAWSOIDCIntegration creates a new AppServer that will be used to grant AWS App Access // using the AWSOIDC credentials. -func NewAppServerForAWSOIDCIntegration(integrationName, hostID, publicAddr string) (*AppServerV3, error) { +func NewAppServerForAWSOIDCIntegration(integrationName, hostID, publicAddr string, labels map[string]string) (*AppServerV3, error) { return NewAppServerV3(Metadata{ - Name: integrationName, + Name: integrationName, + Labels: labels, }, AppServerSpecV3{ HostID: hostID, App: &AppV3{Metadata: Metadata{ - Name: integrationName, + Name: integrationName, + Labels: labels, }, Spec: AppSpecV3{ URI: constants.AWSConsoleURL, Integration: integrationName, diff --git a/api/types/appserver_test.go b/api/types/appserver_test.go index 74a0ab661f7dd..25fa31f6d4d63 100644 --- a/api/types/appserver_test.go +++ b/api/types/appserver_test.go @@ -63,6 +63,7 @@ func TestNewAppServerForAWSOIDCIntegration(t *testing.T) { integratioName string hostID string publicAddr string + labels map[string]string expectedApp *AppServerV3 errCheck require.ErrorAssertionFunc }{ @@ -71,12 +72,14 @@ func TestNewAppServerForAWSOIDCIntegration(t *testing.T) { integratioName: "valid", hostID: "my-host-id", publicAddr: "valid.proxy.example.com", + labels: map[string]string{"account_id": "123456789012"}, expectedApp: &AppServerV3{ Kind: KindAppServer, Version: V3, Metadata: Metadata{ Name: "valid", Namespace: "default", + Labels: map[string]string{"account_id": "123456789012"}, }, Spec: AppServerSpecV3{ Version: api.Version, @@ -87,6 +90,7 @@ func TestNewAppServerForAWSOIDCIntegration(t *testing.T) { Metadata: Metadata{ Name: "valid", Namespace: "default", + Labels: map[string]string{"account_id": "123456789012"}, }, Spec: AppSpecV3{ URI: "https://console.aws.amazon.com", @@ -106,7 +110,7 @@ func TestNewAppServerForAWSOIDCIntegration(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - app, err := NewAppServerForAWSOIDCIntegration(tt.integratioName, tt.hostID, tt.publicAddr) + app, err := NewAppServerForAWSOIDCIntegration(tt.integratioName, tt.hostID, tt.publicAddr, tt.labels) if tt.errCheck != nil { tt.errCheck(t, err) } diff --git a/lib/web/integrations_awsoidc.go b/lib/web/integrations_awsoidc.go index 39ebb250a9452..331df6ba78dd1 100644 --- a/lib/web/integrations_awsoidc.go +++ b/lib/web/integrations_awsoidc.go @@ -35,6 +35,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" + "github.com/gravitational/teleport/api/constants" apidefaults "github.com/gravitational/teleport/api/defaults" integrationv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/integration/v1" "github.com/gravitational/teleport/api/types" @@ -49,6 +50,7 @@ import ( "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/services" libutils "github.com/gravitational/teleport/lib/utils" + awsutils "github.com/gravitational/teleport/lib/utils/aws" "github.com/gravitational/teleport/lib/utils/oidc" "github.com/gravitational/teleport/lib/web/scripts/oneoff" "github.com/gravitational/teleport/lib/web/ui" @@ -984,7 +986,16 @@ func (h *Handler) awsOIDCCreateAWSAppAccess(w http.ResponseWriter, r *http.Reque publicAddr := libutils.DefaultAppPublicAddr(integrationName, h.PublicProxyAddr()) - appServer, err := types.NewAppServerForAWSOIDCIntegration(integrationName, h.cfg.HostUUID, publicAddr) + parsedRoleARN, err := awsutils.ParseRoleARN(ig.GetAWSOIDCIntegrationSpec().RoleARN) + if err != nil { + return nil, trace.Wrap(err) + } + + labels := map[string]string{ + constants.AWSAccountIDLabel: parsedRoleARN.AccountID, + } + + appServer, err := types.NewAppServerForAWSOIDCIntegration(integrationName, h.cfg.HostUUID, publicAddr, labels) if err != nil { return nil, trace.Wrap(err) } From ef0aa8c2bc6aa61a945e8904dfcc4ab5efe3ab86 Mon Sep 17 00:00:00 2001 From: Marco Dinis Date: Tue, 3 Dec 2024 14:51:05 +0000 Subject: [PATCH 2/2] fix test --- lib/web/integrations_awsoidc_test.go | 10 ++++++---- tool/tctl/common/resource_command_test.go | 6 +++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/web/integrations_awsoidc_test.go b/lib/web/integrations_awsoidc_test.go index 200d68ef7d9a0..9b2660fe36e99 100644 --- a/lib/web/integrations_awsoidc_test.go +++ b/lib/web/integrations_awsoidc_test.go @@ -1067,7 +1067,7 @@ func TestAWSOIDCAppAccessAppServerCreationDeletion(t *testing.T) { myIntegration, err := types.NewIntegrationAWSOIDC(types.Metadata{ Name: "my-integration", }, &types.AWSOIDCIntegrationSpecV1{ - RoleARN: "some-arn-role", + RoleARN: "arn:aws:iam::123456789012:role/teleport", }) require.NoError(t, err) @@ -1094,7 +1094,8 @@ func TestAWSOIDCAppAccessAppServerCreationDeletion(t *testing.T) { Kind: types.KindAppServer, Version: types.V3, Metadata: types.Metadata{ - Name: "my-integration", + Name: "my-integration", + Labels: map[string]string{"aws_account_id": "123456789012"}, }, Spec: types.AppServerSpecV3{ Version: api.Version, @@ -1103,7 +1104,8 @@ func TestAWSOIDCAppAccessAppServerCreationDeletion(t *testing.T) { Kind: types.KindApp, Version: types.V3, Metadata: types.Metadata{ - Name: "my-integration", + Name: "my-integration", + Labels: map[string]string{"aws_account_id": "123456789012"}, }, Spec: types.AppSpecV3{ URI: "https://console.aws.amazon.com", @@ -1133,7 +1135,7 @@ func TestAWSOIDCAppAccessAppServerCreationDeletion(t *testing.T) { myIntegrationWithAccountID, err := types.NewIntegrationAWSOIDC(types.Metadata{ Name: "123456789012", }, &types.AWSOIDCIntegrationSpecV1{ - RoleARN: "some-arn-role", + RoleARN: "arn:aws:iam::123456789012:role/teleport", }) require.NoError(t, err) diff --git a/tool/tctl/common/resource_command_test.go b/tool/tctl/common/resource_command_test.go index da1bd0d42bfda..4f51493bbf75e 100644 --- a/tool/tctl/common/resource_command_test.go +++ b/tool/tctl/common/resource_command_test.go @@ -1931,11 +1931,15 @@ func testCreateAppServer(t *testing.T, clt *authclient.Client) { kind: app_server metadata: name: my-integration + labels: + account_id: "123456789012" spec: app: kind: app metadata: name: my-integration + labels: + account_id: "123456789012" spec: uri: https://console.aws.amazon.com integration: my-integration @@ -1979,7 +1983,7 @@ version: v3 appServers := mustDecodeJSON[[]*types.AppServerV3](t, buf) require.Len(t, appServers, 1) - expectedAppServer, err := types.NewAppServerForAWSOIDCIntegration("my-integration", "c6cfe5c2-653f-4e5d-a914-bfac5a7baf38", "integration.example.com") + expectedAppServer, err := types.NewAppServerForAWSOIDCIntegration("my-integration", "c6cfe5c2-653f-4e5d-a914-bfac5a7baf38", "integration.example.com", map[string]string{"account_id": "123456789012"}) require.NoError(t, err) require.Empty(t, cmp.Diff( expectedAppServer,