move validation logic into a separate function

gravitational · Oct 24, 2024 · 2fb2ca4 · 2fb2ca4
1 parent ec45e1c
commit 2fb2ca4
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 32 deletions.
diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx
@@ -86,7 +86,7 @@ describe('app launcher path is properly formed', () => {
     jest.spyOn(service, 'getAppDetails').mockResolvedValue({
       fqdn: 'grafana.localhost',
       localClusterName: 'localhost',
-      publicAddress: 'grafana.localhost'
+      publicAddress: 'grafana.localhost',
     });
     jest.spyOn(service, 'createAppSession').mockResolvedValue({
       cookieValue: 'cookie-value',
@@ -284,7 +284,7 @@ describe('fqdn is matched', () => {
       jest.spyOn(service, 'getAppDetails').mockResolvedValue({
         fqdn: returnedFqdn,
         localClusterName: '',
-        publicAddress:'',
+        publicAddress: '',
       });
       jest.spyOn(service, 'createAppSession');
 
@@ -313,8 +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',
+      localClusterName: 'test.teleport',
+      publicAddress: 'https://test-app.test.teleport',
     });
 
     render(
@@ -342,7 +342,7 @@ describe('fqdn is matched', () => {
     jest.spyOn(service, 'getAppDetails').mockResolvedValue({
       fqdn: 'test-app.root.teleport',
       localClusterName: 'root',
-      publicAddress:'malicious.teleporto',
+      publicAddress: 'malicious.teleporto',
     });
 
     render(
@@ -369,8 +369,8 @@ describe('fqdn is matched', () => {
   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',
+      localClusterName: 'root.teleport',
+      publicAddress: 'test-app.leaf.teleport',
     });
 
     render(
@@ -398,7 +398,7 @@ describe('fqdn is matched', () => {
     jest.spyOn(service, 'getAppDetails').mockResolvedValue({
       fqdn: 'test-app.root.teleport',
       localClusterName: 'root.teleport',
-      publicAddress:'test-app.leaf.teleport',
+      publicAddress: 'test-app.leaf.teleport',
     });
 
     render(
@@ -425,7 +425,7 @@ describe('fqdn is matched', () => {
     jest.spyOn(service, 'getAppDetails').mockResolvedValue({
       fqdn: 'invalid.fqdn:3080:3090',
       localClusterName: '',
-      publicAddress:'',
+      publicAddress: '',
     });
 
     render(

diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx
@@ -37,6 +37,43 @@ export function AppLauncher() {
   const queryParams = new URLSearchParams(search);
   const isRedirectFlow = queryParams.get('required-apps');
 
+  function validateResolvedApp(
+    params: UrlLauncherParams,
+    fqdn: string,
+    localClusterName: string,
+    publicAddress: string
+  ) {
+    // 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(fqdn) == prepareFqdn(params.fqdn)) {
+      return;
+    }
+
+    // The resolved FQDN didn't match what we expect, and the app
+    // belongs to the same cluster that resolved it, this FQDN is not
+    // legitimate.
+    if (params.clusterId == localClusterName) {
+      throw Error(`Failed to match applications with FQDN "${params.fqdn}"`);
+    }
+
+    // If the resolved FQDN doesn't match what we expect, check the
+    // expected FQDN against 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 the app's
+    // cluster differs from the cluster that resolved it the FQDN will be
+    // built with the resolved cluster's name, making this check necessary
+    // to allow access to apps in leaf clusters.
+    if (prepareFqdn(publicAddress) !== prepareFqdn(params.fqdn)) {
+      throw Error(`Failed to match applications with FQDN "${params.fqdn}"`);
+    }
+  }
+
   const createAppSession = useCallback(async (params: UrlLauncherParams) => {
     let fqdn = params.fqdn;
     const port = location.port ? `:${location.port}` : '';
@@ -56,29 +93,12 @@ 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
-      // 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)) {
-        // 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}"`
-          );
-        }
-      }
+      validateResolvedApp(
+        params,
+        resolvedApp.fqdn,
+        resolvedApp.localClusterName,
+        resolvedApp.publicAddress
+      );
 
       let path = '';
       if (queryParams.has('path')) {