Automatically authenticate required applications during web app auth by avatus · Pull Request #45512 · gravitational/teleport

avatus · 2024-08-14T22:35:27Z

part of: #17588
This app fixes the issue where a user would have to manually click the "launch" button for every app that another app may require to work (for instance, a backend API for a client side react app).

The solution is to have admins set a "required apps" field in their app spec, which will allow the app handler and client to automate the authentication redirects when launching the desired app. This value is passed through a query param through the authentication flow and used by the server to determine which app to redirect to next, ultimately landing on the originally requested app.

security considerations

Although this query param can be "user supplied", it does not bypass any of the current authentication flow and goes through the same validation as if it were clicked from the "Launch" button in the Web UI. It is used merely as an indication of which URL to build.
Scenarios checked (please let me know if I've missed any)

If this was somehow maliciously set by an admin to a bogus website, the same checks for "does this app really exist in our cluster" are still made. The same thing happens if the query param is tampered with, the validation is still done.
If an app requires an app that the user doesn't have access to, the authentication flow will still fail as usual.
If an app can't be found during the authentication flow (either via mis-configuration or maliciously set by a user), the failed app is logged and thrown out. This means misconfiguration should not stop authentication of the original requested app

Example breakdown

the app named client requires the app api to serve it static assets

Example old flow:
-> The user would authenticate to Teleport.
-> The user would click "Launch" for client.
-> client would go through the redirect flow
-> client will be authenticated and load. However, assets would not load because api is not authenticated.
-> user goes back to teleport, clicks "Launch" on api, and goes through the redirect flow
-> client now works as expected.
-> repeat above for any other required apps

New flow
-> The user would authentication to Teleport
-> The user would click "Launch" for client
-> client will instead go through the redirect flow for any of the required apps.
-> once all the required apps are done, the last redirect is for client
-> client works as expected.

This essentially boils down to automatically going to all the correct /web/launch/:fqdn URLs in the browser and nothing more.

Example of my app service

app_service:
  enabled: "yes"
  debug_app: true
  apps:
    - name: "api22"
      uri: "http://localhost:4000"
      public_addr: "api.teleport.zarquon.sh"
    - name: "client"
      uri: "http://localhost:3002"
      public_addr: "client.teleport.zarquon.sh"
      required_apps:
        - "api22"

This PR will not solve the preflight CORS requests if client.teleport.zarquon.sh is trying to access api.teleport.zarquon.sh with some sort of custom header (such as Authorization) because currently, we do not accept non-authenticated requests (OPTIONS requests are always unauthenticated). That PR will come after.

You can use this repo for local apps to test with https://github.com/avatus/corstest

changelog: You can now set required app names in your application spec. This will enable the Web UI to automatically authenticate every app required during application authentication.

github-actions · 2024-09-04T19:23:04Z

🤖 Vercel preview here: https://docs-4ny5njkol-goteleport.vercel.app/docs/ver/preview

avatus · 2024-09-06T16:29:02Z

@ryanclark could you give this one a look when you get the chance please 🙏 . Security review came back good so this is ready

flyinghermit

LGTM based on the decision that this is the way we want to implement the feature.

Left a few suggestions on renaming and reducing extra request steps.

flyinghermit · 2024-09-06T17:49:36Z

 	// GetIntegration will return the Integration.
 	// If present, the Application must use the Integration's credentials instead of ambient credentials to access Cloud APIs.
 	GetIntegration() string
+	// GetRequiredApps will return a list of required apps public addrs that should be authenticated during this apps authentication process.


// GetRequiredApps will return a list of required apps public addrs

Does this return public addrs or app name?

flyinghermit · 2024-09-06T18:09:32Z

 	// This field is used to preserve the original requested path through
 	// the app access authentication redirections.
 	path string
+	// requiredApps is a list of required app names to be used during application


// requiredApps is a list of required app names

At this point, the value of requiredApps is fqdn and not app name right?

Suggested change

// requiredApps is a list of required app names to be used during application

// requiredApps is a list of required app fqdn to be used during application

I think it would also be good to update the names so distinction is clear?

requiredAppsName in the app configuration spec and

requiredAppsFQDN here

flyinghermit · 2024-09-06T18:17:07Z

+type GetAppDetailsResponse struct {
 	// FQDN is application FQDN.
 	FQDN string `json:"fqdn"`
+	// RequiredApps is a list of required app names


fqdn here too?

Suggested change

// RequiredApps is a list of required app names

// RequiredApps is a list of required app FQDN

flyinghermit · 2024-09-06T18:46:54Z

      }

+      let requiredApps = resolvedApp.requiredApps || [];
+      if (isRedirectFlow !== null) {


nit:

Suggested change

if (isRedirectFlow !== null) {

if (!!isRedirectFlow) {

(unless you want to make it clear that we are explicitly checking for null :) )

Specifically checking for null because redirectFlow is got from queryParams.get which returns null or string. (also, the double negation throws eslint here based on string type so, i'll leave this one)

flyinghermit · 2024-09-06T19:11:32Z

 // GET /v1/webapi/apps/:fqdnHint/:clusterName/:publicAddr
-func (h *Handler) getAppFQDN(w http.ResponseWriter, r *http.Request, p httprouter.Params, ctx *SessionContext) (interface{}, error) {
-	req := GetAppFQDNRequest{
+func (h *Handler) getAppDetails(w http.ResponseWriter, r *http.Request, p httprouter.Params, ctx *SessionContext) (interface{}, error) {


suggestion: If we can send required apps (fqdn) in the app response from the unified resource api, we can collapse the first two requests we have now to one.

First request /web/launch...?

Second request webapi/apps responds with required apps

Third request /web/launch...?required-apps=<required app from responds received in second request >.

I can see this URL was always called to get the app fqdn which I do not understand why it was done that way. Up to you if you want to reduce number of requests here.

That or looks like we cna also combine the state and required apps response to one, which would reduce few extra requests.

Agree, but I will TODO this for now. I'd like to talk to @capnspacehook to make sure this extra call can be merged with the first. Really like this idea a lot

most of the /web/launch/ calls are from a redirect and not a direct api call so sending the data back might be a bit weird but I'll explore it

github-actions · 2024-09-06T22:52:58Z

🤖 Vercel preview here: https://docs-faqodnc7t-goteleport.vercel.app/docs/ver/preview

avatus · 2024-09-06T23:05:29Z

Im not sure what happened, but a bunch of other changes got scooped up when i did make derive. But its rebased on latest master. I'll inspect before i merge

github-actions · 2024-09-06T23:05:58Z

🤖 Vercel preview here: https://docs-bgt920ijl-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-09-06T23:19:33Z

🤖 Vercel preview here: https://docs-5oxjb7sll-goteleport.vercel.app/docs/ver/preview

github-actions Bot requested review from rudream and ryanclark August 14, 2024 22:35

github-actions Bot added size/md ui labels Aug 14, 2024

avatus force-pushed the avatus/redirect branch from 7b151e8 to eb9b0d9 Compare August 14, 2024 22:52

avatus requested a review from capnspacehook August 15, 2024 14:30

avatus force-pushed the avatus/redirect branch from cccf9d8 to 478244d Compare August 15, 2024 18:01

zmb3 requested a review from flyinghermit August 16, 2024 03:25

avatus force-pushed the avatus/redirect branch from 478244d to 3f18fbe Compare August 16, 2024 14:52

avatus mentioned this pull request Aug 16, 2024

Add CORS policy to app spec #45558

Merged

avatus force-pushed the avatus/redirect branch from 3f18fbe to fa8c486 Compare August 18, 2024 14:49

flyinghermit reviewed Aug 21, 2024

View reviewed changes

Comment thread lib/web/app/auth.go

avatus force-pushed the avatus/redirect branch from 9f31667 to 7fdc1f6 Compare September 4, 2024 19:12

avatus temporarily deployed to vercel September 4, 2024 19:12 — with GitHub Actions Inactive

avatus requested a review from kimlisa September 4, 2024 19:25

ryanclark approved these changes Sep 6, 2024

View reviewed changes

flyinghermit approved these changes Sep 6, 2024

View reviewed changes

avatus force-pushed the avatus/redirect branch from 7fdc1f6 to 0c34e17 Compare September 6, 2024 22:42

avatus temporarily deployed to vercel September 6, 2024 22:42 — with GitHub Actions Inactive

public-teleport-github-review-bot Bot removed request for capnspacehook, kimlisa and rudream September 6, 2024 22:43

avatus force-pushed the avatus/redirect branch from 0c34e17 to 6dc1fcb Compare September 6, 2024 22:55

avatus temporarily deployed to vercel September 6, 2024 22:55 — with GitHub Actions Inactive

Automatically authenticate required applications during web app auth

4698e76

avatus force-pushed the avatus/redirect branch from 6dc1fcb to 4698e76 Compare September 6, 2024 23:09

avatus temporarily deployed to vercel September 6, 2024 23:09 — with GitHub Actions Inactive

avatus added this pull request to the merge queue Sep 7, 2024

Merged via the queue into master with commit 8352594 Sep 7, 2024

avatus deleted the avatus/redirect branch September 7, 2024 01:11

avatus mentioned this pull request Sep 7, 2024

[v16] Automatically authenticate required applications during web app auth #46366

Merged

capnspacehook mentioned this pull request Oct 24, 2024

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

Merged

Conversation

avatus commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

security considerations

Example breakdown

Uh oh!

Uh oh!

github-actions Bot commented Sep 4, 2024

Uh oh!

avatus commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flyinghermit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 6, 2024

Uh oh!

avatus commented Sep 6, 2024

Uh oh!

github-actions Bot commented Sep 6, 2024

Uh oh!

github-actions Bot commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

avatus commented Aug 14, 2024 •

edited

Loading

avatus commented Sep 6, 2024 •

edited

Loading

avatus Sep 6, 2024 •

edited

Loading