fix app access regression when the app is on a leaf cluster

[capnspacehook]

Fixes #46951.

changelog: fix app access regression to apps on leaf clusters

[avatus]

Logic seems fine to be. it'd be nice to change this from some nested if and just into two variables like
const fqdnMatch and const isLeafApp or whatever you think makes most sense. then we can just do if !this && !that, return error

might read a little nicer. but in general this is good!

[aws-amplify-us-west-2]

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-47778.d3pp5qlev8mo18.amplifyapp.com

[capnspacehook]

Thanks @avatus, moved the logic into it's own function to put the logic and comments in one place. Did some more testing and found that this PR fixes an access denied error popping up when attempting to access an app from a leaf cluster, but another issue prevents accessing the leaf app. I get a warning: Request failed: state cookie is not set. app/auth.go:166. I confirmed this issue started after my 2 commits that added and revised these app FQDN checks, the regression seems to have been created between #45024 and #45512. Do you mind confirming this for me?

[avatus]

The fqdn returned from a leaf resource gets updated in a weird way (which might be incorrect and perhaps is what we should really look at and fix). It becomes something like appname-rootcluster. Which then causes the frontend to forward to the wrong app

This is why you see Request failed: state cookie is not set, because the original requested app during the auth exchange is different, so the cookie isn't set on the correct domain.

I believe we need to update our forwarding URL to use publicAddr of the resolved app instead of the FQDN. I tested locally and works, but didn't consider the implications of the change. Please take a look when you get a chance @capnspacehook .

Here was my diff to get it working (copy and then pbpaste | git apply)

Open Me!

diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx
index 58420124e0..1c61b70d71 100644
--- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx
+++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx
@@ -122,7 +122,6 @@ export function AppLauncher() {
       const stateToken = queryParams.get('state');
       if (!stateToken) {
         initiateNewAuthExchange({
-          fqdn,
           port,
           path,
           params,
@@ -138,7 +137,10 @@ export function AppLauncher() {
       const session = await service.createAppSession(params);
 
       // Set all the fields expected by server to validate request.
-      const url = getXTeleportAuthUrl({ fqdn, port });
+      const url = getXTeleportAuthUrl({
+        publicAddr: resolvedApp.publicAddress,
+        port,
+      });
       url.searchParams.set('state', stateToken);
       url.searchParams.set('subject', session.subjectCookieValue);
       if (requiredApps.length > 1) {
@@ -217,9 +219,15 @@ function prepareFqdn(fqdn: string) {
   }
 }
 
-function getXTeleportAuthUrl({ fqdn, port }: { fqdn: string; port: string }) {
+function getXTeleportAuthUrl({
+  port,
+  publicAddr,
+}: {
+  port: string;
+  publicAddr: string;
+}) {
   try {
-    return new URL(`https://${fqdn}${port}/x-teleport-auth`);
+    return new URL(`https://${publicAddr}${port}/x-teleport-auth`);
   } catch (err) {
     throwFailedToParseUrlError(err);
   }
@@ -235,13 +243,11 @@ function getXTeleportAuthUrl({ fqdn, port }: { fqdn: string; port: string }) {
 //      bookmarked URL), in which the server will redirect the user
 //      to this launcher.
 function initiateNewAuthExchange({
-  fqdn,
   port,
   params,
   path,
   requiredApps,
 }: {
-  fqdn: string;
   port: string;
   // params will only be defined if the user clicked our "launch"
   // app button from the web UI.
@@ -255,7 +261,7 @@ function initiateNewAuthExchange({
   path: string;
   requiredApps: string[];
 }) {
-  const url = getXTeleportAuthUrl({ fqdn, port });
+  const url = getXTeleportAuthUrl({ publicAddr: params.publicAddr, port });
 
   if (path) {
     url.searchParams.set('path', path);

Thanks!

[nklaassen]

In the PR description I see what bug this is supposed to fix, but can you please explain how this is meant to fix it?

[zmb3]

Does this (Michael's patch) also fix #10671?

[capnspacehook]

@zmb3 I'm not sure, will test and report back but I'm not optimistic it fixes that. @kimlisa can I get a review please?

[avatus]

Does this (Michael's patch) also fix #10671?

@zmb3 no, it won't fix that unfortunately. However, fixing the issue of same name on root/leaf would probably also fix same name on leaf1/leaf2.

[zmb3]

Suggested change

	host := hostname
	if req.requiresAppRedirect {
	host = req.publicAddr
	}
	if req.requiresAppRedirect {
	hostname = req.publicAddr
	}

If you update the existing hostname variable instead of making a new variable with a similar name, there's less chance of us mistakenly using the wrong one later on in this function and breaking something.

[capnspacehook]

Thinking about this more I agree with how it's done now a bit more; if the variable is set to the public addr calling it hostname isn't very accurate anymore

[zmb3]

Then change the input paramater to something like addr.

Having two variables in scope with a similar name, where one works and the other doesn't is a recipe for failure.

[capnspacehook]

Fair enough, done

[zmb3]

I was hoping you would rename the function parameter, which was not done.

I don't want two variables in scope where one works and one doesn't. Let's make a single variable (which we overwrite if necessary), so there's no potential for confusion. As written, we still have addr and hostname in scope.

[capnspacehook]

Ah I see, I misunderstood. That makes sense, I renamed the hostname parameter to addr. This should make this function less confusing

[nklaassen]

i also tested this yesterday and it works well

[zmb3]

Thanks capn!

[ravicious]

Could we backport this to v16? According to #46951, it's broken on >= 16.4.0.

[avatus]

@ravicious @capnspacehook #49056 backported it to v16 here