From 827c5609278e6547be141ecc281d90845f0193ac Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Fri, 2 Aug 2024 14:50:49 -0400 Subject: [PATCH 1/3] fix regression that denied access to launch some Apps --- .../src/AppLauncher/AppLauncher.test.tsx | 254 ++++++++++++++---- .../teleport/src/AppLauncher/AppLauncher.tsx | 22 +- 2 files changed, 230 insertions(+), 46 deletions(-) diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx index 50f854b0db69e..cf35a36b4413d 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx @@ -26,7 +26,11 @@ import service from 'teleport/services/apps'; import { AppLauncher } from './AppLauncher'; -const testCases: { name: string; path: string; expectedPath: string }[] = [ +const launcherPathTestCases: { + name: string; + path: string; + expectedPath: string; +}[] = [ { name: 'no state and no path', path: '?path=', @@ -95,54 +99,214 @@ describe('app launcher path is properly formed', () => { assignMock.mockClear(); }); - test.each(testCases)('$name', async ({ path: query, expectedPath }) => { - const launcherPath = `/web/launch/grafana.localhost${query}`; - const mockHistory = createMemoryHistory({ - initialEntries: [launcherPath], - }); + test.each(launcherPathTestCases)( + '$name', + async ({ path: query, expectedPath }) => { + const launcherPath = `/web/launch/grafana.localhost${query}`; + const mockHistory = createMemoryHistory({ + initialEntries: [launcherPath], + }); + + render( + + + + + + ); + + await waitFor(() => + expect(window.location.replace).toHaveBeenCalledWith( + `https://grafana.localhost/${expectedPath}` + ) + ); + } + ); +}); + +const appSessionTestCases: { + name: string; + path: string; + returnedFqdn: string; + expectedFqdn: string; + expectedPublicAddr: string; + expectedArn: string; +}[] = [ + { + name: 'ARN URL', + path: 'test-app.test.teleport/test.teleport/test-app.test.teleport/arn:aws:iam::joe123:role%2FEC2FullAccess?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: 'arn:aws:iam::joe123:role/EC2FullAccess', + }, + { + name: 'uppercase resolved FQDN', + path: 'test-app.test.teleport/test.teleport/test-app.test.teleport?state=ABC', + returnedFqdn: 'TEST-APP.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'uppercase public addr', + path: 'test-app.test.teleport/test.teleport/TEST-APP.test.teleport?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'TEST-APP.test.teleport', + expectedArn: undefined, + }, + { + name: 'uppercase FQDN', + path: 'TEST-APP.test.teleport/test.teleport/test-app.test.teleport?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'uppercase resolved FQDN, public addr', + path: 'test-app.test.teleport/test.teleport/TEST-APP.test.teleport?state=ABC', + returnedFqdn: 'TEST-APP.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'TEST-APP.test.teleport', + expectedArn: undefined, + }, + { + name: 'uppercase resolved FQDN,FQDN', + path: 'TEST-APP.test.teleport/test.teleport/test-app.test.teleport?state=ABC', + returnedFqdn: 'TEST-APP.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'uppercase public addr, FQDN', + path: 'TEST-APP.test.teleport/test.teleport/TEST-APP.test.teleport?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'TEST-APP.test.teleport', + expectedArn: undefined, + }, + { + name: 'uppercase FQDN, resolved FQDN, public addr', + path: 'TEST-APP.test.teleport/test.teleport/TEST-APP.test.teleport?state=ABC', + returnedFqdn: 'TEST-APP.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'TEST-APP.test.teleport', + expectedArn: undefined, + }, + { + name: 'public addr with port', + path: 'test-app.test.teleport/test.teleport/test-app.test.teleport:443?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'FQDN with port', + path: 'test-app.test.teleport:443/test.teleport/test-app.test.teleport?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport:443', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'resolved FQDN with port', + path: 'test-app.test.teleport/test.teleport/test-app.test.teleport?state=ABC', + returnedFqdn: 'test-app.test.teleport:443', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'FQDN, public addr with port', + path: 'test-app.test.teleport:443/test.teleport/test-app.test.teleport:443?state=ABC', + returnedFqdn: 'test-app.test.teleport', + expectedFqdn: 'test-app.test.teleport:443', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'FQDN, resolved FQDN with port', + path: 'test-app.test.teleport:443/test.teleport/test-app.test.teleport?state=ABC', + returnedFqdn: 'test-app.test.teleport:443', + expectedFqdn: 'test-app.test.teleport:443', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'public addr, resolved FQDN with port', + path: 'test-app.test.teleport/test.teleport/test-app.test.teleport:443?state=ABC', + returnedFqdn: 'test-app.test.teleport:443', + expectedFqdn: 'test-app.test.teleport', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, + { + name: 'FQDN, public addr, resolved FQDN with port', + path: 'test-app.test.teleport:443/test.teleport/test-app.test.teleport:443?state=ABC', + returnedFqdn: 'test-app.test.teleport:443', + expectedFqdn: 'test-app.test.teleport:443', + expectedPublicAddr: 'test-app.test.teleport', + expectedArn: undefined, + }, +]; + +describe('app session request is properly formed', () => { + const realLocation = window.location; + const assignMock = jest.fn(); + + beforeEach(() => { + global.fetch = jest.fn(() => Promise.resolve({})) as jest.Mock; + jest.spyOn(api, 'get').mockResolvedValue({}); + jest.spyOn(api, 'post').mockResolvedValue({}); - render( - - - - - - ); - - await waitFor(() => - expect(window.location.replace).toHaveBeenCalledWith( - `https://grafana.localhost/${expectedPath}` - ) - ); + delete window.location; + window.location = { ...realLocation, replace: assignMock }; }); - test('arn is url decoded', async () => { - jest.spyOn(service, 'getAppFqdn').mockResolvedValue({ - fqdn: 'test-app.test.teleport', - }); - jest.spyOn(service, 'createAppSession'); + afterEach(() => { + window.location = realLocation; + assignMock.mockClear(); + }); - const launcherPath = - '/web/launch/test-app.test.teleport/test.teleport/test-app.test.teleport/arn:aws:iam::joe123:role%2FEC2FullAccess?state=ABC'; - const mockHistory = createMemoryHistory({ - initialEntries: [launcherPath], - }); + test.each(appSessionTestCases)( + '$name', + async ({ + path, + returnedFqdn, + expectedFqdn, + expectedPublicAddr, + expectedArn, + }) => { + jest.spyOn(service, 'getAppFqdn').mockResolvedValue({ + fqdn: returnedFqdn, + }); + jest.spyOn(service, 'createAppSession'); - render( - - - - - - ); - - await waitFor(() => { - expect(service.createAppSession).toHaveBeenCalledWith({ - fqdn: 'test-app.test.teleport', - clusterId: 'test.teleport', - publicAddr: 'test-app.test.teleport', - arn: 'arn:aws:iam::joe123:role/EC2FullAccess', + const launcherPath = `/web/launch/${path}`; + const mockHistory = createMemoryHistory({ + initialEntries: [launcherPath], }); - }); - }); + + render( + + + + + + ); + + await waitFor(() => { + expect(service.createAppSession).toHaveBeenCalledWith({ + fqdn: expectedFqdn, + clusterId: 'test.teleport', + publicAddr: expectedPublicAddr, + arn: expectedArn, + }); + }); + } + ); }); diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index c767b56e64d9f..ffbeac75d8ce9 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -50,7 +50,13 @@ export function AppLauncher() { publicAddr: params.publicAddr, arn: params.arn, }); - if (resolvedApp.fqdn !== params.fqdn) { + // 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 + // redirect to the public address with an arbitrary port. But because + // 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}`); } @@ -138,6 +144,20 @@ export function AppLauncherAccessDenied(props: AppLauncherAccessDeniedProps) { return ; } +// prepareFqdn removes the port from the FQDN if it has one and ensures +// the FQDN is lowercase. This is to prevent issues matching the +// resolved fqdn with the one that was passed. Apps generally aren't +// supposed to have a port in the public address but some integrations +// create apps that do. The FQDN is also lowercased to prevent +// issues with case sensitivity. +function prepareFqdn(fqdn: string) { + const fqdnUrl = new URL('https://' + fqdn); + fqdnUrl.port = ''; + // The returned FQDN will have a scheme added to it, but that's + // fine because we're just using it to compare the FQDNs. + return fqdnUrl.toString().toLowerCase(); +} + function getXTeleportAuthUrl({ fqdn, port }: { fqdn: string; port: string }) { return new URL(`https://${fqdn}${port}/x-teleport-auth`); } From 7fb1cb05a32cfe62131f3900f9ad36d90429eb1c Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Mon, 5 Aug 2024 18:54:20 -0700 Subject: [PATCH 2/3] Fix test, and wrap new URL with its own try catch block --- .../src/AppLauncher/AppLauncher.test.tsx | 81 +++++++++++++++---- .../teleport/src/AppLauncher/AppLauncher.tsx | 20 +++-- 2 files changed, 81 insertions(+), 20 deletions(-) diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx index cf35a36b4413d..de611401bc399 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx @@ -15,7 +15,7 @@ */ import React from 'react'; -import { render, waitFor } from 'design/utils/testing'; +import { render, waitFor, screen } from 'design/utils/testing'; import { createMemoryHistory } from 'history'; import { Router } from 'react-router'; @@ -102,13 +102,8 @@ describe('app launcher path is properly formed', () => { test.each(launcherPathTestCases)( '$name', async ({ path: query, expectedPath }) => { - const launcherPath = `/web/launch/grafana.localhost${query}`; - const mockHistory = createMemoryHistory({ - initialEntries: [launcherPath], - }); - render( - + @@ -120,6 +115,7 @@ describe('app launcher path is properly formed', () => { `https://grafana.localhost/${expectedPath}` ) ); + expect(screen.queryByText(/access denied/i)).not.toBeInTheDocument(); } ); }); @@ -254,7 +250,7 @@ const appSessionTestCases: { }, ]; -describe('app session request is properly formed', () => { +describe('fqdn is matched', () => { const realLocation = window.location; const assignMock = jest.fn(); @@ -286,13 +282,8 @@ describe('app session request is properly formed', () => { }); jest.spyOn(service, 'createAppSession'); - const launcherPath = `/web/launch/${path}`; - const mockHistory = createMemoryHistory({ - initialEntries: [launcherPath], - }); - render( - + @@ -307,6 +298,68 @@ describe('app session request is properly formed', () => { arn: expectedArn, }); }); + + await waitFor(() => expect(window.location.replace).toHaveBeenCalled()); + expect(screen.queryByText(/access denied/i)).not.toBeInTheDocument(); } ); + + test('not matching fqdns throws error', async () => { + jest.spyOn(service, 'getAppFqdn').mockResolvedValue({ + fqdn: 'different.fqdn', + }); + + render( + + + + + + ); + + await screen.findByText(/access denied/i); + expect( + screen.getByText( + /failed to match applications with FQDN "test-app.test.teleport:443"/i + ) + ).toBeInTheDocument(); + expect(window.location.replace).not.toHaveBeenCalled(); + }); + + test('invalid url when constructing a new URL with a malformed fqdn', async () => { + jest.spyOn(service, 'getAppFqdn').mockResolvedValue({ + fqdn: 'invalid.fqdn:3080:3090', + }); + + render( + + + + + + ); + + await screen.findByText(/access denied/i); + expect( + screen.getByText( + /failed to match applications with FQDN "invalid.fqdn:3080:3090"/i + ) + ).toBeInTheDocument(); + expect(window.location.replace).not.toHaveBeenCalled(); + }); }); + +function createMockHistory(path: string) { + const launcherPath = `/web/launch/${path}`; + return createMemoryHistory({ + initialEntries: [launcherPath], + }); +} diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index ffbeac75d8ce9..917e9a99b46ef 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -57,7 +57,7 @@ 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}`); + throwFailedToMatchFqdnError(params.fqdn); } let path = ''; @@ -151,11 +151,15 @@ export function AppLauncherAccessDenied(props: AppLauncherAccessDeniedProps) { // create apps that do. The FQDN is also lowercased to prevent // issues with case sensitivity. function prepareFqdn(fqdn: string) { - const fqdnUrl = new URL('https://' + fqdn); - fqdnUrl.port = ''; - // The returned FQDN will have a scheme added to it, but that's - // fine because we're just using it to compare the FQDNs. - return fqdnUrl.toString().toLowerCase(); + try { + const fqdnUrl = new URL('https://' + fqdn); + fqdnUrl.port = ''; + // The returned FQDN will have a scheme added to it, but that's + // fine because we're just using it to compare the FQDNs. + return fqdnUrl.toString().toLowerCase(); + } catch (e) { + throwFailedToMatchFqdnError(fqdn); + } } function getXTeleportAuthUrl({ fqdn, port }: { fqdn: string; port: string }) { @@ -215,3 +219,7 @@ function initiateNewAuthExchange({ window.location.replace(url.toString()); } + +function throwFailedToMatchFqdnError(fqdn: string) { + throw Error(`Failed to match applications with FQDN "${fqdn}"`); +} From 4f4bfd4beeb8cce9cfc35d1b121f88de6a059fe5 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Mon, 5 Aug 2024 22:35:20 -0400 Subject: [PATCH 3/3] improve URL parsing fail error message --- .../src/AppLauncher/AppLauncher.test.tsx | 10 +++------- .../teleport/src/AppLauncher/AppLauncher.tsx | 16 ++++++++++------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx index de611401bc399..b1293b94ccb3a 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx @@ -304,7 +304,7 @@ describe('fqdn is matched', () => { } ); - test('not matching fqdns throws error', async () => { + test('not matching FQDN throws error', async () => { jest.spyOn(service, 'getAppFqdn').mockResolvedValue({ fqdn: 'different.fqdn', }); @@ -330,7 +330,7 @@ describe('fqdn is matched', () => { expect(window.location.replace).not.toHaveBeenCalled(); }); - test('invalid url when constructing a new URL with a malformed fqdn', async () => { + test('invalid URL when constructing a new URL with a malformed FQDN', async () => { jest.spyOn(service, 'getAppFqdn').mockResolvedValue({ fqdn: 'invalid.fqdn:3080:3090', }); @@ -348,11 +348,7 @@ describe('fqdn is matched', () => { ); await screen.findByText(/access denied/i); - expect( - screen.getByText( - /failed to match applications with FQDN "invalid.fqdn:3080:3090"/i - ) - ).toBeInTheDocument(); + expect(screen.getByText(/Failed to parse URL:/i)).toBeInTheDocument(); expect(window.location.replace).not.toHaveBeenCalled(); }); }); diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index 917e9a99b46ef..74fbb8bf262e4 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -57,7 +57,7 @@ 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)) { - throwFailedToMatchFqdnError(params.fqdn); + throw Error(`Failed to match applications with FQDN "${params.fqdn}"`); } let path = ''; @@ -157,13 +157,17 @@ function prepareFqdn(fqdn: string) { // The returned FQDN will have a scheme added to it, but that's // fine because we're just using it to compare the FQDNs. return fqdnUrl.toString().toLowerCase(); - } catch (e) { - throwFailedToMatchFqdnError(fqdn); + } catch (err) { + throwFailedToParseUrlError(err); } } function getXTeleportAuthUrl({ fqdn, port }: { fqdn: string; port: string }) { - return new URL(`https://${fqdn}${port}/x-teleport-auth`); + try { + return new URL(`https://${fqdn}${port}/x-teleport-auth`); + } catch (err) { + throwFailedToParseUrlError(err); + } } // initiateNewAuthExchange is the first step to gaining access to an @@ -220,6 +224,6 @@ function initiateNewAuthExchange({ window.location.replace(url.toString()); } -function throwFailedToMatchFqdnError(fqdn: string) { - throw Error(`Failed to match applications with FQDN "${fqdn}"`); +function throwFailedToParseUrlError(err: TypeError) { + throw Error(`Failed to parse URL: ${err.message}`); }