From ec45e1c329c4be7668201a0bd19f27fabe4412cf Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Mon, 21 Oct 2024 20:55:42 -0400 Subject: [PATCH] fix app access regression when the app is on a leaf cluster --- lib/web/apps.go | 13 ++- .../src/AppLauncher/AppLauncher.test.tsx | 91 +++++++++++++++++++ .../teleport/src/AppLauncher/AppLauncher.tsx | 16 +++- .../teleport/src/services/apps/apps.ts | 4 + 4 files changed, 121 insertions(+), 3 deletions(-) diff --git a/lib/web/apps.go b/lib/web/apps.go index 9dfefd4a3eb10..15e22f97386f0 100644 --- a/lib/web/apps.go +++ b/lib/web/apps.go @@ -149,10 +149,17 @@ func (h *Handler) clusterAppsGet(w http.ResponseWriter, r *http.Request, p httpr type GetAppDetailsRequest ResolveAppParams type GetAppDetailsResponse struct { - // FQDN is application FQDN. + // FQDN is application FQDN. See utils.AssembleAppFQDN for details + // on how the FQDN is formed. FQDN string `json:"fqdn"` // RequiredAppFQDNs is a list of required app fqdn RequiredAppFQDNs []string `json:"requiredAppFQDNs"` + // LocalClusterName is the name of the cluster that resolved the + // application, which may belong to another cluster. + LocalClusterName string `json:"localClusterName"` + // PublicAddress is the public address of the application, which + // may be different than the FQDN in some cases. + PublicAddress string `json:"publicAddress"` } // getAppDetails resolves the input params to a known application and returns @@ -179,7 +186,9 @@ func (h *Handler) getAppDetails(w http.ResponseWriter, r *http.Request, p httpro } resp := &GetAppDetailsResponse{ - FQDN: result.FQDN, + FQDN: result.FQDN, + LocalClusterName: h.auth.clusterName, + PublicAddress: result.App.GetPublicAddr(), } requiredAppNames := result.App.GetRequiredAppNames() diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx index 85aabbcd6e662..0c6296b81e34e 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx @@ -85,6 +85,8 @@ describe('app launcher path is properly formed', () => { jest.spyOn(api, 'post').mockResolvedValue({}); jest.spyOn(service, 'getAppDetails').mockResolvedValue({ fqdn: 'grafana.localhost', + localClusterName: 'localhost', + publicAddress: 'grafana.localhost' }); jest.spyOn(service, 'createAppSession').mockResolvedValue({ cookieValue: 'cookie-value', @@ -281,6 +283,8 @@ describe('fqdn is matched', () => { }) => { jest.spyOn(service, 'getAppDetails').mockResolvedValue({ fqdn: returnedFqdn, + localClusterName: '', + publicAddress:'', }); jest.spyOn(service, 'createAppSession'); @@ -309,6 +313,8 @@ describe('fqdn is matched', () => { test('not matching FQDN throws error', async () => { jest.spyOn(service, 'getAppDetails').mockResolvedValue({ fqdn: 'different.fqdn', + localClusterName: 'test', + publicAddress:'https://test-app.test.teleport', }); render( @@ -332,9 +338,94 @@ describe('fqdn is matched', () => { expect(window.location.replace).not.toHaveBeenCalled(); }); + test('not matching FQDN and random public addr from another cluster throws error', async () => { + jest.spyOn(service, 'getAppDetails').mockResolvedValue({ + fqdn: 'test-app.root.teleport', + localClusterName: 'root', + publicAddress:'malicious.teleporto', + }); + + render( + + + + + + ); + + await screen.findByText(/access denied/i); + expect( + screen.getByText( + /failed to match applications with FQDN "test-app.leaf.teleport:443"/i + ) + ).toBeInTheDocument(); + expect(window.location.replace).not.toHaveBeenCalled(); + }); + + test('not matching FQDN and public address from another cluster throws error', async () => { + jest.spyOn(service, 'getAppDetails').mockResolvedValue({ + fqdn: 'test-app.root.teleport', + localClusterName: 'root', + publicAddress:'test-app.leaf.teleport', + }); + + render( + + + + + + ); + + await screen.findByText(/access denied/i); + expect( + screen.getByText( + /failed to match applications with FQDN "malicious:443"/i + ) + ).toBeInTheDocument(); + expect(window.location.replace).not.toHaveBeenCalled(); + }); + + test('matching FQDN from another cluster is allowed', async () => { + jest.spyOn(service, 'getAppDetails').mockResolvedValue({ + fqdn: 'test-app.root.teleport', + localClusterName: 'root.teleport', + publicAddress:'test-app.leaf.teleport', + }); + + render( + + + + + + ); + + await waitFor(() => { + expect(service.createAppSession).toHaveBeenCalled(); + }); + + await waitFor(() => expect(window.location.replace).toHaveBeenCalled()); + expect(screen.queryByText(/access denied/i)).not.toBeInTheDocument(); + }); + test('invalid URL when constructing a new URL with a malformed FQDN', async () => { jest.spyOn(service, 'getAppDetails').mockResolvedValue({ fqdn: 'invalid.fqdn:3080:3090', + localClusterName: '', + publicAddress:'', }); render( diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index 72281dc4df9ea..5714733170f6f 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -56,6 +56,7 @@ export function AppLauncher() { publicAddr: params.publicAddr, arn: params.arn, }); + // Because the ports are stripped from the FQDNs before they are // compared, an attacker can pass a FQDN with a different port than // what the app's public address is configured with and have Teleport @@ -63,7 +64,20 @@ export function AppLauncher() { // the attacker can't control what domain is redirected to this has // a low risk factor. if (prepareFqdn(resolvedApp.fqdn) !== prepareFqdn(params.fqdn)) { - throw Error(`Failed to match applications with FQDN "${params.fqdn}"`); + // If the resolved FQDN doesn't match what we expect, check the + // expected FQDN from the returned public address only if the + // local cluster that resolved the application is different than + // the cluster that the application resides in. This won't allow + // arbitrary addresses through because the resolved public address + // which isn't attacker controlled is matched against. + if ( + params.clusterId !== resolvedApp.localClusterName && + prepareFqdn(resolvedApp.publicAddress) !== prepareFqdn(params.fqdn) + ) { + throw Error( + `Failed to match applications with FQDN "${params.fqdn}"` + ); + } } let path = ''; diff --git a/web/packages/teleport/src/services/apps/apps.ts b/web/packages/teleport/src/services/apps/apps.ts index a301147f8730f..0910be3a85397 100644 --- a/web/packages/teleport/src/services/apps/apps.ts +++ b/web/packages/teleport/src/services/apps/apps.ts @@ -78,6 +78,8 @@ const service = { return api.get(cfg.getAppDetailsUrl(params)).then(json => ({ fqdn: json.fqdn, requiredAppFQDNs: json.requiredAppFQDNs, + localClusterName: json.localClusterName, + publicAddress: json.publicAddress, })); }, }; @@ -85,6 +87,8 @@ const service = { type AppDetails = { fqdn: string; requiredAppFQDNs?: string[]; + localClusterName: string; + publicAddress: string; }; export default service;