Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Nov 2, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0d79b7
only redirect to the public addr of an app when an app redirect is re…
capnspacehook Nov 1, 2024
722e81c
rename local variable from 'host' to 'addr'
capnspacehook Nov 1, 2024
c95e6ac
rename param
capnspacehook Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/web/app/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,10 +618,14 @@ const (
// The URL's are formed this way to help isolate the path params reserved for the app
// launchers route, where order and existence of previous params matter for this route.
func makeAppRedirectURL(r *http.Request, proxyPublicAddr, hostname string, req launcherURLParams) string {
host := hostname
if req.requiresAppRedirect {
host = req.publicAddr
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

u := url.URL{
Scheme: "https",
Host: proxyPublicAddr,
Path: fmt.Sprintf("/web/launch/%s", hostname),
Path: fmt.Sprintf("/web/launch/%s", host),
}

// Presence of a stateToken means we are beginning an app auth exchange.
Expand All @@ -634,7 +638,7 @@ func makeAppRedirectURL(r *http.Request, proxyPublicAddr, hostname string, req l
v.Add("required-apps", req.requiredAppFQDNs)
u.RawQuery = v.Encode()

urlPath := []string{"web", "launch", hostname}
urlPath := []string{"web", "launch", host}

// The order and existence of previous params matter.
//
Expand Down
6 changes: 1 addition & 5 deletions lib/web/app/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,8 @@ func (h *Handler) redirectToLauncher(w http.ResponseWriter, r *http.Request, p l
"https://goteleport.com/docs/application-access/guides/connecting-apps/#start-authproxy-service.")
return trace.BadParameter("public address of the proxy is not set")
}
host := p.publicAddr
if host == "" {
host = r.Host
}

addr, err := utils.ParseAddr(host)
addr, err := utils.ParseAddr(r.Host)
if err != nil {
return trace.Wrap(err)
}
Expand Down
Loading